Re: RFC: Fix f_flags races without the BKL

[Oleg Nesterov <oleg@redhat.com>]

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.