diff mbox

[2/2] vfs: dedupe should return EPERM if permission is not granted

Message ID 20180511192651.21324-3-mfasheh@suse.de (mailing list archive)
State New, archived
Headers show

Commit Message

Mark Fasheh May 11, 2018, 7:26 p.m. UTC
Right now we return EINVAL if a process does not have permission to dedupe a
file. This was an oversight on my part. EPERM gives a true description of
the nature of our error, and EINVAL is already used for the case that the
filesystem does not support dedupe.

Signed-off-by: Mark Fasheh <mfasheh@suse.de>
---
 fs/read_write.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Darrick J. Wong May 12, 2018, 12:06 a.m. UTC | #1
On Fri, May 11, 2018 at 12:26:51PM -0700, Mark Fasheh wrote:
> Right now we return EINVAL if a process does not have permission to dedupe a
> file. This was an oversight on my part. EPERM gives a true description of
> the nature of our error, and EINVAL is already used for the case that the
> filesystem does not support dedupe.
> 
> Signed-off-by: Mark Fasheh <mfasheh@suse.de>
> ---
>  fs/read_write.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/fs/read_write.c b/fs/read_write.c
> index 77986a2e2a3b..8edef43a182c 100644
> --- a/fs/read_write.c
> +++ b/fs/read_write.c
> @@ -2038,7 +2038,7 @@ int vfs_dedupe_file_range(struct file *file, struct file_dedupe_range *same)
>  			info->status = -EINVAL;
>  		} else if (!(is_admin || (dst_file->f_mode & FMODE_WRITE) ||
>  			     uid_eq(current_fsuid(), dst->i_uid))) {
> -			info->status = -EINVAL;
> +			info->status = -EPERM;

Hmm, are we allowed to change this aspect of the kabi after the fact?

Granted, we're only trading one error code for another, but will the
existing users of this care?  xfs_io won't and I assume duperemove won't
either, but what about bees? :)

--D

>  		} else if (file->f_path.mnt != dst_file->f_path.mnt) {
>  			info->status = -EXDEV;
>  		} else if (S_ISDIR(dst->i_mode)) {
> -- 
> 2.15.1
>
Amir Goldstein May 12, 2018, 4:15 a.m. UTC | #2
On Sat, May 12, 2018 at 3:06 AM, Darrick J. Wong
<darrick.wong@oracle.com> wrote:
> On Fri, May 11, 2018 at 12:26:51PM -0700, Mark Fasheh wrote:
>> Right now we return EINVAL if a process does not have permission to dedupe a
>> file. This was an oversight on my part. EPERM gives a true description of
>> the nature of our error, and EINVAL is already used for the case that the
>> filesystem does not support dedupe.
>>
>> Signed-off-by: Mark Fasheh <mfasheh@suse.de>
>> ---
>>  fs/read_write.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/fs/read_write.c b/fs/read_write.c
>> index 77986a2e2a3b..8edef43a182c 100644
>> --- a/fs/read_write.c
>> +++ b/fs/read_write.c
>> @@ -2038,7 +2038,7 @@ int vfs_dedupe_file_range(struct file *file, struct file_dedupe_range *same)
>>                       info->status = -EINVAL;
>>               } else if (!(is_admin || (dst_file->f_mode & FMODE_WRITE) ||
>>                            uid_eq(current_fsuid(), dst->i_uid))) {
>> -                     info->status = -EINVAL;
>> +                     info->status = -EPERM;
>
> Hmm, are we allowed to change this aspect of the kabi after the fact?
>
> Granted, we're only trading one error code for another, but will the
> existing users of this care?  xfs_io won't and I assume duperemove won't
> either, but what about bees? :)
>

Relaxing -EINVAL is the common case with kabi.
Every new flag we add support for does that and is it also common
that a new flag we support is restricted for certain capabilities so
EINVAL is replaced with EPERM.
BTW, man page doesn't say anything about the is_admin case.

IMO EPERM makes perfect sense here and btw, we also return
EPERM from overlayfs if dst is on lower layer.

Mark,

Please be aware that these changes conflict with Miklos' dedupe-cleanup
patches, so I suggest you collaborate on that
https://marc.info/?l=linux-fsdevel&m=152568128031031&w=2

Thanks,
Amir.
Adam Borowski May 13, 2018, 2:30 p.m. UTC | #3
On Fri, May 11, 2018 at 05:06:34PM -0700, Darrick J. Wong wrote:
> On Fri, May 11, 2018 at 12:26:51PM -0700, Mark Fasheh wrote:
> > Right now we return EINVAL if a process does not have permission to dedupe a
> > file. This was an oversight on my part. EPERM gives a true description of
> > the nature of our error, and EINVAL is already used for the case that the
> > filesystem does not support dedupe.

> > -			info->status = -EINVAL;
> > +			info->status = -EPERM;
> 
> Hmm, are we allowed to change this aspect of the kabi after the fact?
> 
> Granted, we're only trading one error code for another, but will the
> existing users of this care?  xfs_io won't and I assume duperemove won't
> either, but what about bees? :)

There's more:
https://codesearch.debian.net/search?q=FILE_EXTENT_SAME

This includes only software that has been packaged for Debian (notably, not
bees), but that gives enough interesting coverage.  And none of these cases
discriminate between error codes -- they merely report them to the user.

Thus, I can't think of a downside of making the error code more accurate.


Meow!
Mark Fasheh May 13, 2018, 6:21 p.m. UTC | #4
On Fri, May 11, 2018 at 05:06:34PM -0700, Darrick J. Wong wrote:
> On Fri, May 11, 2018 at 12:26:51PM -0700, Mark Fasheh wrote:
> > Right now we return EINVAL if a process does not have permission to dedupe a
> > file. This was an oversight on my part. EPERM gives a true description of
> > the nature of our error, and EINVAL is already used for the case that the
> > filesystem does not support dedupe.
> > 
> > Signed-off-by: Mark Fasheh <mfasheh@suse.de>
> > ---
> >  fs/read_write.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/fs/read_write.c b/fs/read_write.c
> > index 77986a2e2a3b..8edef43a182c 100644
> > --- a/fs/read_write.c
> > +++ b/fs/read_write.c
> > @@ -2038,7 +2038,7 @@ int vfs_dedupe_file_range(struct file *file, struct file_dedupe_range *same)
> >  			info->status = -EINVAL;
> >  		} else if (!(is_admin || (dst_file->f_mode & FMODE_WRITE) ||
> >  			     uid_eq(current_fsuid(), dst->i_uid))) {
> > -			info->status = -EINVAL;
> > +			info->status = -EPERM;
> 
> Hmm, are we allowed to change this aspect of the kabi after the fact?
> 
> Granted, we're only trading one error code for another, but will the
> existing users of this care?  xfs_io won't and I assume duperemove won't
> either, but what about bees? :)

Yeah if you see my initial e-mail I check bees and also rust-btrfs. I think
this is fine as we're simply expanding on an error code return. There's no
magic behavior expected with respect to these error codes either.
	--Mark
Darrick J. Wong May 13, 2018, 6:26 p.m. UTC | #5
On Sun, May 13, 2018 at 06:21:52PM +0000, Mark Fasheh wrote:
> On Fri, May 11, 2018 at 05:06:34PM -0700, Darrick J. Wong wrote:
> > On Fri, May 11, 2018 at 12:26:51PM -0700, Mark Fasheh wrote:
> > > Right now we return EINVAL if a process does not have permission to dedupe a
> > > file. This was an oversight on my part. EPERM gives a true description of
> > > the nature of our error, and EINVAL is already used for the case that the
> > > filesystem does not support dedupe.
> > > 
> > > Signed-off-by: Mark Fasheh <mfasheh@suse.de>
> > > ---
> > >  fs/read_write.c | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > 
> > > diff --git a/fs/read_write.c b/fs/read_write.c
> > > index 77986a2e2a3b..8edef43a182c 100644
> > > --- a/fs/read_write.c
> > > +++ b/fs/read_write.c
> > > @@ -2038,7 +2038,7 @@ int vfs_dedupe_file_range(struct file *file, struct file_dedupe_range *same)
> > >  			info->status = -EINVAL;
> > >  		} else if (!(is_admin || (dst_file->f_mode & FMODE_WRITE) ||
> > >  			     uid_eq(current_fsuid(), dst->i_uid))) {
> > > -			info->status = -EINVAL;
> > > +			info->status = -EPERM;
> > 
> > Hmm, are we allowed to change this aspect of the kabi after the fact?
> > 
> > Granted, we're only trading one error code for another, but will the
> > existing users of this care?  xfs_io won't and I assume duperemove won't
> > either, but what about bees? :)
> 
> Yeah if you see my initial e-mail I check bees and also rust-btrfs. I think
> this is fine as we're simply expanding on an error code return. There's no
> magic behavior expected with respect to these error codes either.

Ok.  No objections from me, then.

Acked-by: Darrick J. Wong <darrick.wong@oracle.com>

--D

> 	--Mark
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
David Sterba May 14, 2018, 2:58 p.m. UTC | #6
On Fri, May 11, 2018 at 12:26:51PM -0700, Mark Fasheh wrote:
> Right now we return EINVAL if a process does not have permission to dedupe a
> file. This was an oversight on my part. EPERM gives a true description of
> the nature of our error, and EINVAL is already used for the case that the
> filesystem does not support dedupe.
> 
> Signed-off-by: Mark Fasheh <mfasheh@suse.de>

Acked-by: David Sterba <dsterba@suse.com>

We've been using EINVAL when the request is invalid in the ioctls, eg.
combination of arguments that does not make sense, while EPERM is for
cases when the request is ok but there's still some permission missing,

>  		} else if (!(is_admin || (dst_file->f_mode & FMODE_WRITE) ||
>  			     uid_eq(current_fsuid(), dst->i_uid))) {
> -			info->status = -EINVAL;
> +			info->status = -EPERM;

exactly like this.
Zygo Blaxell May 17, 2018, 5:15 a.m. UTC | #7
On Sun, May 13, 2018 at 11:26:39AM -0700, Darrick J. Wong wrote:
> On Sun, May 13, 2018 at 06:21:52PM +0000, Mark Fasheh wrote:
> > On Fri, May 11, 2018 at 05:06:34PM -0700, Darrick J. Wong wrote:
> > > On Fri, May 11, 2018 at 12:26:51PM -0700, Mark Fasheh wrote:
> > > > Right now we return EINVAL if a process does not have permission to dedupe a
> > > > file. This was an oversight on my part. EPERM gives a true description of
> > > > the nature of our error, and EINVAL is already used for the case that the
> > > > filesystem does not support dedupe.
> > > > 
> > > > Signed-off-by: Mark Fasheh <mfasheh@suse.de>
> > > > ---
> > > >  fs/read_write.c | 2 +-
> > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > > 
> > > > diff --git a/fs/read_write.c b/fs/read_write.c
> > > > index 77986a2e2a3b..8edef43a182c 100644
> > > > --- a/fs/read_write.c
> > > > +++ b/fs/read_write.c
> > > > @@ -2038,7 +2038,7 @@ int vfs_dedupe_file_range(struct file *file, struct file_dedupe_range *same)
> > > >  			info->status = -EINVAL;
> > > >  		} else if (!(is_admin || (dst_file->f_mode & FMODE_WRITE) ||
> > > >  			     uid_eq(current_fsuid(), dst->i_uid))) {
> > > > -			info->status = -EINVAL;
> > > > +			info->status = -EPERM;
> > > 
> > > Hmm, are we allowed to change this aspect of the kabi after the fact?
> > > 
> > > Granted, we're only trading one error code for another, but will the
> > > existing users of this care?  xfs_io won't and I assume duperemove won't
> > > either, but what about bees? :)
> > 
> > Yeah if you see my initial e-mail I check bees and also rust-btrfs. I think
> > this is fine as we're simply expanding on an error code return. There's no
> > magic behavior expected with respect to these error codes either.
> 
> Ok.  No objections from me, then.
> 
> Acked-by: Darrick J. Wong <darrick.wong@oracle.com>

For what it's worth, no objection from me either.  ;)

bees runs only with admin privilege and will never hit the modified line.

If bees is started without admin privilege, the TREE_SEARCH_V2 ioctl
fails.  bees uses this ioctl to walk over all the data in the filesystem,
so without admin privilege, bees never opens, reads, or dedupes anything.

bees relies on having an accurate internal model of btrfs structure and
behavior to issue dedup commands that will work and do useful things;
however, unexpected kernel behavior or concurrent user data changes
will make some dedups fail.  When that happens bees just abandons the
extent immediately:  a user data change will be handled in the next pass
over the filesystem, but an unexpected kernel behavior needs bees code
changes to correctly predict the new kernel behavior before the dedup
can be reattempted.

> --D
> 
> > 	--Mark
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> --
> To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
Mark Fasheh May 17, 2018, 11:03 p.m. UTC | #8
On Thu, May 17, 2018 at 01:15:51AM -0400, Zygo Blaxell wrote:
> On Sun, May 13, 2018 at 11:26:39AM -0700, Darrick J. Wong wrote:
> > On Sun, May 13, 2018 at 06:21:52PM +0000, Mark Fasheh wrote:
> > > On Fri, May 11, 2018 at 05:06:34PM -0700, Darrick J. Wong wrote:
> > > > On Fri, May 11, 2018 at 12:26:51PM -0700, Mark Fasheh wrote:
> > > > > Right now we return EINVAL if a process does not have permission to dedupe a
> > > > > file. This was an oversight on my part. EPERM gives a true description of
> > > > > the nature of our error, and EINVAL is already used for the case that the
> > > > > filesystem does not support dedupe.
> > > > > 
> > > > > Signed-off-by: Mark Fasheh <mfasheh@suse.de>
> > > > > ---
> > > > >  fs/read_write.c | 2 +-
> > > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > > > 
> > > > > diff --git a/fs/read_write.c b/fs/read_write.c
> > > > > index 77986a2e2a3b..8edef43a182c 100644
> > > > > --- a/fs/read_write.c
> > > > > +++ b/fs/read_write.c
> > > > > @@ -2038,7 +2038,7 @@ int vfs_dedupe_file_range(struct file *file, struct file_dedupe_range *same)
> > > > >  			info->status = -EINVAL;
> > > > >  		} else if (!(is_admin || (dst_file->f_mode & FMODE_WRITE) ||
> > > > >  			     uid_eq(current_fsuid(), dst->i_uid))) {
> > > > > -			info->status = -EINVAL;
> > > > > +			info->status = -EPERM;
> > > > 
> > > > Hmm, are we allowed to change this aspect of the kabi after the fact?
> > > > 
> > > > Granted, we're only trading one error code for another, but will the
> > > > existing users of this care?  xfs_io won't and I assume duperemove won't
> > > > either, but what about bees? :)
> > > 
> > > Yeah if you see my initial e-mail I check bees and also rust-btrfs. I think
> > > this is fine as we're simply expanding on an error code return. There's no
> > > magic behavior expected with respect to these error codes either.
> > 
> > Ok.  No objections from me, then.
> > 
> > Acked-by: Darrick J. Wong <darrick.wong@oracle.com>
> 
> For what it's worth, no objection from me either.  ;)
> 
> bees runs only with admin privilege and will never hit the modified line.

Awesome, thanks for the review Zygo.
	--Mark
diff mbox

Patch

diff --git a/fs/read_write.c b/fs/read_write.c
index 77986a2e2a3b..8edef43a182c 100644
--- a/fs/read_write.c
+++ b/fs/read_write.c
@@ -2038,7 +2038,7 @@  int vfs_dedupe_file_range(struct file *file, struct file_dedupe_range *same)
 			info->status = -EINVAL;
 		} else if (!(is_admin || (dst_file->f_mode & FMODE_WRITE) ||
 			     uid_eq(current_fsuid(), dst->i_uid))) {
-			info->status = -EINVAL;
+			info->status = -EPERM;
 		} else if (file->f_path.mnt != dst_file->f_path.mnt) {
 			info->status = -EXDEV;
 		} else if (S_ISDIR(dst->i_mode)) {