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.
