| Message ID | 20241111201101.177412-1-amir73il@gmail.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | fsnotify: fix sending inotify event with unexpected filename | expand |
On Mon, Nov 11, 2024 at 09:11:01PM +0100, 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@szeredi.hu> > Fixes: 7372e79c9eb9 ("fanotify: fix logic of reporting name info with watched parent") > Cc: stable@vger.kernel.org # 5.10+ > Signed-off-by: Amir Goldstein <amir73il@gmail.com> Reviewed-by: Josef Bacik <josef@toxicpanda.com> Thanks, Josef
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@szeredi.hu> > Fixes: 7372e79c9eb9 ("fanotify: fix logic of reporting name info with watched parent") > Cc: stable@vger.kernel.org # 5.10+ > Signed-off-by: Amir Goldstein <amir73il@gmail.com> 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)) I.e., if the event on the inode is not expecting name & dir, clear them. Instead of your variant which I understand as: "if we could have added name & dir only for parent, clear it now". The bitwise difference between these two checks is: FS_DELETE_SELF | FS_MOVE_SELF | FS_UNMOUNT | FS_Q_OVERFLOW | FS_IN_IGNORED | FS_ERROR, none of which should matter. Maybe I'm paranoid but we already had too many subtle bugs in this code so I'm striving for maximum robustness :). What do you think? Honza > /* > * 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, whether the parent is watching or not watching, > * The child watcher is expecting an event without a file name > * and without the FS_EVENT_ON_CHILD flag. > */ > -- > 2.34.1 >
On Wed 13-11-24 14:42:58, Jan Kara 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@szeredi.hu> > > Fixes: 7372e79c9eb9 ("fanotify: fix logic of reporting name info with watched parent") > > Cc: stable@vger.kernel.org # 5.10+ > > Signed-off-by: Amir Goldstein <amir73il@gmail.com> > > 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)) > > I.e., if the event on the inode is not expecting name & dir, clear them. > Instead of your variant which I understand as: "if we could have added name > & dir only for parent, clear it now". The bitwise difference between these > two checks is: FS_DELETE_SELF | FS_MOVE_SELF | FS_UNMOUNT | FS_Q_OVERFLOW | > FS_IN_IGNORED | FS_ERROR, none of which should matter. Maybe I'm paranoid > but we already had too many subtle bugs in this code so I'm striving for > maximum robustness :). What do you think? BTW, I can just massage the patch on commit since you're now busy with HSM stuff but I wanted to check what's your opinion on the change. Honza
On Wed, Nov 13, 2024 at 2:43 PM Jan Kara <jack@suse.cz> 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@szeredi.hu> > > Fixes: 7372e79c9eb9 ("fanotify: fix logic of reporting name info with watched parent") > > Cc: stable@vger.kernel.org # 5.10+ > > Signed-off-by: Amir Goldstein <amir73il@gmail.com> > > 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. > > I.e., if the event on the inode is not expecting name & dir, clear them. > Instead of your variant which I understand as: "if we could have added name > & dir only for parent, clear it now". The bitwise difference between these > two checks is: FS_DELETE_SELF | FS_MOVE_SELF | FS_UNMOUNT | FS_Q_OVERFLOW | > FS_IN_IGNORED | FS_ERROR, none of which should matter. Maybe I'm paranoid > but we already had too many subtle bugs in this code so I'm striving for > maximum robustness :). What do you think? How about a BUILD_BUG_ON(FS_EVENTS_POSS_ON_CHILD & ALL_FSNOTIFY_DIRENT_EVENTS) with a comment to clarify? > BTW, I can just massage the patch on commit since you're now busy with HSM > stuff but I wanted to check what's your opinion on the change. Sure, no problem. Thanks, Amir.
On Wed 13-11-24 15:22:50, Amir Goldstein wrote: > On Wed, Nov 13, 2024 at 2:43 PM Jan Kara <jack@suse.cz> 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@szeredi.hu> > > > Fixes: 7372e79c9eb9 ("fanotify: fix logic of reporting name info with watched parent") > > > Cc: stable@vger.kernel.org # 5.10+ > > > Signed-off-by: Amir Goldstein <amir73il@gmail.com> > > > > 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? Honza
On Wed, Nov 13, 2024 at 4:36 PM Jan Kara <jack@suse.cz> wrote: > > On Wed 13-11-24 15:22:50, Amir Goldstein wrote: > > On Wed, Nov 13, 2024 at 2:43 PM Jan Kara <jack@suse.cz> 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@szeredi.hu> > > > > Fixes: 7372e79c9eb9 ("fanotify: fix logic of reporting name info with watched parent") > > > > Cc: stable@vger.kernel.org # 5.10+ > > > > Signed-off-by: Amir Goldstein <amir73il@gmail.com> > > > > > > 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.
diff --git a/fs/notify/fsnotify.c b/fs/notify/fsnotify.c index 82ae8254c068..316eec309299 100644 --- a/fs/notify/fsnotify.c +++ b/fs/notify/fsnotify.c @@ -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) { /* * 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, whether the parent is watching or not watching, * The child watcher is expecting an event without a file name * and without the FS_EVENT_ON_CHILD flag. */
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@szeredi.hu> Fixes: 7372e79c9eb9 ("fanotify: fix logic of reporting name info with watched parent") Cc: stable@vger.kernel.org # 5.10+ Signed-off-by: Amir Goldstein <amir73il@gmail.com> --- Jan, I tested this fix with the manual reproducer and it passes the LTS sanity tests. Did not have time to write fanotify+inotify test yet. Thanks, Amir. fs/notify/fsnotify.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)