Null-pointer access in netback_uevent

Null-pointer access in netback_uevent

[Bastian Blank]

My slightly patched kernel based on
f6fe6583b77a49b569eef1b66c3d761eec2e561b failed with null-pointer access in
netback_uevent.

| BUG: unable to handle kernel NULL pointer dereference at 0000000000000150
| IP: [<ffffffff812017a3>] netback_uevent+0x83/0xaf

According to gdb the following is the culprit:

| 185             if (add_uevent_var(env, "vif=%s", netif->dev->name))
|    0x0000000000002313 <+131>:   mov    0x150(%r13),%rdx

Complete oops:

| BUG: unable to handle kernel NULL pointer dereference at 0000000000000150
| IP: [<ffffffff812017a3>] netback_uevent+0x83/0xaf
| PGD ce0b1067 PUD ce193067 PMD 0 
| Oops: 0000 [#1] SMP 
| last sysfs file: /sys/devices/vif-1-0/uevent
| CPU 1 
| Modules linked in: blktap xen_evtchn xenfs xt_tcpudp xt_state iptable_filter ipt_MASQUERADE iptable_nat nf_nat nf_conntrack_ipv4 nf_conntrack nf_defrag_ipv4 ip_tables x_tables bridge stp dm_snapshot loop snd_pcm snd_timer snd soundcore tpm_tis radeon snd_page_alloc ttm drm_kms_helper psmouse tpm drm pcspkr amd64_edac_mod ipmi_si tpm_bios evdev serio_raw edac_core i2c_algo_bit shpchp ipmi_msghandler i2c_piix4 edac_mce_amd container i2c_core hpilo processor pci_hotplug button acpi_processor hpwdt ext3 jbd mbcache dm_mod cciss ata_generic libata scsi_mod bnx2 thermal thermal_sys [last unloaded: xen_evtchn]
| Pid: 8076, comm: udevd Tainted: G        W  2.6.32-5-xen-amd64 #2 ProLiant DL385 G6   
| RIP: e030:[<ffffffff812017a3>]  [<ffffffff812017a3>] netback_uevent+0x83/0xaf
| RSP: e02b:ffff880002af7e18  EFLAGS: 00010246
| RAX: 01000000000000c1 RBX: ffff8800029f6000 RCX: 0000000000800078
| RDX: ffff8800c33b13a0 RSI: ffffea0002ab4eb8 RDI: 01000000000002c0
| RBP: ffff8800c33b14e0 R08: 0000000000000000 R09: ffffffff814664f0
| R10: 0000000000000200 R11: ffffffff8100f19c R12: ffff880002f39c00
| R13: 0000000000000000 R14: ffff8800021db000 R15: ffff8800c5026980
| FS:  00007fbd86ad2790(0000) GS:ffff880003a64000(0000) knlGS:0000000000000000
| CS:  e033 DS: 0000 ES: 0000 CR0: 000000008005003b
| CR2: 0000000000000150 CR3: 00000000025f4000 CR4: 0000000000000660
| DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
| DR3: 0000000000000000 DR6: 00000000ffff0ff0 DR7: 0000000000000400
| Process udevd (pid: 8076, threadinfo ffff880002af6000, task ffff88000246c6a0)
| Stack:
|  0000000000000908 ffff880002f39c40 ffff8800029f6000 ffff8800c3795c30
|  ffff8800029f6000 ffffffff8122d9f6 ffff880002f39c50 ffff8800c3795c30
|  ffffffff814a80d0 0000000000000000 ffff880002f39c50 ffffffff8122db35
| Call Trace:
|  [<ffffffff8122d9f6>] ? dev_uevent+0x104/0x146
|  [<ffffffff8122db35>] ? show_uevent+0x81/0xd5
|  [<ffffffff8122d6da>] ? dev_attr_show+0x1f/0x42
|  [<ffffffff8114074f>] ? sysfs_read_file+0xa7/0x125
|  [<ffffffff810f0a6e>] ? vfs_read+0xa6/0xff
|  [<ffffffff810f0b83>] ? sys_read+0x45/0x6e
|  [<ffffffff81011b42>] ? system_call_fastpath+0x16/0x1b
| Code: c7 c6 fe 1c 3f 81 31 c0 48 89 df e8 44 fe f8 ff 85 c0 74 0f 48 89 ef bb f4 ff ff ff e8 39 70 ee ff eb 2a 48 89 ef e8 2f 70 ee ff <49> 8b 95 50 01 00 00 48 89 df 31 c0 48 c7 c6 08 1d 3f 81 e8 11 
| RIP  [<ffffffff812017a3>] netback_uevent+0x83/0xaf
|  RSP <ffff880002af7e18>
| CR2: 0000000000000150
| ---[ end trace a7919e7f17c0a727 ]---

Bastian

-- 
Men will always be men -- no matter where they are.
		-- Harry Mudd, "Mudd's Women", stardate 1329.8

Re: Null-pointer access in netback_uevent

[Jeremy Fitzhardinge]

On 05/27/2010 09:55 AM, Bastian Blank wrote:
> My slightly patched kernel based on
> f6fe6583b77a49b569eef1b66c3d761eec2e561b failed with null-pointer access in
> netback_uevent.
>   

What were you doing at the time?

    J

> | BUG: unable to handle kernel NULL pointer dereference at 0000000000000150
> | IP: [<ffffffff812017a3>] netback_uevent+0x83/0xaf
>
> According to gdb the following is the culprit:
>
> | 185             if (add_uevent_var(env, "vif=%s", netif->dev->name))
> |    0x0000000000002313 <+131>:   mov    0x150(%r13),%rdx
>
> Complete oops:
>
> | BUG: unable to handle kernel NULL pointer dereference at 0000000000000150
> | IP: [<ffffffff812017a3>] netback_uevent+0x83/0xaf
> | PGD ce0b1067 PUD ce193067 PMD 0 
> | Oops: 0000 [#1] SMP 
> | last sysfs file: /sys/devices/vif-1-0/uevent
> | CPU 1 
> | Modules linked in: blktap xen_evtchn xenfs xt_tcpudp xt_state iptable_filter ipt_MASQUERADE iptable_nat nf_nat nf_conntrack_ipv4 nf_conntrack nf_defrag_ipv4 ip_tables x_tables bridge stp dm_snapshot loop snd_pcm snd_timer snd soundcore tpm_tis radeon snd_page_alloc ttm drm_kms_helper psmouse tpm drm pcspkr amd64_edac_mod ipmi_si tpm_bios evdev serio_raw edac_core i2c_algo_bit shpchp ipmi_msghandler i2c_piix4 edac_mce_amd container i2c_core hpilo processor pci_hotplug button acpi_processor hpwdt ext3 jbd mbcache dm_mod cciss ata_generic libata scsi_mod bnx2 thermal thermal_sys [last unloaded: xen_evtchn]
> | Pid: 8076, comm: udevd Tainted: G        W  2.6.32-5-xen-amd64 #2 ProLiant DL385 G6   
> | RIP: e030:[<ffffffff812017a3>]  [<ffffffff812017a3>] netback_uevent+0x83/0xaf
> | RSP: e02b:ffff880002af7e18  EFLAGS: 00010246
> | RAX: 01000000000000c1 RBX: ffff8800029f6000 RCX: 0000000000800078
> | RDX: ffff8800c33b13a0 RSI: ffffea0002ab4eb8 RDI: 01000000000002c0
> | RBP: ffff8800c33b14e0 R08: 0000000000000000 R09: ffffffff814664f0
> | R10: 0000000000000200 R11: ffffffff8100f19c R12: ffff880002f39c00
> | R13: 0000000000000000 R14: ffff8800021db000 R15: ffff8800c5026980
> | FS:  00007fbd86ad2790(0000) GS:ffff880003a64000(0000) knlGS:0000000000000000
> | CS:  e033 DS: 0000 ES: 0000 CR0: 000000008005003b
> | CR2: 0000000000000150 CR3: 00000000025f4000 CR4: 0000000000000660
> | DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
> | DR3: 0000000000000000 DR6: 00000000ffff0ff0 DR7: 0000000000000400
> | Process udevd (pid: 8076, threadinfo ffff880002af6000, task ffff88000246c6a0)
> | Stack:
> |  0000000000000908 ffff880002f39c40 ffff8800029f6000 ffff8800c3795c30
> |  ffff8800029f6000 ffffffff8122d9f6 ffff880002f39c50 ffff8800c3795c30
> |  ffffffff814a80d0 0000000000000000 ffff880002f39c50 ffffffff8122db35
> | Call Trace:
> |  [<ffffffff8122d9f6>] ? dev_uevent+0x104/0x146
> |  [<ffffffff8122db35>] ? show_uevent+0x81/0xd5
> |  [<ffffffff8122d6da>] ? dev_attr_show+0x1f/0x42
> |  [<ffffffff8114074f>] ? sysfs_read_file+0xa7/0x125
> |  [<ffffffff810f0a6e>] ? vfs_read+0xa6/0xff
> |  [<ffffffff810f0b83>] ? sys_read+0x45/0x6e
> |  [<ffffffff81011b42>] ? system_call_fastpath+0x16/0x1b
> | Code: c7 c6 fe 1c 3f 81 31 c0 48 89 df e8 44 fe f8 ff 85 c0 74 0f 48 89 ef bb f4 ff ff ff e8 39 70 ee ff eb 2a 48 89 ef e8 2f 70 ee ff <49> 8b 95 50 01 00 00 48 89 df 31 c0 48 c7 c6 08 1d 3f 81 e8 11 
> | RIP  [<ffffffff812017a3>] netback_uevent+0x83/0xaf
> |  RSP <ffff880002af7e18>
> | CR2: 0000000000000150
> | ---[ end trace a7919e7f17c0a727 ]---
>
> Bastian
>
>   

Re: Re: Null-pointer access in netback_uevent

[Bastian Blank]

On Thu, May 27, 2010 at 02:03:15PM -0700, Jeremy Fitzhardinge wrote:
> On 05/27/2010 09:55 AM, Bastian Blank wrote:
> > My slightly patched kernel based on
> > f6fe6583b77a49b569eef1b66c3d761eec2e561b failed with null-pointer access in
> > netback_uevent.
> What were you doing at the time?

Domain creation.

Bastian

-- 
	"What terrible way to die."
	"There are no good ways."
		-- Sulu and Kirk, "That Which Survives", stardate unknown

Re: Re: Null-pointer access in netback_uevent

[Jeremy Fitzhardinge]

On 05/27/2010 03:27 PM, Bastian Blank wrote:
> On Thu, May 27, 2010 at 02:03:15PM -0700, Jeremy Fitzhardinge wrote:
>   
>> On 05/27/2010 09:55 AM, Bastian Blank wrote:
>>     
>>> My slightly patched kernel based on
>>> f6fe6583b77a49b569eef1b66c3d761eec2e561b failed with null-pointer access in
>>> netback_uevent.
>>>       
>> What were you doing at the time?
>>     
> Domain creation.
>   

Using the xm toolset?  Has it always crashed, or was there some change
which made it start happening?  Or is it an erratic failure?

    J

Re: Re: Null-pointer access in netback_uevent

[Bastian Blank]

On Thu, May 27, 2010 at 03:39:16PM -0700, Jeremy Fitzhardinge wrote:
> On 05/27/2010 03:27 PM, Bastian Blank wrote:
> > Domain creation.
> Using the xm toolset?  Has it always crashed, or was there some change
> which made it start happening?  Or is it an erratic failure?

xm create.

However looking at the code shows it as obvious. backend_info->netif is
only filled while in state connected. netback_uevent does not check if
it is setup but can be called at all times.

Bastian

-- 
Earth -- mother of the most beautiful women in the universe.
		-- Apollo, "Who Mourns for Adonais?" stardate 3468.1

RE: Null-pointer access in netback_uevent

[James Harper]

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

I noticed this just after I posted (subject "oops starting domain on 4.0.0  + 2.6.32.13-gf6fe658"). I'm getting the same error.

James

> -----Original Message-----
> From: xen-devel-bounces@lists.xensource.com [mailto:xen-devel-
> bounces@lists.xensource.com] On Behalf Of Bastian Blank
> Sent: Friday, 28 May 2010 02:56
> To: xen-devel@lists.xensource.com
> Cc: Jeremy Fitzhardinge
> Subject: [Xen-devel] Null-pointer access in netback_uevent
> 
> My slightly patched kernel based on
> f6fe6583b77a49b569eef1b66c3d761eec2e561b failed with null-pointer access in
> netback_uevent.
> 
> | BUG: unable to handle kernel NULL pointer dereference at 0000000000000150
> | IP: [<ffffffff812017a3>] netback_uevent+0x83/0xaf
> 
> According to gdb the following is the culprit:
> 
> | 185             if (add_uevent_var(env, "vif=%s", netif->dev->name))
> |    0x0000000000002313 <+131>:   mov    0x150(%r13),%rdx
> 
> Complete oops:
> 
> | BUG: unable to handle kernel NULL pointer dereference at 0000000000000150
> | IP: [<ffffffff812017a3>] netback_uevent+0x83/0xaf
> | PGD ce0b1067 PUD ce193067 PMD 0
> | Oops: 0000 [#1] SMP
> | last sysfs file: /sys/devices/vif-1-0/uevent
> | CPU 1
> | Modules linked in: blktap xen_evtchn xenfs xt_tcpudp xt_state iptable_filter
> ipt_MASQUERADE iptable_nat nf_nat nf_conntrack_ipv4 nf_conntrack
> nf_defrag_ipv4 ip_tables x_tables bridge stp dm_snapshot loop snd_pcm
> snd_timer snd soundcore tpm_tis radeon snd_page_alloc ttm drm_kms_helper
> psmouse tpm drm pcspkr amd64_edac_mod ipmi_si tpm_bios evdev serio_raw
> edac_core i2c_algo_bit shpchp ipmi_msghandler i2c_piix4 edac_mce_amd container
> i2c_core hpilo processor pci_hotplug button acpi_processor hpwdt ext3 jbd
> mbcache dm_mod cciss ata_generic libata scsi_mod bnx2 thermal thermal_sys
> [last unloaded: xen_evtchn]
> | Pid: 8076, comm: udevd Tainted: G        W  2.6.32-5-xen-amd64 #2 ProLiant
> DL385 G6
> | RIP: e030:[<ffffffff812017a3>]  [<ffffffff812017a3>]
> netback_uevent+0x83/0xaf
> | RSP: e02b:ffff880002af7e18  EFLAGS: 00010246
> | RAX: 01000000000000c1 RBX: ffff8800029f6000 RCX: 0000000000800078
> | RDX: ffff8800c33b13a0 RSI: ffffea0002ab4eb8 RDI: 01000000000002c0
> | RBP: ffff8800c33b14e0 R08: 0000000000000000 R09: ffffffff814664f0
> | R10: 0000000000000200 R11: ffffffff8100f19c R12: ffff880002f39c00
> | R13: 0000000000000000 R14: ffff8800021db000 R15: ffff8800c5026980
> | FS:  00007fbd86ad2790(0000) GS:ffff880003a64000(0000) knlGS:0000000000000000
> | CS:  e033 DS: 0000 ES: 0000 CR0: 000000008005003b
> | CR2: 0000000000000150 CR3: 00000000025f4000 CR4: 0000000000000660
> | DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
> | DR3: 0000000000000000 DR6: 00000000ffff0ff0 DR7: 0000000000000400
> | Process udevd (pid: 8076, threadinfo ffff880002af6000, task
> ffff88000246c6a0)
> | Stack:
> |  0000000000000908 ffff880002f39c40 ffff8800029f6000 ffff8800c3795c30
> |  ffff8800029f6000 ffffffff8122d9f6 ffff880002f39c50 ffff8800c3795c30
> |  ffffffff814a80d0 0000000000000000 ffff880002f39c50 ffffffff8122db35
> | Call Trace:
> |  [<ffffffff8122d9f6>] ? dev_uevent+0x104/0x146
> |  [<ffffffff8122db35>] ? show_uevent+0x81/0xd5
> |  [<ffffffff8122d6da>] ? dev_attr_show+0x1f/0x42
> |  [<ffffffff8114074f>] ? sysfs_read_file+0xa7/0x125
> |  [<ffffffff810f0a6e>] ? vfs_read+0xa6/0xff
> |  [<ffffffff810f0b83>] ? sys_read+0x45/0x6e
> |  [<ffffffff81011b42>] ? system_call_fastpath+0x16/0x1b
> | Code: c7 c6 fe 1c 3f 81 31 c0 48 89 df e8 44 fe f8 ff 85 c0 74 0f 48 89 ef
> bb f4 ff ff ff e8 39 70 ee ff eb 2a 48 89 ef e8 2f 70 ee ff <49> 8b 95 50 01
> 00 00 48 89 df 31 c0 48 c7 c6 08 1d 3f 81 e8 11
> | RIP  [<ffffffff812017a3>] netback_uevent+0x83/0xaf
> |  RSP <ffff880002af7e18>
> | CR2: 0000000000000150
> | ---[ end trace a7919e7f17c0a727 ]---
> 
> Bastian
> 
> --
> Men will always be men -- no matter where they are.
> 		-- Harry Mudd, "Mudd's Women", stardate 1329.8
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xensource.com
> http://lists.xensource.com/xen-devel

[-- Attachment #2: Type: text/plain, Size: 138 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

RE: Null-pointer access in netback_uevent

[James Harper]

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

> 
> I noticed this just after I posted (subject "oops starting domain on 4.0.0  +
> 2.6.32.13-gf6fe658"). I'm getting the same error.
> 

Actually my error is occurring because 'be' is NULL in netback_uevent (eg dev_get_drvdata returns NULL)

James

[-- Attachment #2: Type: text/plain, Size: 138 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

RE: Null-pointer access in netback_uevent

[James Harper]

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

> 
> >
> > I noticed this just after I posted (subject "oops starting domain on 4.0.0
> > + 2.6.32.13-gf6fe658"). I'm getting the same error.
> >
> 
> Actually my error is occurring because 'be' is NULL in netback_uevent (eg
> dev_get_drvdata returns NULL)
> 

Looking into this further, I suspect that the trigger here is a newer version of udev or something in that area. netback_uevent is getting called before the call to netback_probe containing the call to dev_set_drvdata.

James

[-- Attachment #2: Type: text/plain, Size: 138 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Re: Null-pointer access in netback_uevent

[Bastian Blank]

On Fri, May 28, 2010 at 10:47:48AM +1000, James Harper wrote:
> > > I noticed this just after I posted (subject "oops starting domain on 4.0.0
> > > + 2.6.32.13-gf6fe658"). I'm getting the same error.
> > Actually my error is occurring because 'be' is NULL in netback_uevent (eg
> > dev_get_drvdata returns NULL)
> Looking into this further, I suspect that the trigger here is a newer version of udev or something in that area. netback_uevent is getting called before the call to netback_probe containing the call to dev_set_drvdata.

There are two problems there. And yes, the uevent routine can be called
at any time.

Bastian

-- 
You!  What PLANET is this!
		-- McCoy, "The City on the Edge of Forever", stardate 3134.0

Re: Null-pointer access in netback_uevent

[Jeremy Fitzhardinge]

On 05/28/2010 02:03 AM, Bastian Blank wrote:
> On Fri, May 28, 2010 at 10:47:48AM +1000, James Harper wrote:
>   
>>>> I noticed this just after I posted (subject "oops starting domain on 4.0.0
>>>> + 2.6.32.13-gf6fe658"). I'm getting the same error.
>>>>         
>>> Actually my error is occurring because 'be' is NULL in netback_uevent (eg
>>> dev_get_drvdata returns NULL)
>>>       
>> Looking into this further, I suspect that the trigger here is a newer version of udev or something in that area. netback_uevent is getting called before the call to netback_probe containing the call to dev_set_drvdata.
>>     
> There are two problems there. And yes, the uevent routine can be called
> at any time.
>   

So would it be correct for it to just return if either of those are
NULL?  Will it get called again later once the device info is available?

    J

RE: Null-pointer access in netback_uevent

[James Harper]

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

> > > Looking into this further, I suspect that the trigger here is a newer
> > > version of udev or something in that area. netback_uevent is getting called
> > > before the call to netback_probe containing the call to dev_set_drvdata.
> > >
> > There are two problems there. And yes, the uevent routine can be called
> > at any time.
> >
> 
> So would it be correct for it to just return if either of those are
> NULL?

Not sure. It works though.

>  Will it get called again later once the device info is available?

It does get called again later when the device info has been populated. I'm not sure if we can count on that though.

James


[-- Attachment #2: Type: text/plain, Size: 138 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Re: Null-pointer access in netback_uevent

[Jeremy Fitzhardinge]

On 05/28/2010 03:42 PM, James Harper wrote:
>>>> Looking into this further, I suspect that the trigger here is a newer
>>>> version of udev or something in that area. netback_uevent is getting called
>>>> before the call to netback_probe containing the call to dev_set_drvdata.
>>>>
>>>>         
>>> There are two problems there. And yes, the uevent routine can be called
>>> at any time.
>>>
>>>       
>> So would it be correct for it to just return if either of those are
>> NULL?
>>     
> Not sure. It works though.
>   

Can you send me a properly signed-off patch?

Thanks,
    J

RE: Null-pointer access in netback_uevent

[James Harper]

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

Check if drvdata has been set up yet and return if it hasn't

Signed-off-by: James Harper <james.harper@bendigoit.com.au>
---
 drivers/xen/netback/xenbus.c |    8 ++++++--
 1 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/xen/netback/xenbus.c b/drivers/xen/netback/xenbus.c
index 70636d0..a46d8b2 100644
--- a/drivers/xen/netback/xenbus.c
+++ b/drivers/xen/netback/xenbus.c
@@ -162,12 +162,16 @@ fail:
  */
 static int netback_uevent(struct xenbus_device *xdev, struct kobj_uevent_env *env)
 {
-       struct backend_info *be = dev_get_drvdata(&xdev->dev);
-       struct xen_netif *netif = be->netif;
+       struct backend_info *be;
+       struct xen_netif *netif;
        char *val;

        DPRINTK("netback_uevent");

+       be = dev_get_drvdata(&xdev->dev);
+       if (!be)
+               return 0;
+       netif = be->netif;
        val = xenbus_read(XBT_NIL, xdev->nodename, "script", NULL);
        if (IS_ERR(val)) {
                int err = PTR_ERR(val);
--
1.7.1


> -----Original Message-----
> From: Jeremy Fitzhardinge [mailto:jeremy@goop.org]
> Sent: Saturday, 29 May 2010 09:31
> To: James Harper
> Cc: Bastian Blank; xen-devel@lists.xensource.com
> Subject: Re: [Xen-devel] Null-pointer access in netback_uevent
> 
> On 05/28/2010 03:42 PM, James Harper wrote:
> >>>> Looking into this further, I suspect that the trigger here is a newer
> >>>> version of udev or something in that area. netback_uevent is getting
> called
> >>>> before the call to netback_probe containing the call to dev_set_drvdata.
> >>>>
> >>>>
> >>> There are two problems there. And yes, the uevent routine can be called
> >>> at any time.
> >>>
> >>>
> >> So would it be correct for it to just return if either of those are
> >> NULL?
> >>
> > Not sure. It works though.
> >
> 
> Can you send me a properly signed-off patch?
> 
> Thanks,
>     J

[-- Attachment #2: Type: text/plain, Size: 138 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Re: Null-pointer access in netback_uevent

[Bastian Blank]

On Sat, May 29, 2010 at 09:59:22AM +1000, James Harper wrote:
> Check if drvdata has been set up yet and return if it hasn't

This patch is incomplete. Will send another one later.

Bastian

-- 
Not one hundred percent efficient, of course ... but nothing ever is.
		-- Kirk, "Metamorphosis", stardate 3219.8

[PATCH] xen/netback: Fix null-pointer access in netback_uevent

[Bastian Blank]

The uevent method of Xen netback does not check if the the network
device is already setup and tries to dereference a null-pointer it not.

Signed-off-by: Bastian Blank <waldi@debian.org>
---
 drivers/xen/netback/xenbus.c |    3 +--
 1 files changed, 1 insertions(+), 2 deletions(-)

diff --git a/drivers/xen/netback/xenbus.c b/drivers/xen/netback/xenbus.c
index 70636d0..88262bb 100644
--- a/drivers/xen/netback/xenbus.c
+++ b/drivers/xen/netback/xenbus.c
@@ -163,7 +163,6 @@ fail:
 static int netback_uevent(struct xenbus_device *xdev, struct kobj_uevent_env *env)
 {
 	struct backend_info *be = dev_get_drvdata(&xdev->dev);
-	struct xen_netif *netif = be->netif;
 	char *val;
 
 	DPRINTK("netback_uevent");
@@ -182,7 +181,7 @@ static int netback_uevent(struct xenbus_device *xdev, struct kobj_uevent_env *en
 		kfree(val);
 	}
 
-	if (add_uevent_var(env, "vif=%s", netif->dev->name))
+	if (be && be->netif && add_uevent_var(env, "vif=%s", be->netif->dev->name))
 		return -ENOMEM;
 
 	return 0;
-- 
1.7.1

-- 
To live is always desirable.
		-- Eleen the Capellan, "Friday's Child", stardate 3498.9

Re: [PATCH] xen/netback: Fix null-pointer access in netback_uevent

[Jan Beulich]

>>> On 29.05.10 at 20:44, Bastian Blank <waldi@debian.org> wrote:
> The uevent method of Xen netback does not check if the the network
> device is already setup and tries to dereference a null-pointer it not.
> 
> Signed-off-by: Bastian Blank <waldi@debian.org>
> ---
>  drivers/xen/netback/xenbus.c |    3 +--
>  1 files changed, 1 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/xen/netback/xenbus.c b/drivers/xen/netback/xenbus.c
> index 70636d0..88262bb 100644
> --- a/drivers/xen/netback/xenbus.c
> +++ b/drivers/xen/netback/xenbus.c
> @@ -163,7 +163,6 @@ fail:
>  static int netback_uevent(struct xenbus_device *xdev, struct 
> kobj_uevent_env *env)
>  {
>  	struct backend_info *be = dev_get_drvdata(&xdev->dev);
> -	struct xen_netif *netif = be->netif;
>  	char *val;
>  
>  	DPRINTK("netback_uevent");
> @@ -182,7 +181,7 @@ static int netback_uevent(struct xenbus_device *xdev, 
> struct kobj_uevent_env *en
>  		kfree(val);
>  	}
>  
> -	if (add_uevent_var(env, "vif=%s", netif->dev->name))
> +	if (be && be->netif && add_uevent_var(env, "vif=%s", be->netif->dev->name))
>  		return -ENOMEM;
>  
>  	return 0;

Unfortunately this still seems incomplete: Just checking be->netif to
be non-NULL isn't sufficient, as the sysfs access may race backend
teardown afaics. Hence proper serialization is going to be needed for
that case.

Furthermore, the backend creation patch also needs adjustment,
as it currently stores a non-NULL non-pointer value in be->netif if
netif_alloc() fails. To require the sysfs path to use IS_ERR() on
be->netif, I think netif_alloc()'s result should be stored to a local 
variable first and only written to be->netif when valid.

Jan

Re: [PATCH] xen/netback: Fix null-pointer access in netback_uevent

[Jan Beulich]

>>> On 31.05.10 at 09:37, "Jan Beulich" <JBeulich@novell.com> wrote:
> Furthermore, the backend creation patch also needs adjustment,

... path ...

> as it currently stores a non-NULL non-pointer value in be->netif if
> netif_alloc() fails. To require the sysfs path to use IS_ERR() on

... To *not* require ...

> be->netif, I think netif_alloc()'s result should be stored to a local 
> variable first and only written to be->netif when valid.
 
Jan

Re: [PATCH] xen/netback: Fix null-pointer access in netback_uevent

[Bastian Blank]

On Mon, May 31, 2010 at 09:07:04AM +0100, Jan Beulich wrote:
> >>> On 31.05.10 at 09:37, "Jan Beulich" <JBeulich@novell.com> wrote:
> > Furthermore, the backend creation patch also needs adjustment,
> ... path ...
> > as it currently stores a non-NULL non-pointer value in be->netif if
> > netif_alloc() fails. To require the sysfs path to use IS_ERR() on
> ... To *not* require ...
> > be->netif, I think netif_alloc()'s result should be stored to a local 
> > variable first and only written to be->netif when valid.
 
78b55f90e72348e231092dbe3e50ac7414b9e1af still fails:

| BUG: unable to handle kernel NULL pointer dereference at 0000000000000150
| IP: [<ffffffff81200764>] netback_uevent+0x8c/0xb9
| PGD 1fe347067 PUD 1fa575067 PMD 0 
| Oops: 0000 [#3] SMP 
| last sysfs file: /sys/devices/vif-16-0/uevent
| CPU 4 
| Modules linked in: xen_evtchn xenfs xt_tcpudp xt_state iptable_filter ipt_MASQUERADE iptable_nat nf_nat nf_conntr
| ntrack nf_defrag_ipv4 ip_tables x_tables bridge stp dm_snapshot loop snd_pcm snd_timer radeon snd ttm drm_kms_hel
| re amd64_edac_mod snd_page_alloc i2c_algo_bit i2c_piix4 ipmi_si edac_core ipmi_msghandler psmouse shpchp edac_mce
| _core evdev serio_raw container button hpwdt pci_hotplug hpilo processor acpi_processor ext3 jbd mbcache dm_mod b
| te crc32c libcrc32c sg usbhid hid sr_mod cdrom ata_generic sata_svw uhci_hcd cciss ohci_hcd libata bnx2 scsi_mod 
| e thermal nls_base thermal_sys [last unloaded: scsi_wait_scan]
| Pid: 596, comm: udevd Tainted: G      D W  2.6.32-5-xen-amd64 #1 ProLiant DL385 G6   
| RIP: e030:[<ffffffff81200764>]  [<ffffffff81200764>] netback_uevent+0x8c/0xb9
| RSP: e02b:ffff8801faa89e18  EFLAGS: 00010246
| RAX: 02000000000000c1 RBX: ffff8801f2345f00 RCX: 000000000080007d
| RDX: ffff8801f2345f40 RSI: ffffea0006cfb718 RDI: 02000000000002c0
| RBP: ffff8801bd34e000 R08: 0000000000000000 R09: ffffffff8146a4f0
| R10: 000000000000020a R11: ffffffff8100f00c R12: ffff8801fc5af800
| R13: 0000000000000000 R14: ffff8801bd038000 R15: ffff8801fce1ab00
| FS:  00007f7d1072f7a0(0000) GS:ffff88000b0be000(0000) knlGS:0000000000000000
| CS:  e033 DS: 0000 ES: 0000 CR0: 000000008005003b
| CR2: 0000000000000150 CR3: 00000001fe38c000 CR4: 0000000000000660
| DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
| DR3: 0000000000000000 DR6: 00000000ffff0ff0 DR7: 0000000000000400
| Process udevd (pid: 596, threadinfo ffff8801faa88000, task ffff8801fc0ab880)
| Stack:
|  0000000000000908 ffff8801fc5af840 ffff8801bd34e000 ffff8801f65a0b40
| <0> ffff8801bd34e000 ffffffff8122cac2 ffff8801fc5af850 ffff8801f65a0b40
| <0> ffffffff814ac170 0000000000000000 ffff8801fc5af850 ffffffff8122cc01
| Call Trace:
|  [<ffffffff8122cac2>] ? dev_uevent+0x104/0x146
|  [<ffffffff8122cc01>] ? show_uevent+0x81/0xd5
|  [<ffffffff8122c7a6>] ? dev_attr_show+0x1f/0x42
|  [<ffffffff8113f413>] ? sysfs_read_file+0xa7/0x125
|  [<ffffffff810ef95a>] ? vfs_read+0xa6/0xff
|  [<ffffffff810efa6f>] ? sys_read+0x45/0x6e
|  [<ffffffff81011b42>] ? system_call_fastpath+0x16/0x1b
| Code: c7 c6 7a 4a 3f 81 31 c0 48 89 ef e8 a3 fb f8 ff 85 c0 74 0f 48 89 df bd f4 ff ff ff e8 60 6f ee ff eb 2b 48
|  ee ff <49> 8b 95 50 01 00 00 48 89 ef 31 c0 48 c7 c6 84 4a 3f 81 bd f4 
| RIP  [<ffffffff81200764>] netback_uevent+0x8c/0xb9
|  RSP <ffff8801faa89e18>
| CR2: 0000000000000150
| ---[ end trace a7919e7f17c0a729 ]---

Bastian

-- 
Each kiss is as the first.
		-- Miramanee, Kirk's wife, "The Paradise Syndrome",
		   stardate 4842.6

Re: [PATCH] xen/netback: Fix null-pointer access in netback_uevent

[Ian Campbell]

On Wed, 2010-07-28 at 17:20 +0100, Bastian Blank wrote:
> On Mon, May 31, 2010 at 09:07:04AM +0100, Jan Beulich wrote:
> > >>> On 31.05.10 at 09:37, "Jan Beulich" <JBeulich@novell.com> wrote:
> > > Furthermore, the backend creation patch also needs adjustment,
> > ... path ...
> > > as it currently stores a non-NULL non-pointer value in be->netif if
> > > netif_alloc() fails. To require the sysfs path to use IS_ERR() on
> > ... To *not* require ...
> > > be->netif, I think netif_alloc()'s result should be stored to a local 
> > > variable first and only written to be->netif when valid.
>  
> 78b55f90e72348e231092dbe3e50ac7414b9e1af still fails:
> 
> | BUG: unable to handle kernel NULL pointer dereference at 0000000000000150
> | IP: [<ffffffff81200764>] netback_uevent+0x8c/0xb9

I tried to reproduce this on squeeze (installed this morning) but
without luck. I'm running on amd64 with the hypervisor and tools from
squeeze and your
linux-image-2.6.32-5-xen-amd64_2.6.32-19~xen.2_amd64.deb test package
(md5 e3bb29be9588fa75b7740c046352b15b).

I tried pulling in the udev from sid too (since I think udev version has
been implicated in triggering this issue) but that didn't help
reproduce.

Did you apply a fix/workaround to your package? I'm about to try with a
pristine 78b55f90e72348e231092dbe3e50ac7414b9e1af from xen.git but any
tips on reproducing gratefully accepted.

My test VM is running the Lenny installer with the following cfg:

Ian.

#  -*- mode: python; -*-
vcpus = 2

memory = 128

name = "d32-1"
vif = [ 'mac=00:16:3e:72:42:06' ]
disk = ['file:/vm/debian-x86_32-1.img,xvda,w']

hostname = "debian-1"

tsc_mode=2
extra = "xencons=hvc console=hvc0 ro root=/dev/xvda1" #

# Install...
extra = "debian-installer/exit/always_halt=true -- " + extra + " auto-install/enable=true hostname=d32-1 domain=uk.xensource.com url=http://cosworth/users/ianc/ks/lenny.cfg"
kernel = "/scratch/lenny/i386/vmlinuz"
ramdisk = "/scratch/lenny/i386/initrd.gz"

on_crash = "coredump-destroy"
on_reboot = "restart"

Re: [PATCH] xen/netback: Fix null-pointer access in netback_uevent

[Bastian Blank]

On Thu, Jul 29, 2010 at 12:47:15PM +0100, Ian Campbell wrote:
> On Wed, 2010-07-28 at 17:20 +0100, Bastian Blank wrote:
> > 78b55f90e72348e231092dbe3e50ac7414b9e1af still fails:
> > | BUG: unable to handle kernel NULL pointer dereference at 0000000000000150
> > | IP: [<ffffffff81200764>] netback_uevent+0x8c/0xb9
> I tried to reproduce this on squeeze (installed this morning) but
> without luck. I'm running on amd64 with the hypervisor and tools from
> squeeze and your
> linux-image-2.6.32-5-xen-amd64_2.6.32-19~xen.2_amd64.deb test package
> (md5 e3bb29be9588fa75b7740c046352b15b).

Its a race condition, so you may or may not see it. It is rather good
reproducible on my test machine with 12 cores and a lot of guest
creations running at once (32 to be exact in my case).

> Did you apply a fix/workaround to your package?

Yes, this is included since 2.6.32-15.

>                                                 I'm about to try with a
> pristine 78b55f90e72348e231092dbe3e50ac7414b9e1af from xen.git but any
> tips on reproducing gratefully accepted.

Try linux-image-2.6.32-5-xen-amd64_2.6.32-19~xen.1_amd64.deb, the only
difference to the newer is that the fix is again included.

Bastian

-- 
	"Get back to your stations!"
	"We're beaming down to the planet, sir."
		-- Kirk and Mr. Leslie, "This Side of Paradise",
		   stardate 3417.3

Re: [PATCH] xen/netback: Fix null-pointer access in netback_uevent

[Ian Campbell]

On Thu, 2010-07-29 at 13:42 +0100, Bastian Blank wrote:
> On Thu, Jul 29, 2010 at 12:47:15PM +0100, Ian Campbell wrote:
> > On Wed, 2010-07-28 at 17:20 +0100, Bastian Blank wrote:
> > > 78b55f90e72348e231092dbe3e50ac7414b9e1af still fails:
> > > | BUG: unable to handle kernel NULL pointer dereference at 0000000000000150
> > > | IP: [<ffffffff81200764>] netback_uevent+0x8c/0xb9
> > I tried to reproduce this on squeeze (installed this morning) but
> > without luck. I'm running on amd64 with the hypervisor and tools from
> > squeeze and your
> > linux-image-2.6.32-5-xen-amd64_2.6.32-19~xen.2_amd64.deb test package
> > (md5 e3bb29be9588fa75b7740c046352b15b).
> 
> Its a race condition, so you may or may not see it. It is rather good
> reproducible on my test machine with 12 cores and a lot of guest
> creations running at once (32 to be exact in my case).

Thanks, looks like I need to get much more aggressive then.

> > Did you apply a fix/workaround to your package?
> 
> Yes, this is included since 2.6.32-15.

The patch from <20100529184452.GA18365@wavehammer.waldi.eu.org>?

I think the only reason this hasn't gone in already are the additional
concerns which Jan raised in
<4C0383670200007800004B63@vpn.id2.novell.com>. 

You may be inteested in the patch attached to
http://bugzilla.xensource.com/bugzilla/show_bug.cgi?id=1612

Jeremy, given that Bastian's patch seems to help and is correct, if
potentially incomplete, perhaps we should put it in now rather than
waiting for the complete fix?

> >                                                 I'm about to try with a
> > pristine 78b55f90e72348e231092dbe3e50ac7414b9e1af from xen.git but any
> > tips on reproducing gratefully accepted.
> 
> Try linux-image-2.6.32-5-xen-amd64_2.6.32-19~xen.1_amd64.deb, the only
> difference to the newer is that the fix is again included.

I'll try that. Thanks.

Ian.

Re: [PATCH] xen/netback: Fix null-pointer access in netback_uevent

[Ian Campbell]

On Thu, 2010-07-29 at 14:44 +0100, Ian Campbell wrote:
> On Thu, 2010-07-29 at 13:42 +0100, Bastian Blank wrote:
> > On Thu, Jul 29, 2010 at 12:47:15PM +0100, Ian Campbell wrote:
> > > On Wed, 2010-07-28 at 17:20 +0100, Bastian Blank wrote:
> > > > 78b55f90e72348e231092dbe3e50ac7414b9e1af still fails:
> > > > | BUG: unable to handle kernel NULL pointer dereference at 0000000000000150
> > > > | IP: [<ffffffff81200764>] netback_uevent+0x8c/0xb9
> > > I tried to reproduce this on squeeze (installed this morning) but
> > > without luck. I'm running on amd64 with the hypervisor and tools from
> > > squeeze and your
> > > linux-image-2.6.32-5-xen-amd64_2.6.32-19~xen.2_amd64.deb test package
> > > (md5 e3bb29be9588fa75b7740c046352b15b).
> > 
> > Its a race condition, so you may or may not see it. It is rather good
> > reproducible on my test machine with 12 cores and a lot of guest
> > creations running at once (32 to be exact in my case).
> 
> Thanks, looks like I need to get much more aggressive then.

A single guest with 16 vifs was sufficient to repro... 

Thanks,
Ian.

Re: [PATCH] xen/netback: Fix null-pointer access in netback_uevent

[Bastian Blank]

On Thu, Jul 29, 2010 at 02:44:36PM +0100, Ian Campbell wrote:
> On Thu, 2010-07-29 at 13:42 +0100, Bastian Blank wrote:
> > Yes, this is included since 2.6.32-15.
> The patch from <20100529184452.GA18365@wavehammer.waldi.eu.org>?

Currently the following patch is used. The next test should be if it is
really necessary to have this much different structures, a quick look
said: most likely no.

diff --git a/drivers/xen/netback/xenbus.c b/drivers/xen/netback/xenbus.c
index 99831c7..1930f64 100644
--- a/drivers/xen/netback/xenbus.c
+++ b/drivers/xen/netback/xenbus.c
@@ -162,17 +162,11 @@ fail:
  */
 static int netback_uevent(struct xenbus_device *xdev, struct kobj_uevent_env *env)
 {
-	struct backend_info *be;
-	struct xen_netif *netif;
+	struct backend_info *be = dev_get_drvdata(&xdev->dev);
 	char *val;
 
 	DPRINTK("netback_uevent");
 
-	be = dev_get_drvdata(&xdev->dev);
-	if (!be)
-		return 0;
-	netif = be->netif;
-
 	val = xenbus_read(XBT_NIL, xdev->nodename, "script", NULL);
 	if (IS_ERR(val)) {
 		int err = PTR_ERR(val);
@@ -187,7 +181,7 @@ static int netback_uevent(struct xenbus_device *xdev, struct kobj_uevent_env *en
 		kfree(val);
 	}
 
-	if (add_uevent_var(env, "vif=%s", netif->dev->name))
+	if (be && be->netif && add_uevent_var(env, "vif=%s", be->netif->dev->name))
 		return -ENOMEM;
 
 	return 0;

-- 
You!  What PLANET is this!
		-- McCoy, "The City on the Edge of Forever", stardate 3134.0

Re: [PATCH] xen/netback: Fix null-pointer access in netback_uevent

[Jeremy Fitzhardinge]

  On 07/29/2010 08:30 AM, Bastian Blank wrote:
> On Thu, Jul 29, 2010 at 02:44:36PM +0100, Ian Campbell wrote:
>> On Thu, 2010-07-29 at 13:42 +0100, Bastian Blank wrote:
>>> Yes, this is included since 2.6.32-15.
>> The patch from<20100529184452.GA18365@wavehammer.waldi.eu.org>?
> Currently the following patch is used. The next test should be if it is
> really necessary to have this much different structures, a quick look
> said: most likely no.

OK, I'll stick it in.  Do you have a complete commit comment?

Thanks,
     J
> diff --git a/drivers/xen/netback/xenbus.c b/drivers/xen/netback/xenbus.c
> index 99831c7..1930f64 100644
> --- a/drivers/xen/netback/xenbus.c
> +++ b/drivers/xen/netback/xenbus.c
> @@ -162,17 +162,11 @@ fail:
>    */
>   static int netback_uevent(struct xenbus_device *xdev, struct kobj_uevent_env *env)
>   {
> -	struct backend_info *be;
> -	struct xen_netif *netif;
> +	struct backend_info *be = dev_get_drvdata(&xdev->dev);
>   	char *val;
>
>   	DPRINTK("netback_uevent");
>
> -	be = dev_get_drvdata(&xdev->dev);
> -	if (!be)
> -		return 0;
> -	netif = be->netif;
> -
>   	val = xenbus_read(XBT_NIL, xdev->nodename, "script", NULL);
>   	if (IS_ERR(val)) {
>   		int err = PTR_ERR(val);
> @@ -187,7 +181,7 @@ static int netback_uevent(struct xenbus_device *xdev, struct kobj_uevent_env *en
>   		kfree(val);
>   	}
>
> -	if (add_uevent_var(env, "vif=%s", netif->dev->name))
> +	if (be&&  be->netif&&  add_uevent_var(env, "vif=%s", be->netif->dev->name))
>   		return -ENOMEM;
>
>   	return 0;
>

Re: [PATCH] xen/netback: Fix null-pointer access in netback_uevent

[Bastian Blank]

On Thu, Jul 29, 2010 at 08:40:30AM -0700, Jeremy Fitzhardinge wrote:
>  On 07/29/2010 08:30 AM, Bastian Blank wrote:
>> On Thu, Jul 29, 2010 at 02:44:36PM +0100, Ian Campbell wrote:
>>> On Thu, 2010-07-29 at 13:42 +0100, Bastian Blank wrote:
>>>> Yes, this is included since 2.6.32-15.
>>> The patch from<20100529184452.GA18365@wavehammer.waldi.eu.org>?
>> Currently the following patch is used. The next test should be if it is
>> really necessary to have this much different structures, a quick look
>> said: most likely no.
> OK, I'll stick it in.  Do you have a complete commit comment?

    xen/netback: Fix null-pointer access in netback_uevent
    
    The uevent method of Xen netback does not check if the the network
    device is already setup and tries to dereference a null-pointer if not.
    
    Signed-off-by: Bastian Blank <waldi@debian.org>

-- 
The sooner our happiness together begins, the longer it will last.
		-- Miramanee, "The Paradise Syndrome", stardate 4842.6