Re: ping: drivers/staging/usbip/ abuses task_is_dead/exit_state

[Tobias Klauser]

On 2012-03-08 at 19:57:39 +0100, Oleg Nesterov <oleg@xxxxxxxxxx> wrote:
> On 03/06, Tobias Klauser wrote:
> >
> > On 2012-03-06 at 18:39:25 +0100, Oleg Nesterov <oleg@xxxxxxxxxx> 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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/