| Message ID | 20200716084230.30611-17-amir73il@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | fanotify events with name info | expand |
On Thu 16-07-20 11:42:24, Amir Goldstein wrote: > Remove the unneeded check for positive source dentry in > fsnotify_move(). > > fsnotify_move() hook is mostly called from vfs_rename() under > lock_rename() and vfs_rename() starts with may_delete() test that > verifies positive source dentry. The only other caller of > fsnotify_move() - debugfs_rename() also verifies positive source. > > Signed-off-by: Amir Goldstein <amir73il@gmail.com> But in vfs_rename() if RENAME_EXCHANGE is set and target is NULL, new_dentry can be negative when calling fsnotify_move() AFAICT, cannot it? Honza > --- > include/linux/fsnotify.h | 3 +-- > 1 file changed, 1 insertion(+), 2 deletions(-) > > diff --git a/include/linux/fsnotify.h b/include/linux/fsnotify.h > index 044cae3a0628..fe4f2bc5b4c2 100644 > --- a/include/linux/fsnotify.h > +++ b/include/linux/fsnotify.h > @@ -149,8 +149,7 @@ static inline void fsnotify_move(struct inode *old_dir, struct inode *new_dir, > if (target) > fsnotify_link_count(target); > > - if (source) > - fsnotify(source, mask, source, FSNOTIFY_EVENT_INODE, NULL, 0); > + fsnotify(source, mask, source, FSNOTIFY_EVENT_INODE, NULL, 0); > audit_inode_child(new_dir, moved, AUDIT_TYPE_CHILD_CREATE); > } > > -- > 2.17.1 >
On Thu 16-07-20 15:13:11, Jan Kara wrote: > On Thu 16-07-20 11:42:24, Amir Goldstein wrote: > > Remove the unneeded check for positive source dentry in > > fsnotify_move(). > > > > fsnotify_move() hook is mostly called from vfs_rename() under > > lock_rename() and vfs_rename() starts with may_delete() test that > > verifies positive source dentry. The only other caller of > > fsnotify_move() - debugfs_rename() also verifies positive source. > > > > Signed-off-by: Amir Goldstein <amir73il@gmail.com> > > But in vfs_rename() if RENAME_EXCHANGE is set and target is NULL, > new_dentry can be negative when calling fsnotify_move() AFAICT, cannot it? FWIW, renameat2() doesn't allow RENAME_EXCHANGE with non-existent target and I didn't find any other place that could call vfs_rename() with RENAME_EXCHANGE and negative target but still vfs_rename() seems to support that and so fsnotify should likely handle that as well. Honza
On Thu, Jul 16, 2020 at 4:29 PM Jan Kara <jack@suse.cz> wrote: > > On Thu 16-07-20 15:13:11, Jan Kara wrote: > > On Thu 16-07-20 11:42:24, Amir Goldstein wrote: > > > Remove the unneeded check for positive source dentry in > > > fsnotify_move(). > > > > > > fsnotify_move() hook is mostly called from vfs_rename() under > > > lock_rename() and vfs_rename() starts with may_delete() test that > > > verifies positive source dentry. The only other caller of > > > fsnotify_move() - debugfs_rename() also verifies positive source. > > > > > > Signed-off-by: Amir Goldstein <amir73il@gmail.com> > > > > But in vfs_rename() if RENAME_EXCHANGE is set and target is NULL, > > new_dentry can be negative when calling fsnotify_move() AFAICT, cannot it? > > FWIW, renameat2() doesn't allow RENAME_EXCHANGE with non-existent target > and I didn't find any other place that could call vfs_rename() with > RENAME_EXCHANGE and negative target but still vfs_rename() seems to support > that and so fsnotify should likely handle that as well. If some code did call vfs_rename() like that d_exchange() will barf: void d_exchange(struct dentry *dentry1, struct dentry *dentry2) { write_seqlock(&rename_lock); WARN_ON(!dentry1->d_inode); WARN_ON(!dentry2->d_inode); Thanks, Amir.
On Thu 16-07-20 16:54:08, Amir Goldstein wrote: > On Thu, Jul 16, 2020 at 4:29 PM Jan Kara <jack@suse.cz> wrote: > > > > On Thu 16-07-20 15:13:11, Jan Kara wrote: > > > On Thu 16-07-20 11:42:24, Amir Goldstein wrote: > > > > Remove the unneeded check for positive source dentry in > > > > fsnotify_move(). > > > > > > > > fsnotify_move() hook is mostly called from vfs_rename() under > > > > lock_rename() and vfs_rename() starts with may_delete() test that > > > > verifies positive source dentry. The only other caller of > > > > fsnotify_move() - debugfs_rename() also verifies positive source. > > > > > > > > Signed-off-by: Amir Goldstein <amir73il@gmail.com> > > > > > > But in vfs_rename() if RENAME_EXCHANGE is set and target is NULL, > > > new_dentry can be negative when calling fsnotify_move() AFAICT, cannot it? > > > > FWIW, renameat2() doesn't allow RENAME_EXCHANGE with non-existent target > > and I didn't find any other place that could call vfs_rename() with > > RENAME_EXCHANGE and negative target but still vfs_rename() seems to support > > that and so fsnotify should likely handle that as well. > > If some code did call vfs_rename() like that d_exchange() will barf: > > void d_exchange(struct dentry *dentry1, struct dentry *dentry2) > { > write_seqlock(&rename_lock); > > WARN_ON(!dentry1->d_inode); > WARN_ON(!dentry2->d_inode); Good point. Thanks for explanation! Honza
diff --git a/include/linux/fsnotify.h b/include/linux/fsnotify.h index 044cae3a0628..fe4f2bc5b4c2 100644 --- a/include/linux/fsnotify.h +++ b/include/linux/fsnotify.h @@ -149,8 +149,7 @@ static inline void fsnotify_move(struct inode *old_dir, struct inode *new_dir, if (target) fsnotify_link_count(target); - if (source) - fsnotify(source, mask, source, FSNOTIFY_EVENT_INODE, NULL, 0); + fsnotify(source, mask, source, FSNOTIFY_EVENT_INODE, NULL, 0); audit_inode_child(new_dir, moved, AUDIT_TYPE_CHILD_CREATE); }
Remove the unneeded check for positive source dentry in fsnotify_move(). fsnotify_move() hook is mostly called from vfs_rename() under lock_rename() and vfs_rename() starts with may_delete() test that verifies positive source dentry. The only other caller of fsnotify_move() - debugfs_rename() also verifies positive source. Signed-off-by: Amir Goldstein <amir73il@gmail.com> --- include/linux/fsnotify.h | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)