lkml.org 
[lkml]   [2012]   [Mar]   [13]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: ping: drivers/staging/usbip/ abuses task_is_dead/exit_state
On 2012-03-08 at 19:57:39 +0100, Oleg Nesterov <oleg@redhat.com> wrote:
> On 03/06, Tobias Klauser wrote:
> >
> > On 2012-03-06 at 18:39:25 +0100, Oleg Nesterov <oleg@redhat.com> wrote:
> > >
> > > OK, since nobody cares, probably I should make the patch even if I don't
> > > understand this code at all and can't test the change.
> > >
> > > But, Tobias, may be you can explain what this task_is_dead() check was
> > > supposed to do?
> >
> > As mentioned in the commit message, this was needed for me to work
> > around a NULL pointer dereference I got during unbinding
>
> Where? OK, I guess you do not remember the trace ;)

Yup, sorry I can't precisely recall where it happened anymore.

> > (I only
> > experienced this behaviour on the nios2 platform though, couldn't
> > reproduce it on e.g. x86_64).
>
> OK,
>
> > I wasn't really familiar with the codebase of usbip (and still am not)
> > but came up with the fix by more or less blindly copying what the
> > opposite side is checking for in stub_shutdown_connection(). This fixed
> > the behaviour for me and seemed legitimate as it was done equally there.
>
> But this looks "obviously wrong", and afaics just hides the problem.
> Not to mention this check is racy, it is simply unsafe to dereference
> this task_struct if the kthread has already exited.
>
> At first glance we need something like the patch below (and stub_dev.c
> needs the same fix). It assumes that:
>
> - vhci_shutdown_connection() is the only caller of kthread_stop(),
> iow nobody else does kthread_stop(tcp_*x)
>
> - we can't leak the task_struct, vhci_shutdown_connection() should
> be called in any case at some point.
>
> I'll try to grep more, but perhaps you can ack my understanding?

Looks OK to me at a first glance. I'll try to dig up my test system
where I was able to produce that NULL pointer dereference and test your
patch there and will then let you know my results.

Thanks
Tobias


\
 
 \ /
  Last update: 2012-03-13 12:47    [W:0.076 / U:7.472 seconds]
©2003-2018 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site