Re: [PATCH 1/1] r8152: fix NULL pointer dereference in r8152_poll
> > > Unfortunately this doesn't work. Code in r8152.c doesn't use > > > local_bh_enable()/local_bh_disable(). I tried to lock it with > > > spin_lock_bh()/spin_unlock_bh() and with mutex_lock()/mutex_unlock() > > > but neither work. > > The local_bh_disable() / local_bh_enable() definitely is the right > > answer to the issue you described. > > It does not matter what code in r8152.c currently does. > > https://git.kernel.org/pub/scm/linux/kernel/git/davem/net-next.git/commit/?id=8cf699ec849f4ca1413cea01289bd7d37dbcc626 > You also have to protect other napi_schedule(), like the ones in > rtl_work_func_t() or rtl8152_post_reset() I've tested that before :-). I'll be more precise what "not working" means: it fixes invalid pointer issue, but kernel crashes for different reason: ... Call Trace: net_rx_action+0x23c/0x3f0 __do_softirq+0x104/0x2e1 ? usb_runtime_suspend+0x70/0x70 [usbcore] do_softirq_own_stack+8x1c/0x30 do_softirq.part.18+0x41/0x50 __local_bh_enable_ip+0x88/0xa0 rtl8152_resume+0xe2/0x1a0 [r8152] usb_resume_interface.isra.60x99/0xf0 [usbcore] usb_resume_both+0x6a/0x130 [usbcore] __rpm_callback+0xb9/0x1f0 rpm_callback+Ox5f/0x80 ? usb_runtime_suspend+0x70/0x70 [usbcore] usb_resume+0x495/0x6b0 ? update_load_avg+Ox79/0x520 ? update_load_avg+Ox79/0x520 ? refcount_dec_and_test+0x11/0x20 __pm_runtime_resume+0x3f/0x60 usb_autoresume_device+0x23/0x50 [usbcore] usb_dev_open+0xe7/0x250 [usbcore] chrdev_open+0xa1/0x200 do_dentry_open+0x20a/0x2f0 ? cdev_put+0x30/0x30 vfs_open+0x4c/0x70 ? may_open+0x9b/0x100 path_openat+0x5ec/0x1430 do_filp_open+0x7e/0xe0 ? __vfs_write+0x28/0x140 ? __alloc_fd+0xb2/0x160 do_sys_open+0x123/0x200 SyS_open+0x1e/0x20 entry_SYSCALL_64_fastpath+0x1e/0xad ... Kernel panic - not syncing: Fatal exception in interrupt ... Patch: http://pastebin.com/Uejjc0Bh (I don't post patch here, as it's not working). Kind regards, Petr
Re: [PATCH 1/1] r8152: fix NULL pointer dereference in r8152_poll
On Mon, 2017-03-13 at 08:44 -0700, Eric Dumazet wrote: > On Mon, 2017-03-13 at 16:37 +0100, Petr Vorel wrote: > > Hi Eric, > > > > > > The proper work around is to enclose the napi_schedule() in a > > > > local_bh_enable()/local_bh_disable() pair. > > > > > Something like : > > > --- a/drivers/net/usb/r8152.c > > > +++ b/drivers/net/usb/r8152.c > > > @@ -3703,8 +3703,10 @@ static int rtl8152_resume(struct usb_interface > > > *intf) > > > napi_enable(>napi); > > > clear_bit(SELECTIVE_SUSPEND, >flags); > > > smp_mb__after_atomic(); > > > + local_bh_disable(); > > > if (!list_empty(>rx_done)) > > > napi_schedule(>napi); > > > + local_bh_enable(); > > > > Unfortunately this doesn't work. Code in r8152.c doesn't use > > local_bh_enable()/local_bh_disable(). I tried to lock it with > > spin_lock_bh()/spin_unlock_bh() and with mutex_lock()/mutex_unlock() > > but neither work. > > The local_bh_disable() / local_bh_enable() definitely is the right > answer to the issue you described. > > It does not matter what code in r8152.c currently does. > > https://git.kernel.org/pub/scm/linux/kernel/git/davem/net-next.git/commit/?id=8cf699ec849f4ca1413cea01289bd7d37dbcc626 You also have to protect other napi_schedule(), like the ones in rtl_work_func_t() or rtl8152_post_reset()
Re: [PATCH 1/1] r8152: fix NULL pointer dereference in r8152_poll
On Mon, 2017-03-13 at 16:37 +0100, Petr Vorel wrote: > Hi Eric, > > > > The proper work around is to enclose the napi_schedule() in a > > > local_bh_enable()/local_bh_disable() pair. > > > Something like : > > --- a/drivers/net/usb/r8152.c > > +++ b/drivers/net/usb/r8152.c > > @@ -3703,8 +3703,10 @@ static int rtl8152_resume(struct usb_interface *intf) > > napi_enable(>napi); > > clear_bit(SELECTIVE_SUSPEND, >flags); > > smp_mb__after_atomic(); > > + local_bh_disable(); > > if (!list_empty(>rx_done)) > > napi_schedule(>napi); > > + local_bh_enable(); > > Unfortunately this doesn't work. Code in r8152.c doesn't use > local_bh_enable()/local_bh_disable(). I tried to lock it with > spin_lock_bh()/spin_unlock_bh() and with mutex_lock()/mutex_unlock() > but neither work. The local_bh_disable() / local_bh_enable() definitely is the right answer to the issue you described. It does not matter what code in r8152.c currently does. https://git.kernel.org/pub/scm/linux/kernel/git/davem/net-next.git/commit/?id=8cf699ec849f4ca1413cea01289bd7d37dbcc626
Re: [PATCH 1/1] r8152: fix NULL pointer dereference in r8152_poll
Hi Eric, > > The proper work around is to enclose the napi_schedule() in a > > local_bh_enable()/local_bh_disable() pair. > Something like : > --- a/drivers/net/usb/r8152.c > +++ b/drivers/net/usb/r8152.c > @@ -3703,8 +3703,10 @@ static int rtl8152_resume(struct usb_interface *intf) > napi_enable(>napi); > clear_bit(SELECTIVE_SUSPEND, >flags); > smp_mb__after_atomic(); > + local_bh_disable(); > if (!list_empty(>rx_done)) > napi_schedule(>napi); > + local_bh_enable(); Unfortunately this doesn't work. Code in r8152.c doesn't use local_bh_enable()/local_bh_disable(). I tried to lock it with spin_lock_bh()/spin_unlock_bh() and with mutex_lock()/mutex_unlock() but neither work. Kind regards, Petr
Re: [PATCH 1/1] r8152: fix NULL pointer dereference in r8152_poll
On Mon, 2017-03-13 at 06:18 -0700, Eric Dumazet wrote: > On Mon, 2017-03-13 at 13:47 +0100, Petr Vorel wrote: > > commit 7489bdadb7d1 (r8152: check rx after napi is enabled) causes null > > pointer dereference when using device as under root: > > > > # rmmod r8152 # or lsusb -v > > NOHZ: local_softirq_pending 08 > > BUG: unable to handle kernel NULL pointer dereference at 0008 > > IP: r8152_poll+0x125/0x570 [r8152] > > PGD 89b4cf067 > > PUD 898ff2067 > > PMD 0 > > Oops: 0002 [#1] PREEMPT SMP > > > > Signed-off-by: Petr Vorel> > --- > > NOTE: This is just a workaround, I suppose, there is better way how to fix > > that > > (which allows keeping scheduling the napi for rx after napi_enable()). > > --- > > drivers/net/usb/r8152.c | 2 -- > > 1 file changed, 2 deletions(-) > > > > diff --git a/drivers/net/usb/r8152.c b/drivers/net/usb/r8152.c > > index 986243c932cc..79c665a89a47 100644 > > --- a/drivers/net/usb/r8152.c > > +++ b/drivers/net/usb/r8152.c > > @@ -3703,8 +3703,6 @@ static int rtl8152_resume(struct usb_interface *intf) > > napi_enable(>napi); > > clear_bit(SELECTIVE_SUSPEND, >flags); > > smp_mb__after_atomic(); > > - if (!list_empty(>rx_done)) > > - napi_schedule(>napi); > > } else { > > tp->rtl_ops.up(tp); > > netif_carrier_off(tp->netdev); > > > The proper work around is to enclose the napi_schedule() in a > local_bh_enable()/local_bh_disable() pair. Something like : diff --git a/drivers/net/usb/r8152.c b/drivers/net/usb/r8152.c index 986243c932ccd6fe19c592805c1c63274f5e..b6bb1720c383946ea6142ec2f79f5b7a69031d7f 100644 --- a/drivers/net/usb/r8152.c +++ b/drivers/net/usb/r8152.c @@ -3703,8 +3703,10 @@ static int rtl8152_resume(struct usb_interface *intf) napi_enable(>napi); clear_bit(SELECTIVE_SUSPEND, >flags); smp_mb__after_atomic(); + local_bh_disable(); if (!list_empty(>rx_done)) napi_schedule(>napi); + local_bh_enable(); } else { tp->rtl_ops.up(tp); netif_carrier_off(tp->netdev);
Re: [PATCH 1/1] r8152: fix NULL pointer dereference in r8152_poll
On Mon, 2017-03-13 at 13:47 +0100, Petr Vorel wrote: > commit 7489bdadb7d1 (r8152: check rx after napi is enabled) causes null > pointer dereference when using device as under root: > > # rmmod r8152 # or lsusb -v > NOHZ: local_softirq_pending 08 > BUG: unable to handle kernel NULL pointer dereference at 0008 > IP: r8152_poll+0x125/0x570 [r8152] > PGD 89b4cf067 > PUD 898ff2067 > PMD 0 > Oops: 0002 [#1] PREEMPT SMP > > Signed-off-by: Petr Vorel> --- > NOTE: This is just a workaround, I suppose, there is better way how to fix > that > (which allows keeping scheduling the napi for rx after napi_enable()). > --- > drivers/net/usb/r8152.c | 2 -- > 1 file changed, 2 deletions(-) > > diff --git a/drivers/net/usb/r8152.c b/drivers/net/usb/r8152.c > index 986243c932cc..79c665a89a47 100644 > --- a/drivers/net/usb/r8152.c > +++ b/drivers/net/usb/r8152.c > @@ -3703,8 +3703,6 @@ static int rtl8152_resume(struct usb_interface *intf) > napi_enable(>napi); > clear_bit(SELECTIVE_SUSPEND, >flags); > smp_mb__after_atomic(); > - if (!list_empty(>rx_done)) > - napi_schedule(>napi); > } else { > tp->rtl_ops.up(tp); > netif_carrier_off(tp->netdev); The proper work around is to enclose the napi_schedule() in a local_bh_enable()/local_bh_disable() pair.
[PATCH 1/1] r8152: fix NULL pointer dereference in r8152_poll
commit 7489bdadb7d1 (r8152: check rx after napi is enabled) causes null pointer dereference when using device as under root: # rmmod r8152 # or lsusb -v NOHZ: local_softirq_pending 08 BUG: unable to handle kernel NULL pointer dereference at 0008 IP: r8152_poll+0x125/0x570 [r8152] PGD 89b4cf067 PUD 898ff2067 PMD 0 Oops: 0002 [#1] PREEMPT SMP Signed-off-by: Petr Vorel--- NOTE: This is just a workaround, I suppose, there is better way how to fix that (which allows keeping scheduling the napi for rx after napi_enable()). --- drivers/net/usb/r8152.c | 2 -- 1 file changed, 2 deletions(-) diff --git a/drivers/net/usb/r8152.c b/drivers/net/usb/r8152.c index 986243c932cc..79c665a89a47 100644 --- a/drivers/net/usb/r8152.c +++ b/drivers/net/usb/r8152.c @@ -3703,8 +3703,6 @@ static int rtl8152_resume(struct usb_interface *intf) napi_enable(>napi); clear_bit(SELECTIVE_SUSPEND, >flags); smp_mb__after_atomic(); - if (!list_empty(>rx_done)) - napi_schedule(>napi); } else { tp->rtl_ops.up(tp); netif_carrier_off(tp->netdev); -- 2.12.0