Re: fanotify read returns with errno == EOPENSTALE

On Thu, Apr 20, 2017 at 6:06 PM, Amir Goldstein [off-list ref] wrote:

On Thu, Apr 20, 2017 at 5:20 PM, Jan Kara [off-list ref] wrote:

quoted

On Thu 20-04-17 14:33:04, Amir Goldstein wrote:

quoted

Sorry I messed up the previous patch. please try this one:

diff --git a/fs/notify/fanotify/fanotify_user.c

b/fs/notify/fanotify/fanotify_user.c
index 2b37f27..7864354 100644

--- a/fs/notify/fanotify/fanotify_user.c
+++ b/fs/notify/fanotify/fanotify_user.c

@@ -295,6 +295,16 @@ 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.

@@ -305,7 +315,7 @@ static ssize_t fanotify_read(struct file *file,

char __user *buf,
                                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;

I don't think you want to break out of the reading loop when ret == 0 and
the code might be more readable as:

               if (!(kevent->mask & FAN_ALL_PERM_EVENTS)) {
                        fsnotify_destroy_event(group, kevent);
               } else {
#ifdef CONFIG_FANOTIFY_ACCESS_PERMISSIONS
                        if (ret <= 0) {
                                FANOTIFY_PE(kevent)->response = FAN_DENY;
                                wake_up(&group->fanotify_data.access_waitq);
                        } else {
                                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;

Hmm?

On Fri, Apr 21, 2017 at 5:27 PM, Marko Rauhamaa
[off-list ref] wrote:

Amir Goldstein [off-list ref]:

quoted

Did you notice Jan's comments on my patch? I had a bug that broke out
of the loop. Without his corrections read will return even if there
are more events in the queue.

Yes, I now tried Jan's fix, and it did the trick.

It will now take months or years before distros have a proper fix. In
the interim, I will absorb EOPENSTALE and schedule a reread in that
situation.

Thank you both for your attention.

Marko,

Were you able to verify that both blocking and non-blocking mode work correctly?
May I add your Tested-by and Reported-by tags?

Thanks!
Amir.