[v5,16/22] fsnotify: remove check that source dentry is positive
diff mbox series

Message ID 20200716084230.30611-17-amir73il@gmail.com
State New
Headers show
Series
  • fanotify events with name info
Related show

Commit Message

Amir Goldstein July 16, 2020, 8:42 a.m. UTC
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(-)

Comments

Jan Kara July 16, 2020, 1:13 p.m. UTC | #1
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
>
Jan Kara July 16, 2020, 1:29 p.m. UTC | #2
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
Amir Goldstein July 16, 2020, 1:54 p.m. UTC | #3
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.
Jan Kara July 16, 2020, 2:06 p.m. UTC | #4
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

Patch
diff mbox series

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);
 }