Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Tue, 19 Jun 2018 13:42:48 +0300
From:   Dan Carpenter <dan.carpenter@oracle.com>
To:     Hugo Lefeuvre <hle@owl.eu.com>
Cc:     Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        devel@driverdev.osuosl.org,
        Marcus Wolf <linux@wolf-entwicklungen.de>,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH] staging: pi433: fix race condition in pi433_open
Message-ID: <20180619104248.z5nbbakwhjgscufb@mwanda>
References: <20180618022400.GA1893@hle-laptop.local>
 <20180618101838.gzbrxabilnqyilsi@mwanda>
 <20180619030850.GA1876@hle-laptop.local>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20180619030850.GA1876@hle-laptop.local>
User-Agent: NeoMutt/20170609 (1.8.3)
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Mon, Jun 18, 2018 at 11:11:36PM -0400, Hugo Lefeuvre wrote:
> Hi Dan,
> 
> > We need to decrement device->users-- on the error paths as well.
> > This function was already slightly broken with respect to counting the
> > users, but let's not make it worse.
> > 
> > I think it's still a tiny bit racy because it's not an atomic type.
> 
> Oh right, I missed that. I'll fix it in the v2. :)
> 
> > I'm not sure the error handling in open() works either.  It's releasing
> > device->rx_buffer but there could be another user.
> 
> Agree.
> 
> > The ->rx_buffer
> > should be allocated in probe() instead of open() probably, no?  And then
> > freed in pi433_remove().  Then once we put that in the right layer
> > it means we can just get rid of ->users...
> 
> 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.

> 
> For example, how do you handle the case where remove() is called but
> some operations are still running on existing fds ?
> 
> What if remove frees the rx_buffer while a read() call executes this ?
> 
> copy_to_user(buf, device->rx_buffer, bytes_received)
> 
> rx_buffer is freed by release() because it's the only buffer from the
> device structure used in read/write/ioctl, meaning that we can only
> free it when we are sure that it isn't used anywhere anymore.
> 
> So, we can't do it in remove() unless remove() is delayed until the
> last release() has returned.
> 
> > The lines:
> >
> >   1008                  if (!device->spi)
> >   1009                          kfree(device);
> >
> > make no sort of sense at all...  Fortunately it's not posssible for
> > device->spi to be NULL so it's dead code.
> 
> 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;
> 
> Thanks for your time !

That's when we're unloading the module so there aren't any users left.

regards,
dan carpenter