Re: BUG? "Call fasync() functions without the BKL" is racy

[Oleg Nesterov <oleg@redhat.com>]

On 12/01, Jonathan Corbet wrote:
>
> 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(&current->files->file_lock);
> > 	+	if (!(filp->f_flags & O_LOCK_FLAGS))
> > 	+		filp->f_flags |= O_LOCK_FLAGS;
> > 	+	else
> > 	+		error = -EAGAIN;
> > 	+	spin_unlock(&current->files->file_lock);
> > 	+	if (error) /* pretend ->f_flags was changed after us
> > */
> > 	+		return 0;
> > 	+
>
> This strikes me as overkill.

and doesn't really help. fork() clones files_struct, we can't rely
on ->file_lock. And we have fd passing.

> 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.

A bit ugly to use the global mutex to protect all ->f_flags, but yes,
I agree. Imho this is 2.6.28 material, we need the simple fix or the
user can crash the kernel.

Still I'd like to see the better fix for the long term, the only (afaics)
flag with the "side effect" is FASYNC, perhaps we can move it to (say)
->f_mode, but this is ugly of course and still need serialization for the
pathes which play with FASYNC.

Otherwise, any code which changes ->f_flags can clear FASYNC if we race
with sys_fcntl(F_SETFL, FASYNC).

If we introduce the new lock, we should change for example tty_io.c:fionbio(),
tty has the ->fasync() method. Currently tty_io.c relies on lock_kernel().

Oleg.