| Message ID | 20211019000015.1666608-12-krisman@collabora.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | file system-wide error monitoring | expand |
On Tue, Oct 19, 2021 at 3:01 AM Gabriel Krisman Bertazi <krisman@collabora.com> wrote: > > FAN_FS_ERROR allows events without inodes - i.e. for file system-wide > errors. Even though fsnotify_handle_inode_event is not currently used > by fanotify, this patch protects this path to handle this new case. > > Suggested-by: Amir Goldstein <amir73il@gmail.com> > Signed-off-by: Gabriel Krisman Bertazi <krisman@collabora.com> > --- > fs/notify/fsnotify.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/fs/notify/fsnotify.c b/fs/notify/fsnotify.c > index fde3a1115a17..47f931fb571c 100644 > --- a/fs/notify/fsnotify.c > +++ b/fs/notify/fsnotify.c > @@ -252,6 +252,9 @@ static int fsnotify_handle_inode_event(struct fsnotify_group *group, > if (WARN_ON_ONCE(!ops->handle_inode_event)) > return 0; > > + if (!inode) > + return 0; > + Sigh.. the plot thickens. There are three in-tree backends that implement the ->handle_inode_event() interface. inotify and dnotify can take NULL inode and the above will make the CREATE events on kernfs vanish, so we cannot do that. Sorry for not noticing this earlier when I asked for this change. nfsd_file_fsnotify_handle_event() can most certainly not take NULL inode, but nfsd does not watch for CREATE events. I think what we need to do is (Jan please correct me if you think otherwise): 1. Document the handle_inode_event() interface that either inode or dir must be non-NULL 2. WARN_ON_ONCE(!inode && !dir) instead of just (!inode) above 3. Add WARN_ON_ONCE(!inode) before trace_nfsd_file_fsnotify_handle_event() in nfsd_file_fsnotify_handle_event() Apologies, Gabriel, for having to cleanup our mess ;-) Thanks, Amir.
On Tue 19-10-21 08:34:41, Amir Goldstein wrote: > On Tue, Oct 19, 2021 at 3:01 AM Gabriel Krisman Bertazi > <krisman@collabora.com> wrote: > > > > FAN_FS_ERROR allows events without inodes - i.e. for file system-wide > > errors. Even though fsnotify_handle_inode_event is not currently used > > by fanotify, this patch protects this path to handle this new case. > > > > Suggested-by: Amir Goldstein <amir73il@gmail.com> > > Signed-off-by: Gabriel Krisman Bertazi <krisman@collabora.com> > > --- > > fs/notify/fsnotify.c | 3 +++ > > 1 file changed, 3 insertions(+) > > > > diff --git a/fs/notify/fsnotify.c b/fs/notify/fsnotify.c > > index fde3a1115a17..47f931fb571c 100644 > > --- a/fs/notify/fsnotify.c > > +++ b/fs/notify/fsnotify.c > > @@ -252,6 +252,9 @@ static int fsnotify_handle_inode_event(struct fsnotify_group *group, > > if (WARN_ON_ONCE(!ops->handle_inode_event)) > > return 0; > > > > + if (!inode) > > + return 0; > > + > > Sigh.. the plot thickens. > There are three in-tree backends that implement the ->handle_inode_event() > interface. > > inotify and dnotify can take NULL inode and the above will make the CREATE > events on kernfs vanish, so we cannot do that. > Sorry for not noticing this earlier when I asked for this change. > > nfsd_file_fsnotify_handle_event() can most certainly not take NULL inode, > but nfsd does not watch for CREATE events. And furthermore you cannot really export kernfs :) > I think what we need to do is (Jan please correct me if you think otherwise): > 1. Document the handle_inode_event() interface that either inode or dir > must be non-NULL > 2. WARN_ON_ONCE(!inode && !dir) instead of just (!inode) above Yeah, like: if (WARN_ON_ONCE(!inode && !dir)) return 0; > 3. Add WARN_ON_ONCE(!inode) before trace_nfsd_file_fsnotify_handle_event() > in nfsd_file_fsnotify_handle_event() And: if (WARN_ON_ONCE(!inode)) return 0; Sounds like a good plan to me. Honza
diff --git a/fs/notify/fsnotify.c b/fs/notify/fsnotify.c index fde3a1115a17..47f931fb571c 100644 --- a/fs/notify/fsnotify.c +++ b/fs/notify/fsnotify.c @@ -252,6 +252,9 @@ static int fsnotify_handle_inode_event(struct fsnotify_group *group, if (WARN_ON_ONCE(!ops->handle_inode_event)) return 0; + if (!inode) + return 0; + if ((inode_mark->mask & FS_EXCL_UNLINK) && path && d_unlinked(path->dentry)) return 0;
FAN_FS_ERROR allows events without inodes - i.e. for file system-wide errors. Even though fsnotify_handle_inode_event is not currently used by fanotify, this patch protects this path to handle this new case. Suggested-by: Amir Goldstein <amir73il@gmail.com> Signed-off-by: Gabriel Krisman Bertazi <krisman@collabora.com> --- fs/notify/fsnotify.c | 3 +++ 1 file changed, 3 insertions(+)