fanotify read returns with errno == EOPENSTALE

Commit Message

Amir Goldstein April 20, 2017, 11:06 a.m. UTC

On Wed, Apr 19, 2017 at 4:46 PM, Marko Rauhamaa
<[e-mail]> wrote:
> Amir Goldstein <[e-mail]>:
>
>> On Thu, Mar 23, 2017 at 8:43 AM, Marko Rauhamaa
>> <[e-mail]> wrote:
>>> Jeff Layton <[e-mail]>:
>>>
>>>> It was definitely not the intention to leak this error code to
>>>> userland. EOPENSTALE is not a POSIX sanctioned error code, so
>>>> applications generally don't know anything about it and will be
>>>> confused.
>>>
>>> Got it. I will try to work on a reproduction and make a proper bug
>>> report.
>>
>> Try this:
>>
>> - watch a single file for permissions events (so you will only have
>> one event in the queue)
>> - open file from client to generate single event (don't read event yet)
>> - remove file from server (to make it stale)
>> - read event (with stale file)
>
> I did that and reproduced the problem on a recent development kernel.
> Happens every time.
>
> 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.
>

Thanks for the reproducer.
I'll try it myself when I get to it.

> So what do we conclude? Is this a kernel bug or works as designed?
>

Exposing EOPENSTALE to userspace is definitely a kernel bug.


>> Oh my. I completely misread your report before. I though you were
>> trying to read from the event->fd. Now I understand that you mean read
>> from fanotify fd. That will definitely return the error, but only in
>> the special case where open error happened on the first event being
>> read to the buffer. If error happens after adding some events to the
>> buffer, fanotify process will not know about this. Regular event will
>> be silently dropped and permission event will be denied.
>>
>> [...]
>>
>> You do NOT need to call fanotify_init() again, the next read will read
>> the next event.
>
> It does appear that reading the fanotify fd again does the trick.
>
> However, the client gets an EPERM instead of ENOENT, which is a bit
> weird.
>

Why would the client get ENOENT? That EOPENSTALE event is already
consumed, the client reads the next event in the queue.

>> The fix seems trivial and I can post it once you have the test:
>> - return EAGAIN for read in case of a single event in queue without fd
>>   so apps getting the read error will have a good idea what to do
>> - in case of non single event, maybe copy event with error on event->fd
>>   to the buffer for specific errors that make sense to report (EMFILE)
>>   so a watcher checks the values of negative event->fd can maybe do
>>   something about it (e.g. provide a smaller buffer).
>
> EAGAIN would be perfect for me since I'm using fanotify in a nonblocking
> mode. It might be a bit surprising in the blocking case.
>
>

Can you please try this patch?
Can you please try it with blocking and non-blocking
Can you please try to add to reproducer the non empty queue case:
- Add another mark on another mount without PERM events in the mask
- Populate other mount with some files
- Before reading from nfsshare, read from other mount to fill the
   event queue, e.g.:
# cat /tmp/foo* /nfsshare/bar.txt /tmp/bar*

This should result (depending on number of files) with
>= 2 buffer reads - first with /tmp/foo* files access
last with /tmp/bar* files access


                 * events can be destroyed now.

---

Patch

diff --git a/fs/notify/fanotify/fanotify_user.c
b/fs/notify/fanotify/fanotify_user.c
index 2b37f27..5b14890 100644
--- a/fs/notify/fanotify/fanotify_user.c
+++ b/fs/notify/fanotify/fanotify_user.c
@@ -295,6 +295,17 @@  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/mask 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).
+                        */
+                       kevent->mask = 0;
+                       ret = 0;
+               }
+
                /*
                 * Permission events get queued to wait for response.  Other