From: Mark Fasheh <mfasheh@suse.de>
To: Andrew Morton <akpm@linux-foundation.org>
Cc: Al Viro <viro@ZenIV.linux.org.uk>,
linux-fsdevel@vger.kernel.org, linux-api@vger.kernel.org,
Michael Kerrisk <mtk.manpages@gmail.com>,
linux-btrfs@vger.kernel.org, linux-xfs@vger.kernel.org,
"Darrick J . Wong" <darrick.wong@oracle.com>,
Adam Borowski <kilobyte@angband.pl>,
David Sterba <dsterba@suse.cz>, Mark Fasheh <mfasheh@suse.de>
Subject: [PATCH v6 0/2] vfs: fix dedupe permission check
Date: Mon, 10 Sep 2018 16:21:16 -0700 [thread overview]
Message-ID: <20180910232118.14424-1-mfasheh@suse.de> (raw)
Hi Andrew/Al,
Could I please have these patches put in a tree for more public testing?
They've hit fsdevel a few times now, I have links to the discussions in the
change log below.
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).
One way I tested these patches was to make non-root owned files with
read-only permissions and see if I could dedupe them as the owning user. For
example, the following script fails on an unpatched kernel and succeeds with
the patches applied.
TESTDIR=/btrfs
USER=mfasheh
rm -f $TESTDIR/file*
dd if=/dev/zero of=$TESTDIR/file1 count=1024 bs=1024
dd if=/dev/zero of=$TESTDIR/file2 count=1024 bs=1024
chown $USER $TESTDIR/file*
chmod 444 $TESTDIR/file*
# open file* for read and dedupe
sudo -u $USER duperemove -Ad $TESTDIR/file*
Lastly, I have an update to the fi_deduperange manpage to reflect these
changes. That patch is attached below.
git pull https://github.com/markfasheh/linux dedupe-perms
Thanks,
--Mark
Changes from V5 to V6:
- Rebase and retest on 4.19-rc3
- Add a note on testing
Changes from V4 to V5:
- Rebase and retest on 4.18-rc8
- Place updated manpage patch below, CC linux-api
- V4 discussion: https://patchwork.kernel.org/patch/10530365/
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 | 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
next reply other threads:[~2018-09-11 4:17 UTC|newest]
Thread overview: 3+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-09-10 23:21 Mark Fasheh [this message]
2018-09-10 23:21 ` [PATCH 1/2] vfs: allow dedupe of user owned read-only files Mark Fasheh
2018-09-10 23:21 ` [PATCH 2/2] vfs: dedupe should return EPERM if permission is not granted Mark Fasheh
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20180910232118.14424-1-mfasheh@suse.de \
--to=mfasheh@suse.de \
--cc=akpm@linux-foundation.org \
--cc=darrick.wong@oracle.com \
--cc=dsterba@suse.cz \
--cc=kilobyte@angband.pl \
--cc=linux-api@vger.kernel.org \
--cc=linux-btrfs@vger.kernel.org \
--cc=linux-fsdevel@vger.kernel.org \
--cc=linux-xfs@vger.kernel.org \
--cc=mtk.manpages@gmail.com \
--cc=viro@ZenIV.linux.org.uk \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.