Patchwork net: Fix corruption in /proc/*/net/dev_mcast

login
register
mail settings
Submitter Anton Blanchard
Date Nov. 28, 2011, 7:14 a.m.
Message ID <20111128181446.2ab784d0@kryten>
Download mbox | patch
Permalink /patch/127931/
State Accepted
Delegated to: David Miller
Headers show

Comments

Anton Blanchard - Nov. 28, 2011, 7:14 a.m.
Hi,

> I got the following output when running some tests (I'm not really sure
> what exactly happened when this bug was triggered):
> 
> [13850.947279] =============================================================================
> [13850.948024] BUG kmalloc-8: Redzone overwritten
> [13850.948024] -----------------------------------------------------------------------------
> [13850.948024] 
> [13850.948024] INFO: 0xffff8800104f6d28-0xffff8800104f6d2b. First byte 0x0 instead of 0xcc
> [13850.948024] INFO: Allocated in __seq_open_private+0x20/0x5e age=4436 cpu=0 pid=17295
> [13850.948024] 	__slab_alloc.clone.46+0x3e7/0x456
> [13850.948024] 	__kmalloc+0x8c/0x110
> [13850.948024] 	__seq_open_private+0x20/0x5e
> [13850.948024] 	seq_open_net+0x3b/0x5d
> [13850.948024] 	dev_mc_seq_open+0x15/0x17
> [13850.948024] 	proc_reg_open+0xad/0x127

I just hit this during my testing. Isn't there another bug lurking?

Anton
--


With slub debugging on I see red zone issues in /proc/*/net/dev_mcast:

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Eric Dumazet - Nov. 28, 2011, 9:55 a.m.
Le lundi 28 novembre 2011 à 18:14 +1100, Anton Blanchard a écrit :
> Hi,
> 
> > I got the following output when running some tests (I'm not really sure
> > what exactly happened when this bug was triggered):
> > 
> > [13850.947279] =============================================================================
> > [13850.948024] BUG kmalloc-8: Redzone overwritten
> > [13850.948024] -----------------------------------------------------------------------------
> > [13850.948024] 
> > [13850.948024] INFO: 0xffff8800104f6d28-0xffff8800104f6d2b. First byte 0x0 instead of 0xcc
> > [13850.948024] INFO: Allocated in __seq_open_private+0x20/0x5e age=4436 cpu=0 pid=17295
> > [13850.948024] 	__slab_alloc.clone.46+0x3e7/0x456
> > [13850.948024] 	__kmalloc+0x8c/0x110
> > [13850.948024] 	__seq_open_private+0x20/0x5e
> > [13850.948024] 	seq_open_net+0x3b/0x5d
> > [13850.948024] 	dev_mc_seq_open+0x15/0x17
> > [13850.948024] 	proc_reg_open+0xad/0x127
> 
> I just hit this during my testing. Isn't there another bug lurking?
> 
> Anton
> --
> 
> 
> With slub debugging on I see red zone issues in /proc/*/net/dev_mcast:
> 
> =============================================================================
> BUG kmalloc-8: Redzone overwritten
> -----------------------------------------------------------------------------
> 
> INFO: 0xc0000000de9dec48-0xc0000000de9dec4b. First byte 0x0 instead of 0xcc
> INFO: Allocated in .__seq_open_private+0x30/0xa0 age=0 cpu=5 pid=3896
> 	.__kmalloc+0x1e0/0x2d0
> 	.__seq_open_private+0x30/0xa0
> 	.seq_open_net+0x60/0xe0
> 	.dev_mc_seq_open+0x4c/0x70
> 	.proc_reg_open+0xd8/0x260
> 	.__dentry_open.clone.11+0x2b8/0x400
> 	.do_last+0xf4/0x950
> 	.path_openat+0xf8/0x480
> 	.do_filp_open+0x48/0xc0
> 	.do_sys_open+0x140/0x250
> 	syscall_exit+0x0/0x40
> 
> dev_mc_seq_ops uses dev_seq_start/next/stop but only allocates
> sizeof(struct seq_net_private) of private data, whereas it expects
> sizeof(struct dev_iter_state):
> 
> struct dev_iter_state {
> 	struct seq_net_private p;
> 	unsigned int pos; /* bucket << BUCKET_SPACE + offset */
> };
> 
> Create dev_seq_open_ops and use it so we don't have to expose
> struct dev_iter_state.
> 
> Signed-off-by: Anton Blanchard <anton@samba.org>
> ---
> 
> Index: linux-net/include/linux/netdevice.h
> ===================================================================
> --- linux-net.orig/include/linux/netdevice.h	2011-11-28 17:55:51.469508056 +1100
> +++ linux-net/include/linux/netdevice.h	2011-11-28 17:55:52.985535812 +1100
> @@ -2536,6 +2536,8 @@ extern void		net_disable_timestamp(void)
>  extern void *dev_seq_start(struct seq_file *seq, loff_t *pos);
>  extern void *dev_seq_next(struct seq_file *seq, void *v, loff_t *pos);
>  extern void dev_seq_stop(struct seq_file *seq, void *v);
> +extern int dev_seq_open_ops(struct inode *inode, struct file *file,
> +			    const struct seq_operations *ops);
>  #endif
>  
>  extern int netdev_class_create_file(struct class_attribute *class_attr);
> Index: linux-net/net/core/dev.c
> ===================================================================
> --- linux-net.orig/net/core/dev.c	2011-11-28 17:55:51.481508276 +1100
> +++ linux-net/net/core/dev.c	2011-11-28 17:55:52.989535885 +1100
> @@ -4282,6 +4282,12 @@ static int dev_seq_open(struct inode *in
>  			    sizeof(struct dev_iter_state));
>  }
>  
> +int dev_seq_open_ops(struct inode *inode, struct file *file,
> +		     const struct seq_operations *ops)
> +{
> +	return seq_open_net(inode, file, ops, sizeof(struct dev_iter_state));
> +}
> +
>  static const struct file_operations dev_seq_fops = {
>  	.owner	 = THIS_MODULE,
>  	.open    = dev_seq_open,
> Index: linux-net/net/core/dev_addr_lists.c
> ===================================================================
> --- linux-net.orig/net/core/dev_addr_lists.c	2011-11-28 17:55:47.845441705 +1100
> +++ linux-net/net/core/dev_addr_lists.c	2011-11-28 17:55:52.989535885 +1100
> @@ -696,8 +696,7 @@ static const struct seq_operations dev_m
>  
>  static int dev_mc_seq_open(struct inode *inode, struct file *file)
>  {
> -	return seq_open_net(inode, file, &dev_mc_seq_ops,
> -			    sizeof(struct seq_net_private));
> +	return dev_seq_open_ops(inode, file, &dev_mc_seq_ops);
>  }
>  
>  static const struct file_operations dev_mc_seq_fops = {


Good catch, thanks !

Problem added by commit f04565ddf52e4 (dev: use name hash for
dev_seq_ops)


Acked-by: Eric Dumazet <eric.dumazet@gmail.com>
CC: Mihai Maruseac <mihai.maruseac@gmail.com>



--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Daniel Baluta - Nov. 28, 2011, 10:40 a.m.
>> dev_mc_seq_ops uses dev_seq_start/next/stop but only allocates
>> sizeof(struct seq_net_private) of private data, whereas it expects
>> sizeof(struct dev_iter_state):
>>
>> struct dev_iter_state {
>>       struct seq_net_private p;
>>       unsigned int pos; /* bucket << BUCKET_SPACE + offset */
>> };
>>
>> Create dev_seq_open_ops and use it so we don't have to expose
>> struct dev_iter_state.

Good catch, indeed! We've now checked and this is the only place
where the problem happens.


>> +int dev_seq_open_ops(struct inode *inode, struct file *file,
>> +                  const struct seq_operations *ops)
>> +{
>> +     return seq_open_net(inode, file, ops, sizeof(struct dev_iter_state));
>> +}

Perhaps you could use this function also in dev_seq_open (dev.c:4280).

>>  static int dev_mc_seq_open(struct inode *inode, struct file *file)
>>  {
>> -     return seq_open_net(inode, file, &dev_mc_seq_ops,
>> -                         sizeof(struct seq_net_private));
>> +     return dev_seq_open_ops(inode, file, &dev_mc_seq_ops);
>>  }
>>
>>  static const struct file_operations dev_mc_seq_fops = {


thanks,
Daniel.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
David Miller - Nov. 28, 2011, 11:08 p.m.
From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Mon, 28 Nov 2011 10:55:16 +0100

>> With slub debugging on I see red zone issues in /proc/*/net/dev_mcast:
>> 
>> =============================================================================
>> BUG kmalloc-8: Redzone overwritten
>> -----------------------------------------------------------------------------
 ...
>> dev_mc_seq_ops uses dev_seq_start/next/stop but only allocates
>> sizeof(struct seq_net_private) of private data, whereas it expects
>> sizeof(struct dev_iter_state):
>> 
>> struct dev_iter_state {
>> 	struct seq_net_private p;
>> 	unsigned int pos; /* bucket << BUCKET_SPACE + offset */
>> };
>> 
>> Create dev_seq_open_ops and use it so we don't have to expose
>> struct dev_iter_state.
>> 
>> Signed-off-by: Anton Blanchard <anton@samba.org>
 ...
> Problem added by commit f04565ddf52e4 (dev: use name hash for
> dev_seq_ops)
> 
> 
> Acked-by: Eric Dumazet <eric.dumazet@gmail.com>
> CC: Mihai Maruseac <mihai.maruseac@gmail.com>

Applied, thanks everyone.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

=============================================================================
BUG kmalloc-8: Redzone overwritten
-----------------------------------------------------------------------------

INFO: 0xc0000000de9dec48-0xc0000000de9dec4b. First byte 0x0 instead of 0xcc
INFO: Allocated in .__seq_open_private+0x30/0xa0 age=0 cpu=5 pid=3896
	.__kmalloc+0x1e0/0x2d0
	.__seq_open_private+0x30/0xa0
	.seq_open_net+0x60/0xe0
	.dev_mc_seq_open+0x4c/0x70
	.proc_reg_open+0xd8/0x260
	.__dentry_open.clone.11+0x2b8/0x400
	.do_last+0xf4/0x950
	.path_openat+0xf8/0x480
	.do_filp_open+0x48/0xc0
	.do_sys_open+0x140/0x250
	syscall_exit+0x0/0x40

dev_mc_seq_ops uses dev_seq_start/next/stop but only allocates
sizeof(struct seq_net_private) of private data, whereas it expects
sizeof(struct dev_iter_state):

struct dev_iter_state {
	struct seq_net_private p;
	unsigned int pos; /* bucket << BUCKET_SPACE + offset */
};

Create dev_seq_open_ops and use it so we don't have to expose
struct dev_iter_state.

Signed-off-by: Anton Blanchard <anton@samba.org>
---

Index: linux-net/include/linux/netdevice.h
===================================================================
--- linux-net.orig/include/linux/netdevice.h	2011-11-28 17:55:51.469508056 +1100
+++ linux-net/include/linux/netdevice.h	2011-11-28 17:55:52.985535812 +1100
@@ -2536,6 +2536,8 @@  extern void		net_disable_timestamp(void)
 extern void *dev_seq_start(struct seq_file *seq, loff_t *pos);
 extern void *dev_seq_next(struct seq_file *seq, void *v, loff_t *pos);
 extern void dev_seq_stop(struct seq_file *seq, void *v);
+extern int dev_seq_open_ops(struct inode *inode, struct file *file,
+			    const struct seq_operations *ops);
 #endif
 
 extern int netdev_class_create_file(struct class_attribute *class_attr);
Index: linux-net/net/core/dev.c
===================================================================
--- linux-net.orig/net/core/dev.c	2011-11-28 17:55:51.481508276 +1100
+++ linux-net/net/core/dev.c	2011-11-28 17:55:52.989535885 +1100
@@ -4282,6 +4282,12 @@  static int dev_seq_open(struct inode *in
 			    sizeof(struct dev_iter_state));
 }
 
+int dev_seq_open_ops(struct inode *inode, struct file *file,
+		     const struct seq_operations *ops)
+{
+	return seq_open_net(inode, file, ops, sizeof(struct dev_iter_state));
+}
+
 static const struct file_operations dev_seq_fops = {
 	.owner	 = THIS_MODULE,
 	.open    = dev_seq_open,
Index: linux-net/net/core/dev_addr_lists.c
===================================================================
--- linux-net.orig/net/core/dev_addr_lists.c	2011-11-28 17:55:47.845441705 +1100
+++ linux-net/net/core/dev_addr_lists.c	2011-11-28 17:55:52.989535885 +1100
@@ -696,8 +696,7 @@  static const struct seq_operations dev_m
 
 static int dev_mc_seq_open(struct inode *inode, struct file *file)
 {
-	return seq_open_net(inode, file, &dev_mc_seq_ops,
-			    sizeof(struct seq_net_private));
+	return dev_seq_open_ops(inode, file, &dev_mc_seq_ops);
 }
 
 static const struct file_operations dev_mc_seq_fops = {