Messages in this thread | ![/](/images/icornerl.gif) | | Date | Fri, 9 Jan 2009 11:08:21 +0100 | From | Oleg Nesterov <> | Subject | Re: RFC: Fix f_flags races without the BKL |
| |
On 01/08, Jonathan Corbet wrote: > > This patch returns -ENOTTY in both places. It seems better > to me, but it *is* a change, and we may well not want to do that.
I'm afraid, this can break user-space applications. But I agree it looks better.
> static int setfl(int fd, struct file * filp, unsigned long arg) > { > @@ -176,25 +179,52 @@ 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); > - if (error < 0) > - goto out; > - } > + error = fasync_change(fd, filp, (arg & FASYNC) != 0); > + if (error < 0) > + goto out;
So, fasync_change() sets/clears FASYNC,
> + lock_file_flags(); > filp->f_flags = (arg & SETFL_MASK) | (filp->f_flags & ~SETFL_MASK); > + unlock_file_flags();
and then we change f_flags again, including F_ASYNC bit.
This is racy?
Suppose T1 does setfl(arg == 0) and preempted before lock_file_flags() above. T2 does setfl(FASYNC) and succeeds. T1 resumes and clears FASYNC. Now we have the same problem, the file's state is not consistent.
> +int fasync_change(int fd, struct file *filp, int on) > +{ > + int ret; > + static DEFINE_MUTEX(fasync_mutex); > + > + if (filp->f_op->fasync == NULL) > + return -ENOTTY; > + > + mutex_lock(&fasync_mutex); > + lock_file_flags(); > + if (((filp->f_flags & FASYNC) == 0) == (on == 0)) { > + unlock_file_flags(); > + return 0; > + } > + if (on) > + filp->f_flags |= FASYNC; > + else > + filp->f_flags &= ~FASYNC; > + unlock_file_flags(); > + ret = filp->f_op->fasync(fd, filp, on); > + mutex_unlock(&fasync_mutex); > + return ret;
But we must not change ->f_flags if ->fasync() fails?
Now we have the global mutex for ->fasync... Well, not very good but fasync_helper() takes fasync_lock anyway.
Oleg.
| ![\](/images/icornerr.gif) |