On Thu, Apr 20, 2017 at 6:06 PM, Amir Goldstein <amir73il@xxxxxxxxx> wrote: > On Thu, Apr 20, 2017 at 5:20 PM, Jan Kara <jack@xxxxxxx> wrote: >> On Thu 20-04-17 14:33:04, Amir Goldstein wrote: >>> >>> 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 <marko.rauhamaa@xxxxxxxxxxxx> wrote: > Amir Goldstein <amir73il@xxxxxxxxx>: > >> 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. -- To unsubscribe from this list: send the line "unsubscribe linux-api" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html