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

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

[Oleg Nesterov]

Hi.

Let's suppose we have the tasks T1, T2, T3 which share the same file,
all do sys_fcntl(file, F_SETFL) in parallel. file->f_flags == 0.

setfl(arg) does:

	if ((arg ^ filp->f_flags) & FASYNC)
// --- WINDOW_1 ---
		filp->f_op->fasync(fd, filp, (arg & FASYNC) != 0)
// --- WINDOW_2 ---
	filp->f_flags = arg;

T1 calls setfl(FASYNC), preempted in WINDOW_1.

T2 calls setfl(FASYNC), does all job and returns.

T3 calls setfl(0), sees ->f_flags & FASYNC, does ->fasync(on => 0),
preempted in WINDOW_2.

T1 resumes, does ->fasync(on => 1) again, update ->f_flags (it
already has FASYNC) and returns.

T3 resumes, and clears FASYNC from ->f_flags.



Now, this file was added into some "struct fasync_struct", but
->f_flags doesn't have FASYNC. This means __fput() will skip
->fasync(on => 0) and the next kill_fasync() can crash because
fa_file points to the freed/reused memory.

I think a238b790d5f99c7832f9b73ac8847025815b85f7 should be reverted.
Or do you see the better fix?

Unless I missed something of course.

Oleg.

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

[Andi Kleen]

Oleg Nesterov wrote:
> Hi.
> 
> Let's suppose we have the tasks T1, T2, T3 which share the same file,
> all do sys_fcntl(file, F_SETFL) in parallel. file->f_flags == 0.
> 
> setfl(arg) does:
> 
> 	if ((arg ^ filp->f_flags) & FASYNC)
> // --- WINDOW_1 ---
> 		filp->f_op->fasync(fd, filp, (arg & FASYNC) != 0)
> // --- WINDOW_2 ---
> 	filp->f_flags = arg;
> 
> T1 calls setfl(FASYNC), preempted in WINDOW_1.
> 
> T2 calls setfl(FASYNC), does all job and returns.
> 
> T3 calls setfl(0), sees ->f_flags & FASYNC, does ->fasync(on => 0),
> preempted in WINDOW_2.
> 
> T1 resumes, does ->fasync(on => 1) again, update ->f_flags (it
> already has FASYNC) and returns.
> 
> T3 resumes, and clears FASYNC from ->f_flags.
> 
> 
> 
> Now, this file was added into some "struct fasync_struct", but
> ->f_flags doesn't have FASYNC. This means __fput() will skip
> ->fasync(on => 0) and the next kill_fasync() can crash because
> fa_file points to the freed/reused memory.

Nasty. Thanks for catching.
> 
> I think a238b790d5f99c7832f9b73ac8847025815b85f7 should be reverted.
> Or do you see the better fix?

Hmm, about checking for this case and retrying?

Or put a fasync mutex into files_struct.

-Andi

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

[Oleg Nesterov]

On 12/01, Andi Kleen wrote:
>
> Oleg Nesterov wrote:
>>
>> Let's suppose we have the tasks T1, T2, T3 which share the same file,
>> all do sys_fcntl(file, F_SETFL) in parallel. file->f_flags == 0.
>>
>> setfl(arg) does:
>>
>> 	if ((arg ^ filp->f_flags) & FASYNC)
>> // --- WINDOW_1 ---
>> 		filp->f_op->fasync(fd, filp, (arg & FASYNC) != 0)
>> // --- WINDOW_2 ---
>> 	filp->f_flags = arg;
>>
>> T1 calls setfl(FASYNC), preempted in WINDOW_1.
>>
>> T2 calls setfl(FASYNC), does all job and returns.
>>
>> T3 calls setfl(0), sees ->f_flags & FASYNC, does ->fasync(on => 0),
>> preempted in WINDOW_2.
>>
>> T1 resumes, does ->fasync(on => 1) again, update ->f_flags (it
>> already has FASYNC) and returns.
>>
>> T3 resumes, and clears FASYNC from ->f_flags.
>>
>>
>>
>> Now, this file was added into some "struct fasync_struct", but
>> ->f_flags doesn't have FASYNC. This means __fput() will skip
>> ->fasync(on => 0) and the next kill_fasync() can crash because
>> fa_file points to the freed/reused memory.
>
> Nasty. Thanks for catching.

Actually, this is more simple. Somehow I missed that setf() _always_
updates ->f_flags, so we have a trivial race with 2 threads.

>>
>> I think a238b790d5f99c7832f9b73ac8847025815b85f7 should be reverted.
>> Or do you see the better fix?
>
> 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;
	+
		if ((arg ^ filp->f_flags) & FASYNC) {
			if (filp->f_op && filp->f_op->fasync) {
				error = filp->f_op->fasync(fd, filp, (arg & FASYNC) != 0);
	@@ -183,7 +192,8 @@ static int setfl(int fd, struct file * f
			}
		}
	 
	-	filp->f_flags = (arg & SETFL_MASK) | (filp->f_flags & ~SETFL_MASK);
	+	filp->f_flags = (arg & SETFL_MASK) |
	+			(filp->f_flags & ~(SETFL_MASK | O_LOCK_FLAGS));
	  out:
		return error;
	 }

What do you think?

(btw, ioctl_fioasync() calls lock_kernel() but it doesn't protect ->f_flags)

Oleg.

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

[Jonathan Corbet]

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

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

[Oleg Nesterov]

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.

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

[Andi Kleen]

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

I wonder if we need FASYNC at all. This could be gotten implicitely by
looking at the fasync_list

-Andi

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

[Oleg Nesterov]

On 12/02, Andi Kleen wrote:
>
>> 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.
>
> I wonder if we need FASYNC at all. This could be gotten implicitely by
> looking at the fasync_list

Only if socket.

Serioulsy, I think the best (partial, yes) fix for now is to restore
lock_kernel() in setfl() and change ioctl_fioxxx() accordingly.
At least this protect us from tty too.


I agree with Jonathan, we need the lock to protect ->f_flags, but
this needs changes. Personally, I do not like the global mutex,
perhaps we can use some "unused" bit in struct file. Say, ->f_mode
is never changed (afaics), we can place it here. Now, any code
which changes ->f_flags can do

	if (test_and_set_bit(FLAGS_LOCK_BIT))
		return;

	whatever();

	->f_flags = new_flags;

	clear_bit(FLAGS_LOCK_BIT);

No need to disable preemption, we never spin waiting for the
lock bit. If it is locked - somebody else updates ->f_flags,
we can pretend it does this after us. This can confuse F_GETFL
after F_SETFL (if F_SETFL "fails"), but I think in that case
user-space is wrong anyway, it must not do F_GETFL in parallel.

Not that I think this is very good idea though ;)

Oleg.

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

[Andi Kleen]

>> I wonder if we need FASYNC at all. This could be gotten implicitely by
>> looking at the fasync_list
> 
> Only if socket.

But the helpers used by the character drivers add it too I think.

> Serioulsy, I think the best (partial, yes) fix for now is to restore
> lock_kernel() in setfl() and change ioctl_fioxxx() accordingly.
> At least this protect us from tty too.

For 2.6.28 I agree.

> Not that I think this is very good idea though ;)

The lock bit sounds reasonable.

-Andi

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

[Jonathan Corbet]

On Wed, 03 Dec 2008 22:21:02 +0100
Andi Kleen <ak@linux.intel.com> wrote:

> > Serioulsy, I think the best (partial, yes) fix for now is to restore
> > lock_kernel() in setfl() and change ioctl_fioxxx() accordingly.
> > At least this protect us from tty too.  
> 
> For 2.6.28 I agree.

OK, what do you all think of the following?  It returns fcntl.c to its
previous state, and adds locking in fs/ioctl.c.  It's worth noting that
ioctl_fioasync() has always been racy in exactly the same way.

If this passes muster I'll send it upward shortly.

jon

--

Restore BKL protection to manipulations of f_flags

Changeset a238b790d5f99c7832f9b73ac8847025815b85f7 (Call fasync()
functions without the BKL) introduced a race which could leave
file->f_flags in a state inconsistent with what the underlying
driver/filesystem believes.  Revert that change, and also fix the same
races in ioctl_fioasync() and ioctl_fionbio().

This is a minimal, short-term fix; the real fix will not involve the
BKL.

Reported-by: Oleg Nesterov <oleg@redhat.com>
Signed-off-by: Jonathan Corbet <corbet@lwn.net>

diff --git a/fs/fcntl.c b/fs/fcntl.c
index ac4f7db..549daf8 100644
--- a/fs/fcntl.c
+++ b/fs/fcntl.c
@@ -19,6 +19,7 @@
 #include <linux/signal.h>
 #include <linux/rcupdate.h>
 #include <linux/pid_namespace.h>
+#include <linux/smp_lock.h>
 
 #include <asm/poll.h>
 #include <asm/siginfo.h>
@@ -175,6 +176,11 @@ static int setfl(int fd, struct file * filp, unsigned long arg)
 	if (error)
 		return error;
 
+	/*
+	 * We still need a lock here for now to keep multiple FASYNC calls
+	 * from racing with each other.
+	 */
+	lock_kernel();
 	if ((arg ^ filp->f_flags) & FASYNC) {
 		if (filp->f_op && filp->f_op->fasync) {
 			error = filp->f_op->fasync(fd, filp, (arg & FASYNC) != 0);
@@ -185,6 +191,7 @@ static int setfl(int fd, struct file * filp, unsigned long arg)
 
 	filp->f_flags = (arg & SETFL_MASK) | (filp->f_flags & ~SETFL_MASK);
  out:
+	unlock_kernel();
 	return error;
 }
 
diff --git a/fs/ioctl.c b/fs/ioctl.c
index d152856..fc8fcaa 100644
--- a/fs/ioctl.c
+++ b/fs/ioctl.c
@@ -400,11 +400,9 @@ static int ioctl_fioasync(unsigned int fd, struct file *filp,
 
 	/* Did FASYNC state change ? */
 	if ((flag ^ filp->f_flags) & FASYNC) {
-		if (filp->f_op && filp->f_op->fasync) {
-			lock_kernel();
+		if (filp->f_op && filp->f_op->fasync)
 			error = filp->f_op->fasync(fd, filp, on);
-			unlock_kernel();
-		} else
+		else
 			error = -ENOTTY;
 	}
 	if (error)
@@ -440,11 +438,17 @@ int do_vfs_ioctl(struct file *filp, unsigned int fd, unsigned int cmd,
 		break;
 
 	case FIONBIO:
+    		/* BKL needed to avoid races tweaking f_flags */
+	        lock_kernel();
 		error = ioctl_fionbio(filp, argp);
+		unlock_kernel();
 		break;
 
 	case FIOASYNC:
+    		/* BKL needed to avoid races tweaking f_flags */
+	        lock_kernel();
 		error = ioctl_fioasync(fd, filp, argp);
+		unlock_kernel();
 		break;
 
 	case FIOQSIZE:

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

[Oleg Nesterov]

On 12/03, Jonathan Corbet wrote:
>
> On Wed, 03 Dec 2008 22:21:02 +0100
> Andi Kleen <ak@linux.intel.com> wrote:
>
> > > Serioulsy, I think the best (partial, yes) fix for now is to restore
> > > lock_kernel() in setfl() and change ioctl_fioxxx() accordingly.
> > > At least this protect us from tty too.
> >
> > For 2.6.28 I agree.
>
> OK, what do you all think of the following?  It returns fcntl.c to its
> previous state, and adds locking in fs/ioctl.c.  It's worth noting that
> ioctl_fioasync() has always been racy in exactly the same way.

Yes, I agree with you and Andi, imho this is what we need for 2.6.28.

Thanks Jonathan!

Oleg.

[PATCH 2.6.28] Fix FASYNC race

[Jonathan Corbet]

Hi, Linus,

The following patch closes a race which was introduced by the fasync()
BKL pushdown patch.  As it turns out, f_flags handling was already
racy, but that change made it worse.  Here, we fix it (and a
longer-standing race) minimally by reintroducing the BKL; a better fix
will be prepared for 2.6.29.

jon

---
Fix a race condition in FASYNC handling

Changeset a238b790d5f99c7832f9b73ac8847025815b85f7 (Call fasync()
functions without the BKL) introduced a race which could leave
file->f_flags in a state inconsistent with what the underlying
driver/filesystem believes.  Revert that change, and also fix the same
races in ioctl_fioasync() and ioctl_fionbio().

This is a minimal, short-term fix; the real fix will not involve the
BKL.

Reported-by: Oleg Nesterov <oleg@redhat.com>
Cc: Andi Kleen <ak@linux.intel.com>
Cc: Al Viro <viro@zeniv.linux.org.uk>
Cc: stable@kernel.org
Signed-off-by: Jonathan Corbet <corbet@lwn.net>
---
 fs/fcntl.c |    7 +++++++
 fs/ioctl.c |   12 ++++++++----
 2 files changed, 15 insertions(+), 4 deletions(-)

diff --git a/fs/fcntl.c b/fs/fcntl.c
index ac4f7db..549daf8 100644
--- a/fs/fcntl.c
+++ b/fs/fcntl.c
@@ -19,6 +19,7 @@
 #include <linux/signal.h>
 #include <linux/rcupdate.h>
 #include <linux/pid_namespace.h>
+#include <linux/smp_lock.h>
 
 #include <asm/poll.h>
 #include <asm/siginfo.h>
@@ -175,6 +176,11 @@ static int setfl(int fd, struct file * filp, unsigned long arg)
 	if (error)
 		return error;
 
+	/*
+	 * We still need a lock here for now to keep multiple FASYNC calls
+	 * from racing with each other.
+	 */
+	lock_kernel();
 	if ((arg ^ filp->f_flags) & FASYNC) {
 		if (filp->f_op && filp->f_op->fasync) {
 			error = filp->f_op->fasync(fd, filp, (arg & FASYNC) != 0);
@@ -185,6 +191,7 @@ static int setfl(int fd, struct file * filp, unsigned long arg)
 
 	filp->f_flags = (arg & SETFL_MASK) | (filp->f_flags & ~SETFL_MASK);
  out:
+	unlock_kernel();
 	return error;
 }
 
diff --git a/fs/ioctl.c b/fs/ioctl.c
index d152856..fc8fcaa 100644
--- a/fs/ioctl.c
+++ b/fs/ioctl.c
@@ -400,11 +400,9 @@ static int ioctl_fioasync(unsigned int fd, struct file *filp,
 
 	/* Did FASYNC state change ? */
 	if ((flag ^ filp->f_flags) & FASYNC) {
-		if (filp->f_op && filp->f_op->fasync) {
-			lock_kernel();
+		if (filp->f_op && filp->f_op->fasync)
 			error = filp->f_op->fasync(fd, filp, on);
-			unlock_kernel();
-		} else
+		else
 			error = -ENOTTY;
 	}
 	if (error)
@@ -440,11 +438,17 @@ int do_vfs_ioctl(struct file *filp, unsigned int fd, unsigned int cmd,
 		break;
 
 	case FIONBIO:
+    		/* BKL needed to avoid races tweaking f_flags */
+	        lock_kernel();
 		error = ioctl_fionbio(filp, argp);
+		unlock_kernel();
 		break;
 
 	case FIOASYNC:
+    		/* BKL needed to avoid races tweaking f_flags */
+	        lock_kernel();
 		error = ioctl_fioasync(fd, filp, argp);
+		unlock_kernel();
 		break;
 
 	case FIOQSIZE:
-- 
1.6.0.4

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

[Andi Kleen]

> 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;
> 	+
> 		if ((arg ^ filp->f_flags) & FASYNC) {
> 			if (filp->f_op && filp->f_op->fasync) {
> 				error = filp->f_op->fasync(fd, filp, (arg & FASYNC) != 0);
> 	@@ -183,7 +192,8 @@ static int setfl(int fd, struct file * f
> 			}
> 		}
> 	 
> 	-	filp->f_flags = (arg & SETFL_MASK) | (filp->f_flags & ~SETFL_MASK);
> 	+	filp->f_flags = (arg & SETFL_MASK) |
> 	+			(filp->f_flags & ~(SETFL_MASK | O_LOCK_FLAGS));
> 	  out:
> 		return error;
> 	 }
> 
> What do you think?

Looks reasonable. Just would need to make sure that O_LOCK_FLAGS doesn't
leak out to user space.

-Andi

>