Re: [PATCH] staging: pi433: fix race condition in pi433_open

[Hugo Lefeuvre <hle@owl.eu.com>]

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

> > It would be great to get rid of this counter, indeed. But how to do it
> > properly without breaking things ? It seems to be useful to me...
> 
> These things are refcounted so you can't unload the module while a file
> is open.  When we do an open it does a cdev_get().  When we call the
> delete_module syscall, it checks the refcount in try_stop_module() ->
> try_release_module_ref().  It will still let us force a release but
> at that point you're expecting use after frees.

Then I'd like to understand why this counter was introduced if remove
always gets executed after the last release call returned. This was
probably unclear to the original developer.

This TODO seems to be related to this misunderstanding too:

 890         /* TODO? guard against device removal before, or while,
 891          * we issue this ioctl. --> device_get()
 892          */

Device removal can't happen before or during the ioctl execution.

> > Really ? device->spi is NULL-ed in remove() so that operations on
> > remaining fds can detect remove() was already called and free remaining
> > resources:
> >  
> > 1296         /* make sure ops on existing fds can abort cleanly */
> > 1297         device->spi = NULL;
> 
> That's when we're unloading the module so there aren't any users left.

I'll submit an updated version of my patch getting rid of the counter
and addressing the remaining race conditions.

Thanks !

Regards,
 Hugo

-- 
             Hugo Lefeuvre (hle)    |    www.owl.eu.com
4096/ 9C4F C8BF A4B0 8FC5 48EB 56B8 1962 765B B9A8 BACA

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]