Messages in this thread | | | Date | Mon, 1 Dec 2008 12:34:05 -0700 | From | Jonathan Corbet <> | Subject | Re: BUG? "Call fasync() functions without the BKL" is racy |
| |
On Mon, 1 Dec 2008 20:15:55 +0100 Oleg Nesterov <oleg@redhat.com> wrote:
> > Hmm, about checking for this case and retrying? > > > > Or put a fasync mutex into files_struct. > > Perhaps, we can add O_LOCK_FLAGS, then something like > > --- a/fs/fcntl.c > +++ b/fs/fcntl.c > @@ -175,6 +175,15 @@ static int setfl(int fd, struct file * f > if (error) > return error; > > + spin_lock(¤t->files->file_lock); > + if (!(filp->f_flags & O_LOCK_FLAGS)) > + filp->f_flags |= O_LOCK_FLAGS; > + else > + error = -EAGAIN; > + spin_unlock(¤t->files->file_lock); > + if (error) /* pretend ->f_flags was changed after us > */ > + return 0; > +
This strikes me as overkill. What we really want to do is to protect against concurrent access to f_flags - something which could come about in a couple of other situations (nfsd/vfs.c tweaks it, for example). We *could* just extend files_lock to cover f_flags too, but that comes at the cost of making ->fasync() atomic when it never has been before - doesn't seem like a good idea.
Perhaps we just need a single f_flags_mutex? For code changing f_flags only (it woudn't be needed to query the flags)? Then ioctl_fionbio() and ioctl_fioasync() could use it too. It's hard to imagine that there's enough contention to warrant any more than that, especially given that it all (except ioctl_fionbio()) has been under the BKL until now.
jon
| |