FYI, we still have this one on latest mainline:
[ 2.159614] NET: Registered protocol family 16
[ 2.163109] initcall netlink_proto_init+0x0/0x1b0 returned 0 after 5859 usecs
[ 2.164008] WARNING: kmemcheck: Caught 32-bit read from uninitialized memory (bf438284)
[ 2.165006] 0100000002000000000000000000000000000000ad4eaddeffffffffffffffff
[ 2.172006] i i i i i i u u i i i i i i i i i i i i i i i i i i i i i i i i
[ 2.179005] ^
[ 2.180005]
[ 2.181008] Pid: 1, comm: swapper Not tainted (2.6.31-tip-02389-gc9f313c-dirty #151)
[ 2.182006] EIP: 0060:[<815a8101>] EFLAGS: 00010282 CPU: 0
[ 2.183009] EIP is at sock_init_data+0xe1/0x210
[ 2.184006] EAX: 0001b000 EBX: bf855938 ECX: 8233b614 EDX: 819ac7bf
[ 2.185006] ESI: bf855800 EDI: bf438280 EBP: bf867f10 ESP: 81b3afcc
[ 2.186006] DS: 007b ES: 007b FS: 00d8 GS: 0000 SS: 0068
[ 2.187006] CR0: 8005003b CR2: bf83bdf0 CR3: 01b2c000 CR4: 000006d0
[ 2.188006] DR0: 00000000 DR1: 00000000 DR2: 00000000 DR3: 00000000
[ 2.189006] DR6: ffff4ff0 DR7: 00000400
[ 2.190005] [<815d82b5>] __netlink_create+0x35/0xa0
[ 2.192005] [<815dabaa>] netlink_kernel_create+0x5a/0x150
[ 2.194004] [<815bc8ee>] rtnetlink_net_init+0x1e/0x40
[ 2.196005] [<815af381>] register_pernet_operations+0x11/0x30
[ 2.198004] [<815af4be>] register_pernet_subsys+0x1e/0x30
[ 2.200004] [<81adb49c>] rtnetlink_init+0x4c/0x100
[ 2.202004] [<81adbfe5>] netlink_proto_init+0x105/0x1b0
[ 2.204004] [<81001127>] do_one_initcall+0x27/0x190
[ 2.206004] [<81a9f567>] do_initcalls+0x27/0x40
[ 2.208004] [<81a9f5a6>] do_basic_setup+0x26/0x30
[ 2.210004] [<81a9f907>] kernel_init+0x57/0xa0
[ 2.212004] [<81004867>] kernel_thread_helper+0x7/0x30
[ 2.214004] [<ffffffff>] 0xffffffff
[ 2.216021] calling bdi_class_init+0x0/0x30 @ 1
[ 2.217015] device class 'bdi': registering
[ 2.218702] initcall bdi_class_init+0x0/0x30 returned 0 after 976 usecs
[ 2.219041] calling kobject_uevent_init+0x0/0x50 @ 1
config attached.
Ingo
Ingo Molnar a ?crit :
> FYI, we still have this one on latest mainline:
>
> [ 2.159614] NET: Registered protocol family 16
> [ 2.163109] initcall netlink_proto_init+0x0/0x1b0 returned 0 after 5859 usecs
> [ 2.164008] WARNING: kmemcheck: Caught 32-bit read from uninitialized memory (bf438284)
> [ 2.165006] 0100000002000000000000000000000000000000ad4eaddeffffffffffffffff
> [ 2.172006] i i i i i i u u i i i i i i i i i i i i i i i i i i i i i i i i
> [ 2.179005] ^
> [ 2.180005]
> [ 2.181008] Pid: 1, comm: swapper Not tainted (2.6.31-tip-02389-gc9f313c-dirty #151)
> [ 2.182006] EIP: 0060:[<815a8101>] EFLAGS: 00010282 CPU: 0
> [ 2.183009] EIP is at sock_init_data+0xe1/0x210
> [ 2.184006] EAX: 0001b000 EBX: bf855938 ECX: 8233b614 EDX: 819ac7bf
> [ 2.185006] ESI: bf855800 EDI: bf438280 EBP: bf867f10 ESP: 81b3afcc
> [ 2.186006] DS: 007b ES: 007b FS: 00d8 GS: 0000 SS: 0068
> [ 2.187006] CR0: 8005003b CR2: bf83bdf0 CR3: 01b2c000 CR4: 000006d0
> [ 2.188006] DR0: 00000000 DR1: 00000000 DR2: 00000000 DR3: 00000000
> [ 2.189006] DR6: ffff4ff0 DR7: 00000400
> [ 2.190005] [<815d82b5>] __netlink_create+0x35/0xa0
> [ 2.192005] [<815dabaa>] netlink_kernel_create+0x5a/0x150
> [ 2.194004] [<815bc8ee>] rtnetlink_net_init+0x1e/0x40
> [ 2.196005] [<815af381>] register_pernet_operations+0x11/0x30
> [ 2.198004] [<815af4be>] register_pernet_subsys+0x1e/0x30
> [ 2.200004] [<81adb49c>] rtnetlink_init+0x4c/0x100
> [ 2.202004] [<81adbfe5>] netlink_proto_init+0x105/0x1b0
> [ 2.204004] [<81001127>] do_one_initcall+0x27/0x190
> [ 2.206004] [<81a9f567>] do_initcalls+0x27/0x40
> [ 2.208004] [<81a9f5a6>] do_basic_setup+0x26/0x30
> [ 2.210004] [<81a9f907>] kernel_init+0x57/0xa0
> [ 2.212004] [<81004867>] kernel_thread_helper+0x7/0x30
> [ 2.214004] [<ffffffff>] 0xffffffff
> [ 2.216021] calling bdi_class_init+0x0/0x30 @ 1
> [ 2.217015] device class 'bdi': registering
> [ 2.218702] initcall bdi_class_init+0x0/0x30 returned 0 after 976 usecs
> [ 2.219041] calling kobject_uevent_init+0x0/0x50 @ 1
>
> config attached.
>
> Ingo
>
I thought this was already discussed and fixed somehow ?
Either we add kmemcheck annotations, or we switch sock->type from short
to int to avoid the hole, and possibly to speedup things...
[PATCH] net: kmemcheck annotation in struct socket
struct socket has a 16 bit hole that triggers kmemcheck warnings.
As suggested by Ingo, use kmemcheck annotations
Signed-off-by: Eric Dumazet <[email protected]>
---
include/linux/net.h | 5 +++++
net/socket.c | 1 +
2 files changed, 6 insertions(+)
diff --git a/include/linux/net.h b/include/linux/net.h
index 4fc2ffd..9040a10 100644
--- a/include/linux/net.h
+++ b/include/linux/net.h
@@ -57,6 +57,7 @@ typedef enum {
#include <linux/random.h>
#include <linux/wait.h>
#include <linux/fcntl.h> /* For O_CLOEXEC and O_NONBLOCK */
+#include <linux/kmemcheck.h>
struct poll_table_struct;
struct pipe_inode_info;
@@ -127,7 +128,11 @@ enum sock_shutdown_cmd {
*/
struct socket {
socket_state state;
+
+ kmemcheck_bitfield_begin(type);
short type;
+ kmemcheck_bitfield_end(type);
+
unsigned long flags;
/*
* Please keep fasync_list & wait fields in the same cache line
diff --git a/net/socket.c b/net/socket.c
index 6d47165..2a022c0 100644
--- a/net/socket.c
+++ b/net/socket.c
@@ -489,6 +489,7 @@ static struct socket *sock_alloc(void)
sock = SOCKET_I(inode);
+ kmemcheck_annotate_bitfield(sock, type);
inode->i_mode = S_IFSOCK | S_IRWXUGO;
inode->i_uid = current_fsuid();
inode->i_gid = current_fsgid();
* Eric Dumazet <[email protected]> wrote:
> Ingo Molnar a ?crit :
> > FYI, we still have this one on latest mainline:
> >
> > [ 2.159614] NET: Registered protocol family 16
> > [ 2.163109] initcall netlink_proto_init+0x0/0x1b0 returned 0 after 5859 usecs
> > [ 2.164008] WARNING: kmemcheck: Caught 32-bit read from uninitialized memory (bf438284)
> > [ 2.165006] 0100000002000000000000000000000000000000ad4eaddeffffffffffffffff
> > [ 2.172006] i i i i i i u u i i i i i i i i i i i i i i i i i i i i i i i i
> > [ 2.179005] ^
> > [ 2.180005]
> > [ 2.181008] Pid: 1, comm: swapper Not tainted (2.6.31-tip-02389-gc9f313c-dirty #151)
> > [ 2.182006] EIP: 0060:[<815a8101>] EFLAGS: 00010282 CPU: 0
> > [ 2.183009] EIP is at sock_init_data+0xe1/0x210
> > [ 2.184006] EAX: 0001b000 EBX: bf855938 ECX: 8233b614 EDX: 819ac7bf
> > [ 2.185006] ESI: bf855800 EDI: bf438280 EBP: bf867f10 ESP: 81b3afcc
> > [ 2.186006] DS: 007b ES: 007b FS: 00d8 GS: 0000 SS: 0068
> > [ 2.187006] CR0: 8005003b CR2: bf83bdf0 CR3: 01b2c000 CR4: 000006d0
> > [ 2.188006] DR0: 00000000 DR1: 00000000 DR2: 00000000 DR3: 00000000
> > [ 2.189006] DR6: ffff4ff0 DR7: 00000400
> > [ 2.190005] [<815d82b5>] __netlink_create+0x35/0xa0
> > [ 2.192005] [<815dabaa>] netlink_kernel_create+0x5a/0x150
> > [ 2.194004] [<815bc8ee>] rtnetlink_net_init+0x1e/0x40
> > [ 2.196005] [<815af381>] register_pernet_operations+0x11/0x30
> > [ 2.198004] [<815af4be>] register_pernet_subsys+0x1e/0x30
> > [ 2.200004] [<81adb49c>] rtnetlink_init+0x4c/0x100
> > [ 2.202004] [<81adbfe5>] netlink_proto_init+0x105/0x1b0
> > [ 2.204004] [<81001127>] do_one_initcall+0x27/0x190
> > [ 2.206004] [<81a9f567>] do_initcalls+0x27/0x40
> > [ 2.208004] [<81a9f5a6>] do_basic_setup+0x26/0x30
> > [ 2.210004] [<81a9f907>] kernel_init+0x57/0xa0
> > [ 2.212004] [<81004867>] kernel_thread_helper+0x7/0x30
> > [ 2.214004] [<ffffffff>] 0xffffffff
> > [ 2.216021] calling bdi_class_init+0x0/0x30 @ 1
> > [ 2.217015] device class 'bdi': registering
> > [ 2.218702] initcall bdi_class_init+0x0/0x30 returned 0 after 976 usecs
> > [ 2.219041] calling kobject_uevent_init+0x0/0x50 @ 1
> >
> > config attached.
> >
> > Ingo
> >
>
> I thought this was already discussed and fixed somehow ?
yes, it looked familar.
> Either we add kmemcheck annotations, or we switch sock->type from short
> to int to avoid the hole, and possibly to speedup things...
>
> [PATCH] net: kmemcheck annotation in struct socket
>
> struct socket has a 16 bit hole that triggers kmemcheck warnings.
>
> As suggested by Ingo, use kmemcheck annotations
>
> Signed-off-by: Eric Dumazet <[email protected]>
> ---
> include/linux/net.h | 5 +++++
> net/socket.c | 1 +
> 2 files changed, 6 insertions(+)
Acked-by: Ingo Molnar <[email protected]>
Ingo
From: Ingo Molnar <[email protected]>
Date: Tue, 15 Sep 2009 11:34:40 +0200
>
> * Eric Dumazet <[email protected]> wrote:
>
>> Either we add kmemcheck annotations, or we switch sock->type from short
>> to int to avoid the hole, and possibly to speedup things...
>>
>> [PATCH] net: kmemcheck annotation in struct socket
>>
>> struct socket has a 16 bit hole that triggers kmemcheck warnings.
>>
>> As suggested by Ingo, use kmemcheck annotations
>>
>> Signed-off-by: Eric Dumazet <[email protected]>
>> ---
>> include/linux/net.h | 5 +++++
>> net/socket.c | 1 +
>> 2 files changed, 6 insertions(+)
>
> Acked-by: Ingo Molnar <[email protected]>
Applied, thanks everyone.
here's another one:
[ 0.337000] Total of 1 processors activated (3088.38 BogoMIPS).
[ 0.352000] CPU0 attaching NULL sched-domain.
[ 0.360000] WARNING: kmemcheck: Caught 32-bit read from uninitialized memory (9f8020fc)
[ 0.361000] a44240820000000041f6998100000000000000000000000000000000ff030000
[ 0.368000] i i i i i i i i i i i i i i i i u u u u i i i i i i i i i i u u
[ 0.375000] ^
[ 0.376000]
[ 0.377000] Pid: 9, comm: khelper Not tainted (2.6.31-tip #206) P4DC6
[ 0.378000] EIP: 0060:[<810a3a95>] EFLAGS: 00010246 CPU: 0
[ 0.379000] EIP is at shmem_fill_super+0xb5/0x120
[ 0.380000] EAX: 00000000 EBX: 9f845400 ECX: 824042a4 EDX: 8199f641
[ 0.381000] ESI: 9f8020c0 EDI: 9f845400 EBP: 9f81af68 ESP: 81cd6eec
[ 0.382000] DS: 007b ES: 007b FS: 00d8 GS: 0000 SS: 0068
[ 0.383000] CR0: 8005003b CR2: 9f806200 CR3: 01ccd000 CR4: 000006d0
[ 0.384000] DR0: 00000000 DR1: 00000000 DR2: 00000000 DR3: 00000000
[ 0.385000] DR6: ffff4ff0 DR7: 00000400
[ 0.386000] [<810c25fc>] get_sb_nodev+0x3c/0x80
[ 0.388000] [<810a3514>] shmem_get_sb+0x14/0x20
[ 0.390000] [<810c207f>] vfs_kern_mount+0x4f/0x120
[ 0.392000] [<81b2849e>] init_tmpfs+0x7e/0xb0
[ 0.394000] [<81b11597>] do_basic_setup+0x17/0x30
[ 0.396000] [<81b11907>] kernel_init+0x57/0xa0
[ 0.398000] [<810039b7>] kernel_thread_helper+0x7/0x10
[ 0.400000] [<ffffffff>] 0xffffffff
[ 0.402000] khelper used greatest stack depth: 2820 bytes left
[ 0.407000] calling init_mmap_min_addr+0x0/0x10 @ 1
[ 0.408000] initcall init_mmap_min_addr+0x0/0x10 returned 0 after 0 usecs
Is this one known too?
Ingo
2009/9/20 Ingo Molnar <[email protected]>:
>
> here's another one:
>
> [ 0.337000] Total of 1 processors activated (3088.38 BogoMIPS).
> [ 0.352000] CPU0 attaching NULL sched-domain.
> [ 0.360000] WARNING: kmemcheck: Caught 32-bit read from uninitialized memory (9f8020fc)
> [ 0.361000] a44240820000000041f6998100000000000000000000000000000000ff030000
> [ 0.368000] i i i i i i i i i i i i i i i i u u u u i i i i i i i i i i u u
> [ 0.375000] ^
> [ 0.376000]
> [ 0.377000] Pid: 9, comm: khelper Not tainted (2.6.31-tip #206) P4DC6
> [ 0.378000] EIP: 0060:[<810a3a95>] EFLAGS: 00010246 CPU: 0
> [ 0.379000] EIP is at shmem_fill_super+0xb5/0x120
> [ 0.380000] EAX: 00000000 EBX: 9f845400 ECX: 824042a4 EDX: 8199f641
> [ 0.381000] ESI: 9f8020c0 EDI: 9f845400 EBP: 9f81af68 ESP: 81cd6eec
>
> Is this one known too?
>
> Ingo
>
Thanks for the report.
AFAICT it's this line of mm/shmem.c:
2356 inode = shmem_get_inode(sb, S_IFDIR | sbinfo->mode, 0,
VM_NORESERVE );
and the loading of sbinfo->mode. It fits with the offset 0x3c(%esi) ==
the address reported by kmemcheck and the offset of ->mode:
(gdb) p &((struct shmem_sb_info *) 0).mode
$1 = (mode_t *) 0x3c
Looking for the definition of mode_t, it seems to be defined in x86
sources as unsigned short:
arch/x86/include/asm/posix_types_32.h:11:typedef unsigned short __kernel_mode_t;
include/linux/types.h:typedef __kernel_mode_t mode_t;
And the load was clearly 32-bit (kmemcheck said so) and in my assembly
dump it is also so.
As I said before, I really don't like the solution of sprinkling the
kmemcheck annotations all over the place to cover up field padding
inside structs, not in the least because they confuse more than they
help, and they are not maintainable -- when somebody changes the
struct definitions, anything may happen to the field layout, and then
the annotation may have to change too. And it's not exactly obvious.
I still vote for patching gcc as the long-term solution. There is
-fmudflap, there is -fstack-protector, why not a -fsacred-padding? Of
course it has to be implemented too...
Vegard
On Sun, 20 Sep 2009, Vegard Nossum wrote:
> Thanks for the report.
>
> AFAICT it's this line of mm/shmem.c:
>
> 2356 inode = shmem_get_inode(sb, S_IFDIR | sbinfo->mode, 0,
> VM_NORESERVE );
>
> and the loading of sbinfo->mode. It fits with the offset 0x3c(%esi) ==
> the address reported by kmemcheck and the offset of ->mode:
>
> (gdb) p &((struct shmem_sb_info *) 0).mode
> $1 = (mode_t *) 0x3c
>
> Looking for the definition of mode_t, it seems to be defined in x86
> sources as unsigned short:
>
> arch/x86/include/asm/posix_types_32.h:11:typedef unsigned short __kernel_mode_t;
> include/linux/types.h:typedef __kernel_mode_t mode_t;
>
> And the load was clearly 32-bit (kmemcheck said so) and in my assembly
> dump it is also so.
>
> As I said before, I really don't like the solution of sprinkling the
> kmemcheck annotations all over the place to cover up field padding
> inside structs, not in the least because they confuse more than they
> help, and they are not maintainable -- when somebody changes the
> struct definitions, anything may happen to the field layout, and then
> the annotation may have to change too. And it's not exactly obvious.
>
> I still vote for patching gcc as the long-term solution. There is
> -fmudflap, there is -fstack-protector, why not a -fsacred-padding? Of
> course it has to be implemented too...
As Ingo already explained, we would need to wait for a year or so for
"-fscared-padding" to appear in a GCC release and probably one year more
for it to be picked up by distributions.
So while we wait for such a thing to appear, how about something like
this?
Pekka
>From a7cb569beb2d2fe769d558d1a017b6f5aa05d7eb Mon Sep 17 00:00:00 2001
From: Pekka Enberg <[email protected]>
Date: Sun, 20 Sep 2009 20:43:35 +0300
Subject: [PATCH] shmem: initialize struct shmem_sb_info to zero
Fixes the following kmemcheck false positive:
[ 0.337000] Total of 1 processors activated (3088.38 BogoMIPS).
[ 0.352000] CPU0 attaching NULL sched-domain.
[ 0.360000] WARNING: kmemcheck: Caught 32-bit read from uninitialized memory (9f8020fc)
[ 0.361000] a44240820000000041f6998100000000000000000000000000000000ff030000
[ 0.368000] i i i i i i i i i i i i i i i i u u u u i i i i i i i i i i u u
[ 0.375000] ^
[ 0.376000]
[ 0.377000] Pid: 9, comm: khelper Not tainted (2.6.31-tip #206) P4DC6
[ 0.378000] EIP: 0060:[<810a3a95>] EFLAGS: 00010246 CPU: 0
[ 0.379000] EIP is at shmem_fill_super+0xb5/0x120
[ 0.380000] EAX: 00000000 EBX: 9f845400 ECX: 824042a4 EDX: 8199f641
[ 0.381000] ESI: 9f8020c0 EDI: 9f845400 EBP: 9f81af68 ESP: 81cd6eec
[ 0.382000] DS: 007b ES: 007b FS: 00d8 GS: 0000 SS: 0068
[ 0.383000] CR0: 8005003b CR2: 9f806200 CR3: 01ccd000 CR4: 000006d0
[ 0.384000] DR0: 00000000 DR1: 00000000 DR2: 00000000 DR3: 00000000
[ 0.385000] DR6: ffff4ff0 DR7: 00000400
[ 0.386000] [<810c25fc>] get_sb_nodev+0x3c/0x80
[ 0.388000] [<810a3514>] shmem_get_sb+0x14/0x20
[ 0.390000] [<810c207f>] vfs_kern_mount+0x4f/0x120
[ 0.392000] [<81b2849e>] init_tmpfs+0x7e/0xb0
[ 0.394000] [<81b11597>] do_basic_setup+0x17/0x30
[ 0.396000] [<81b11907>] kernel_init+0x57/0xa0
[ 0.398000] [<810039b7>] kernel_thread_helper+0x7/0x10
[ 0.400000] [<ffffffff>] 0xffffffff
[ 0.402000] khelper used greatest stack depth: 2820 bytes left
[ 0.407000] calling init_mmap_min_addr+0x0/0x10 @ 1
[ 0.408000] initcall init_mmap_min_addr+0x0/0x10 returned 0 after 0 usecs
Reported-by: Ingo Molnar <[email protected]>
Signed-off-by: Pekka Enberg <[email protected]>
---
mm/shmem.c | 5 +----
1 files changed, 1 insertions(+), 4 deletions(-)
diff --git a/mm/shmem.c b/mm/shmem.c
index d713239..a8f54f3 100644
--- a/mm/shmem.c
+++ b/mm/shmem.c
@@ -2307,17 +2307,14 @@ static int shmem_fill_super(struct super_block *sb,
int err = -ENOMEM;
/* Round up to L1_CACHE_BYTES to resist false sharing */
- sbinfo = kmalloc(max((int)sizeof(struct shmem_sb_info),
+ sbinfo = kzalloc(max((int)sizeof(struct shmem_sb_info),
L1_CACHE_BYTES), GFP_KERNEL);
if (!sbinfo)
return -ENOMEM;
- sbinfo->max_blocks = 0;
- sbinfo->max_inodes = 0;
sbinfo->mode = S_IRWXUGO | S_ISVTX;
sbinfo->uid = current_fsuid();
sbinfo->gid = current_fsgid();
- sbinfo->mpol = NULL;
sb->s_fs_info = sbinfo;
#ifdef CONFIG_TMPFS
--
1.5.6.4
* Pekka J Enberg <[email protected]> wrote:
> On Sun, 20 Sep 2009, Vegard Nossum wrote:
>> Thanks for the report.
>>
>> AFAICT it's this line of mm/shmem.c:
>>
>> 2356 inode = shmem_get_inode(sb, S_IFDIR | sbinfo->mode, 0,
>> VM_NORESERVE );
>>
>> and the loading of sbinfo->mode. It fits with the offset 0x3c(%esi) ==
>> the address reported by kmemcheck and the offset of ->mode:
>>
>> (gdb) p &((struct shmem_sb_info *) 0).mode
>> $1 = (mode_t *) 0x3c
>>
>> Looking for the definition of mode_t, it seems to be defined in x86
>> sources as unsigned short:
>>
>> arch/x86/include/asm/posix_types_32.h:11:typedef unsigned short __kernel_mode_t;
>> include/linux/types.h:typedef __kernel_mode_t mode_t;
>>
>> And the load was clearly 32-bit (kmemcheck said so) and in my assembly
>> dump it is also so.
>>
>> As I said before, I really don't like the solution of sprinkling the
>> kmemcheck annotations all over the place to cover up field padding
>> inside structs, not in the least because they confuse more than they
>> help, and they are not maintainable -- when somebody changes the
>> struct definitions, anything may happen to the field layout, and then
>> the annotation may have to change too. And it's not exactly obvious.
>>
>> I still vote for patching gcc as the long-term solution. There is
>> -fmudflap, there is -fstack-protector, why not a -fsacred-padding? Of
>> course it has to be implemented too...
>
> As Ingo already explained, we would need to wait for a year or so for
> "-fscared-padding" to appear in a GCC release and probably one year more
> for it to be picked up by distributions.
>
> So while we wait for such a thing to appear, how about something like
> this?
>
> Pekka
>
>> From a7cb569beb2d2fe769d558d1a017b6f5aa05d7eb Mon Sep 17 00:00:00 2001
> From: Pekka Enberg <[email protected]>
> Date: Sun, 20 Sep 2009 20:43:35 +0300
> Subject: [PATCH] shmem: initialize struct shmem_sb_info to zero
>
> Fixes the following kmemcheck false positive:
>
> [ 0.337000] Total of 1 processors activated (3088.38 BogoMIPS).
> [ 0.352000] CPU0 attaching NULL sched-domain.
> [ 0.360000] WARNING: kmemcheck: Caught 32-bit read from uninitialized memory (9f8020fc)
> [ 0.361000] a44240820000000041f6998100000000000000000000000000000000ff030000
> [ 0.368000] i i i i i i i i i i i i i i i i u u u u i i i i i i i i i i u u
> [ 0.375000] ^
> [ 0.376000]
> [ 0.377000] Pid: 9, comm: khelper Not tainted (2.6.31-tip #206) P4DC6
> [ 0.378000] EIP: 0060:[<810a3a95>] EFLAGS: 00010246 CPU: 0
> [ 0.379000] EIP is at shmem_fill_super+0xb5/0x120
> [ 0.380000] EAX: 00000000 EBX: 9f845400 ECX: 824042a4 EDX: 8199f641
> [ 0.381000] ESI: 9f8020c0 EDI: 9f845400 EBP: 9f81af68 ESP: 81cd6eec
> [ 0.382000] DS: 007b ES: 007b FS: 00d8 GS: 0000 SS: 0068
> [ 0.383000] CR0: 8005003b CR2: 9f806200 CR3: 01ccd000 CR4: 000006d0
> [ 0.384000] DR0: 00000000 DR1: 00000000 DR2: 00000000 DR3: 00000000
> [ 0.385000] DR6: ffff4ff0 DR7: 00000400
> [ 0.386000] [<810c25fc>] get_sb_nodev+0x3c/0x80
> [ 0.388000] [<810a3514>] shmem_get_sb+0x14/0x20
> [ 0.390000] [<810c207f>] vfs_kern_mount+0x4f/0x120
> [ 0.392000] [<81b2849e>] init_tmpfs+0x7e/0xb0
> [ 0.394000] [<81b11597>] do_basic_setup+0x17/0x30
> [ 0.396000] [<81b11907>] kernel_init+0x57/0xa0
> [ 0.398000] [<810039b7>] kernel_thread_helper+0x7/0x10
> [ 0.400000] [<ffffffff>] 0xffffffff
> [ 0.402000] khelper used greatest stack depth: 2820 bytes left
> [ 0.407000] calling init_mmap_min_addr+0x0/0x10 @ 1
> [ 0.408000] initcall init_mmap_min_addr+0x0/0x10 returned 0 after 0 usecs
>
> Reported-by: Ingo Molnar <[email protected]>
> Signed-off-by: Pekka Enberg <[email protected]>
> ---
> mm/shmem.c | 5 +----
> 1 files changed, 1 insertions(+), 4 deletions(-)
>
> diff --git a/mm/shmem.c b/mm/shmem.c
> index d713239..a8f54f3 100644
> --- a/mm/shmem.c
> +++ b/mm/shmem.c
> @@ -2307,17 +2307,14 @@ static int shmem_fill_super(struct super_block *sb,
> int err = -ENOMEM;
>
> /* Round up to L1_CACHE_BYTES to resist false sharing */
> - sbinfo = kmalloc(max((int)sizeof(struct shmem_sb_info),
> + sbinfo = kzalloc(max((int)sizeof(struct shmem_sb_info),
> L1_CACHE_BYTES), GFP_KERNEL);
> if (!sbinfo)
> return -ENOMEM;
>
> - sbinfo->max_blocks = 0;
> - sbinfo->max_inodes = 0;
> sbinfo->mode = S_IRWXUGO | S_ISVTX;
> sbinfo->uid = current_fsuid();
> sbinfo->gid = current_fsgid();
> - sbinfo->mpol = NULL;
> sb->s_fs_info = sbinfo;
That looks like a step forward even without kmemcheck considered, right?
Ingo
Hi Ingo,
On Sun, Sep 20, 2009 at 8:58 PM, Ingo Molnar <[email protected]> wrote:
>> From: Pekka Enberg <[email protected]>
>> Date: Sun, 20 Sep 2009 20:43:35 +0300
>> Subject: [PATCH] shmem: initialize struct shmem_sb_info to zero
>>
>> Fixes the following kmemcheck false positive:
>>
>> [ ? ?0.337000] Total of 1 processors activated (3088.38 BogoMIPS).
>> [ ? ?0.352000] CPU0 attaching NULL sched-domain.
>> [ ? ?0.360000] WARNING: kmemcheck: Caught 32-bit read from uninitialized memory (9f8020fc)
>> [ ? ?0.361000] a44240820000000041f6998100000000000000000000000000000000ff030000
>> [ ? ?0.368000] ?i i i i i i i i i i i i i i i i u u u u i i i i i i i i i i u u
>> [ ? ?0.375000] ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?^
>> [ ? ?0.376000]
>> [ ? ?0.377000] Pid: 9, comm: khelper Not tainted (2.6.31-tip #206) P4DC6
>> [ ? ?0.378000] EIP: 0060:[<810a3a95>] EFLAGS: 00010246 CPU: 0
>> [ ? ?0.379000] EIP is at shmem_fill_super+0xb5/0x120
>> [ ? ?0.380000] EAX: 00000000 EBX: 9f845400 ECX: 824042a4 EDX: 8199f641
>> [ ? ?0.381000] ESI: 9f8020c0 EDI: 9f845400 EBP: 9f81af68 ESP: 81cd6eec
>> [ ? ?0.382000] ?DS: 007b ES: 007b FS: 00d8 GS: 0000 SS: 0068
>> [ ? ?0.383000] CR0: 8005003b CR2: 9f806200 CR3: 01ccd000 CR4: 000006d0
>> [ ? ?0.384000] DR0: 00000000 DR1: 00000000 DR2: 00000000 DR3: 00000000
>> [ ? ?0.385000] DR6: ffff4ff0 DR7: 00000400
>> [ ? ?0.386000] ?[<810c25fc>] get_sb_nodev+0x3c/0x80
>> [ ? ?0.388000] ?[<810a3514>] shmem_get_sb+0x14/0x20
>> [ ? ?0.390000] ?[<810c207f>] vfs_kern_mount+0x4f/0x120
>> [ ? ?0.392000] ?[<81b2849e>] init_tmpfs+0x7e/0xb0
>> [ ? ?0.394000] ?[<81b11597>] do_basic_setup+0x17/0x30
>> [ ? ?0.396000] ?[<81b11907>] kernel_init+0x57/0xa0
>> [ ? ?0.398000] ?[<810039b7>] kernel_thread_helper+0x7/0x10
>> [ ? ?0.400000] ?[<ffffffff>] 0xffffffff
>> [ ? ?0.402000] khelper used greatest stack depth: 2820 bytes left
>> [ ? ?0.407000] calling ?init_mmap_min_addr+0x0/0x10 @ 1
>> [ ? ?0.408000] initcall init_mmap_min_addr+0x0/0x10 returned 0 after 0 usecs
>>
>> Reported-by: Ingo Molnar <[email protected]>
>> Signed-off-by: Pekka Enberg <[email protected]>
>> ---
>> ?mm/shmem.c | ? ?5 +----
>> ?1 files changed, 1 insertions(+), 4 deletions(-)
>>
>> diff --git a/mm/shmem.c b/mm/shmem.c
>> index d713239..a8f54f3 100644
>> --- a/mm/shmem.c
>> +++ b/mm/shmem.c
>> @@ -2307,17 +2307,14 @@ static int shmem_fill_super(struct super_block *sb,
>> ? ? ? int err = -ENOMEM;
>>
>> ? ? ? /* Round up to L1_CACHE_BYTES to resist false sharing */
>> - ? ? sbinfo = kmalloc(max((int)sizeof(struct shmem_sb_info),
>> + ? ? sbinfo = kzalloc(max((int)sizeof(struct shmem_sb_info),
>> ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? L1_CACHE_BYTES), GFP_KERNEL);
>> ? ? ? if (!sbinfo)
>> ? ? ? ? ? ? ? return -ENOMEM;
>>
>> - ? ? sbinfo->max_blocks = 0;
>> - ? ? sbinfo->max_inodes = 0;
>> ? ? ? sbinfo->mode = S_IRWXUGO | S_ISVTX;
>> ? ? ? sbinfo->uid = current_fsuid();
>> ? ? ? sbinfo->gid = current_fsgid();
>> - ? ? sbinfo->mpol = NULL;
>> ? ? ? sb->s_fs_info = sbinfo;
>
> That looks like a step forward even without kmemcheck considered, right?
Oh, sure. It usually less error prone to use kzalloc() for infrequent
allocations such as this.
Pekka
[Ingo Molnar - Sun, Sep 20, 2009 at 07:58:03PM +0200]
...
|
| > [ 0.408000] initcall init_mmap_min_addr+0x0/0x10 returned 0 after 0 usecs
| >
| > Reported-by: Ingo Molnar <[email protected]>
| > Signed-off-by: Pekka Enberg <[email protected]>
| > ---
| > mm/shmem.c | 5 +----
| > 1 files changed, 1 insertions(+), 4 deletions(-)
| >
| > diff --git a/mm/shmem.c b/mm/shmem.c
| > index d713239..a8f54f3 100644
| > --- a/mm/shmem.c
| > +++ b/mm/shmem.c
| > @@ -2307,17 +2307,14 @@ static int shmem_fill_super(struct super_block *sb,
| > int err = -ENOMEM;
| >
| > /* Round up to L1_CACHE_BYTES to resist false sharing */
| > - sbinfo = kmalloc(max((int)sizeof(struct shmem_sb_info),
| > + sbinfo = kzalloc(max((int)sizeof(struct shmem_sb_info),
| > L1_CACHE_BYTES), GFP_KERNEL);
| > if (!sbinfo)
| > return -ENOMEM;
| >
| > - sbinfo->max_blocks = 0;
| > - sbinfo->max_inodes = 0;
| > sbinfo->mode = S_IRWXUGO | S_ISVTX;
| > sbinfo->uid = current_fsuid();
| > sbinfo->gid = current_fsgid();
| > - sbinfo->mpol = NULL;
| > sb->s_fs_info = sbinfo;
|
| That looks like a step forward even without kmemcheck considered, right?
|
| Ingo
Yeah, and we save a few cpu cycles as well :)
-- Cyrill
* Pekka Enberg <[email protected]> wrote:
> Hi Ingo,
>
> On Sun, Sep 20, 2009 at 8:58 PM, Ingo Molnar <[email protected]> wrote:
> >> From: Pekka Enberg <[email protected]>
> >> Date: Sun, 20 Sep 2009 20:43:35 +0300
> >> Subject: [PATCH] shmem: initialize struct shmem_sb_info to zero
> >>
> >> Fixes the following kmemcheck false positive:
> >>
> >> [ ? ?0.337000] Total of 1 processors activated (3088.38 BogoMIPS).
> >> [ ? ?0.352000] CPU0 attaching NULL sched-domain.
> >> [ ? ?0.360000] WARNING: kmemcheck: Caught 32-bit read from uninitialized memory (9f8020fc)
> >> [ ? ?0.361000] a44240820000000041f6998100000000000000000000000000000000ff030000
> >> [ ? ?0.368000] ?i i i i i i i i i i i i i i i i u u u u i i i i i i i i i i u u
> >> [ ? ?0.375000] ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?^
> >> [ ? ?0.376000]
> >> [ ? ?0.377000] Pid: 9, comm: khelper Not tainted (2.6.31-tip #206) P4DC6
> >> [ ? ?0.378000] EIP: 0060:[<810a3a95>] EFLAGS: 00010246 CPU: 0
> >> [ ? ?0.379000] EIP is at shmem_fill_super+0xb5/0x120
> >> [ ? ?0.380000] EAX: 00000000 EBX: 9f845400 ECX: 824042a4 EDX: 8199f641
> >> [ ? ?0.381000] ESI: 9f8020c0 EDI: 9f845400 EBP: 9f81af68 ESP: 81cd6eec
> >> [ ? ?0.382000] ?DS: 007b ES: 007b FS: 00d8 GS: 0000 SS: 0068
> >> [ ? ?0.383000] CR0: 8005003b CR2: 9f806200 CR3: 01ccd000 CR4: 000006d0
> >> [ ? ?0.384000] DR0: 00000000 DR1: 00000000 DR2: 00000000 DR3: 00000000
> >> [ ? ?0.385000] DR6: ffff4ff0 DR7: 00000400
> >> [ ? ?0.386000] ?[<810c25fc>] get_sb_nodev+0x3c/0x80
> >> [ ? ?0.388000] ?[<810a3514>] shmem_get_sb+0x14/0x20
> >> [ ? ?0.390000] ?[<810c207f>] vfs_kern_mount+0x4f/0x120
> >> [ ? ?0.392000] ?[<81b2849e>] init_tmpfs+0x7e/0xb0
> >> [ ? ?0.394000] ?[<81b11597>] do_basic_setup+0x17/0x30
> >> [ ? ?0.396000] ?[<81b11907>] kernel_init+0x57/0xa0
> >> [ ? ?0.398000] ?[<810039b7>] kernel_thread_helper+0x7/0x10
> >> [ ? ?0.400000] ?[<ffffffff>] 0xffffffff
> >> [ ? ?0.402000] khelper used greatest stack depth: 2820 bytes left
> >> [ ? ?0.407000] calling ?init_mmap_min_addr+0x0/0x10 @ 1
> >> [ ? ?0.408000] initcall init_mmap_min_addr+0x0/0x10 returned 0 after 0 usecs
> >>
> >> Reported-by: Ingo Molnar <[email protected]>
> >> Signed-off-by: Pekka Enberg <[email protected]>
> >> ---
> >> ?mm/shmem.c | ? ?5 +----
> >> ?1 files changed, 1 insertions(+), 4 deletions(-)
> >>
> >> diff --git a/mm/shmem.c b/mm/shmem.c
> >> index d713239..a8f54f3 100644
> >> --- a/mm/shmem.c
> >> +++ b/mm/shmem.c
> >> @@ -2307,17 +2307,14 @@ static int shmem_fill_super(struct super_block *sb,
> >> ? ? ? int err = -ENOMEM;
> >>
> >> ? ? ? /* Round up to L1_CACHE_BYTES to resist false sharing */
> >> - ? ? sbinfo = kmalloc(max((int)sizeof(struct shmem_sb_info),
> >> + ? ? sbinfo = kzalloc(max((int)sizeof(struct shmem_sb_info),
> >> ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? L1_CACHE_BYTES), GFP_KERNEL);
> >> ? ? ? if (!sbinfo)
> >> ? ? ? ? ? ? ? return -ENOMEM;
> >>
> >> - ? ? sbinfo->max_blocks = 0;
> >> - ? ? sbinfo->max_inodes = 0;
> >> ? ? ? sbinfo->mode = S_IRWXUGO | S_ISVTX;
> >> ? ? ? sbinfo->uid = current_fsuid();
> >> ? ? ? sbinfo->gid = current_fsgid();
> >> - ? ? sbinfo->mpol = NULL;
> >> ? ? ? sb->s_fs_info = sbinfo;
> >
> > That looks like a step forward even without kmemcheck considered, right?
>
> Oh, sure. It usually less error prone to use kzalloc() for infrequent
> allocations such as this.
So in that sense kmemcheck was useful here: it made kernel code a bit
better.
Ingo
On Sun, 20 Sep 2009, Pekka J Enberg wrote:
> On Sun, 20 Sep 2009, Vegard Nossum wrote:
> > Thanks for the report.
> >
> > AFAICT it's this line of mm/shmem.c:
> >
> > 2356 inode = shmem_get_inode(sb, S_IFDIR | sbinfo->mode, 0,
> > VM_NORESERVE );
> >
> > and the loading of sbinfo->mode. It fits with the offset 0x3c(%esi) ==
> > the address reported by kmemcheck and the offset of ->mode:
> >
> > (gdb) p &((struct shmem_sb_info *) 0).mode
> > $1 = (mode_t *) 0x3c
> >
> > Looking for the definition of mode_t, it seems to be defined in x86
> > sources as unsigned short:
> >
> > arch/x86/include/asm/posix_types_32.h:11:typedef unsigned short
> > __kernel_mode_t;
> > include/linux/types.h:typedef __kernel_mode_t mode_t;
> >
> > And the load was clearly 32-bit (kmemcheck said so) and in my assembly
> > dump it is also so.
> >
> > As I said before, I really don't like the solution of sprinkling the
> > kmemcheck annotations all over the place to cover up field padding
> > inside structs, not in the least because they confuse more than they
> > help, and they are not maintainable -- when somebody changes the
> > struct definitions, anything may happen to the field layout, and then
> > the annotation may have to change too. And it's not exactly obvious.
> >
> > I still vote for patching gcc as the long-term solution. There is
> > -fmudflap, there is -fstack-protector, why not a -fsacred-padding? Of
> > course it has to be implemented too...
>
> As Ingo already explained, we would need to wait for a year or so for
> "-fscared-padding" to appear in a GCC release and probably one year more for
> it to be picked up by distributions.
>
> So while we wait for such a thing to appear, how about something like this?
>
> Pekka
> From a7cb569beb2d2fe769d558d1a017b6f5aa05d7eb Mon Sep 17 00:00:00 2001
Ingo, Vegard, Pekka: thanks for doing all the work on this, especially
Vegard for deciphering it (I've added you to the credits below, and
added a further line of explanation to Pekka's comment). Andrew,
please pick this up into mmotm and send on to Linus - thanks.
I got a bit anxious when I saw that the mode arg to shmem_get_inode()
is declared as an int: was afraid that compiler was then passing a bad
upper half down, which in fact would cause no trouble, but how could it
be sure of that? However, it looks okay: after doing the 32-bit load,
it goes on to do a movzwl %ax,%eax - seems an odd way to proceed to me,
but I bet it knows a lot more about efficiency of memory loads than I do.
Hugh
From: Pekka Enberg <[email protected]>
Date: Sun, 20 Sep 2009 20:43:35 +0300
Subject: [PATCH] shmem: initialize struct shmem_sb_info to zero
Fixes the following kmemcheck false positive (the compiler is using
a 32-bit mov to load the 16-bit sbinfo->mode in shmem_fill_super):
[ 0.337000] Total of 1 processors activated (3088.38 BogoMIPS).
[ 0.352000] CPU0 attaching NULL sched-domain.
[ 0.360000] WARNING: kmemcheck: Caught 32-bit read from uninitialized
memory (9f8020fc)
[ 0.361000]
a44240820000000041f6998100000000000000000000000000000000ff030000
[ 0.368000] i i i i i i i i i i i i i i i i u u u u i i i i i i i i i i u
u
[ 0.375000] ^
[ 0.376000]
[ 0.377000] Pid: 9, comm: khelper Not tainted (2.6.31-tip #206) P4DC6
[ 0.378000] EIP: 0060:[<810a3a95>] EFLAGS: 00010246 CPU: 0
[ 0.379000] EIP is at shmem_fill_super+0xb5/0x120
[ 0.380000] EAX: 00000000 EBX: 9f845400 ECX: 824042a4 EDX: 8199f641
[ 0.381000] ESI: 9f8020c0 EDI: 9f845400 EBP: 9f81af68 ESP: 81cd6eec
[ 0.382000] DS: 007b ES: 007b FS: 00d8 GS: 0000 SS: 0068
[ 0.383000] CR0: 8005003b CR2: 9f806200 CR3: 01ccd000 CR4: 000006d0
[ 0.384000] DR0: 00000000 DR1: 00000000 DR2: 00000000 DR3: 00000000
[ 0.385000] DR6: ffff4ff0 DR7: 00000400
[ 0.386000] [<810c25fc>] get_sb_nodev+0x3c/0x80
[ 0.388000] [<810a3514>] shmem_get_sb+0x14/0x20
[ 0.390000] [<810c207f>] vfs_kern_mount+0x4f/0x120
[ 0.392000] [<81b2849e>] init_tmpfs+0x7e/0xb0
[ 0.394000] [<81b11597>] do_basic_setup+0x17/0x30
[ 0.396000] [<81b11907>] kernel_init+0x57/0xa0
[ 0.398000] [<810039b7>] kernel_thread_helper+0x7/0x10
[ 0.400000] [<ffffffff>] 0xffffffff
[ 0.402000] khelper used greatest stack depth: 2820 bytes left
[ 0.407000] calling init_mmap_min_addr+0x0/0x10 @ 1
[ 0.408000] initcall init_mmap_min_addr+0x0/0x10 returned 0 after 0 usecs
Reported-by: Ingo Molnar <[email protected]>
Analysed-by: Vegard Nossum <[email protected]>
Signed-off-by: Pekka Enberg <[email protected]>
Acked-by: Hugh Dickins <[email protected]>
---
mm/shmem.c | 5 +----
1 files changed, 1 insertions(+), 4 deletions(-)
diff --git a/mm/shmem.c b/mm/shmem.c
index d713239..a8f54f3 100644
--- a/mm/shmem.c
+++ b/mm/shmem.c
@@ -2307,17 +2307,14 @@ static int shmem_fill_super(struct super_block *sb,
int err = -ENOMEM;
/* Round up to L1_CACHE_BYTES to resist false sharing */
- sbinfo = kmalloc(max((int)sizeof(struct shmem_sb_info),
+ sbinfo = kzalloc(max((int)sizeof(struct shmem_sb_info),
L1_CACHE_BYTES), GFP_KERNEL);
if (!sbinfo)
return -ENOMEM;
- sbinfo->max_blocks = 0;
- sbinfo->max_inodes = 0;
sbinfo->mode = S_IRWXUGO | S_ISVTX;
sbinfo->uid = current_fsuid();
sbinfo->gid = current_fsgid();
- sbinfo->mpol = NULL;
sb->s_fs_info = sbinfo;
#ifdef CONFIG_TMPFS
--
1.5.6.4
Hi Hugh,
On Mon, 2009-09-21 at 11:49 +0100, Hugh Dickins wrote:
> I got a bit anxious when I saw that the mode arg to shmem_get_inode()
> is declared as an int: was afraid that compiler was then passing a bad
> upper half down, which in fact would cause no trouble, but how could it
> be sure of that? However, it looks okay: after doing the 32-bit load,
> it goes on to do a movzwl %ax,%eax - seems an odd way to proceed to me,
> but I bet it knows a lot more about efficiency of memory loads than I do.
Yeah, that's a common cause of kmemcheck false positives. I guess GCC
wants to avoid a partial register stall in the memory load and expects
CPU register renaming to work for "movzwl %ax, %eax" or something.
Pekka