[PATCH] fanotify: do not allow setting FAN_RENAME on non-dir

[PATCH] fanotify: do not allow setting FAN_RENAME on non-dir

[Amir Goldstein]

The desired sematics of this action are not clear, so for now deny
this action.  We may relax it when we decide on the semantics and
implement them.

Fixes: 8cc3b1ccd930 ("fanotify: wire up FAN_RENAME event")
Link: https://lore.kernel.org/linux-fsdevel/20220505133057.zm5t6vumc4xdcnsg@quack3.lan/
Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
 fs/notify/fanotify/fanotify_user.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/fs/notify/fanotify/fanotify_user.c b/fs/notify/fanotify/fanotify_user.c
index edad67d674dc..ae0c27fac651 100644
--- a/fs/notify/fanotify/fanotify_user.c
+++ b/fs/notify/fanotify/fanotify_user.c
@@ -1695,6 +1695,11 @@ static int do_fanotify_mark(int fanotify_fd, unsigned int flags, __u64 mask,
 	else
 		mnt = path.mnt;
 
+	/* FAN_RENAME is not allowed on non-dir (for now) */
+	ret = -EINVAL;
+	if (inode && (mask & FAN_RENAME) && !S_ISDIR(inode->i_mode))
+		goto path_put_and_out;
+
 	/* Mask out FAN_EVENT_ON_CHILD flag for sb/mount/non-dir marks */
 	if (mnt || !S_ISDIR(inode->i_mode)) {
 		mask &= ~FAN_EVENT_ON_CHILD;
-- 
2.25.1

Re: [PATCH] fanotify: do not allow setting FAN_RENAME on non-dir

[Matthew Bobrowski]

On Fri, May 06, 2022 at 04:46:26AM +0300, Amir Goldstein wrote:
> The desired sematics of this action are not clear, so for now deny
> this action.  We may relax it when we decide on the semantics and
> implement them.
> 
> Fixes: 8cc3b1ccd930 ("fanotify: wire up FAN_RENAME event")
> Link: https://lore.kernel.org/linux-fsdevel/20220505133057.zm5t6vumc4xdcnsg@quack3.lan/
> Signed-off-by: Amir Goldstein <amir73il@gmail.com>

Thanks for sending this patch through Amir, LGTM.

Feel free to add:

Reviewed-by: Matthew Bobrowski <repnop@google.com>

> ---
>  fs/notify/fanotify/fanotify_user.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/fs/notify/fanotify/fanotify_user.c b/fs/notify/fanotify/fanotify_user.c
> index edad67d674dc..ae0c27fac651 100644
> --- a/fs/notify/fanotify/fanotify_user.c
> +++ b/fs/notify/fanotify/fanotify_user.c
> @@ -1695,6 +1695,11 @@ static int do_fanotify_mark(int fanotify_fd, unsigned int flags, __u64 mask,
>  	else
>  		mnt = path.mnt;
>  
> +	/* FAN_RENAME is not allowed on non-dir (for now) */
> +	ret = -EINVAL;
> +	if (inode && (mask & FAN_RENAME) && !S_ISDIR(inode->i_mode))
> +		goto path_put_and_out;
> +
>  	/* Mask out FAN_EVENT_ON_CHILD flag for sb/mount/non-dir marks */
>  	if (mnt || !S_ISDIR(inode->i_mode)) {
>  		mask &= ~FAN_EVENT_ON_CHILD;
> -- 
> 2.25.1

/M

Re: [PATCH] fanotify: do not allow setting FAN_RENAME on non-dir

[Jan Kara]

On Fri 06-05-22 04:46:26, Amir Goldstein wrote:
> The desired sematics of this action are not clear, so for now deny
> this action.  We may relax it when we decide on the semantics and
> implement them.
> 
> Fixes: 8cc3b1ccd930 ("fanotify: wire up FAN_RENAME event")
> Link: https://lore.kernel.org/linux-fsdevel/20220505133057.zm5t6vumc4xdcnsg@quack3.lan/
> Signed-off-by: Amir Goldstein <amir73il@gmail.com>

Thanks guys. I've merged the fix to my tree (fast_track branch) and will
push it to Linus on Monday once it gets at least some exposure to
auto-testers.

								Honza

> ---
>  fs/notify/fanotify/fanotify_user.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/fs/notify/fanotify/fanotify_user.c b/fs/notify/fanotify/fanotify_user.c
> index edad67d674dc..ae0c27fac651 100644
> --- a/fs/notify/fanotify/fanotify_user.c
> +++ b/fs/notify/fanotify/fanotify_user.c
> @@ -1695,6 +1695,11 @@ static int do_fanotify_mark(int fanotify_fd, unsigned int flags, __u64 mask,
>  	else
>  		mnt = path.mnt;
>  
> +	/* FAN_RENAME is not allowed on non-dir (for now) */
> +	ret = -EINVAL;
> +	if (inode && (mask & FAN_RENAME) && !S_ISDIR(inode->i_mode))
> +		goto path_put_and_out;
> +
>  	/* Mask out FAN_EVENT_ON_CHILD flag for sb/mount/non-dir marks */
>  	if (mnt || !S_ISDIR(inode->i_mode)) {
>  		mask &= ~FAN_EVENT_ON_CHILD;
> -- 
> 2.25.1
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [PATCH] fanotify: do not allow setting FAN_RENAME on non-dir

[Amir Goldstein]

On Fri, May 6, 2022 at 12:12 PM Jan Kara <jack@suse.cz> wrote:
>
> On Fri 06-05-22 04:46:26, Amir Goldstein wrote:
> > The desired sematics of this action are not clear, so for now deny
> > this action.  We may relax it when we decide on the semantics and
> > implement them.
> >
> > Fixes: 8cc3b1ccd930 ("fanotify: wire up FAN_RENAME event")
> > Link: https://lore.kernel.org/linux-fsdevel/20220505133057.zm5t6vumc4xdcnsg@quack3.lan/
> > Signed-off-by: Amir Goldstein <amir73il@gmail.com>
>
> Thanks guys. I've merged the fix to my tree (fast_track branch) and will
> push it to Linus on Monday once it gets at least some exposure to
> auto-testers.

Actually, I made a mistake.
It should not return -EINVAL, it should return -ENOTDIR, because
the error is the object not the syscall argument values.

Going forward, when Matthew implements FAN_RENAME on
non-dir, the error should be changed to -EPERM (for unpriv caller).
This will allow better documentation of behavior in kernel 5.17 and
newer kernels.

Can you please fix that before sending to Linus?

Thanks,
Amir.

Re: [PATCH] fanotify: do not allow setting FAN_RENAME on non-dir

[Amir Goldstein]

On Sat, May 7, 2022 at 8:57 AM Amir Goldstein <amir73il@gmail.com> wrote:
>
> On Fri, May 6, 2022 at 12:12 PM Jan Kara <jack@suse.cz> wrote:
> >
> > On Fri 06-05-22 04:46:26, Amir Goldstein wrote:
> > > The desired sematics of this action are not clear, so for now deny
> > > this action.  We may relax it when we decide on the semantics and
> > > implement them.
> > >
> > > Fixes: 8cc3b1ccd930 ("fanotify: wire up FAN_RENAME event")
> > > Link: https://lore.kernel.org/linux-fsdevel/20220505133057.zm5t6vumc4xdcnsg@quack3.lan/
> > > Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> >
> > Thanks guys. I've merged the fix to my tree (fast_track branch) and will
> > push it to Linus on Monday once it gets at least some exposure to
> > auto-testers.
>
> Actually, I made a mistake.
> It should not return -EINVAL, it should return -ENOTDIR, because
> the error is the object not the syscall argument values.
>
> Going forward, when Matthew implements FAN_RENAME on
> non-dir, the error should be changed to -EPERM (for unpriv caller).
> This will allow better documentation of behavior in kernel 5.17 and
> newer kernels.
>
> Can you please fix that before sending to Linus?
>

Actually, nevermind, I have thought of another issue so posting v2.
I think we should deny all dirent events on non-dir with
FAN_REPORT_TARGET_FID.

This is what happens when posting patches from airports :-P

Thanks,
Amir.

Re: [PATCH] fanotify: do not allow setting FAN_RENAME on non-dir

[Jan Kara]

On Sat 07-05-22 08:57:00, Amir Goldstein wrote:
> On Fri, May 6, 2022 at 12:12 PM Jan Kara <jack@suse.cz> wrote:
> >
> > On Fri 06-05-22 04:46:26, Amir Goldstein wrote:
> > > The desired sematics of this action are not clear, so for now deny
> > > this action.  We may relax it when we decide on the semantics and
> > > implement them.
> > >
> > > Fixes: 8cc3b1ccd930 ("fanotify: wire up FAN_RENAME event")
> > > Link: https://lore.kernel.org/linux-fsdevel/20220505133057.zm5t6vumc4xdcnsg@quack3.lan/
> > > Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> >
> > Thanks guys. I've merged the fix to my tree (fast_track branch) and will
> > push it to Linus on Monday once it gets at least some exposure to
> > auto-testers.
> 
> Actually, I made a mistake.
> It should not return -EINVAL, it should return -ENOTDIR, because
> the error is the object not the syscall argument values.

Yeah, makes sense.

> Going forward, when Matthew implements FAN_RENAME on
> non-dir, the error should be changed to -EPERM (for unpriv caller).
> This will allow better documentation of behavior in kernel 5.17 and
> newer kernels.
> 
> Can you please fix that before sending to Linus?

Yup, I'll update that.

								Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR