Re: [PATCH v2] fanotify: don't expose EOPENSTALE to userspace

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Tue, Apr 25, 2017 at 02:29:35PM +0300, Amir Goldstein wrote:
> When delivering an event to userspace for a file on an NFS share,
> if the file is deleted on server side before user reads the event,
> user will not get the event.
> 
> If the event queue contained several events, the stale event is
> quietly dropped and read() returns to user with events read so far
> in the buffer.
> 
> If the event queue contains a single stale event or if the stale
> event is a permission event, read() returns to user with the kernel
> internal error code 518 (EOPENSTALE), which is not a POSIX error code.
> 
> Check the internal return value -EOPENSTALE in fanotify_read(), just
> the same as it is checked in path_openat() and drop the event in the
> cases that it is not already dropped.

Makes sense to me.

Of course we can't prevent the case where a file's deleted on the server
after the read but before the application uses the event, so the
application could see ESTALE on a later operation; but that's not the
case you're talking about here.

--b.

> 
> This is a reproducer from Marko Rauhamaa:
> 
> Just take the example program listed under "man fanotify" ("fantest")
> and follow these steps:
> 
>     ==============================================================
>     NFS Server    NFS Client(1)     NFS Client(2)
>     ==============================================================
>     # echo foo >/nfsshare/bar.txt
>                   # cat /nfsshare/bar.txt
>                   foo
>                                     # ./fantest /nfsshare
>                                     Press enter key to terminate.
>                                     Listening for events.
>     # rm -f /nfsshare/bar.txt
>                   # cat /nfsshare/bar.txt
>                                     read: Unknown error 518
>                   cat: /nfsshare/bar.txt: Operation not permitted
>     ==============================================================
> 
> where NFS Client (1) and (2) are two terminal sessions on a single NFS
> Client machine.
> 
> Reported-by: Marko Rauhamaa <marko.rauhamaa@xxxxxxxxxxxx>
> Tested-by: Marko Rauhamaa <marko.rauhamaa@xxxxxxxxxxxx>
> Cc: <linux-api@xxxxxxxxxxxxxxx>
> Cc: <stable@xxxxxxxxxxxxxxx>
> Signed-off-by: Amir Goldstein <amir73il@xxxxxxxxx>
> ---
>  fs/notify/fanotify/fanotify_user.c | 26 ++++++++++++++++++--------
>  1 file changed, 18 insertions(+), 8 deletions(-)
> 
> Jan,
> 
> This is v2 patch that was tested by Marko.
> It includes your fix to my v1 patch.
> 
> Marko tested both blocking and non-blocking fanotify fd.
> I did not test with stale NFS files myself, but I did test by emulating
> stale NFS files with orphan files, using this test patch to create_fd():
> 
>                 put_unused_fd(client_fd);
>                 client_fd = PTR_ERR(new_file);
> >       } else if (!event->path.dentry->d_inode ||
> >                  !event->path.dentry->d_inode->i_nlink) {
> >               put_unused_fd(client_fd);
> >               fput(new_file);
> >               return -EOPENSTALE;
>         } else {
>                 *file = new_file;
>         }
> 
> I CC'ed linux-api in case Michael would want to add a BUGS note on this
> and CC'ed stable because we should consider patching this bug in older
> kernels. After all, Marko discovered this bug in a RHEL kernel.
> 
> Thanks,
> Amir.
> 
> 
> diff --git a/fs/notify/fanotify/fanotify_user.c b/fs/notify/fanotify/fanotify_user.c
> index 2b37f27..4b3437d 100644
> --- a/fs/notify/fanotify/fanotify_user.c
> +++ b/fs/notify/fanotify/fanotify_user.c
> @@ -295,27 +295,37 @@ static ssize_t fanotify_read(struct file *file, char __user *buf,
>  		}
>  
>  		ret = copy_event_to_user(group, kevent, buf);
> +		if (unlikely(ret == -EOPENSTALE)) {
> +			/*
> +			 * We cannot report events with stale fd so drop it.
> +			 * Setting ret to 0 will continue the event loop and
> +			 * do the right thing if there are no more events to
> +			 * read (i.e. return bytes read, -EAGAIN or wait).
> +			 */
> +			ret = 0;
> +		}
> +
>  		/*
>  		 * Permission events get queued to wait for response.  Other
>  		 * events can be destroyed now.
>  		 */
>  		if (!(kevent->mask & FAN_ALL_PERM_EVENTS)) {
>  			fsnotify_destroy_event(group, kevent);
> -			if (ret < 0)
> -				break;
>  		} else {
>  #ifdef CONFIG_FANOTIFY_ACCESS_PERMISSIONS
> -			if (ret < 0) {
> +			if (ret <= 0) {
>  				FANOTIFY_PE(kevent)->response = FAN_DENY;
>  				wake_up(&group->fanotify_data.access_waitq);
> -				break;
> +			} else {
> +				spin_lock(&group->notification_lock);
> +				list_add_tail(&kevent->list,
> +					&group->fanotify_data.access_list);
> +				spin_unlock(&group->notification_lock);
>  			}
> -			spin_lock(&group->notification_lock);
> -			list_add_tail(&kevent->list,
> -				      &group->fanotify_data.access_list);
> -			spin_unlock(&group->notification_lock);
>  #endif
>  		}
> +		if (ret < 0)
> +			break;
>  		buf += ret;
>  		count -= ret;
>  	}
> -- 
> 2.7.4



[Index of Archives]     [Linux Kernel]     [Kernel Development Newbies]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite Hiking]     [Linux Kernel]     [Linux SCSI]