[REGRESSION] coredumps truncated after "new helper: dump_align()"

[REGRESSION] coredumps truncated after "new helper: dump_align()"

[Peter Wu]

Hi Al,

Somewhere in the merge window of 3.13, coredumps appear truncated.
Instead of 319488 bytes, I get 868 bytes (tested with x86_64 only).

The latest Linus' master (v3.12-9579-g049ffa8) is still affected.
Bisection leads to:

commit 22a8cb8248ba5d340307ba72432253b1dbdb5cf7
Author: Al Viro <viro@zeniv.linux.org.uk>
Date:   Tue Oct 8 11:05:01 2013 -0400

    new helper: dump_align()
    
    dump_skip to given alignment...
    
    Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>

:040000 040000 880920de653955485453c8cc11045bb09e2934f7 f8b348d32b59ae7a16cbc3eb2a6319f4df0210eb M      arch
:040000 040000 eaeffe2378806e95d23b3f3a9dc0fa8f078d6052 88866abde93296a05b4adda869280bc05403936f M      fs
:040000 040000 218263e01dd7473998c54a5a68ed5ae072854d24 ff531dce919862b662fef19a9a049f0de3db38ce M      include

I first noticed this bug on my laptop (v3.12-7033-g42a2d92) and
reproduced this bug in a QEMU virtual machine too. If you want, I
can also provide the git bisect script that starts QEMU and tests
the serial output. That allows a kernel to be verified in two seconds
(minus build times).

Regards,
Peter

Re: [REGRESSION] coredumps truncated after "new helper: dump_align()"

[Al Viro]

On Fri, Nov 15, 2013 at 03:26:10PM +0100, Peter Wu wrote:
> Hi Al,
> 
> Somewhere in the merge window of 3.13, coredumps appear truncated.
> Instead of 319488 bytes, I get 868 bytes (tested with x86_64 only).
> 
> The latest Linus' master (v3.12-9579-g049ffa8) is still affected.
> Bisection leads to:
> 
> commit 22a8cb8248ba5d340307ba72432253b1dbdb5cf7
> Author: Al Viro <viro@zeniv.linux.org.uk>
> Date:   Tue Oct 8 11:05:01 2013 -0400
> 
>     new helper: dump_align()
>     
>     dump_skip to given alignment...

Argh...  Really dumb braino when getting rid of long long division in
dump_align().  Please, see if this gets rid of the entire problem on your
setup - it definitely fixes a bug in there and it seems to restore the
normal behaviour on the reproducer I've got here, but...

I'm going to send that to Linus, along with another coredump fix
(dump_emit() ought to use __kernel_write() instead of vfs_write())
shortly anyway, but if you spot anything still broken...

diff --git a/fs/coredump.c b/fs/coredump.c
index 62406b6..a2856f7 100644
--- a/fs/coredump.c
+++ b/fs/coredump.c
@@ -733,7 +733,7 @@ int dump_align(struct coredump_params *cprm, int align)
 {
 	unsigned mod = cprm->written & (align - 1);
 	if (align & (align - 1))
-		return -EINVAL;
-	return mod ? dump_skip(cprm, align - mod) : 0;
+		return 0;
+	return mod ? dump_skip(cprm, align - mod) : 1;
 }
 EXPORT_SYMBOL(dump_align);

Re: [REGRESSION] coredumps truncated after "new helper: dump_align()"

[Joe Perches]

On Fri, 2013-11-15 at 20:38 +0000, Al Viro wrote:
> On Fri, Nov 15, 2013 at 03:26:10PM +0100, Peter Wu wrote:
> > Somewhere in the merge window of 3.13, coredumps appear truncated.
> > Instead of 319488 bytes, I get 868 bytes (tested with x86_64 only).
> > 
> > The latest Linus' master (v3.12-9579-g049ffa8) is still affected.
> > Bisection leads to:
> > 
> > commit 22a8cb8248ba5d340307ba72432253b1dbdb5cf7

trivia:

> diff --git a/fs/coredump.c b/fs/coredump.c
[]
> @@ -733,7 +733,7 @@ int dump_align(struct coredump_params *cprm, int align)
>  {
>  	unsigned mod = cprm->written & (align - 1);
>  	if (align & (align - 1))
> -		return -EINVAL;
> -	return mod ? dump_skip(cprm, align - mod) : 0;
> +		return 0;
> +	return mod ? dump_skip(cprm, align - mod) : 1;
>  }
>  EXPORT_SYMBOL(dump_align);

extern int dump_skip(struct coredump_params *cprm, size_t nr);
extern int dump_emit(struct coredump_params *cprm, const void *addr, int nr);
extern int dump_align(struct coredump_params *cprm, int align);

Perhaps all of these should return bool not int.
Is there a reason these should be EXPORT_SYMBOL?

Re: [REGRESSION] coredumps truncated after "new helper: dump_align()"

[Peter Wu]

[-- Attachment #1: Type: text/plain, Size: 2297 bytes --]

On Friday 15 November 2013 20:38:38 Al Viro wrote:
> On Fri, Nov 15, 2013 at 03:26:10PM +0100, Peter Wu wrote:
> > Hi Al,
> > 
> > Somewhere in the merge window of 3.13, coredumps appear truncated.
> > Instead of 319488 bytes, I get 868 bytes (tested with x86_64 only).
> > 
> > The latest Linus' master (v3.12-9579-g049ffa8) is still affected.
> > Bisection leads to:
> > 
> > commit 22a8cb8248ba5d340307ba72432253b1dbdb5cf7
> > Author: Al Viro <viro@zeniv.linux.org.uk>
> > Date:   Tue Oct 8 11:05:01 2013 -0400
> > 
> >     new helper: dump_align()
> >     
> >     dump_skip to given alignment...
> 
> Argh...  Really dumb braino when getting rid of long long division in
> dump_align().  Please, see if this gets rid of the entire problem on your
> setup - it definitely fixes a bug in there and it seems to restore the
> normal behaviour on the reproducer I've got here, but...
> 
> I'm going to send that to Linus, along with another coredump fix
> (dump_emit() ought to use __kernel_write() instead of vfs_write())
> shortly anyway, but if you spot anything still broken...
> 
> diff --git a/fs/coredump.c b/fs/coredump.c
> index 62406b6..a2856f7 100644
> --- a/fs/coredump.c
> +++ b/fs/coredump.c
> @@ -733,7 +733,7 @@ int dump_align(struct coredump_params *cprm, int align)
>  {
>  	unsigned mod = cprm->written & (align - 1);
>  	if (align & (align - 1))
> -		return -EINVAL;
> -	return mod ? dump_skip(cprm, align - mod) : 0;
> +		return 0;
> +	return mod ? dump_skip(cprm, align - mod) : 1;
>  }
>  EXPORT_SYMBOL(dump_align);

Unfortunately, this patch still does not fix the issue. I rm'd the output 
directory just to be sure, but the bug is still there. What does this commit 
do anyway? The commit message is quite vague.

Please find the files to reproduce the bug in the tarball. Edit bisecter.sh to 
change the paths to the initrd, object output directory and serial output 
("dmesg") as needed.

The initrd should contain:
/init
/bin/busybox (statically linked)
/bin/segfault (also statically linked, source included)

initrd containing these three files can be generated with:
find | cpio --owner=root:root -H newc -o > initrd

The .config file (named qemucore.config) which I use for reproduction is also 
included. The resulting segfault program is 742752 bytes (stripped).

[-- Attachment #2: bisect-kernel.tar.gz --]
[-- Type: application/x-compressed-tar, Size: 8688 bytes --]

Re: [REGRESSION] coredumps truncated after "new helper: dump_align()"

[Al Viro]

On Fri, Nov 15, 2013 at 10:34:43PM +0100, Peter Wu wrote:

> Unfortunately, this patch still does not fix the issue. I rm'd the output 
> directory just to be sure, but the bug is still there. What does this commit 
> do anyway? The commit message is quite vague.

Introduces a helper that used to be open-coded in a bunch of places -
pads the coredump to given alignment.  And switches those places
to that new helper...

FWIW, I haven't tried that on your config yet, but here (with the patch
in my previous mail) I'm seeing a sane-looking coredump -
-rw------- 1 root root 315392 Nov 15 17:48 core
Different userland, presumably, since that static binary is 684349
bytes long.

I'll try to reproduce with your config...

Re: [REGRESSION] coredumps truncated after "new helper: dump_align()"

[Al Viro]

On Sat, Nov 16, 2013 at 12:04:08AM +0000, Al Viro wrote:
> On Fri, Nov 15, 2013 at 10:34:43PM +0100, Peter Wu wrote:
> 
> > Unfortunately, this patch still does not fix the issue. I rm'd the output 
> > directory just to be sure, but the bug is still there. What does this commit 
> > do anyway? The commit message is quite vague.
> 
> Introduces a helper that used to be open-coded in a bunch of places -
> pads the coredump to given alignment.  And switches those places
> to that new helper...
> 
> FWIW, I haven't tried that on your config yet, but here (with the patch
> in my previous mail) I'm seeing a sane-looking coredump -
> -rw------- 1 root root 315392 Nov 15 17:48 core
> Different userland, presumably, since that static binary is 684349
> bytes long.
> 
> I'll try to reproduce with your config...

... and on your config I'm seeing
Inited
Segmentation fault (core dumped)
[    0.123351] Core size: 315392
in the log.  Same size, same apparently sane coredump.  Can you check what you
get on mainline + diff below (combination of dump_align() and locking fix)?

diff --git a/fs/coredump.c b/fs/coredump.c
index 62406b6..bc3fbcd 100644
--- a/fs/coredump.c
+++ b/fs/coredump.c
@@ -695,7 +695,7 @@ int dump_emit(struct coredump_params *cprm, const void *addr, int nr)
 	while (nr) {
 		if (dump_interrupted())
 			return 0;
-		n = vfs_write(file, addr, nr, &pos);
+		n = __kernel_write(file, addr, nr, &pos);
 		if (n <= 0)
 			return 0;
 		file->f_pos = pos;
@@ -733,7 +733,7 @@ int dump_align(struct coredump_params *cprm, int align)
 {
 	unsigned mod = cprm->written & (align - 1);
 	if (align & (align - 1))
-		return -EINVAL;
-	return mod ? dump_skip(cprm, align - mod) : 0;
+		return 0;
+	return mod ? dump_skip(cprm, align - mod) : 1;
 }
 EXPORT_SYMBOL(dump_align);

Re: [REGRESSION] coredumps truncated after "new helper: dump_align()"

[Peter Wu]

On Sunday 17 November 2013 06:19:07 Al Viro wrote:
> On Sat, Nov 16, 2013 at 12:04:08AM +0000, Al Viro wrote:
> > On Fri, Nov 15, 2013 at 10:34:43PM +0100, Peter Wu wrote:
> > 
> >
> > > Unfortunately, this patch still does not fix the issue. I rm'd the
> > > output 
> > > directory just to be sure, but the bug is still there. What does this
> > > commit  do anyway? The commit message is quite vague.
> >
> > 
> >
> > Introduces a helper that used to be open-coded in a bunch of places -
> > pads the coredump to given alignment.  And switches those places
> > to that new helper...
> >
> > 
> >
> > FWIW, I haven't tried that on your config yet, but here (with the patch
> > in my previous mail) I'm seeing a sane-looking coredump -
> > -rw------- 1 root root 315392 Nov 15 17:48 core
> > Different userland, presumably, since that static binary is 684349
> > bytes long.
> >
> > 
> >
> > I'll try to reproduce with your config...
> 
> ... and on your config I'm seeing
> Inited
> Segmentation fault (core dumped)
> [    0.123351] Core size: 315392
> in the log.  Same size, same apparently sane coredump.  Can you check what
> you get on mainline + diff below (combination of dump_align() and locking
> fix)?

Hmm, I cannot reproduce the bug with the two-liner patch you provided before. 
I must have made a mistake with the previous test as the environment is still 
the same.

The bug is fixed with both your two-liner patch and the __kernel_write one.
In addition, the __kernel_write patch removes the lockdep warning reported by 
Dave Jones[1].

Thanks,
Peter

 [1]: http://lkml.org/lkml/2013/11/13/450