On Wed, Nov 13, 2024 at 4:36 PM Jan Kara <jack@xxxxxxx> wrote:
>
> On Wed 13-11-24 15:22:50, Amir Goldstein wrote:
> > On Wed, Nov 13, 2024 at 2:43 PM Jan Kara <jack@xxxxxxx> wrote:
> > >
> > > On Mon 11-11-24 21:11:01, Amir Goldstein wrote:
> > > > We got a report that adding a fanotify filsystem watch prevents tail -f
> > > > from receiving events.
> > > >
> > > > Reproducer:
> > > >
> > > > 1. Create 3 windows / login sessions. Become root in each session.
> > > > 2. Choose a mounted filesystem that is pretty quiet; I picked /boot.
> > > > 3. In the first window, run: fsnotifywait -S -m /boot
> > > > 4. In the second window, run: echo data >> /boot/foo
> > > > 5. In the third window, run: tail -f /boot/foo
> > > > 6. Go back to the second window and run: echo more data >> /boot/foo
> > > > 7. Observe that the tail command doesn't show the new data.
> > > > 8. In the first window, hit control-C to interrupt fsnotifywait.
> > > > 9. In the second window, run: echo still more data >> /boot/foo
> > > > 10. Observe that the tail command in the third window has now printed
> > > > the missing data.
> > > >
> > > > When stracing tail, we observed that when fanotify filesystem mark is
> > > > set, tail does get the inotify event, but the event is receieved with
> > > > the filename:
> > > >
> > > > read(4, "\1\0\0\0\2\0\0\0\0\0\0\0\20\0\0\0foo\0\0\0\0\0\0\0\0\0\0\0\0\0",
> > > > 50) = 32
> > > >
> > > > This is unexpected, because tail is watching the file itself and not its
> > > > parent and is inconsistent with the inotify event received by tail when
> > > > fanotify filesystem mark is not set:
> > > >
> > > > read(4, "\1\0\0\0\2\0\0\0\0\0\0\0\0\0\0\0", 50) = 16
> > > >
> > > > The inteference between different fsnotify groups was caused by the fact
> > > > that the mark on the sb requires the filename, so the filename is passed
> > > > to fsnotify(). Later on, fsnotify_handle_event() tries to take care of
> > > > not passing the filename to groups (such as inotify) that are interested
> > > > in the filename only when the parent is watching.
> > > >
> > > > But the logic was incorrect for the case that no group is watching the
> > > > parent, some groups are watching the sb and some watching the inode.
> > > >
> > > > Reported-by: Miklos Szeredi <miklos@xxxxxxxxxx>
> > > > Fixes: 7372e79c9eb9 ("fanotify: fix logic of reporting name info with watched parent")
> > > > Cc: stable@xxxxxxxxxxxxxxx # 5.10+
> > > > Signed-off-by: Amir Goldstein <amir73il@xxxxxxxxx>
> > >
> > > Thanks for analysis, Amir!
> > >
> > > > @@ -333,12 +333,14 @@ static int fsnotify_handle_event(struct fsnotify_group *group, __u32 mask,
> > > > if (!inode_mark)
> > > > return 0;
> > > >
> > > > - if (mask & FS_EVENT_ON_CHILD) {
> > > > + if (mask & FS_EVENTS_POSS_ON_CHILD) {
> > >
> > > So this is going to work but as far as I'm reading the code in
> > > fsnotify_handle_event() I would be maybe calmer if we instead wrote the
> > > condition as:
> > >
> > > if (!(mask & ALL_FSNOTIFY_DIRENT_EVENTS))
> >
> > The problem is that the comment below
> > "Some events can be sent on both parent dir and child marks..."
> > is relevant in the context of FS_EVENTS_POSS_ON_CHILD
> > and FS_EVENT_ON_CHILD, meaning those are exactly the set of
> > events that could be sent to parent with FS_EVENT_ON_CHILD
> > and to child without it.
> >
> > The comment makes no sense in the context of the
> > ALL_FSNOTIFY_DIRENT_EVENTS check,
> > Unless we add a comment saying the dirent events set has
> > zero intersection with events possible on child.
>
> Good point and what I *actually* wanted to do is:
>
> /*
> * Some events can be sent on both parent dir and child marks (e.g.
> * FS_ATTRIB). If both parent dir and child are watching, report the
> * event once to parent dir with name (if interested) and once to child
> * without name (if interested).
> *
> * In any case regardless whether the parent is watching or not, the
> * child watcher is expecting an event without the FS_EVENT_ON_CHILD
> * flag. The file name is expected if and only if this is a directory
> * event.
> */
> mask &= ~FS_EVENT_ON_CHILD;
> if (!(mask & ALL_FSNOTIFY_DIRENT_EVENTS)) {
> dir = NULL;
> name = NULL;
> }
>
> Hmm?
Ok, that works for me :)
Thanks,
Amir.
