* [PATCH v2 0/2] vfs: better dedupe permission check
@ 2018-05-18 21:57 Mark Fasheh
2018-05-18 21:57 ` [PATCH v2 1/2] vfs: allow dedupe of user owned read-only files Mark Fasheh
2018-05-18 21:57 ` [PATCH v2 2/2] vfs: dedupe should return EPERM if permission is not granted Mark Fasheh
0 siblings, 2 replies; 8+ messages in thread
From: Mark Fasheh @ 2018-05-18 21:57 UTC (permalink / raw)
To: linux-fsdevel; +Cc: linux-btrfs, linux-xfs, Darrick J . Wong, Adam Borowski
Hi,
The following patches fix a couple of issues with the permission
check we do in vfs_dedupe_file_range().
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).
The patches are also available in git:
git pull https://github.com/markfasheh/linux dedupe-perms
Thanks,
--Mark
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
^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH v2 1/2] vfs: allow dedupe of user owned read-only files
2018-05-18 21:57 [PATCH v2 0/2] vfs: better dedupe permission check Mark Fasheh
@ 2018-05-18 21:57 ` Mark Fasheh
2018-05-18 22:03 ` Darrick J. Wong
2018-05-18 21:57 ` [PATCH v2 2/2] vfs: dedupe should return EPERM if permission is not granted Mark Fasheh
1 sibling, 1 reply; 8+ messages in thread
From: Mark Fasheh @ 2018-05-18 21:57 UTC (permalink / raw)
To: linux-fsdevel
Cc: linux-btrfs, linux-xfs, Darrick J . Wong, Adam Borowski, 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>
---
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 c4eabbfc90df..cbea4ce58ad1 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 int allow_file_dedupe(struct file *file)
+{
+ if (capable(CAP_SYS_ADMIN))
+ return 1;
+ if (file->f_mode & FMODE_WRITE)
+ return 1;
+ if (uid_eq(current_fsuid(), file_inode(file)->i_uid))
+ return 1;
+ if (!inode_permission(file_inode(file), MAY_WRITE))
+ return 1;
+ return 0;
+}
+
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
^ permalink raw reply related [flat|nested] 8+ messages in thread
* [PATCH v2 2/2] vfs: dedupe should return EPERM if permission is not granted
2018-05-18 21:57 [PATCH v2 0/2] vfs: better dedupe permission check Mark Fasheh
2018-05-18 21:57 ` [PATCH v2 1/2] vfs: allow dedupe of user owned read-only files Mark Fasheh
@ 2018-05-18 21:57 ` Mark Fasheh
2018-05-18 22:04 ` Darrick J. Wong
2018-05-21 12:35 ` David Sterba
1 sibling, 2 replies; 8+ messages in thread
From: Mark Fasheh @ 2018-05-18 21:57 UTC (permalink / raw)
To: linux-fsdevel
Cc: linux-btrfs, linux-xfs, Darrick J . Wong, Adam Borowski, 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>
---
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 cbea4ce58ad1..2238928ca819 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
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH v2 1/2] vfs: allow dedupe of user owned read-only files
2018-05-18 21:57 ` [PATCH v2 1/2] vfs: allow dedupe of user owned read-only files Mark Fasheh
@ 2018-05-18 22:03 ` Darrick J. Wong
2018-05-18 22:06 ` Mark Fasheh
0 siblings, 1 reply; 8+ messages in thread
From: Darrick J. Wong @ 2018-05-18 22:03 UTC (permalink / raw)
To: Mark Fasheh; +Cc: linux-fsdevel, linux-btrfs, linux-xfs, Adam Borowski
On Fri, May 18, 2018 at 02:57:26PM -0700, Mark Fasheh wrote:
> 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>
> ---
> 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 c4eabbfc90df..cbea4ce58ad1 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 int allow_file_dedupe(struct file *file)
Shouldn't this return bool? It's a predicate, after all...
--D
> +{
> + if (capable(CAP_SYS_ADMIN))
> + return 1;
> + if (file->f_mode & FMODE_WRITE)
> + return 1;
> + if (uid_eq(current_fsuid(), file_inode(file)->i_uid))
> + return 1;
> + if (!inode_permission(file_inode(file), MAY_WRITE))
> + return 1;
> + return 0;
> +}
> +
> 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
>
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v2 2/2] vfs: dedupe should return EPERM if permission is not granted
2018-05-18 21:57 ` [PATCH v2 2/2] vfs: dedupe should return EPERM if permission is not granted Mark Fasheh
@ 2018-05-18 22:04 ` Darrick J. Wong
2018-05-18 22:08 ` Mark Fasheh
2018-05-21 12:35 ` David Sterba
1 sibling, 1 reply; 8+ messages in thread
From: Darrick J. Wong @ 2018-05-18 22:04 UTC (permalink / raw)
To: Mark Fasheh; +Cc: linux-fsdevel, linux-btrfs, linux-xfs, Adam Borowski
On Fri, May 18, 2018 at 02:57:27PM -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>
Looks ok what with all the okays after I squawked last time,
Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>
--D
> ---
> 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 cbea4ce58ad1..2238928ca819 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
>
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v2 1/2] vfs: allow dedupe of user owned read-only files
2018-05-18 22:03 ` Darrick J. Wong
@ 2018-05-18 22:06 ` Mark Fasheh
0 siblings, 0 replies; 8+ messages in thread
From: Mark Fasheh @ 2018-05-18 22:06 UTC (permalink / raw)
To: Darrick J. Wong; +Cc: linux-fsdevel, linux-btrfs, linux-xfs, Adam Borowski
On Fri, May 18, 2018 at 03:03:38PM -0700, Darrick J. Wong wrote:
> > +/* Check whether we are allowed to dedupe the destination file */
> > +static int allow_file_dedupe(struct file *file)
>
> Shouldn't this return bool? It's a predicate, after all...
Yeah that should be bool. Thanks for pointing it out, I'll fix it up in the
next round.
--Mark
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v2 2/2] vfs: dedupe should return EPERM if permission is not granted
2018-05-18 22:04 ` Darrick J. Wong
@ 2018-05-18 22:08 ` Mark Fasheh
0 siblings, 0 replies; 8+ messages in thread
From: Mark Fasheh @ 2018-05-18 22:08 UTC (permalink / raw)
To: Darrick J. Wong; +Cc: linux-fsdevel, linux-btrfs, linux-xfs, Adam Borowski
On Fri, May 18, 2018 at 03:04:13PM -0700, Darrick J. Wong wrote:
> On Fri, May 18, 2018 at 02:57:27PM -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>
>
> Looks ok what with all the okays after I squawked last time,
> Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>
Awesome, I'll put that in the patch. Thanks Darrick.
--Mark
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v2 2/2] vfs: dedupe should return EPERM if permission is not granted
2018-05-18 21:57 ` [PATCH v2 2/2] vfs: dedupe should return EPERM if permission is not granted Mark Fasheh
2018-05-18 22:04 ` Darrick J. Wong
@ 2018-05-21 12:35 ` David Sterba
1 sibling, 0 replies; 8+ messages in thread
From: David Sterba @ 2018-05-21 12:35 UTC (permalink / raw)
To: Mark Fasheh
Cc: linux-fsdevel, linux-btrfs, linux-xfs, Darrick J . Wong, Adam Borowski
On Fri, May 18, 2018 at 02:57:27PM -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>
I gave my ack for v1, no change so
Acked-by: David Sterba <dsterba@suse.com>
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2018-05-21 12:38 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-05-18 21:57 [PATCH v2 0/2] vfs: better dedupe permission check Mark Fasheh
2018-05-18 21:57 ` [PATCH v2 1/2] vfs: allow dedupe of user owned read-only files Mark Fasheh
2018-05-18 22:03 ` Darrick J. Wong
2018-05-18 22:06 ` Mark Fasheh
2018-05-18 21:57 ` [PATCH v2 2/2] vfs: dedupe should return EPERM if permission is not granted Mark Fasheh
2018-05-18 22:04 ` Darrick J. Wong
2018-05-18 22:08 ` Mark Fasheh
2018-05-21 12:35 ` David Sterba
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).