[RESEND][PATCH v4 0/2] vfs: better dedupe permission check

[RESEND][PATCH v4 0/2] vfs: better dedupe permission check

[Mark Fasheh]

Hi Al,

The following patches fix a couple of issues with the permission check
we do in vfs_dedupe_file_range(). I sent them out for a few times now,
a changelog is attached. If they look ok to you, I'd appreciate them
being pushed upstream.

You can get them from git if you like:

git pull https://github.com/markfasheh/linux dedupe-perms

I also have a set of patches against 4.17 if you prefer. The code and
testing are identical:

git pull https://github.com/markfasheh/linux dedupe-perms-v4.17


The first patch expands our check to allow dedupe of a file if the
user owns it or otherwise would be allowed to write to it.

Current behavior is that we'll allow dedupe only if:

- the user is an admin (root)
- the user has the file open for write

This makes it impossible for a user to dedupe their own file set
unless they do it as root, or ensure that all files have write
permission. There's a couple of duperemove bugs open for this:

https://github.com/markfasheh/duperemove/issues/129
https://github.com/markfasheh/duperemove/issues/86

The other problem we have is also related to forcing the user to open
target files for write - A process trying to exec a file currently
being deduped gets ETXTBUSY. The answer (as above) is to allow them to
open the targets ro - root can already do this. There was a patch from
Adam Borowski to fix this back in 2016:

https://lkml.org/lkml/2016/7/17/130

which I have incorporated into my changes.


The 2nd patch fixes our return code for permission denied to be
EPERM. For some reason we're returning EINVAL - I think that's
probably my fault. At any rate, we need to be returning something
descriptive of the actual problem, otherwise callers see EINVAL and
can't really make a valid determination of what's gone wrong.

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

Please apply.

Thanks,
  --Mark

Changes from V3 to V4:
- Add a patch (below) to ioctl_fideduperange.2 explaining our
  changes. I will send this patch once the kernel update is
  accepted. Thanks to Darrick Wong for this suggestion.
- V3 discussion: https://www.spinics.net/lists/linux-btrfs/msg79135.html

Changes from V2 to V3:
- Return bool from allow_file_dedupe
- V2 discussion: https://www.spinics.net/lists/linux-btrfs/msg78421.html

Changes from V1 to V2:
- Add inode_permission check as suggested by Adam Borowski
- V1 discussion: https://marc.info/?l=linux-xfs&m=152606684017965&w=2


From: Mark Fasheh <mfasheh@suse.de>

[PATCH] ioctl_fideduperange.2: clarify permission requirements

dedupe permission checks were recently relaxed - update our man page to
reflect those changes.

Signed-off-by: Mark Fasheh <mfasheh@suse.de>
---
 man2/ioctl_fideduperange.2 | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/man2/ioctl_fideduperange.2 b/man2/ioctl_fideduperange.2
index 84d20a276..7dea0323d 100644
--- a/man2/ioctl_fideduperange.2
+++ b/man2/ioctl_fideduperange.2
@@ -105,9 +105,11 @@ The field
 must be zero.
 During the call,
 .IR src_fd
-must be open for reading and
+must be open for reading.
 .IR dest_fd
-must be open for writing.
+can be open for writing, or reading. If
+.IR dest_fd
+is open for reading, the user should be have write access to the file.
 The combined size of the struct
 .IR file_dedupe_range
 and the struct
@@ -185,8 +187,8 @@ This can appear if the filesystem does not support deduplicating either file
 descriptor, or if either file descriptor refers to special inodes.
 .TP
 .B EPERM
+This will be returned if the user lacks permission to dedupe the file referenced by
 .IR dest_fd
-is immutable.
 .TP
 .B ETXTBSY
 One of the files is a swap file.
-- 
2.15.1

[PATCH v4 1/2] vfs: allow dedupe of user owned read-only files

[Mark Fasheh]

The permission check in vfs_dedupe_file_range() is too coarse - We
only allow dedupe of the destination file if the user is root, or
they have the file open for write.

This effectively limits a non-root user from deduping their own read-only
files. In addition, the write file descriptor that the user is forced to
hold open can prevent execution of files. As file data during a dedupe
does not change, the behavior is unexpected and this has caused a number of
issue reports. For an example, see:

https://github.com/markfasheh/duperemove/issues/129

So change the check so we allow dedupe on the target if:

- the root or admin is asking for it
- the process has write access
- the owner of the file is asking for the dedupe
- the process could get write access

That way users can open read-only and still get dedupe.

Signed-off-by: Mark Fasheh <mfasheh@suse.de>
Acked-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 fs/read_write.c | 17 +++++++++++++++--
 1 file changed, 15 insertions(+), 2 deletions(-)

diff --git a/fs/read_write.c b/fs/read_write.c
index e83bd9744b5d..71e9077f8bc1 100644
--- a/fs/read_write.c
+++ b/fs/read_write.c
@@ -1964,6 +1964,20 @@ int vfs_dedupe_file_range_compare(struct inode *src, loff_t srcoff,
 }
 EXPORT_SYMBOL(vfs_dedupe_file_range_compare);
 
+/* Check whether we are allowed to dedupe the destination file */
+static bool allow_file_dedupe(struct file *file)
+{
+	if (capable(CAP_SYS_ADMIN))
+		return true;
+	if (file->f_mode & FMODE_WRITE)
+		return true;
+	if (uid_eq(current_fsuid(), file_inode(file)->i_uid))
+		return true;
+	if (!inode_permission(file_inode(file), MAY_WRITE))
+		return true;
+	return false;
+}
+
 int vfs_dedupe_file_range(struct file *file, struct file_dedupe_range *same)
 {
 	struct file_dedupe_range_info *info;
@@ -1972,7 +1986,6 @@ int vfs_dedupe_file_range(struct file *file, struct file_dedupe_range *same)
 	u64 len;
 	int i;
 	int ret;
-	bool is_admin = capable(CAP_SYS_ADMIN);
 	u16 count = same->dest_count;
 	struct file *dst_file;
 	loff_t dst_off;
@@ -2036,7 +2049,7 @@ int vfs_dedupe_file_range(struct file *file, struct file_dedupe_range *same)
 
 		if (info->reserved) {
 			info->status = -EINVAL;
-		} else if (!(is_admin || (dst_file->f_mode & FMODE_WRITE))) {
+		} else if (!allow_file_dedupe(dst_file)) {
 			info->status = -EINVAL;
 		} else if (file->f_path.mnt != dst_file->f_path.mnt) {
 			info->status = -EXDEV;
-- 
2.15.1

[PATCH v4 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 <mfasheh@suse.de>
Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>
Acked-by: David Sterba <dsterba@suse.com>
---
 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 71e9077f8bc1..7188982e2733 100644
--- a/fs/read_write.c
+++ b/fs/read_write.c
@@ -2050,7 +2050,7 @@ int vfs_dedupe_file_range(struct file *file, struct file_dedupe_range *same)
 		if (info->reserved) {
 			info->status = -EINVAL;
 		} else if (!allow_file_dedupe(dst_file)) {
-			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

Re: [RESEND][PATCH v4 0/2] vfs: better dedupe permission check

[Darrick J. Wong]

On Tue, Jul 17, 2018 at 12:09:04PM -0700, Mark Fasheh wrote:
> Hi Al,
> 
> The following patches fix a couple of issues with the permission check
> we do in vfs_dedupe_file_range(). I sent them out for a few times now,
> a changelog is attached. If they look ok to you, I'd appreciate them
> being pushed upstream.
> 
> You can get them from git if you like:
> 
> git pull https://github.com/markfasheh/linux dedupe-perms
> 
> I also have a set of patches against 4.17 if you prefer. The code and
> testing are identical:
> 
> git pull https://github.com/markfasheh/linux dedupe-perms-v4.17
> 
> 
> The first patch expands our check to allow dedupe of a file if the
> user owns it or otherwise would be allowed to write to it.
> 
> Current behavior is that we'll allow dedupe only if:
> 
> - the user is an admin (root)
> - the user has the file open for write
> 
> This makes it impossible for a user to dedupe their own file set
> unless they do it as root, or ensure that all files have write
> permission. There's a couple of duperemove bugs open for this:
> 
> https://github.com/markfasheh/duperemove/issues/129
> https://github.com/markfasheh/duperemove/issues/86
> 
> The other problem we have is also related to forcing the user to open
> target files for write - A process trying to exec a file currently
> being deduped gets ETXTBUSY. The answer (as above) is to allow them to
> open the targets ro - root can already do this. There was a patch from
> Adam Borowski to fix this back in 2016:
> 
> https://lkml.org/lkml/2016/7/17/130
> 
> which I have incorporated into my changes.
> 
> 
> The 2nd patch fixes our return code for permission denied to be
> EPERM. For some reason we're returning EINVAL - I think that's
> probably my fault. At any rate, we need to be returning something
> descriptive of the actual problem, otherwise callers see EINVAL and
> can't really make a valid determination of what's gone wrong.
> 
> 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).
> 
> Please apply.
> 
> Thanks,
>   --Mark
> 
> Changes from V3 to V4:
> - Add a patch (below) to ioctl_fideduperange.2 explaining our
>   changes. I will send this patch once the kernel update is
>   accepted. Thanks to Darrick Wong for this suggestion.
> - V3 discussion: https://www.spinics.net/lists/linux-btrfs/msg79135.html
> 
> Changes from V2 to V3:
> - Return bool from allow_file_dedupe
> - V2 discussion: https://www.spinics.net/lists/linux-btrfs/msg78421.html
> 
> Changes from V1 to V2:
> - Add inode_permission check as suggested by Adam Borowski
> - V1 discussion: https://marc.info/?l=linux-xfs&m=152606684017965&w=2
> 
> 
> From: Mark Fasheh <mfasheh@suse.de>
> 
> [PATCH] ioctl_fideduperange.2: clarify permission requirements
> 
> dedupe permission checks were recently relaxed - update our man page to
> reflect those changes.
> 
> Signed-off-by: Mark Fasheh <mfasheh@suse.de>
> ---
>  man2/ioctl_fideduperange.2 | 8 +++++---

Mmm, man page update, thank you for editing the documentation too!

Please cc linux-api and Michael Kerrisk so this can go upstream.

>  1 file changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/man2/ioctl_fideduperange.2 b/man2/ioctl_fideduperange.2
> index 84d20a276..7dea0323d 100644
> --- a/man2/ioctl_fideduperange.2
> +++ b/man2/ioctl_fideduperange.2
> @@ -105,9 +105,11 @@ The field
>  must be zero.
>  During the call,
>  .IR src_fd
> -must be open for reading and
> +must be open for reading.
>  .IR dest_fd
> -must be open for writing.
> +can be open for writing, or reading. If

Manpages usually start each new sentence on its own line (though I defer
to mkerrisk on that).

> +.IR dest_fd
> +is open for reading, the user should be have write access to the file.

"...the user must have write access..."

>  The combined size of the struct
>  .IR file_dedupe_range
>  and the struct
> @@ -185,8 +187,8 @@ This can appear if the filesystem does not support deduplicating either file
>  descriptor, or if either file descriptor refers to special inodes.
>  .TP
>  .B EPERM
> +This will be returned if the user lacks permission to dedupe the file referenced by
>  .IR dest_fd
> -is immutable.

(Did the period fall off the end of the sentence here?  I am bad at
reading manpage markup...)

--D

>  .TP
>  .B ETXTBSY
>  One of the files is a swap file.
> -- 
> 2.15.1
> 
> --
> 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

Re: [RESEND][PATCH v4 0/2] vfs: better dedupe permission check

[Mark Fasheh]

On Tue, Jul 17, 2018 at 12:48:18PM -0700, Darrick J. Wong wrote:
> On Tue, Jul 17, 2018 at 12:09:04PM -0700, Mark Fasheh wrote:
> > From: Mark Fasheh <mfasheh@suse.de>
> > 
> > [PATCH] ioctl_fideduperange.2: clarify permission requirements
> > 
> > dedupe permission checks were recently relaxed - update our man page to
> > reflect those changes.
> > 
> > Signed-off-by: Mark Fasheh <mfasheh@suse.de>
> > ---
> >  man2/ioctl_fideduperange.2 | 8 +++++---
> 
> Mmm, man page update, thank you for editing the documentation too!

No problem, thanks for suggesting I do it.


> Please cc linux-api and Michael Kerrisk so this can go upstream.

Will do. The changes you point out below all look good to me so I'll
incorporate them and send an updated version to the appropriate folks.

Thanks
	--Mark

Re: [RESEND][PATCH v4 0/2] vfs: better dedupe permission check

[Mark Fasheh]

CCing Michael Kerrisk and linux-api 

The patch at the end of this e-mail updates our man page for
ioctl_fideduperange to reflect the changes proposed in this patch series:

https://marc.info/?l=linux-fsdevel&m=153185457324037&w=2

Any feedback would be appreciated. Thanks in advance.
	--Mark

On Tue, Jul 17, 2018 at 12:48:18PM -0700, Darrick J. Wong wrote:
> On Tue, Jul 17, 2018 at 12:09:04PM -0700, Mark Fasheh wrote:
> > Hi Al,
> > 
> > The following patches fix a couple of issues with the permission check
> > we do in vfs_dedupe_file_range(). I sent them out for a few times now,
> > a changelog is attached. If they look ok to you, I'd appreciate them
> > being pushed upstream.
> > 
> > You can get them from git if you like:
> > 
> > git pull https://github.com/markfasheh/linux dedupe-perms
> > 
> > I also have a set of patches against 4.17 if you prefer. The code and
> > testing are identical:
> > 
> > git pull https://github.com/markfasheh/linux dedupe-perms-v4.17
> > 
> > 
> > The first patch expands our check to allow dedupe of a file if the
> > user owns it or otherwise would be allowed to write to it.
> > 
> > Current behavior is that we'll allow dedupe only if:
> > 
> > - the user is an admin (root)
> > - the user has the file open for write
> > 
> > This makes it impossible for a user to dedupe their own file set
> > unless they do it as root, or ensure that all files have write
> > permission. There's a couple of duperemove bugs open for this:
> > 
> > https://github.com/markfasheh/duperemove/issues/129
> > https://github.com/markfasheh/duperemove/issues/86
> > 
> > The other problem we have is also related to forcing the user to open
> > target files for write - A process trying to exec a file currently
> > being deduped gets ETXTBUSY. The answer (as above) is to allow them to
> > open the targets ro - root can already do this. There was a patch from
> > Adam Borowski to fix this back in 2016:
> > 
> > https://lkml.org/lkml/2016/7/17/130
> > 
> > which I have incorporated into my changes.
> > 
> > 
> > The 2nd patch fixes our return code for permission denied to be
> > EPERM. For some reason we're returning EINVAL - I think that's
> > probably my fault. At any rate, we need to be returning something
> > descriptive of the actual problem, otherwise callers see EINVAL and
> > can't really make a valid determination of what's gone wrong.
> > 
> > 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).
> > 
> > Please apply.
> > 
> > Thanks,
> >   --Mark
> > 
> > Changes from V3 to V4:
> > - Add a patch (below) to ioctl_fideduperange.2 explaining our
> >   changes. I will send this patch once the kernel update is
> >   accepted. Thanks to Darrick Wong for this suggestion.
> > - V3 discussion: https://www.spinics.net/lists/linux-btrfs/msg79135.html
> > 
> > Changes from V2 to V3:
> > - Return bool from allow_file_dedupe
> > - V2 discussion: https://www.spinics.net/lists/linux-btrfs/msg78421.html
> > 
> > Changes from V1 to V2:
> > - Add inode_permission check as suggested by Adam Borowski
> > - V1 discussion: https://marc.info/?l=linux-xfs&m=152606684017965&w=2
> > 
> > 
> > From: Mark Fasheh <mfasheh@suse.de>
> > 
> > [PATCH] ioctl_fideduperange.2: clarify permission requirements
> > 
> > dedupe permission checks were recently relaxed - update our man page to
> > reflect those changes.
> > 
> > Signed-off-by: Mark Fasheh <mfasheh@suse.de>
> > ---
> >  man2/ioctl_fideduperange.2 | 8 +++++---
> 
> Mmm, man page update, thank you for editing the documentation too!
> 
> Please cc linux-api and Michael Kerrisk so this can go upstream.


From: Mark Fasheh <mfasheh@suse.de>

[PATCH] ioctl_fideduperange.2: clarify permission requirements

dedupe permission checks were recently relaxed - update our man page to
reflect those changes.

Signed-off-by: Mark Fasheh <mfasheh@suse.de>
---
 man2/ioctl_fideduperange.2 | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/man2/ioctl_fideduperange.2 b/man2/ioctl_fideduperange.2
index 84d20a276..4040ee064 100644
--- a/man2/ioctl_fideduperange.2
+++ b/man2/ioctl_fideduperange.2
@@ -105,9 +105,12 @@ The field
 must be zero.
 During the call,
 .IR src_fd
-must be open for reading and
+must be open for reading.
 .IR dest_fd
-must be open for writing.
+can be open for writing, or reading.
+If
+.IR dest_fd
+is open for reading, the user must have write access to the file.
 The combined size of the struct
 .IR file_dedupe_range
 and the struct
@@ -185,8 +188,8 @@ This can appear if the filesystem does not support deduplicating either file
 descriptor, or if either file descriptor refers to special inodes.
 .TP
 .B EPERM
-.IR dest_fd
-is immutable.
+This will be returned if the user lacks permission to dedupe the file referenced by
+.IR dest_fd .
 .TP
 .B ETXTBSY
 One of the files is a swap file.
-- 
2.15.1