Re: [RFC][PATCH 0/4] fsnotify: pass single mark to handle_event()

[Jan Kara <jack@suse.cz>]

Hi,

On Tue 27-12-16 21:32:24, Amir Goldstein wrote:
> I thought this would turn out simpler, so you may be able to use it
> for your work, but I'm afraid that's not the case.
> 
> Anyway, since I am leaving for new year's vacation, I am posting
> what I have in case you want to use any of it.
> 
> It passed some initial tests I ran, but when I wanted to test the
> corner case referred to in patch 1, I found that my test program
> hangs open() syscalls with kernel 4.10-rc1 before any of my changes.
> 
> This is the mark setup I was testing [1]:
>   fanotify_mark(fd, FAN_MARK_ADD,
>                 FAN_OPEN_PERM | FAN_EVENT_ON_CHILD, AT_FDCWD,
>                 path);
>   fanotify_mark(fd, FAN_MARK_ADD | \
>                 FAN_MARK_IGNORED_SURV_MODIFY | FAN_MARK_IGNORED_MASK
>                 FAN_OPEN_PERM | FAN_EVENT_ON_CHILD, AT_FDCWD,
>                 FAN_CLOSE_WRITE, AT_FDCWD,
>                 path);
>   fanotify_mark(fd, FAN_MARK_ADD | FAN_MARK_MOUNT,
>                 FAN_OPEN_PERM | FAN_CLOSE_WRITE, AT_FDCWD,
>                 path);
> 
> Without FAN_EVENT_ON_CHILD it works fine, but with FAN_EVENT_ON_CHILD,
> something bad is going on and I did not have time to look into it.

I had a look at the patches and the result does not look simpler than what
we had before AFAICT. Sure we don't have to pass both marks into
->handle_event but is that really such a big win? And actually my patches
for dropping SRCU lock when waiting for userspace response to fanotify
permission event need both marks in ->handle_event because they both need
to be protected against freeing when SRCU lock is dropped... So I don't
think this is really viable path.

However one thing that may be worth cleaning up is that
fanotify_should_send_event() needlessly checks the masks - send to group
already did this. So I'd move the check for FS_EVENT_ON_CHILD from
fanotify_should_send_event() to send_to_group() - arguably it belongs there
- and then just completely drop checking of the masks from
fanotify_should_send_event(). What do you think?

> In general, I would like to start working on an fsnotify testsuite,
> so if you have any plans wrt writing extra tests or ideas about specific
> missing tests, please let me know about them.

That would be certainly worthwhile. Actually when I find some useful
testcase I add it to LTP under the
testcases/kernel/syscalls/{fanotify|inotify}. So please extend that if you
have some more ideas for testcases.

								Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR