| Message ID | 20201202120713.702387-2-amir73il@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | fsnotify fixes and cleanups | expand |
On Wed 02-12-20 14:07:07, Amir Goldstein wrote: > The handle_inode_event() interface was added as (quoting comment): > "a simple variant of handle_event() for groups that only have inode > marks and don't have ignore mask". > > In other words, all backends except fanotify. The inotify backend > also falls under this category, but because it required extra arguments > it was left out of the initial pass of backends conversion to the > simple interface. > > This results in code duplication between the generic helper > fsnotify_handle_event() and the inotify_handle_event() callback > which also happen to be buggy code. > > Generalize the handle_inode_event() arguments and add the check for > FS_EXCL_UNLINK flag to the generic helper, so inotify backend could > be converted to use the simple interface. > > Signed-off-by: Amir Goldstein <amir73il@gmail.com> The patch looks good to me. Just one curious question below. > +static int fsnotify_handle_inode_event(struct fsnotify_group *group, > + struct fsnotify_mark *inode_mark, > + u32 mask, const void *data, int data_type, > + struct inode *dir, const struct qstr *name, > + u32 cookie) > +{ > + const struct path *path = fsnotify_data_path(data, data_type); > + struct inode *inode = fsnotify_data_inode(data, data_type); > + const struct fsnotify_ops *ops = group->ops; > + > + if (WARN_ON_ONCE(!ops->handle_inode_event)) > + return 0; > + > + if ((inode_mark->mask & FS_EXCL_UNLINK) && > + path && d_unlinked(path->dentry)) > + return 0; When I was looking at this condition I was wondering why do we check d_unlinked() and not inode->i_nlink? When is there a difference? Honza
On Thu, Dec 3, 2020 at 11:51 AM Jan Kara <jack@suse.cz> wrote: > > On Wed 02-12-20 14:07:07, Amir Goldstein wrote: > > The handle_inode_event() interface was added as (quoting comment): > > "a simple variant of handle_event() for groups that only have inode > > marks and don't have ignore mask". > > > > In other words, all backends except fanotify. The inotify backend > > also falls under this category, but because it required extra arguments > > it was left out of the initial pass of backends conversion to the > > simple interface. > > > > This results in code duplication between the generic helper > > fsnotify_handle_event() and the inotify_handle_event() callback > > which also happen to be buggy code. > > > > Generalize the handle_inode_event() arguments and add the check for > > FS_EXCL_UNLINK flag to the generic helper, so inotify backend could > > be converted to use the simple interface. > > > > Signed-off-by: Amir Goldstein <amir73il@gmail.com> > > The patch looks good to me. Just one curious question below. > > > +static int fsnotify_handle_inode_event(struct fsnotify_group *group, > > + struct fsnotify_mark *inode_mark, > > + u32 mask, const void *data, int data_type, > > + struct inode *dir, const struct qstr *name, > > + u32 cookie) > > +{ > > + const struct path *path = fsnotify_data_path(data, data_type); > > + struct inode *inode = fsnotify_data_inode(data, data_type); > > + const struct fsnotify_ops *ops = group->ops; > > + > > + if (WARN_ON_ONCE(!ops->handle_inode_event)) > > + return 0; > > + > > + if ((inode_mark->mask & FS_EXCL_UNLINK) && > > + path && d_unlinked(path->dentry)) > > + return 0; > > When I was looking at this condition I was wondering why do we check > d_unlinked() and not inode->i_nlink? When is there a difference? When a hardlink has been unlinked. inotify gets the filename and it doesn't want to get events with unlinked names (although another name could still be linked) I suppose... Thanks, Amir.
On Thu 03-12-20 12:41:41, Amir Goldstein wrote: > On Thu, Dec 3, 2020 at 11:51 AM Jan Kara <jack@suse.cz> wrote: > > > > On Wed 02-12-20 14:07:07, Amir Goldstein wrote: > > > The handle_inode_event() interface was added as (quoting comment): > > > "a simple variant of handle_event() for groups that only have inode > > > marks and don't have ignore mask". > > > > > > In other words, all backends except fanotify. The inotify backend > > > also falls under this category, but because it required extra arguments > > > it was left out of the initial pass of backends conversion to the > > > simple interface. > > > > > > This results in code duplication between the generic helper > > > fsnotify_handle_event() and the inotify_handle_event() callback > > > which also happen to be buggy code. > > > > > > Generalize the handle_inode_event() arguments and add the check for > > > FS_EXCL_UNLINK flag to the generic helper, so inotify backend could > > > be converted to use the simple interface. > > > > > > Signed-off-by: Amir Goldstein <amir73il@gmail.com> > > > > The patch looks good to me. Just one curious question below. > > > > > +static int fsnotify_handle_inode_event(struct fsnotify_group *group, > > > + struct fsnotify_mark *inode_mark, > > > + u32 mask, const void *data, int data_type, > > > + struct inode *dir, const struct qstr *name, > > > + u32 cookie) > > > +{ > > > + const struct path *path = fsnotify_data_path(data, data_type); > > > + struct inode *inode = fsnotify_data_inode(data, data_type); > > > + const struct fsnotify_ops *ops = group->ops; > > > + > > > + if (WARN_ON_ONCE(!ops->handle_inode_event)) > > > + return 0; > > > + > > > + if ((inode_mark->mask & FS_EXCL_UNLINK) && > > > + path && d_unlinked(path->dentry)) > > > + return 0; > > > > When I was looking at this condition I was wondering why do we check > > d_unlinked() and not inode->i_nlink? When is there a difference? > > When a hardlink has been unlinked. > inotify gets the filename and it doesn't want to get events with unlinked > names (although another name could still be linked) I suppose... OK, fair enough. I didn't think about this case. Honza
diff --git a/fs/nfsd/filecache.c b/fs/nfsd/filecache.c index 3c6c2f7d1688..5849c1bd88f1 100644 --- a/fs/nfsd/filecache.c +++ b/fs/nfsd/filecache.c @@ -600,7 +600,7 @@ static struct notifier_block nfsd_file_lease_notifier = { static int nfsd_file_fsnotify_handle_event(struct fsnotify_mark *mark, u32 mask, struct inode *inode, struct inode *dir, - const struct qstr *name) + const struct qstr *name, u32 cookie) { trace_nfsd_file_fsnotify_handle_event(inode, mask); diff --git a/fs/notify/dnotify/dnotify.c b/fs/notify/dnotify/dnotify.c index 5dcda8f20c04..e45ca6ecba95 100644 --- a/fs/notify/dnotify/dnotify.c +++ b/fs/notify/dnotify/dnotify.c @@ -72,7 +72,7 @@ static void dnotify_recalc_inode_mask(struct fsnotify_mark *fsn_mark) */ static int dnotify_handle_event(struct fsnotify_mark *inode_mark, u32 mask, struct inode *inode, struct inode *dir, - const struct qstr *name) + const struct qstr *name, u32 cookie) { struct dnotify_mark *dn_mark; struct dnotify_struct *dn; diff --git a/fs/notify/fsnotify.c b/fs/notify/fsnotify.c index 8d3ad5ef2925..c5c68bcbaadf 100644 --- a/fs/notify/fsnotify.c +++ b/fs/notify/fsnotify.c @@ -232,6 +232,26 @@ int __fsnotify_parent(struct dentry *dentry, __u32 mask, const void *data, } EXPORT_SYMBOL_GPL(__fsnotify_parent); +static int fsnotify_handle_inode_event(struct fsnotify_group *group, + struct fsnotify_mark *inode_mark, + u32 mask, const void *data, int data_type, + struct inode *dir, const struct qstr *name, + u32 cookie) +{ + const struct path *path = fsnotify_data_path(data, data_type); + struct inode *inode = fsnotify_data_inode(data, data_type); + const struct fsnotify_ops *ops = group->ops; + + if (WARN_ON_ONCE(!ops->handle_inode_event)) + return 0; + + if ((inode_mark->mask & FS_EXCL_UNLINK) && + path && d_unlinked(path->dentry)) + return 0; + + return ops->handle_inode_event(inode_mark, mask, inode, dir, name, cookie); +} + static int fsnotify_handle_event(struct fsnotify_group *group, __u32 mask, const void *data, int data_type, struct inode *dir, const struct qstr *name, @@ -239,13 +259,8 @@ static int fsnotify_handle_event(struct fsnotify_group *group, __u32 mask, { struct fsnotify_mark *inode_mark = fsnotify_iter_inode_mark(iter_info); struct fsnotify_mark *child_mark = fsnotify_iter_child_mark(iter_info); - struct inode *inode = fsnotify_data_inode(data, data_type); - const struct fsnotify_ops *ops = group->ops; int ret; - if (WARN_ON_ONCE(!ops->handle_inode_event)) - return 0; - if (WARN_ON_ONCE(fsnotify_iter_sb_mark(iter_info)) || WARN_ON_ONCE(fsnotify_iter_vfsmount_mark(iter_info))) return 0; @@ -262,7 +277,8 @@ static int fsnotify_handle_event(struct fsnotify_group *group, __u32 mask, name = NULL; } - ret = ops->handle_inode_event(inode_mark, mask, inode, dir, name); + ret = fsnotify_handle_inode_event(group, inode_mark, mask, data, data_type, + dir, name, cookie); if (ret || !child_mark) return ret; @@ -272,7 +288,8 @@ static int fsnotify_handle_event(struct fsnotify_group *group, __u32 mask, * report the event once to parent dir with name and once to child * without name. */ - return ops->handle_inode_event(child_mark, mask, inode, NULL, NULL); + return fsnotify_handle_inode_event(group, child_mark, mask, data, data_type, + NULL, NULL, 0); } static int send_to_group(__u32 mask, const void *data, int data_type, diff --git a/include/linux/fsnotify_backend.h b/include/linux/fsnotify_backend.h index f8529a3a2923..4ee3044eedd0 100644 --- a/include/linux/fsnotify_backend.h +++ b/include/linux/fsnotify_backend.h @@ -137,6 +137,7 @@ struct mem_cgroup; * if @file_name is not NULL, this is the directory that * @file_name is relative to. * @file_name: optional file name associated with event + * @cookie: inotify rename cookie * * free_group_priv - called when a group refcnt hits 0 to clean up the private union * freeing_mark - called when a mark is being destroyed for some reason. The group @@ -151,7 +152,7 @@ struct fsnotify_ops { struct fsnotify_iter_info *iter_info); int (*handle_inode_event)(struct fsnotify_mark *mark, u32 mask, struct inode *inode, struct inode *dir, - const struct qstr *file_name); + const struct qstr *file_name, u32 cookie); void (*free_group_priv)(struct fsnotify_group *group); void (*freeing_mark)(struct fsnotify_mark *mark, struct fsnotify_group *group); void (*free_event)(struct fsnotify_event *event); diff --git a/kernel/audit_fsnotify.c b/kernel/audit_fsnotify.c index bfcfcd61adb6..5b3f01da172b 100644 --- a/kernel/audit_fsnotify.c +++ b/kernel/audit_fsnotify.c @@ -154,7 +154,7 @@ static void audit_autoremove_mark_rule(struct audit_fsnotify_mark *audit_mark) /* Update mark data in audit rules based on fsnotify events. */ static int audit_mark_handle_event(struct fsnotify_mark *inode_mark, u32 mask, struct inode *inode, struct inode *dir, - const struct qstr *dname) + const struct qstr *dname, u32 cookie) { struct audit_fsnotify_mark *audit_mark; diff --git a/kernel/audit_tree.c b/kernel/audit_tree.c index 83e1c07fc99e..6c91902f4f45 100644 --- a/kernel/audit_tree.c +++ b/kernel/audit_tree.c @@ -1037,7 +1037,7 @@ static void evict_chunk(struct audit_chunk *chunk) static int audit_tree_handle_event(struct fsnotify_mark *mark, u32 mask, struct inode *inode, struct inode *dir, - const struct qstr *file_name) + const struct qstr *file_name, u32 cookie) { return 0; } diff --git a/kernel/audit_watch.c b/kernel/audit_watch.c index 246e5ba704c0..2acf7ca49154 100644 --- a/kernel/audit_watch.c +++ b/kernel/audit_watch.c @@ -466,7 +466,7 @@ void audit_remove_watch_rule(struct audit_krule *krule) /* Update watch data in audit rules based on fsnotify events. */ static int audit_watch_handle_event(struct fsnotify_mark *inode_mark, u32 mask, struct inode *inode, struct inode *dir, - const struct qstr *dname) + const struct qstr *dname, u32 cookie) { struct audit_parent *parent;
The handle_inode_event() interface was added as (quoting comment): "a simple variant of handle_event() for groups that only have inode marks and don't have ignore mask". In other words, all backends except fanotify. The inotify backend also falls under this category, but because it required extra arguments it was left out of the initial pass of backends conversion to the simple interface. This results in code duplication between the generic helper fsnotify_handle_event() and the inotify_handle_event() callback which also happen to be buggy code. Generalize the handle_inode_event() arguments and add the check for FS_EXCL_UNLINK flag to the generic helper, so inotify backend could be converted to use the simple interface. Signed-off-by: Amir Goldstein <amir73il@gmail.com> --- fs/nfsd/filecache.c | 2 +- fs/notify/dnotify/dnotify.c | 2 +- fs/notify/fsnotify.c | 31 ++++++++++++++++++++++++------- include/linux/fsnotify_backend.h | 3 ++- kernel/audit_fsnotify.c | 2 +- kernel/audit_tree.c | 2 +- kernel/audit_watch.c | 2 +- 7 files changed, 31 insertions(+), 13 deletions(-)