dle-develop — Dependable Linux Effort mailing list for developers

Re: [Dle-develop] [PATCH -tip v2 0/3] tracepoint: Add signal events

[Masami H. <mhi...@re...>]

Hi Jason,

Jason Baron wrote:
> On Wed, Nov 18, 2009 at 04:16:09PM -0500, Masami Hiramatsu wrote:
>> Hi,
>>
>> These patches add signal related tracepoints including
>> signal generation, delivery, and loss. First patch also
>> moves signal-sending tracepoint from events/sched.h to
>> events/signal.h.
>>
>> Changes in v2
>> - Add siginfo arguments
>>
>
> hi Masami,
>
> It would be really great if you could add docbook style comments to
> your tracepoints. Here's the initial path I posted for the irq chapter:
> http://lkml.indiana.edu/hypermail/linux/kernel/0904.3/02651.html
>
> So you could add a new chapter for signals.

Ah, that's a good suggestion. I'll add it next version.

Thank you,

>
> The current tracepoint docbook can be viewed at:
>
> http://www.kernel.org/doc/htmldocs/tracepoint/
>
> thanks,
>
> -Jason

-- 
Masami Hiramatsu

Software Engineer
Hitachi Computer Products (America), Inc.
Software Solutions Division

e-mail: mhi...@re...

Re: [Dle-develop] [PATCH -tip v2 0/3] tracepoint: Add signal events

[Jason B. <jb...@re...>]

On Wed, Nov 18, 2009 at 04:16:09PM -0500, Masami Hiramatsu wrote:
> Hi,
> 
> These patches add signal related tracepoints including
> signal generation, delivery, and loss. First patch also
> moves signal-sending tracepoint from events/sched.h to
> events/signal.h.
> 
> Changes in v2
> - Add siginfo arguments
> 

hi Masami,

It would be really great if you could add docbook style comments to
your tracepoints. Here's the initial path I posted for the irq chapter:
http://lkml.indiana.edu/hypermail/linux/kernel/0904.3/02651.html

So you could add a new chapter for signals.

The current tracepoint docbook can be viewed at:

http://www.kernel.org/doc/htmldocs/tracepoint/

thanks,

-Jason

Re: [Dle-develop] [PATCH -tip 0/2] Fix mm->flags consistency issue in coredump

[Américo W. <xiy...@gm...>]

On Tue, Nov 17, 2009 at 06:53:05PM -0500, Masami Hiramatsu wrote:
>
>Hi,
>
>These patches are for fixing coredump mm->flags consistency issue.
>
>---
>1787         if (mm->core_state || !get_dumpable(mm)) {  <- (1)
>1788                 up_write(&mm->mmap_sem);
>1789                 put_cred(cred);
>1790                 goto fail;
>1791         }
>1792
>[...]
>1798         if (get_dumpable(mm) == 2) {    /* Setuid core dump mode */ <-(2)
>1799                 flag = O_EXCL;          /* Stop rewrite attacks */
>1800                 cred->fsuid = 0;        /* Dump root private */
>1801         }
>---
>
>Since dumpable bits are not protected by lock, there is a
>chance to change these bits between (1) and (2).
>
>To solve this issue, this patch copies mm->flags to
>coredump_params.mm_flags at the beginning of do_coredump()                      and uses it instead of get_dumpable() while dumping core.
>This series also introduce coredump parameter structure
>for simplify bimfmt->core_dump interface.


So, this patch set hides 'mm_flags' from globally in mm_struct
to locally in do_coredump() function, by copying it to a local
data structure?

Hmm, seems reasonable.

Reviewed-by: WANG Cong <xiy...@gm...>

Thanks.


>
>Thank you,
>
>---
>
>Masami Hiramatsu (2):
>      Pass mm->flags as a coredump parameter for consistency
>      mm: Introduce coredump parameter structure
>
>
> fs/binfmt_aout.c        |   13 ++++++-----
> fs/binfmt_elf.c         |   50 +++++++++++++++++++----------------------
> fs/binfmt_elf_fdpic.c   |   38 ++++++++++++-------------------
> fs/binfmt_flat.c        |    6 ++---
> fs/binfmt_som.c         |    2 +-
> fs/exec.c               |   58 ++++++++++++++++++++++++++++++-----------------
> include/linux/binfmts.h |   11 ++++++++-
> 7 files changed, 97 insertions(+), 81 deletions(-)
>
>-- 
>Masami Hiramatsu
>
>Software Engineer
>Hitachi Computer Products (America), Inc.
>Software Solutions Division
>e-mail: mhi...@re...
>--
>To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
>the body of a message to maj...@vg...
>More majordomo info at  http://vger.kernel.org/majordomo-info.html
>Please read the FAQ at  http://www.tux.org/lkml/

-- 
Live like a child, think like the god.
 

Re: [Dle-develop] [PATCH -tip 2/2] Pass mm->flags as a coredump parameter for consistency

[Hidehiro K. <hid...@hi...>]

Hi Hiramatsu-san,

Masami Hiramatsu wrote:

>> Pass mm->flags as a coredump parameter for consistency.
>> 
>>  ---
>> 1787         if (mm->core_state || !get_dumpable(mm)) {  <- (1)
>> 1788                 up_write(&mm->mmap_sem);
>> 1789                 put_cred(cred);
>> 1790                 goto fail;
>> 1791         }
>> 1792
>> [...]
>> 1798         if (get_dumpable(mm) == 2) {    /* Setuid core dump mode */ <-(2)
>> 1799                 flag = O_EXCL;          /* Stop rewrite attacks */
>> 1800                 cred->fsuid = 0;        /* Dump root private */
>> 1801         }
>>  ---
>> 
>> Since dumpable bits are not protected by lock, there is a
>> chance to change these bits between (1) and (2).

Yes, a race condition can be caused by prctl from another thread.
And your patch is fine with me.  Thanks!

Reviewed-by: Hidehiro Kawai <hid...@hi...>
--
Hidehiro Kawai
Hitachi, Systems Development Laboratory
Linux Technology Center

Re: [Dle-develop] [PATCH -tip 0/3] tracepoint: Add signal events

[Roland M. <ro...@re...>]

> Roland McGrath wrote:
> > I would add the siginfo_t * argument to each of these tracepoints.
> 
> I see. Is it OK to record only si_errno and si_code? because si_signo
> might be as same as sig.

Yes, si_signo will match sig.  Which siginfo_t fields are interesting to
record can vary by signal (i.e. by si_code).  si_code and si_errno are
certainly the first things that come to mind as most useful.  For SI_TIMER,
si_tid, si_overrun, and si_sigval are interesting.  For others, si_pid and
si_uid are interesting.


Thanks,
Roland

[Dle-develop] [PATCH -tip v2 2/3] tracepoint: Add signal deliver event

[Masami H. <mhi...@re...>]

Add a tracepoint where a process gets a signal. This tracepoint
shows signal-number, sa-handler and sa-flag.

Changes in v2:
 - Add siginfo argument
 - Fix comment

Signed-off-by: Masami Hiramatsu <mhi...@re...>
Cc: Oleg Nesterov <ol...@re...>
Cc: Roland McGrath <ro...@re...>
Cc: Ingo Molnar <mi...@el...>
---

 include/trace/events/signal.h |   28 ++++++++++++++++++++++++++++
 kernel/signal.c               |    3 +++
 2 files changed, 31 insertions(+), 0 deletions(-)

diff --git a/include/trace/events/signal.h b/include/trace/events/signal.h
index cfe9f0a..9a043db 100644
--- a/include/trace/events/signal.h
+++ b/include/trace/events/signal.h
@@ -51,6 +51,34 @@ TRACE_EVENT(signal_generate,
 		  __entry->comm, __entry->pid)
 );
 
+/*
+ * Tracepoint for delivering a signal:
+ */
+TRACE_EVENT(signal_deliver,
+
+	TP_PROTO(int sig, struct siginfo *info, struct k_sigaction *ka),
+
+	TP_ARGS(sig, info, ka),
+
+	TP_STRUCT__entry(
+		__field(	int,		sig		)
+		__field(	int,		errno		)
+		__field(	int,		code		)
+		__field(	unsigned long,	sa_handler	)
+		__field(	unsigned long,	sa_flags	)
+	),
+
+	TP_fast_assign(
+		__entry->sig	= sig;
+		TP_STORE_SIGINFO(__entry, info);
+		__entry->sa_handler	= (unsigned long)ka->sa.sa_handler;
+		__entry->sa_flags	= ka->sa.sa_flags;
+	),
+
+	TP_printk("sig=%d errno=%d code=%d sa_handler=%lx sa_flags=%lx",
+		  __entry->sig, __entry->errno, __entry->code,
+		  __entry->sa_handler, __entry->sa_flags)
+);
 #endif /* _TRACE_SIGNAL_H */
 
 /* This part must be outside protection */
diff --git a/kernel/signal.c b/kernel/signal.c
index 54ac4c5..d518984 100644
--- a/kernel/signal.c
+++ b/kernel/signal.c
@@ -1860,6 +1860,9 @@ relock:
 			ka = &sighand->action[signr-1];
 		}
 
+		/* Trace actually delivered signals. */
+		trace_signal_deliver(signr, info, ka);
+
 		if (ka->sa.sa_handler == SIG_IGN) /* Do nothing.  */
 			continue;
 		if (ka->sa.sa_handler != SIG_DFL) {


-- 
Masami Hiramatsu

Software Engineer
Hitachi Computer Products (America), Inc.
Software Solutions Division

e-mail: mhi...@re...

[Dle-develop] [PATCH -tip v2 1/3] tracepoint: Move signal sending tracepoint to events/signal.h

[Masami H. <mhi...@re...>]

Move signal sending event to events/signal.h. This patch also renames
sched_signal_send event to signal_generate.

Changes in v2:
 - Add siginfo argument
 - Add siginfo storing macro

Signed-off-by: Masami Hiramatsu <mhi...@re...>
Cc: Oleg Nesterov <ol...@re...>
Cc: Roland McGrath <ro...@re...>
Cc: Ingo Molnar <mi...@el...>
---

 include/trace/events/sched.h  |   25 ------------------
 include/trace/events/signal.h |   57 +++++++++++++++++++++++++++++++++++++++++
 kernel/signal.c               |    5 ++--
 3 files changed, 60 insertions(+), 27 deletions(-)
 create mode 100644 include/trace/events/signal.h

diff --git a/include/trace/events/sched.h b/include/trace/events/sched.h
index b50b985..b221bb3 100644
--- a/include/trace/events/sched.h
+++ b/include/trace/events/sched.h
@@ -320,31 +320,6 @@ TRACE_EVENT(sched_process_fork,
 );
 
 /*
- * Tracepoint for sending a signal:
- */
-TRACE_EVENT(sched_signal_send,
-
-	TP_PROTO(int sig, struct task_struct *p),
-
-	TP_ARGS(sig, p),
-
-	TP_STRUCT__entry(
-		__field(	int,	sig			)
-		__array(	char,	comm,	TASK_COMM_LEN	)
-		__field(	pid_t,	pid			)
-	),
-
-	TP_fast_assign(
-		memcpy(__entry->comm, p->comm, TASK_COMM_LEN);
-		__entry->pid	= p->pid;
-		__entry->sig	= sig;
-	),
-
-	TP_printk("sig=%d comm=%s pid=%d",
-		  __entry->sig, __entry->comm, __entry->pid)
-);
-
-/*
  * XXX the below sched_stat tracepoints only apply to SCHED_OTHER/BATCH/IDLE
  *     adding sched_stat support to SCHED_FIFO/RR would be welcome.
  */
diff --git a/include/trace/events/signal.h b/include/trace/events/signal.h
new file mode 100644
index 0000000..cfe9f0a
--- /dev/null
+++ b/include/trace/events/signal.h
@@ -0,0 +1,57 @@
+#undef TRACE_SYSTEM
+#define TRACE_SYSTEM signal
+
+#if !defined(_TRACE_SIGNAL_H) || defined(TRACE_HEADER_MULTI_READ)
+#define _TRACE_SIGNAL_H
+
+#include <linux/signal.h>
+#include <linux/sched.h>
+#include <linux/tracepoint.h>
+
+#define TP_STORE_SIGINFO(__entry, info)				\
+	do {							\
+		if (info == SEND_SIG_NOINFO) {			\
+			__entry->errno	= 0;			\
+			__entry->code	= SI_USER;		\
+		} else if (info == SEND_SIG_PRIV) {		\
+			__entry->errno	= 0;			\
+			__entry->code	= SI_KERNEL;		\
+		} else {					\
+			__entry->errno	= info->si_errno;	\
+			__entry->code	= info->si_code;	\
+		}						\
+	} while (0)
+
+/*
+ * Tracepoint for generating a signal:
+ */
+TRACE_EVENT(signal_generate,
+
+	TP_PROTO(int sig, struct siginfo *info, struct task_struct *p),
+
+	TP_ARGS(sig, info, p),
+
+	TP_STRUCT__entry(
+		__field(	int,	sig			)
+		__field(	int,	errno			)
+		__field(	int,	code			)
+		__array(	char,	comm,	TASK_COMM_LEN	)
+		__field(	pid_t,	pid			)
+	),
+
+	TP_fast_assign(
+		__entry->sig	= sig;
+		TP_STORE_SIGINFO(__entry, info);
+		memcpy(__entry->comm, p->comm, TASK_COMM_LEN);
+		__entry->pid	= p->pid;
+	),
+
+	TP_printk("sig=%d errno=%d code=%d comm=%s pid=%d",
+		  __entry->sig, __entry->errno, __entry->code,
+		  __entry->comm, __entry->pid)
+);
+
+#endif /* _TRACE_SIGNAL_H */
+
+/* This part must be outside protection */
+#include <trace/define_trace.h>
diff --git a/kernel/signal.c b/kernel/signal.c
index fe08008..54ac4c5 100644
--- a/kernel/signal.c
+++ b/kernel/signal.c
@@ -28,7 +28,8 @@
 #include <linux/freezer.h>
 #include <linux/pid_namespace.h>
 #include <linux/nsproxy.h>
-#include <trace/events/sched.h>
+#define CREATE_TRACE_POINTS
+#include <trace/events/signal.h>
 
 #include <asm/param.h>
 #include <asm/uaccess.h>
@@ -856,7 +857,7 @@ static int __send_signal(int sig, struct siginfo *info, struct task_struct *t,
 	struct sigqueue *q;
 	int override_rlimit;
 
-	trace_sched_signal_send(sig, t);
+	trace_signal_generate(sig, info, t);
 
 	assert_spin_locked(&t->sighand->siglock);
 


-- 
Masami Hiramatsu

Software Engineer
Hitachi Computer Products (America), Inc.
Software Solutions Division

e-mail: mhi...@re...

[Dle-develop] [PATCH -tip v2 3/3] tracepoint: Add signal loss events

[Masami H. <mhi...@re...>]

Add signal_overflow_fail and signal_lose_info tracepoints
for signal-lost events.

Changes in v2:
 - Use siginfo string macro

Signed-off-by: Masami Hiramatsu <mhi...@re...>
Suggested-by: Roland McGrath <ro...@re...>
Cc: Oleg Nesterov <ol...@re...>
Cc: Ingo Molnar <mi...@el...>
---

 include/trace/events/signal.h |   52 +++++++++++++++++++++++++++++++++++++++++
 kernel/signal.c               |   19 +++++++++++----
 2 files changed, 66 insertions(+), 5 deletions(-)

diff --git a/include/trace/events/signal.h b/include/trace/events/signal.h
index 9a043db..f7215ed 100644
--- a/include/trace/events/signal.h
+++ b/include/trace/events/signal.h
@@ -79,6 +79,58 @@ TRACE_EVENT(signal_deliver,
 		  __entry->sig, __entry->errno, __entry->code,
 		  __entry->sa_handler, __entry->sa_flags)
 );
+
+/*
+ * Tracepoint for losing a signal by overflow:
+ */
+TRACE_EVENT(signal_overflow_fail,
+
+	TP_PROTO(int sig, int group, struct siginfo *info),
+
+	TP_ARGS(sig, group, info),
+
+	TP_STRUCT__entry(
+		__field(	int,	sig	)
+		__field(	int,	group	)
+		__field(	int,	errno	)
+		__field(	int,	code	)
+	),
+
+	TP_fast_assign(
+		__entry->sig	= sig;
+		__entry->group	= group;
+		TP_STORE_SIGINFO(__entry, info);
+	),
+
+	TP_printk("sig=%d group=%d errno=%d code=%d",
+		  __entry->sig, __entry->group, __entry->errno, __entry->code)
+);
+
+/*
+ * Tracepoint for losing a siginfo:
+ */
+TRACE_EVENT(signal_lose_info,
+
+	TP_PROTO(int sig, int group, struct siginfo *info),
+
+	TP_ARGS(sig, group, info),
+
+	TP_STRUCT__entry(
+		__field(	int,	sig	)
+		__field(	int,	group	)
+		__field(	int,	errno	)
+		__field(	int,	code	)
+	),
+
+	TP_fast_assign(
+		__entry->sig	= sig;
+		__entry->group	= group;
+		TP_STORE_SIGINFO(__entry, info);
+	),
+
+	TP_printk("sig=%d group=%d errno=%d code=%d",
+		  __entry->sig, __entry->group, __entry->errno, __entry->code)
+);
 #endif /* _TRACE_SIGNAL_H */
 
 /* This part must be outside protection */
diff --git a/kernel/signal.c b/kernel/signal.c
index d518984..6b982f2 100644
--- a/kernel/signal.c
+++ b/kernel/signal.c
@@ -919,12 +919,21 @@ static int __send_signal(int sig, struct siginfo *info, struct task_struct *t,
 			break;
 		}
 	} else if (!is_si_special(info)) {
-		if (sig >= SIGRTMIN && info->si_code != SI_USER)
-		/*
-		 * Queue overflow, abort.  We may abort if the signal was rt
-		 * and sent by user using something other than kill().
-		 */
+		if (sig >= SIGRTMIN && info->si_code != SI_USER) {
+			/*
+			 * Queue overflow, abort.  We may abort if the
+			 * signal was rt and sent by user using something
+			 * other than kill().
+			 */
+			trace_signal_overflow_fail(sig, group, info);
 			return -EAGAIN;
+		} else {
+			/*
+			 * This is a silent loss of information.  We still
+			 * send the signal, but the *info bits are lost.
+			 */
+			trace_signal_lose_info(sig, group, info);
+		}
 	}
 
 out_set:


-- 
Masami Hiramatsu

Software Engineer
Hitachi Computer Products (America), Inc.
Software Solutions Division

e-mail: mhi...@re...

[Dle-develop] [PATCH -tip v2 0/3] tracepoint: Add signal events

[Masami H. <mhi...@re...>]

Hi,

These patches add signal related tracepoints including
signal generation, delivery, and loss. First patch also
moves signal-sending tracepoint from events/sched.h to
events/signal.h.

Changes in v2
- Add siginfo arguments

Thank you,

---

Masami Hiramatsu (3):
      tracepoint: Add signal loss events
      tracepoint: Add signal deliver event
      tracepoint: Move signal sending tracepoint to events/signal.h


 include/trace/events/sched.h  |   25 -------
 include/trace/events/signal.h |  137 +++++++++++++++++++++++++++++++++++++++++
 kernel/signal.c               |   27 ++++++--
 3 files changed, 157 insertions(+), 32 deletions(-)
 create mode 100644 include/trace/events/signal.h

-- 
Masami Hiramatsu

Software Engineer
Hitachi Computer Products (America), Inc.
Software Solutions Division
e-mail: mhi...@re...

Re: [Dle-develop] [PATCH -tip 0/3] tracepoint: Add signal events

[Masami H. <mhi...@re...>]

Roland McGrath wrote:
> I would add the siginfo_t * argument to each of these tracepoints.

I see. Is it OK to record only si_errno and si_code? because si_signo
might be as same as sig.

> In 2/3 this comment:
>
> +		/* Trace the actual delivered signals including SIG_IGN.*/
>
> might be slightly misleading.  In most cases SIG_IGN signals won't make it
> this far.  The same is true of SIG_DFL signals whose default action is to
> ignore.  So one should be clear that trace_signal_deliver() is by no means
> expected to get all SIG_IGN signals that are otherwise eligible for delivery.

Ah, thank you for noticing. I'll remove the last words.

Thank you,

-- 
Masami Hiramatsu

Software Engineer
Hitachi Computer Products (America), Inc.
Software Solutions Division

e-mail: mhi...@re...

Re: [Dle-develop] [PATCH -next 0/3] x86 insn decoder test updates (Re: linux-next: Tree for October 29 (x86 posttest))

[H. P. A. <hp...@zy...>]

On 11/16/2009 10:13 PM, Ingo Molnar wrote:
> 
> * Masami Hiramatsu <mhi...@re...> wrote:
> 
>> Here are the patches which update x86 instruction decoder build-time 
>> test. As Stephen reported on linux-next, sometimes objdump decodes bad 
>> instructions as normal. This will cause a false positive result on x86 
>> insn decoder test. This patches update the test as below;
>>
>>  - Show more information with V=1
>>  - Show in which symbol the difference places.
>>  - Just warning instead of build failure.
> 
> yes, -tip testing was showing such build bugs too:
> 
>  Error: ffffffff8104aae3:	c5 83 3d 49 80 ee    	lds    0xffffffffee80493d(%rbx),%eax
>  Error: objdump says 6 bytes, but insn_get_length() says 3 (attr:0)
> 
> it happens with older tools, such as binutils-2.17. Modern binutils 
> (2.19) is fine.
> 
> We dont want to remove the build error: it helped us fix a number of 
> real bugs in the decoder - instead please try to create a make based 
> workaround based on binutils, to not run the test with binutils older 
> than 2.19 or so.
> 

One idea might be to instead of binutils to use NASM.  The entire NASM
disassembler is small enough (about 10,000 lines including build tools
and instruction database) that we could fit it in the tree in a pinch.

	-hpa

-- 
H. Peter Anvin, Intel Open Source Technology Center
I work for Intel.  I don't speak on their behalf.

[Dle-develop] [PATCH -tip 2/2] Pass mm->flags as a coredump parameter for consistency

[Masami H. <mhi...@re...>]

Pass mm->flags as a coredump parameter for consistency.

 ---
1787         if (mm->core_state || !get_dumpable(mm)) {  <- (1)
1788                 up_write(&mm->mmap_sem);
1789                 put_cred(cred);
1790                 goto fail;
1791         }
1792
[...]
1798         if (get_dumpable(mm) == 2) {    /* Setuid core dump mode */ <-(2)
1799                 flag = O_EXCL;          /* Stop rewrite attacks */
1800                 cred->fsuid = 0;        /* Dump root private */
1801         }
 ---

Since dumpable bits are not protected by lock, there is a
chance to change these bits between (1) and (2).

To solve this issue, this patch copies mm->flags to
coredump_params.mm_flags at the beginning of do_coredump()                      and uses it instead of get_dumpable() while dumping core.

This copy is also passed to binfmt->core_dump, since
elf*_core_dump() uses dump_filter bits in mm->flags.

Signed-off-by: Masami Hiramatsu <mhi...@re...>
Cc: Roland McGrath <ro...@re...>
Cc: Hidehiro Kawai <hid...@hi...>
Cc: Andrew Morton <ak...@li...>
Cc: Oleg Nesterov <ol...@re...>
Cc: Ingo Molnar <mi...@el...>
---

 fs/binfmt_elf.c         |   12 ++----------
 fs/binfmt_elf_fdpic.c   |   12 ++----------
 fs/exec.c               |   20 ++++++++++++++++----
 include/linux/binfmts.h |    1 +
 4 files changed, 21 insertions(+), 24 deletions(-)

diff --git a/fs/binfmt_elf.c b/fs/binfmt_elf.c
index 5e27303..2f76489 100644
--- a/fs/binfmt_elf.c
+++ b/fs/binfmt_elf.c
@@ -1911,7 +1911,6 @@ static int elf_core_dump(struct coredump_params *cprm)
 	struct vm_area_struct *vma, *gate_vma;
 	struct elfhdr *elf = NULL;
 	loff_t offset = 0, dataoff, foffset;
-	unsigned long mm_flags;
 	struct elf_note_info info;
 
 	/*
@@ -1982,13 +1981,6 @@ static int elf_core_dump(struct coredump_params *cprm)
 
 	dataoff = offset = roundup(offset, ELF_EXEC_PAGESIZE);
 
-	/*
-	 * We must use the same mm->flags while dumping core to avoid
-	 * inconsistency between the program headers and bodies, otherwise an
-	 * unusable core file can be generated.
-	 */
-	mm_flags = current->mm->flags;
-
 	/* Write program headers for segments dump */
 	for (vma = first_vma(current, gate_vma); vma != NULL;
 			vma = next_vma(vma, gate_vma)) {
@@ -1998,7 +1990,7 @@ static int elf_core_dump(struct coredump_params *cprm)
 		phdr.p_offset = offset;
 		phdr.p_vaddr = vma->vm_start;
 		phdr.p_paddr = 0;
-		phdr.p_filesz = vma_dump_size(vma, mm_flags);
+		phdr.p_filesz = vma_dump_size(vma, cprm->mm_flags);
 		phdr.p_memsz = vma->vm_end - vma->vm_start;
 		offset += phdr.p_filesz;
 		phdr.p_flags = vma->vm_flags & VM_READ ? PF_R : 0;
@@ -2034,7 +2026,7 @@ static int elf_core_dump(struct coredump_params *cprm)
 		unsigned long addr;
 		unsigned long end;
 
-		end = vma->vm_start + vma_dump_size(vma, mm_flags);
+		end = vma->vm_start + vma_dump_size(vma, cprm->mm_flags);
 
 		for (addr = vma->vm_start; addr < end; addr += PAGE_SIZE) {
 			struct page *page;
diff --git a/fs/binfmt_elf_fdpic.c b/fs/binfmt_elf_fdpic.c
index e65ab9d..28af3e9 100644
--- a/fs/binfmt_elf_fdpic.c
+++ b/fs/binfmt_elf_fdpic.c
@@ -1604,7 +1604,6 @@ static int elf_fdpic_core_dump(struct coredump_params *cprm)
 #endif
 	int thread_status_size = 0;
 	elf_addr_t *auxv;
-	unsigned long mm_flags;
 
 	/*
 	 * We no longer stop all VM operations.
@@ -1735,13 +1734,6 @@ static int elf_fdpic_core_dump(struct coredump_params *cprm)
 	/* Page-align dumped data */
 	dataoff = offset = roundup(offset, ELF_EXEC_PAGESIZE);
 
-	/*
-	 * We must use the same mm->flags while dumping core to avoid
-	 * inconsistency between the program headers and bodies, otherwise an
-	 * unusable core file can be generated.
-	 */
-	mm_flags = current->mm->flags;
-
 	/* write program headers for segments dump */
 	for (vma = current->mm->mmap; vma; vma = vma->vm_next) {
 		struct elf_phdr phdr;
@@ -1753,7 +1745,7 @@ static int elf_fdpic_core_dump(struct coredump_params *cprm)
 		phdr.p_offset = offset;
 		phdr.p_vaddr = vma->vm_start;
 		phdr.p_paddr = 0;
-		phdr.p_filesz = maydump(vma, mm_flags) ? sz : 0;
+		phdr.p_filesz = maydump(vma, cprm->mm_flags) ? sz : 0;
 		phdr.p_memsz = sz;
 		offset += phdr.p_filesz;
 		phdr.p_flags = vma->vm_flags & VM_READ ? PF_R : 0;
@@ -1789,7 +1781,7 @@ static int elf_fdpic_core_dump(struct coredump_params *cprm)
 		goto end_coredump;
 
 	if (elf_fdpic_dump_segments(cprm->file, &size, &cprm->limit,
-				    mm_flags) < 0)
+				    cprm->mm_flags) < 0)
 		goto end_coredump;
 
 #ifdef ELF_CORE_WRITE_EXTRA_DATA
diff --git a/fs/exec.c b/fs/exec.c
index 5daf7d5..2ec6973 100644
--- a/fs/exec.c
+++ b/fs/exec.c
@@ -1718,14 +1718,19 @@ void set_dumpable(struct mm_struct *mm, int value)
 	}
 }
 
-int get_dumpable(struct mm_struct *mm)
+static int __get_dumpable(unsigned long mm_flags)
 {
 	int ret;
 
-	ret = mm->flags & 0x3;
+	ret = mm_flags & MMF_DUMPABLE_MASK;
 	return (ret >= 2) ? 2 : ret;
 }
 
+int get_dumpable(struct mm_struct *mm)
+{
+	return __get_dumpable(mm->flags);
+}
+
 static void wait_for_dump_helpers(struct file *file)
 {
 	struct pipe_inode_info *pipe;
@@ -1769,6 +1774,12 @@ void do_coredump(long signr, int exit_code, struct pt_regs *regs)
 		.signr = signr,
 		.regs = regs,
 		.limit = current->signal->rlim[RLIMIT_CORE].rlim_cur,
+		/*
+		 * We must use the same mm->flags while dumping core to avoid
+		 * inconsistency of bit flags, since this flag is not protected
+		 * by any locks.
+		 */
+		.mm_flags = mm->flags,
 	};
 
 	audit_core_dumps(signr);
@@ -1787,7 +1798,7 @@ void do_coredump(long signr, int exit_code, struct pt_regs *regs)
 	/*
 	 * If another thread got here first, or we are not dumpable, bail out.
 	 */
-	if (mm->core_state || !get_dumpable(mm)) {
+	if (mm->core_state || !__get_dumpable(cprm.mm_flags)) {
 		up_write(&mm->mmap_sem);
 		put_cred(cred);
 		goto fail;
@@ -1798,7 +1809,8 @@ void do_coredump(long signr, int exit_code, struct pt_regs *regs)
 	 *	process nor do we know its entire history. We only know it
 	 *	was tainted so we dump it as root in mode 2.
 	 */
-	if (get_dumpable(mm) == 2) {	/* Setuid core dump mode */
+	if (__get_dumpable(cprm.mm_flags) == 2) {
+		/* Setuid core dump mode */
 		flag = O_EXCL;		/* Stop rewrite attacks */
 		cred->fsuid = 0;	/* Dump root private */
 	}
diff --git a/include/linux/binfmts.h b/include/linux/binfmts.h
index cd4349b..99e529b 100644
--- a/include/linux/binfmts.h
+++ b/include/linux/binfmts.h
@@ -74,6 +74,7 @@ struct coredump_params {
 	struct pt_regs *regs;
 	struct file *file;
 	unsigned long limit;
+	unsigned long mm_flags;
 };
 
 /*


-- 
Masami Hiramatsu

Software Engineer
Hitachi Computer Products (America), Inc.
Software Solutions Division

e-mail: mhi...@re...

[Dle-develop] [PATCH -tip 0/2] Fix mm->flags consistency issue in coredump

[Masami H. <mhi...@re...>]

Hi,

These patches are for fixing coredump mm->flags consistency issue.

---
1787         if (mm->core_state || !get_dumpable(mm)) {  <- (1)
1788                 up_write(&mm->mmap_sem);
1789                 put_cred(cred);
1790                 goto fail;
1791         }
1792
[...]
1798         if (get_dumpable(mm) == 2) {    /* Setuid core dump mode */ <-(2)
1799                 flag = O_EXCL;          /* Stop rewrite attacks */
1800                 cred->fsuid = 0;        /* Dump root private */
1801         }
---

Since dumpable bits are not protected by lock, there is a
chance to change these bits between (1) and (2).

To solve this issue, this patch copies mm->flags to
coredump_params.mm_flags at the beginning of do_coredump()                      and uses it instead of get_dumpable() while dumping core.
This series also introduce coredump parameter structure
for simplify bimfmt->core_dump interface.

Thank you,

---

Masami Hiramatsu (2):
      Pass mm->flags as a coredump parameter for consistency
      mm: Introduce coredump parameter structure


 fs/binfmt_aout.c        |   13 ++++++-----
 fs/binfmt_elf.c         |   50 +++++++++++++++++++----------------------
 fs/binfmt_elf_fdpic.c   |   38 ++++++++++++-------------------
 fs/binfmt_flat.c        |    6 ++---
 fs/binfmt_som.c         |    2 +-
 fs/exec.c               |   58 ++++++++++++++++++++++++++++++-----------------
 include/linux/binfmts.h |   11 ++++++++-
 7 files changed, 97 insertions(+), 81 deletions(-)

-- 
Masami Hiramatsu

Software Engineer
Hitachi Computer Products (America), Inc.
Software Solutions Division
e-mail: mhi...@re...

Re: [Dle-develop] [PATCH -tip 0/3] tracepoint: Add signal events

[Roland M. <ro...@re...>]

I would add the siginfo_t * argument to each of these tracepoints.

In 2/3 this comment:

+		/* Trace the actual delivered signals including SIG_IGN.*/

might be slightly misleading.  In most cases SIG_IGN signals won't make it
this far.  The same is true of SIG_DFL signals whose default action is to
ignore.  So one should be clear that trace_signal_deliver() is by no means
expected to get all SIG_IGN signals that are otherwise eligible for delivery.


Thanks,
Roland

Re: [Dle-develop] [PATCH -tip 1/2] mm: Introduce coredump parameter structure

[Masami H. <mhi...@re...>]

Oops, it's not mm, but bimfmts.

Masami Hiramatsu wrote:
> Introduce coredump parameter data structure (struct coredump_params)
> for simplifying binfmt->core_dump() arguments.
> This also cleanup DUMP_WRITE() in elf_core_dump() by style issue.
>
> Signed-off-by: Masami Hiramatsu<mhi...@re...>
> Suggested-by: Ingo Molnar<mi...@el...>
> Cc: Hidehiro Kawai<hid...@hi...>
> Cc: Andrew Morton<ak...@li...>
> Cc: Oleg Nesterov<ol...@re...>
> Cc: Roland McGrath<ro...@re...>

-- 
Masami Hiramatsu

Software Engineer
Hitachi Computer Products (America), Inc.
Software Solutions Division

e-mail: mhi...@re...

[Dle-develop] [PATCH -tip 2/3] tracepoint: Add signal deliver event

[Masami H. <mhi...@re...>]

Add a tracepoint where a process gets a signal. This tracepoint
shows signal-number, sa-handler and sa-flag.

Signed-off-by: Masami Hiramatsu <mhi...@re...>
Cc: Oleg Nesterov <ol...@re...>
Cc: Roland McGrath <ro...@re...>
Cc: Ingo Molnar <mi...@el...>
---

 include/trace/events/signal.h |   24 ++++++++++++++++++++++++
 kernel/signal.c               |    3 +++
 2 files changed, 27 insertions(+), 0 deletions(-)

diff --git a/include/trace/events/signal.h b/include/trace/events/signal.h
index c7817f3..6dfd88d 100644
--- a/include/trace/events/signal.h
+++ b/include/trace/events/signal.h
@@ -32,6 +32,30 @@ TRACE_EVENT(signal_generate,
 		  __entry->sig, __entry->comm, __entry->pid)
 );
 
+/*
+ * Tracepoint for delivering a signal:
+ */
+TRACE_EVENT(signal_deliver,
+
+	TP_PROTO(int sig, struct k_sigaction *ka),
+
+	TP_ARGS(sig, ka),
+
+	TP_STRUCT__entry(
+		__field(	int,		sig		)
+		__field(	unsigned long,	sa_handler	)
+		__field(	unsigned long,	sa_flags	)
+	),
+
+	TP_fast_assign(
+		__entry->sig	= sig;
+		__entry->sa_handler	= (unsigned long)ka->sa.sa_handler;
+		__entry->sa_flags	= ka->sa.sa_flags;
+	),
+
+	TP_printk("sig=%d sa_handler=%lx sa_flags=%lx",
+		  __entry->sig, __entry->sa_handler, __entry->sa_flags)
+);
 #endif /* _TRACE_SIGNAL_H */
 
 /* This part must be outside protection */
diff --git a/kernel/signal.c b/kernel/signal.c
index 82ab077..5615f41 100644
--- a/kernel/signal.c
+++ b/kernel/signal.c
@@ -1860,6 +1860,9 @@ relock:
 			ka = &sighand->action[signr-1];
 		}
 
+		/* Trace the actual delivered signals including SIG_IGN.*/
+		trace_signal_deliver(signr, ka);
+
 		if (ka->sa.sa_handler == SIG_IGN) /* Do nothing.  */
 			continue;
 		if (ka->sa.sa_handler != SIG_DFL) {


-- 
Masami Hiramatsu

Software Engineer
Hitachi Computer Products (America), Inc.
Software Solutions Division

e-mail: mhi...@re...

[Dle-develop] [PATCH -tip 1/2] mm: Introduce coredump parameter structure

[Masami H. <mhi...@re...>]

Introduce coredump parameter data structure (struct coredump_params)
for simplifying binfmt->core_dump() arguments.
This also cleanup DUMP_WRITE() in elf_core_dump() by style issue.

Signed-off-by: Masami Hiramatsu <mhi...@re...>
Suggested-by: Ingo Molnar <mi...@el...>
Cc: Hidehiro Kawai <hid...@hi...>
Cc: Andrew Morton <ak...@li...>
Cc: Oleg Nesterov <ol...@re...>
Cc: Roland McGrath <ro...@re...>
---

 fs/binfmt_aout.c        |   13 +++++++------
 fs/binfmt_elf.c         |   38 ++++++++++++++++++++++----------------
 fs/binfmt_elf_fdpic.c   |   28 ++++++++++++++--------------
 fs/binfmt_flat.c        |    6 +++---
 fs/binfmt_som.c         |    2 +-
 fs/exec.c               |   38 +++++++++++++++++++++-----------------
 include/linux/binfmts.h |   10 +++++++++-
 7 files changed, 77 insertions(+), 58 deletions(-)

diff --git a/fs/binfmt_aout.c b/fs/binfmt_aout.c
index b639dcf..346b694 100644
--- a/fs/binfmt_aout.c
+++ b/fs/binfmt_aout.c
@@ -32,7 +32,7 @@
 
 static int load_aout_binary(struct linux_binprm *, struct pt_regs * regs);
 static int load_aout_library(struct file*);
-static int aout_core_dump(long signr, struct pt_regs *regs, struct file *file, unsigned long limit);
+static int aout_core_dump(struct coredump_params *cprm);
 
 static struct linux_binfmt aout_format = {
 	.module		= THIS_MODULE,
@@ -89,8 +89,9 @@ if (file->f_op->llseek) { \
  * dumping of the process results in another error..
  */
 
-static int aout_core_dump(long signr, struct pt_regs *regs, struct file *file, unsigned long limit)
+static int aout_core_dump(struct coredump_params *cprm)
 {
+	struct file *file = cprm->file;
 	mm_segment_t fs;
 	int has_dumped = 0;
 	unsigned long dump_start, dump_size;
@@ -108,16 +109,16 @@ static int aout_core_dump(long signr, struct pt_regs *regs, struct file *file, u
 	current->flags |= PF_DUMPCORE;
        	strncpy(dump.u_comm, current->comm, sizeof(dump.u_comm));
 	dump.u_ar0 = offsetof(struct user, regs);
-	dump.signal = signr;
-	aout_dump_thread(regs, &dump);
+	dump.signal = cprm->signr;
+	aout_dump_thread(cprm->regs, &dump);
 
 /* If the size of the dump file exceeds the rlimit, then see what would happen
    if we wrote the stack, but not the data area.  */
-	if ((dump.u_dsize + dump.u_ssize+1) * PAGE_SIZE > limit)
+	if ((dump.u_dsize + dump.u_ssize+1) * PAGE_SIZE > cprm->limit)
 		dump.u_dsize = 0;
 
 /* Make sure we have enough room to write the stack and data areas. */
-	if ((dump.u_ssize + 1) * PAGE_SIZE > limit)
+	if ((dump.u_ssize + 1) * PAGE_SIZE > cprm->limit)
 		dump.u_ssize = 0;
 
 /* make sure we actually have a data and stack area to dump */
diff --git a/fs/binfmt_elf.c b/fs/binfmt_elf.c
index b9b3bb5..5e27303 100644
--- a/fs/binfmt_elf.c
+++ b/fs/binfmt_elf.c
@@ -45,7 +45,7 @@ static unsigned long elf_map(struct file *, unsigned long, struct elf_phdr *,
  * don't even try.
  */
 #if defined(USE_ELF_CORE_DUMP) && defined(CONFIG_ELF_CORE)
-static int elf_core_dump(long signr, struct pt_regs *regs, struct file *file, unsigned long limit);
+static int elf_core_dump(struct coredump_params *cprm);
 #else
 #define elf_core_dump	NULL
 #endif
@@ -1277,10 +1277,6 @@ static int writenote(struct memelfnote *men, struct file *file,
 }
 #undef DUMP_WRITE
 
-#define DUMP_WRITE(addr, nr)	\
-	if ((size += (nr)) > limit || !dump_write(file, (addr), (nr))) \
-		goto end_coredump;
-
 static void fill_elf_header(struct elfhdr *elf, int segs,
 			    u16 machine, u32 flags, u8 osabi)
 {
@@ -1906,7 +1902,7 @@ static struct vm_area_struct *next_vma(struct vm_area_struct *this_vma,
  * and then they are actually written out.  If we run out of core limit
  * we just truncate.
  */
-static int elf_core_dump(long signr, struct pt_regs *regs, struct file *file, unsigned long limit)
+static int elf_core_dump(struct coredump_params *cprm)
 {
 	int has_dumped = 0;
 	mm_segment_t fs;
@@ -1952,7 +1948,7 @@ static int elf_core_dump(long signr, struct pt_regs *regs, struct file *file, un
 	 * notes.  This also sets up the file header.
 	 */
 	if (!fill_note_info(elf, segs + 1, /* including notes section */
-			    &info, signr, regs))
+			    &info, cprm->signr, cprm->regs))
 		goto cleanup;
 
 	has_dumped = 1;
@@ -1961,7 +1957,10 @@ static int elf_core_dump(long signr, struct pt_regs *regs, struct file *file, un
 	fs = get_fs();
 	set_fs(KERNEL_DS);
 
-	DUMP_WRITE(elf, sizeof(*elf));
+	size += sizeof(*elf);
+	if (size > cprm->limit || !dump_write(cprm->file, elf, sizeof(*elf)))
+		goto end_coredump;
+
 	offset += sizeof(*elf);				/* Elf header */
 	offset += (segs + 1) * sizeof(struct elf_phdr); /* Program headers */
 	foffset = offset;
@@ -1975,7 +1974,10 @@ static int elf_core_dump(long signr, struct pt_regs *regs, struct file *file, un
 
 		fill_elf_note_phdr(&phdr, sz, offset);
 		offset += sz;
-		DUMP_WRITE(&phdr, sizeof(phdr));
+		size += sizeof(phdr);
+		if (size > cprm->limit ||
+		    !dump_write(cprm->file, &phdr, sizeof(phdr)))
+			goto end_coredump;
 	}
 
 	dataoff = offset = roundup(offset, ELF_EXEC_PAGESIZE);
@@ -2006,7 +2008,10 @@ static int elf_core_dump(long signr, struct pt_regs *regs, struct file *file, un
 			phdr.p_flags |= PF_X;
 		phdr.p_align = ELF_EXEC_PAGESIZE;
 
-		DUMP_WRITE(&phdr, sizeof(phdr));
+		size += sizeof(phdr);
+		if (size > cprm->limit ||
+		    !dump_write(cprm->file, &phdr, sizeof(phdr)))
+			goto end_coredump;
 	}
 
 #ifdef ELF_CORE_WRITE_EXTRA_PHDRS
@@ -2014,14 +2019,14 @@ static int elf_core_dump(long signr, struct pt_regs *regs, struct file *file, un
 #endif
 
  	/* write out the notes section */
-	if (!write_note_info(&info, file, &foffset))
+	if (!write_note_info(&info, cprm->file, &foffset))
 		goto end_coredump;
 
-	if (elf_coredump_extra_notes_write(file, &foffset))
+	if (elf_coredump_extra_notes_write(cprm->file, &foffset))
 		goto end_coredump;
 
 	/* Align to page */
-	if (!dump_seek(file, dataoff - foffset))
+	if (!dump_seek(cprm->file, dataoff - foffset))
 		goto end_coredump;
 
 	for (vma = first_vma(current, gate_vma); vma != NULL;
@@ -2038,12 +2043,13 @@ static int elf_core_dump(long signr, struct pt_regs *regs, struct file *file, un
 			page = get_dump_page(addr);
 			if (page) {
 				void *kaddr = kmap(page);
-				stop = ((size += PAGE_SIZE) > limit) ||
-					!dump_write(file, kaddr, PAGE_SIZE);
+				stop = ((size += PAGE_SIZE) > cprm->limit) ||
+					!dump_write(cprm->file, kaddr,
+						    PAGE_SIZE);
 				kunmap(page);
 				page_cache_release(page);
 			} else
-				stop = !dump_seek(file, PAGE_SIZE);
+				stop = !dump_seek(cprm->file, PAGE_SIZE);
 			if (stop)
 				goto end_coredump;
 		}
diff --git a/fs/binfmt_elf_fdpic.c b/fs/binfmt_elf_fdpic.c
index 38502c6..e65ab9d 100644
--- a/fs/binfmt_elf_fdpic.c
+++ b/fs/binfmt_elf_fdpic.c
@@ -76,7 +76,7 @@ static int elf_fdpic_map_file_by_direct_mmap(struct elf_fdpic_params *,
 					     struct file *, struct mm_struct *);
 
 #if defined(USE_ELF_CORE_DUMP) && defined(CONFIG_ELF_CORE)
-static int elf_fdpic_core_dump(long, struct pt_regs *, struct file *, unsigned long limit);
+static int elf_fdpic_core_dump(struct coredump_params *cprm);
 #endif
 
 static struct linux_binfmt elf_fdpic_format = {
@@ -1581,8 +1581,7 @@ static int elf_fdpic_dump_segments(struct file *file, size_t *size,
  * and then they are actually written out.  If we run out of core limit
  * we just truncate.
  */
-static int elf_fdpic_core_dump(long signr, struct pt_regs *regs,
-			       struct file *file, unsigned long limit)
+static int elf_fdpic_core_dump(struct coredump_params *cprm)
 {
 #define	NUM_NOTES	6
 	int has_dumped = 0;
@@ -1641,7 +1640,7 @@ static int elf_fdpic_core_dump(long signr, struct pt_regs *regs,
 		goto cleanup;
 #endif
 
-	if (signr) {
+	if (cprm->signr) {
 		struct core_thread *ct;
 		struct elf_thread_status *tmp;
 
@@ -1660,14 +1659,14 @@ static int elf_fdpic_core_dump(long signr, struct pt_regs *regs,
 			int sz;
 
 			tmp = list_entry(t, struct elf_thread_status, list);
-			sz = elf_dump_thread_status(signr, tmp);
+			sz = elf_dump_thread_status(cprm->signr, tmp);
 			thread_status_size += sz;
 		}
 	}
 
 	/* now collect the dump for the current */
-	fill_prstatus(prstatus, current, signr);
-	elf_core_copy_regs(&prstatus->pr_reg, regs);
+	fill_prstatus(prstatus, current, cprm->signr);
+	elf_core_copy_regs(&prstatus->pr_reg, cprm->regs);
 
 	segs = current->mm->map_count;
 #ifdef ELF_CORE_EXTRA_PHDRS
@@ -1702,7 +1701,7 @@ static int elf_fdpic_core_dump(long signr, struct pt_regs *regs,
 
   	/* Try to dump the FPU. */
 	if ((prstatus->pr_fpvalid =
-	     elf_core_copy_task_fpregs(current, regs, fpu)))
+	     elf_core_copy_task_fpregs(current, cprm->regs, fpu)))
 		fill_note(notes + numnote++,
 			  "CORE", NT_PRFPREG, sizeof(*fpu), fpu);
 #ifdef ELF_CORE_COPY_XFPREGS
@@ -1773,7 +1772,7 @@ static int elf_fdpic_core_dump(long signr, struct pt_regs *regs,
 
  	/* write out the notes section */
 	for (i = 0; i < numnote; i++)
-		if (!writenote(notes + i, file))
+		if (!writenote(notes + i, cprm->file))
 			goto end_coredump;
 
 	/* write out the thread status notes section */
@@ -1782,25 +1781,26 @@ static int elf_fdpic_core_dump(long signr, struct pt_regs *regs,
 				list_entry(t, struct elf_thread_status, list);
 
 		for (i = 0; i < tmp->num_notes; i++)
-			if (!writenote(&tmp->notes[i], file))
+			if (!writenote(&tmp->notes[i], cprm->file))
 				goto end_coredump;
 	}
 
-	if (!dump_seek(file, dataoff))
+	if (!dump_seek(cprm->file, dataoff))
 		goto end_coredump;
 
-	if (elf_fdpic_dump_segments(file, &size, &limit, mm_flags) < 0)
+	if (elf_fdpic_dump_segments(cprm->file, &size, &cprm->limit,
+				    mm_flags) < 0)
 		goto end_coredump;
 
 #ifdef ELF_CORE_WRITE_EXTRA_DATA
 	ELF_CORE_WRITE_EXTRA_DATA;
 #endif
 
-	if (file->f_pos != offset) {
+	if (cprm->file->f_pos != offset) {
 		/* Sanity check */
 		printk(KERN_WARNING
 		       "elf_core_dump: file->f_pos (%lld) != offset (%lld)\n",
-		       file->f_pos, offset);
+		       cprm->file->f_pos, offset);
 	}
 
 end_coredump:
diff --git a/fs/binfmt_flat.c b/fs/binfmt_flat.c
index a279665..d4a00ea 100644
--- a/fs/binfmt_flat.c
+++ b/fs/binfmt_flat.c
@@ -87,7 +87,7 @@ static int load_flat_shared_library(int id, struct lib_info *p);
 #endif
 
 static int load_flat_binary(struct linux_binprm *, struct pt_regs * regs);
-static int flat_core_dump(long signr, struct pt_regs *regs, struct file *file, unsigned long limit);
+static int flat_core_dump(struct coredump_params *cprm);
 
 static struct linux_binfmt flat_format = {
 	.module		= THIS_MODULE,
@@ -102,10 +102,10 @@ static struct linux_binfmt flat_format = {
  * Currently only a stub-function.
  */
 
-static int flat_core_dump(long signr, struct pt_regs *regs, struct file *file, unsigned long limit)
+static int flat_core_dump(struct coredump_params *cprm)
 {
 	printk("Process %s:%d received signr %d and should have core dumped\n",
-			current->comm, current->pid, (int) signr);
+			current->comm, current->pid, (int) cprm->signr);
 	return(1);
 }
 
diff --git a/fs/binfmt_som.c b/fs/binfmt_som.c
index eff74b9..2a9b533 100644
--- a/fs/binfmt_som.c
+++ b/fs/binfmt_som.c
@@ -43,7 +43,7 @@ static int load_som_library(struct file *);
  * don't even try.
  */
 #if 0
-static int som_core_dump(long signr, struct pt_regs *regs, unsigned long limit);
+static int som_core_dump(struct coredump_params *cprm);
 #else
 #define som_core_dump	NULL
 #endif
diff --git a/fs/exec.c b/fs/exec.c
index ba112bd..5daf7d5 100644
--- a/fs/exec.c
+++ b/fs/exec.c
@@ -1756,17 +1756,20 @@ void do_coredump(long signr, int exit_code, struct pt_regs *regs)
 	struct mm_struct *mm = current->mm;
 	struct linux_binfmt * binfmt;
 	struct inode * inode;
-	struct file * file;
 	const struct cred *old_cred;
 	struct cred *cred;
 	int retval = 0;
 	int flag = 0;
 	int ispipe = 0;
-	unsigned long core_limit = current->signal->rlim[RLIMIT_CORE].rlim_cur;
 	char **helper_argv = NULL;
 	int helper_argc = 0;
 	int dump_count = 0;
 	static atomic_t core_dump_count = ATOMIC_INIT(0);
+	struct coredump_params cprm = {
+		.signr = signr,
+		.regs = regs,
+		.limit = current->signal->rlim[RLIMIT_CORE].rlim_cur,
+	};
 
 	audit_core_dumps(signr);
 
@@ -1822,15 +1825,15 @@ void do_coredump(long signr, int exit_code, struct pt_regs *regs)
 	ispipe = format_corename(corename, signr);
 	unlock_kernel();
 
-	if ((!ispipe) && (core_limit < binfmt->min_coredump))
+	if ((!ispipe) && (cprm.limit < binfmt->min_coredump))
 		goto fail_unlock;
 
  	if (ispipe) {
-		if (core_limit == 0) {
+		if (cprm.limit == 0) {
 			/*
 			 * Normally core limits are irrelevant to pipes, since
 			 * we're not writing to the file system, but we use
-			 * core_limit of 0 here as a speacial value. Any
+			 * cprm.limit of 0 here as a speacial value. Any
 			 * non-zero limit gets set to RLIM_INFINITY below, but
 			 * a limit of 0 skips the dump.  This is a consistent
 			 * way to catch recursive crashes.  We can still crash
@@ -1863,25 +1866,25 @@ void do_coredump(long signr, int exit_code, struct pt_regs *regs)
 			goto fail_dropcount;
 		}
 
-		core_limit = RLIM_INFINITY;
+		cprm.limit = RLIM_INFINITY;
 
 		/* SIGPIPE can happen, but it's just never processed */
 		if (call_usermodehelper_pipe(helper_argv[0], helper_argv, NULL,
-				&file)) {
+				&cprm.file)) {
  			printk(KERN_INFO "Core dump to %s pipe failed\n",
 			       corename);
 			goto fail_dropcount;
  		}
  	} else
- 		file = filp_open(corename,
+		cprm.file = filp_open(corename,
 				 O_CREAT | 2 | O_NOFOLLOW | O_LARGEFILE | flag,
 				 0600);
-	if (IS_ERR(file))
+	if (IS_ERR(cprm.file))
 		goto fail_dropcount;
-	inode = file->f_path.dentry->d_inode;
+	inode = cprm.file->f_path.dentry->d_inode;
 	if (inode->i_nlink > 1)
 		goto close_fail;	/* multiple links - don't dump */
-	if (!ispipe && d_unhashed(file->f_path.dentry))
+	if (!ispipe && d_unhashed(cprm.file->f_path.dentry))
 		goto close_fail;
 
 	/* AK: actually i see no reason to not allow this for named pipes etc.,
@@ -1894,21 +1897,22 @@ void do_coredump(long signr, int exit_code, struct pt_regs *regs)
 	 */
 	if (inode->i_uid != current_fsuid())
 		goto close_fail;
-	if (!file->f_op)
+	if (!cprm.file->f_op)
 		goto close_fail;
-	if (!file->f_op->write)
+	if (!cprm.file->f_op->write)
 		goto close_fail;
-	if (!ispipe && do_truncate(file->f_path.dentry, 0, 0, file) != 0)
+	if (!ispipe &&
+	    do_truncate(cprm.file->f_path.dentry, 0, 0, cprm.file) != 0)
 		goto close_fail;
 
-	retval = binfmt->core_dump(signr, regs, file, core_limit);
+	retval = binfmt->core_dump(&cprm);
 
 	if (retval)
 		current->signal->group_exit_code |= 0x80;
 close_fail:
 	if (ispipe && core_pipe_limit)
-		wait_for_dump_helpers(file);
-	filp_close(file, NULL);
+		wait_for_dump_helpers(cprm.file);
+	filp_close(cprm.file, NULL);
 fail_dropcount:
 	if (dump_count)
 		atomic_dec(&core_dump_count);
diff --git a/include/linux/binfmts.h b/include/linux/binfmts.h
index aece486..cd4349b 100644
--- a/include/linux/binfmts.h
+++ b/include/linux/binfmts.h
@@ -68,6 +68,14 @@ struct linux_binprm{
 
 #define BINPRM_MAX_RECURSION 4
 
+/* Function parameter for binfmt->coredump */
+struct coredump_params {
+	long signr;
+	struct pt_regs *regs;
+	struct file *file;
+	unsigned long limit;
+};
+
 /*
  * This structure defines the functions that are used to load the binary formats that
  * linux accepts.
@@ -77,7 +85,7 @@ struct linux_binfmt {
 	struct module *module;
 	int (*load_binary)(struct linux_binprm *, struct  pt_regs * regs);
 	int (*load_shlib)(struct file *);
-	int (*core_dump)(long signr, struct pt_regs *regs, struct file *file, unsigned long limit);
+	int (*core_dump)(struct coredump_params *cprm);
 	unsigned long min_coredump;	/* minimal dump size */
 	int hasvdso;
 };


-- 
Masami Hiramatsu

Software Engineer
Hitachi Computer Products (America), Inc.
Software Solutions Division

e-mail: mhi...@re...

[Dle-develop] [PATCH -tip 1/3] tracepoint: Move signal sending tracepoint to events/signal.h

[Masami H. <mhi...@re...>]

Move signal sending event to events/signal.h. This patch also renames
sched_signal_send event to signal_generate.

Signed-off-by: Masami Hiramatsu <mhi...@re...>
Cc: Oleg Nesterov <ol...@re...>
Cc: Roland McGrath <ro...@re...>
Cc: Ingo Molnar <mi...@el...>
---

 include/trace/events/sched.h  |   25 -------------------------
 include/trace/events/signal.h |   38 ++++++++++++++++++++++++++++++++++++++
 kernel/signal.c               |    5 +++--
 3 files changed, 41 insertions(+), 27 deletions(-)
 create mode 100644 include/trace/events/signal.h

diff --git a/include/trace/events/sched.h b/include/trace/events/sched.h
index b50b985..b221bb3 100644
--- a/include/trace/events/sched.h
+++ b/include/trace/events/sched.h
@@ -320,31 +320,6 @@ TRACE_EVENT(sched_process_fork,
 );
 
 /*
- * Tracepoint for sending a signal:
- */
-TRACE_EVENT(sched_signal_send,
-
-	TP_PROTO(int sig, struct task_struct *p),
-
-	TP_ARGS(sig, p),
-
-	TP_STRUCT__entry(
-		__field(	int,	sig			)
-		__array(	char,	comm,	TASK_COMM_LEN	)
-		__field(	pid_t,	pid			)
-	),
-
-	TP_fast_assign(
-		memcpy(__entry->comm, p->comm, TASK_COMM_LEN);
-		__entry->pid	= p->pid;
-		__entry->sig	= sig;
-	),
-
-	TP_printk("sig=%d comm=%s pid=%d",
-		  __entry->sig, __entry->comm, __entry->pid)
-);
-
-/*
  * XXX the below sched_stat tracepoints only apply to SCHED_OTHER/BATCH/IDLE
  *     adding sched_stat support to SCHED_FIFO/RR would be welcome.
  */
diff --git a/include/trace/events/signal.h b/include/trace/events/signal.h
new file mode 100644
index 0000000..c7817f3
--- /dev/null
+++ b/include/trace/events/signal.h
@@ -0,0 +1,38 @@
+#undef TRACE_SYSTEM
+#define TRACE_SYSTEM signal
+
+#if !defined(_TRACE_SIGNAL_H) || defined(TRACE_HEADER_MULTI_READ)
+#define _TRACE_SIGNAL_H
+
+#include <linux/sched.h>
+#include <linux/tracepoint.h>
+
+/*
+ * Tracepoint for generating a signal:
+ */
+TRACE_EVENT(signal_generate,
+
+	TP_PROTO(int sig, struct task_struct *p),
+
+	TP_ARGS(sig, p),
+
+	TP_STRUCT__entry(
+		__field(	int,	sig			)
+		__array(	char,	comm,	TASK_COMM_LEN	)
+		__field(	pid_t,	pid			)
+	),
+
+	TP_fast_assign(
+		memcpy(__entry->comm, p->comm, TASK_COMM_LEN);
+		__entry->pid	= p->pid;
+		__entry->sig	= sig;
+	),
+
+	TP_printk("sig=%d comm=%s pid=%d",
+		  __entry->sig, __entry->comm, __entry->pid)
+);
+
+#endif /* _TRACE_SIGNAL_H */
+
+/* This part must be outside protection */
+#include <trace/define_trace.h>
diff --git a/kernel/signal.c b/kernel/signal.c
index fe08008..82ab077 100644
--- a/kernel/signal.c
+++ b/kernel/signal.c
@@ -28,7 +28,8 @@
 #include <linux/freezer.h>
 #include <linux/pid_namespace.h>
 #include <linux/nsproxy.h>
-#include <trace/events/sched.h>
+#define CREATE_TRACE_POINTS
+#include <trace/events/signal.h>
 
 #include <asm/param.h>
 #include <asm/uaccess.h>
@@ -856,7 +857,7 @@ static int __send_signal(int sig, struct siginfo *info, struct task_struct *t,
 	struct sigqueue *q;
 	int override_rlimit;
 
-	trace_sched_signal_send(sig, t);
+	trace_signal_generate(sig, t);
 
 	assert_spin_locked(&t->sighand->siglock);
 


-- 
Masami Hiramatsu

Software Engineer
Hitachi Computer Products (America), Inc.
Software Solutions Division

e-mail: mhi...@re...

[Dle-develop] [PATCH -tip 3/3] tracepoint: Add signal loss events

[Masami H. <mhi...@re...>]

Add signal_overflow_fail and signal_lose_info tracepoints
for signal-lost events.

Signed-off-by: Masami Hiramatsu <mhi...@re...>
Suggested-by: Roland McGrath <ro...@re...>
Cc: Oleg Nesterov <ol...@re...>
Cc: Ingo Molnar <mi...@el...>
---

 include/trace/events/signal.h |   71 +++++++++++++++++++++++++++++++++++++++++
 kernel/signal.c               |   19 ++++++++---
 2 files changed, 85 insertions(+), 5 deletions(-)

diff --git a/include/trace/events/signal.h b/include/trace/events/signal.h
index 6dfd88d..eb57285 100644
--- a/include/trace/events/signal.h
+++ b/include/trace/events/signal.h
@@ -4,6 +4,7 @@
 #if !defined(_TRACE_SIGNAL_H) || defined(TRACE_HEADER_MULTI_READ)
 #define _TRACE_SIGNAL_H
 
+#include <linux/signal.h>
 #include <linux/sched.h>
 #include <linux/tracepoint.h>
 
@@ -56,6 +57,76 @@ TRACE_EVENT(signal_deliver,
 	TP_printk("sig=%d sa_handler=%lx sa_flags=%lx",
 		  __entry->sig, __entry->sa_handler, __entry->sa_flags)
 );
+
+/*
+ * Tracepoint for losing a signal by overflow:
+ */
+TRACE_EVENT(signal_overflow_fail,
+
+	TP_PROTO(int sig, int group, struct siginfo *info),
+
+	TP_ARGS(sig, group, info),
+
+	TP_STRUCT__entry(
+		__field(	int,	sig	)
+		__field(	int,	group	)
+		__field(	int,	errno	)
+		__field(	int,	code	)
+	),
+
+	TP_fast_assign(
+		__entry->sig	= sig;
+		__entry->group	= group;
+		if (info == SEND_SIG_NOINFO) {
+			__entry->errno	= 0;
+			__entry->code	= SI_USER;
+		} else if (info == SEND_SIG_PRIV) {
+			__entry->errno	= 0;
+			__entry->code	= SI_KERNEL;
+		} else {
+			__entry->errno	= info->si_errno;
+			__entry->code	= info->si_code;
+		}
+	),
+
+	TP_printk("sig=%d group=%d errno=%d code=%d",
+		  __entry->sig, __entry->group, __entry->errno, __entry->code)
+);
+
+/*
+ * Tracepoint for losing a siginfo:
+ */
+TRACE_EVENT(signal_lose_info,
+
+	TP_PROTO(int sig, int group, struct siginfo *info),
+
+	TP_ARGS(sig, group, info),
+
+	TP_STRUCT__entry(
+		__field(	int,	sig	)
+		__field(	int,	group	)
+		__field(	int,	errno	)
+		__field(	int,	code	)
+	),
+
+	TP_fast_assign(
+		__entry->sig	= sig;
+		__entry->group	= group;
+		if (info == SEND_SIG_NOINFO) {
+			__entry->errno	= 0;
+			__entry->code	= SI_USER;
+		} else if (info == SEND_SIG_PRIV) {
+			__entry->errno	= 0;
+			__entry->code	= SI_KERNEL;
+		} else {
+			__entry->errno	= info->si_errno;
+			__entry->code	= info->si_code;
+		}
+	),
+
+	TP_printk("sig=%d group=%d errno=%d code=%d",
+		  __entry->sig, __entry->group, __entry->errno, __entry->code)
+);
 #endif /* _TRACE_SIGNAL_H */
 
 /* This part must be outside protection */
diff --git a/kernel/signal.c b/kernel/signal.c
index 5615f41..65f67c5 100644
--- a/kernel/signal.c
+++ b/kernel/signal.c
@@ -919,12 +919,21 @@ static int __send_signal(int sig, struct siginfo *info, struct task_struct *t,
 			break;
 		}
 	} else if (!is_si_special(info)) {
-		if (sig >= SIGRTMIN && info->si_code != SI_USER)
-		/*
-		 * Queue overflow, abort.  We may abort if the signal was rt
-		 * and sent by user using something other than kill().
-		 */
+		if (sig >= SIGRTMIN && info->si_code != SI_USER) {
+			/*
+			 * Queue overflow, abort.  We may abort if the
+			 * signal was rt and sent by user using something
+			 * other than kill().
+			 */
+			trace_signal_overflow_fail(sig, group, info);
 			return -EAGAIN;
+		} else {
+			/*
+			 * This is a silent loss of information.  We still
+			 * send the signal, but the *info bits are lost.
+			 */
+			trace_signal_lose_info(sig, group, info);
+		}
 	}
 
 out_set:


-- 
Masami Hiramatsu

Software Engineer
Hitachi Computer Products (America), Inc.
Software Solutions Division

e-mail: mhi...@re...

[Dle-develop] [PATCH -tip 0/3] tracepoint: Add signal events

[Masami H. <mhi...@re...>]

Hi,

These patches add signal related tracepoints including
signal generation, delivery, and loss. First patch also
moves signal-sending tracepoint from events/sched.h to
events/signal.h.

TODO:
- Add signal-wakeup event?

Thank you,

---

Masami Hiramatsu (3):
      tracepoint: Add signal loss events
      tracepoint: Add signal deliver event
      tracepoint: Move signal sending tracepoint to events/signal.h


 include/trace/events/sched.h  |   25 --------
 include/trace/events/signal.h |  133 +++++++++++++++++++++++++++++++++++++++++
 kernel/signal.c               |   27 ++++++--
 3 files changed, 153 insertions(+), 32 deletions(-)
 create mode 100644 include/trace/events/signal.h

-- 
Masami Hiramatsu

Software Engineer
Hitachi Computer Products (America), Inc.
Software Solutions Division
e-mail: mhi...@re...

Re: [Dle-develop] [PATCH -tip 3/3] Add get_signal tracepoint

[Masami H. <mhi...@re...>]

Ingo Molnar wrote:
> 
> * Masami Hiramatsu <mhi...@re...> wrote:
> 
>>>  - signal IPI/wakeup events
>>
>> All signals might be used for IPI, isn't it? :-)
> 
> I mean, to analyze the various dynamic delivery details of how a signal 
> send affects a target task:
> 
>  1) which task/PID was selected to be woken
> 
>  2) if the task got woken (from sleep) due to the signal sending
> 
>  3) if it was already woken, whether it needed an IPI via kick_process()

Hmm, as far as I can see, some of these events can be caught by
sched layer too.
- trace_signal_send() will record target task.
- wake_up_state() just calls try_to_wake_up(), and trace_sched_wakeup()
  will be called from it.
- kick_process() might better have its own tracepoint.

And also, I think signal_wake_up() might not be a good tracepoint for
signal event, since there is no signr. Moreover some signal_wake_up()
caller(e.g. recalc_sigpending*) silently wake up processes :-(.

> What proportion of signals were wakeups and what proportion hit an 
> already running task is a relevant question to ask when analyzing the 
> performance characteristics of signals.

Hmm, does it really require wakeup events in signal layer?
I think that we can analyze the characteristics by combination
of signal events and sched events.

Thank you,
-- 
Masami Hiramatsu

Software Engineer
Hitachi Computer Products (America), Inc.
Software Solutions Division

e-mail: mhi...@re...

Re: [Dle-develop] [PATCH -next 0/3] x86 insn decoder test updates (Re: linux-next: Tree for October 29 (x86 posttest))

[Masami H. <mhi...@re...>]

Ingo Molnar wrote:
> 
> * Masami Hiramatsu <mhi...@re...> wrote:
> 
>> Here are the patches which update x86 instruction decoder build-time 
>> test. As Stephen reported on linux-next, sometimes objdump decodes bad 
>> instructions as normal. This will cause a false positive result on x86 
>> insn decoder test. This patches update the test as below;
>>
>>  - Show more information with V=1
>>  - Show in which symbol the difference places.
>>  - Just warning instead of build failure.
> 
> yes, -tip testing was showing such build bugs too:
> 
>  Error: ffffffff8104aae3:	c5 83 3d 49 80 ee    	lds    0xffffffffee80493d(%rbx),%eax
>  Error: objdump says 6 bytes, but insn_get_length() says 3 (attr:0)
> 
> it happens with older tools, such as binutils-2.17. Modern binutils 
> (2.19) is fine.

Thank you for telling me.

> We dont want to remove the build error: it helped us fix a number of 
> real bugs in the decoder - instead please try to create a make based 
> workaround based on binutils, to not run the test with binutils older 
> than 2.19 or so.

OK, that's fine for me.

Thank you,
-- 
Masami Hiramatsu

Software Engineer
Hitachi Computer Products (America), Inc.
Software Solutions Division

e-mail: mhi...@re...

[Dle-develop] [tip:perf/probes] x86: Show symbol name if insn decoder test failed

[tip-bot f. M. H. <mhi...@re...>]

Commit-ID:  35039eb6b199749943547c8572be6604edf00229
Gitweb:     http://git.kernel.org/tip/35039eb6b199749943547c8572be6604edf00229
Author:     Masami Hiramatsu <mhi...@re...>
AuthorDate: Mon, 16 Nov 2009 18:06:24 -0500
Committer:  Ingo Molnar <mi...@el...>
CommitDate: Tue, 17 Nov 2009 07:16:50 +0100

x86: Show symbol name if insn decoder test failed

Show symbol name if insn decoder test find a difference.
This will help us to find out where the issue is.

Signed-off-by: Masami Hiramatsu <mhi...@re...>
Cc: systemtap <sys...@so...>
Cc: DLE <dle...@li...>
Cc: Stephen Rothwell <sf...@ca...>
Cc: Randy Dunlap <rd...@xe...>
Cc: Jim Keniston <jke...@us...>
Cc: Stephen Rothwell <sf...@ca...>
LKML-Reference: <20091116230624.5250.49813.stgit@harusame>
Signed-off-by: Ingo Molnar <mi...@el...>
---
 arch/x86/tools/distill.awk    |    5 +++++
 arch/x86/tools/test_get_len.c |   10 +++++++++-
 2 files changed, 14 insertions(+), 1 deletions(-)

diff --git a/arch/x86/tools/distill.awk b/arch/x86/tools/distill.awk
index d433619..c13c0ee 100644
--- a/arch/x86/tools/distill.awk
+++ b/arch/x86/tools/distill.awk
@@ -15,6 +15,11 @@ BEGIN {
 	fwait_str="9b\tfwait"
 }
 
+/^ *[0-9a-f]+ <[^>]*>:/ {
+	# Symbol entry
+	printf("%s%s\n", $2, $1)
+}
+
 /^ *[0-9a-f]+:/ {
 	if (split($0, field, "\t") < 3) {
 		# This is a continuation of the same insn.
diff --git a/arch/x86/tools/test_get_len.c b/arch/x86/tools/test_get_len.c
index 5743e51..af75e07 100644
--- a/arch/x86/tools/test_get_len.c
+++ b/arch/x86/tools/test_get_len.c
@@ -110,7 +110,7 @@ static void parse_args(int argc, char **argv)
 
 int main(int argc, char **argv)
 {
-	char line[BUFSIZE];
+	char line[BUFSIZE], sym[BUFSIZE] = "<unknown>";
 	unsigned char insn_buf[16];
 	struct insn insn;
 	int insns = 0, c;
@@ -122,6 +122,12 @@ int main(int argc, char **argv)
 		int nb = 0;
 		unsigned int b;
 
+		if (line[0] == '<') {
+			/* Symbol line */
+			strcpy(sym, line);
+			continue;
+		}
+
 		insns++;
 		memset(insn_buf, 0, 16);
 		strcpy(copy, line);
@@ -145,6 +151,8 @@ int main(int argc, char **argv)
 		insn_init(&insn, insn_buf, x86_64);
 		insn_get_length(&insn);
 		if (insn.length != nb) {
+			fprintf(stderr, "Error: %s found a difference at %s\n",
+				prog, sym);
 			fprintf(stderr, "Error: %s", line);
 			fprintf(stderr, "Error: objdump says %d bytes, but "
 				"insn_get_length() says %d\n", nb,

[Dle-develop] [tip:perf/probes] x86: Add verbose option to insn decoder test

[tip-bot f. M. H. <mhi...@re...>]

Commit-ID:  d65ff75fbe6f8ac7c17f18e4108521898468822c
Gitweb:     http://git.kernel.org/tip/d65ff75fbe6f8ac7c17f18e4108521898468822c
Author:     Masami Hiramatsu <mhi...@re...>
AuthorDate: Mon, 16 Nov 2009 18:06:18 -0500
Committer:  Ingo Molnar <mi...@el...>
CommitDate: Tue, 17 Nov 2009 07:16:48 +0100

x86: Add verbose option to insn decoder test

Add verbose option to insn decoder test. This dumps decoded
instruction when building kernel with V=1.

Signed-off-by: Masami Hiramatsu <mhi...@re...>
Cc: systemtap <sys...@so...>
Cc: DLE <dle...@li...>
Cc: Stephen Rothwell <sf...@ca...>
Cc: Randy Dunlap <rd...@xe...>
Cc: Jim Keniston <jke...@us...>
Cc: Stephen Rothwell <sf...@ca...>
LKML-Reference: <20091116230618.5250.18762.stgit@harusame>
Signed-off-by: Ingo Molnar <mi...@el...>
---
 arch/x86/tools/Makefile       |    9 ++++-
 arch/x86/tools/test_get_len.c |   74 ++++++++++++++++++++++++++++++++++------
 2 files changed, 71 insertions(+), 12 deletions(-)

diff --git a/arch/x86/tools/Makefile b/arch/x86/tools/Makefile
index 5e295d9..4688f90 100644
--- a/arch/x86/tools/Makefile
+++ b/arch/x86/tools/Makefile
@@ -1,6 +1,13 @@
 PHONY += posttest
+
+ifeq ($(KBUILD_VERBOSE),1)
+  postest_verbose = -v
+else
+  postest_verbose =
+endif
+
 quiet_cmd_posttest = TEST    $@
-      cmd_posttest = $(OBJDUMP) -d -j .text $(objtree)/vmlinux | awk -f $(srctree)/arch/x86/tools/distill.awk | $(obj)/test_get_len $(CONFIG_64BIT)
+      cmd_posttest = $(OBJDUMP) -d -j .text $(objtree)/vmlinux | awk -f $(srctree)/arch/x86/tools/distill.awk | $(obj)/test_get_len -$(CONFIG_64BIT) $(posttest_verbose)
 
 posttest: $(obj)/test_get_len vmlinux
 	$(call cmd,posttest)
diff --git a/arch/x86/tools/test_get_len.c b/arch/x86/tools/test_get_len.c
index 376d338..5743e51 100644
--- a/arch/x86/tools/test_get_len.c
+++ b/arch/x86/tools/test_get_len.c
@@ -20,6 +20,7 @@
 #include <stdio.h>
 #include <string.h>
 #include <assert.h>
+#include <unistd.h>
 
 #define unlikely(cond) (cond)
 
@@ -36,11 +37,16 @@
  */
 
 const char *prog;
+static int verbose;
+static int x86_64;
 
 static void usage(void)
 {
 	fprintf(stderr, "Usage: objdump -d a.out | awk -f distill.awk |"
-		" %s [y|n](64bit flag)\n", prog);
+		" %s [-y|-n] [-v] \n", prog);
+	fprintf(stderr, "\t-y	64bit mode\n");
+	fprintf(stderr, "\t-n	32bit mode\n");
+	fprintf(stderr, "\t-v	verbose mode\n");
 	exit(1);
 }
 
@@ -50,6 +56,56 @@ static void malformed_line(const char *line, int line_nr)
 	exit(3);
 }
 
+static void dump_field(FILE *fp, const char *name, const char *indent,
+		       struct insn_field *field)
+{
+	fprintf(fp, "%s.%s = {\n", indent, name);
+	fprintf(fp, "%s\t.value = %d, bytes[] = {%x, %x, %x, %x},\n",
+		indent, field->value, field->bytes[0], field->bytes[1],
+		field->bytes[2], field->bytes[3]);
+	fprintf(fp, "%s\t.got = %d, .nbytes = %d},\n", indent,
+		field->got, field->nbytes);
+}
+
+static void dump_insn(FILE *fp, struct insn *insn)
+{
+	fprintf(fp, "Instruction = { \n");
+	dump_field(fp, "prefixes", "\t",	&insn->prefixes);
+	dump_field(fp, "rex_prefix", "\t",	&insn->rex_prefix);
+	dump_field(fp, "vex_prefix", "\t",	&insn->vex_prefix);
+	dump_field(fp, "opcode", "\t",		&insn->opcode);
+	dump_field(fp, "modrm", "\t",		&insn->modrm);
+	dump_field(fp, "sib", "\t",		&insn->sib);
+	dump_field(fp, "displacement", "\t",	&insn->displacement);
+	dump_field(fp, "immediate1", "\t",	&insn->immediate1);
+	dump_field(fp, "immediate2", "\t",	&insn->immediate2);
+	fprintf(fp, "\t.attr = %x, .opnd_bytes = %d, .addr_bytes = %d,\n",
+		insn->attr, insn->opnd_bytes, insn->addr_bytes);
+	fprintf(fp, "\t.length = %d, .x86_64 = %d, .kaddr = %p}\n",
+		insn->length, insn->x86_64, insn->kaddr);
+}
+
+static void parse_args(int argc, char **argv)
+{
+	int c;
+	prog = argv[0];
+	while ((c = getopt(argc, argv, "ynv")) != -1) {
+		switch (c) {
+		case 'y':
+			x86_64 = 1;
+			break;
+		case 'n':
+			x86_64 = 0;
+			break;
+		case 'v':
+			verbose = 1;
+			break;
+		default:
+			usage();
+		}
+	}
+}
+
 #define BUFSIZE 256
 
 int main(int argc, char **argv)
@@ -57,15 +113,9 @@ int main(int argc, char **argv)
 	char line[BUFSIZE];
 	unsigned char insn_buf[16];
 	struct insn insn;
-	int insns = 0;
-	int x86_64 = 0;
-
-	prog = argv[0];
-	if (argc > 2)
-		usage();
+	int insns = 0, c;
 
-	if (argc == 2 && argv[1][0] == 'y')
-		x86_64 = 1;
+	parse_args(argc, argv);
 
 	while (fgets(line, BUFSIZE, stdin)) {
 		char copy[BUFSIZE], *s, *tab1, *tab2;
@@ -97,8 +147,10 @@ int main(int argc, char **argv)
 		if (insn.length != nb) {
 			fprintf(stderr, "Error: %s", line);
 			fprintf(stderr, "Error: objdump says %d bytes, but "
-				"insn_get_length() says %d (attr:%x)\n", nb,
-				insn.length, insn.attr);
+				"insn_get_length() says %d\n", nb,
+				insn.length);
+			if (verbose)
+				dump_insn(stderr, &insn);
 			exit(2);
 		}
 	}

Re: [Dle-develop] [PATCH -next 0/3] x86 insn decoder test updates (Re: linux-next: Tree for October 29 (x86 posttest))

[Ingo M. <mi...@el...>]

* Masami Hiramatsu <mhi...@re...> wrote:

> Here are the patches which update x86 instruction decoder build-time 
> test. As Stephen reported on linux-next, sometimes objdump decodes bad 
> instructions as normal. This will cause a false positive result on x86 
> insn decoder test. This patches update the test as below;
>
>  - Show more information with V=1
>  - Show in which symbol the difference places.
>  - Just warning instead of build failure.

yes, -tip testing was showing such build bugs too:

 Error: ffffffff8104aae3:	c5 83 3d 49 80 ee    	lds    0xffffffffee80493d(%rbx),%eax
 Error: objdump says 6 bytes, but insn_get_length() says 3 (attr:0)

it happens with older tools, such as binutils-2.17. Modern binutils 
(2.19) is fine.

We dont want to remove the build error: it helped us fix a number of 
real bugs in the decoder - instead please try to create a make based 
workaround based on binutils, to not run the test with binutils older 
than 2.19 or so.

Thanks,

	Ingo