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

[Mark Fasheh]

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 
Reviewed-by: Darrick J. Wong 
Acked-by: David Sterba 
---
 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 be0e8723a049..c734bc2880a5 100644
--- a/fs/read_write.c
+++ b/fs/read_write.c
@@ -1991,7 +1991,7 @@ int vfs_dedupe_file_range_one(struct file *src_file, 
loff_t src_pos,
if (ret < 0)
goto out_drop_write;
 
-   ret = -EINVAL;
+   ret = -EPERM;
if (!allow_file_dedupe(dst_file))
goto out_drop_write;
 
-- 
2.15.1

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

[Mark Fasheh]

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 +, 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 
> > > > > ---
> > > > >  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 
> 
> 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
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Zygo Blaxell]

On Sun, May 13, 2018 at 11:26:39AM -0700, Darrick J. Wong wrote:
> On Sun, May 13, 2018 at 06:21:52PM +, 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 
> > > > ---
> > > >  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 

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 majord...@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 majord...@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html


signature.asc
Description: PGP signature

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

[David Sterba]

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 

Acked-by: David Sterba 

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.
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Darrick J. Wong]

On Sun, May 13, 2018 at 06:21:52PM +, 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 
> > > ---
> > >  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 

--D

>   --Mark
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majord...@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 majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Mark Fasheh]

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 
> > ---
> >  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
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Adam Borowski]

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!
-- 
⢀⣴⠾⠻⢶⣦⠀ 
⣾⠁⢰⠒⠀⣿⡁ 
⢿⡄⠘⠷⠚⠋⠀ Certified airhead; got the CT scan to prove that!
⠈⠳⣄ 
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Duncan]

Darrick J. Wong posted on Fri, 11 May 2018 17:06:34 -0700 as excerpted:

> 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 
>> ---
>>  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? :)

>From the 0/2 cover-letter:

>>> This has also popped up in duperemove, mostly in the form of cryptic
>>> error messages. Because this is a code returned to userspace, I did
>>> check the other users of extent-same that I could find. Both 'bees'
>>> and 'rust-btrfs' do the same as duperemove and simply report the error
>>> (as they should).

> --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
>>

-- 
Duncan - List replies preferred.   No HTML msgs.
"Every nonfree program has a lord, a master --
and if you use the program, he is your master."  Richard Stallman

--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Amir Goldstein]

On Sat, May 12, 2018 at 3:06 AM, 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 
>> ---
>>  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=152568128031031=2

Thanks,
Amir.
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Darrick J. Wong]

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 
> ---
>  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
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Mark Fasheh]

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 
---
 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;
} else if (file->f_path.mnt != dst_file->f_path.mnt) {
info->status = -EXDEV;
} else if (S_ISDIR(dst->i_mode)) {
-- 
2.15.1

--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html