On Fri, Jan 04, 2008 at 02:08:18PM -0700, Matthew Wilcox wrote:
> On Fri, Jan 04, 2008 at 03:53:04PM -0500, J. Bruce Fields wrote:
> > So, the problem is that fcntl_setlease() does
> >
> > vfs_setlease()
> > fasync_helper()
> >
> > which the bkl held over both, and you want to preserve that?
> >
> > But what that BKL is doing is a mystery to me--the very first thing that
> > fasync_helper() does is kmem_cache_alloc(., GFP_KERNEL). So you won't
> > be introducing any new problem if you lock those two operations
> > separately. Unless I'm totally missing something.
>
> A very good point.
>
> So yet another race caused by using the BKL rather than thinking ... but
> maybe it's an inconsequential race. The consequences are that (if the
> kmalloc in fasync_helper sleeps) a lease appears that isn't fully set-up
> yet (and may be removed if the kmalloc fails). Actually, it seems bad
> if the kmalloc eventually succeeds -- there's a window while kmalloc is
> sleeping where another process could open the file, break the lease,
> fl_fasync will be NULL, so no signal is sent. Then 30 seconds later the
> lease is removed without the leaseholder being sent a signal. Bad.
>
> How can we fix this situation? I think we need a better interface than
> fasync_helper() -- fasync_alloc() and fasync_setup() would seem to do
> the trick.
Or re-check the lease after doing the fasync_helper() setup and remove
it if it's been broken in the interim?
(Not that fasync_helper() couldn't independently use a little love:
- The documentation:
/*
* fasync_helper() is used by some character device drivers
* (mainly mice) to set up the fasync queue. It returns negative
* on error, 0 if it did no changes and positive if it
* added/deleted the entry.
*/
could be more helpful.
- I find the "on" parameter a little confusing. (Shouldn't we just have
two separate functions for those two cases?)
- It should return ERR_PTR(-ERRNO) or the fasync_struct rather than
using an fasync_struct ** to return the result.
- And what's up with FASYNC_MAGIC? I thought the consensus was not to
do that kind of thing in the kernel.
)
--b.
-
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
