Join GitHub today
GitHub is home to over 50 million developers working together to host and review code, manage projects, and build software together.
Sign upcan't dedupe read only files #129
Open
Comments
|
Hi Matthias, did you try the '-A' option? That should force duperemove to open readonly. I agree that it might not make sense to open read-write. I mean, it did to me at some point obviously but maybe it's a decision to revisit :) |
|
Hm, it seems that adding -A causes additional "had status (0) "[unknown status]"." messages in some cases. |
|
Do you remember what stage that happened at? Sounds like dedupe stage. |
|
Never mind, that's in process_dedupe_results() as I can see now. |
|
Sorry to flood the bug with messages but I'm looking at the code and that you see this message is kind of weird to me. The variable being printed is 'target_status' and we check against it being 0 immediately before the print: https://github.com/markfasheh/duperemove/blob/master/run_dedupe.c#L110 Did you compile from source? Could you check that line in your version of the code? |
|
Interestingly enough, I hit this just now testing some dedupe code on xfs: [0x1ba98a0] Dedupe for file "/xfs/boot/System.map-3.12.53" had status (0) "[unknown status]". So never mind that last comment obviously this happens. |
|
Ok that ugly print was fixed in master branch, thanks for reporting. It turned out to be a missing '='! |
|
Hmm, I still have problems when for example deduping packfiles of git repos. |
|
Huh, doing the same thing on my end (from master): Using 128K blocks So I wonder what the difference between our setups is. This was on kernel v4.5.0. What's 'uname -r' give you on the machine you tested on? What is the size of '/home/matthias/tmp/repotest/duperemove1/.git/objects/pack/pack-863c44a704cd9a490ea880a8711474915248905c.pack' ? |
|
du -sb : size, md5, sha1 and sh224 sums of both pack seem to match. kernel is: 4.6.4-301.fc24.x86_64 EDIT: |
|
testcase: * copy some sufficiently large file to test directory*
cat file1 > file2 # make a non-COW copy
chmod 400 file1 # make files readonly
chmod 400 file2
duperemove -dA file1 file2=> " had status (-22) "Invalid argument"." |
|
Thanks for the testcase btw. |
|
Unless there's a real reason to open in read/write mode, I say it's a useless security risk and maybe even a resource abuse. why is this not readonly everywhere? |
|
Yeah let me look over all the relevant code and patches... I'm reasonably sure that the kernel is doing the check of read/write mode in which case there won't be much to do in duperemove until we get that changed. |
|
Ok, there's a detailed description of the problem in the wiki under Kernel Tasks in the Development Tasks wiki page. There's one potential solution there too. I'll paste it here to save you all the time:
|
|
In #86 , @markfasheh said:
If the user is |
fengguang
pushed a commit
to 0day-ci/linux
that referenced
this issue
Jun 8, 2018
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: markfasheh/duperemove#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>
markfasheh
pushed a commit
to markfasheh/linux
that referenced
this issue
Jul 17, 2018
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: markfasheh/duperemove#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>
ColinIanKing
pushed a commit
to ColinIanKing/linux-next-mirror
that referenced
this issue
Sep 13, 2018
Patch series "vfs: fix dedupe permission check", v6. 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: markfasheh/duperemove#129 markfasheh/duperemove#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. This patch (of 2): The permission check in vfs_dedupe_file_range_one() 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: markfasheh/duperemove#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. Link: http://lkml.kernel.org/r/20180910232118.14424-2-mfasheh@suse.de Signed-off-by: Mark Fasheh <mfasheh@suse.de> Cc: Al Viro <viro@zeniv.linux.org.uk> Cc: Darrick J. Wong <darrick.wong@oracle.com> Cc: David Sterba <dsterba@suse.com> Signed-off-by: Andrew Morton <akpm@linux-foundation.org> Signed-off-by: Stephen Rothwell <sfr@canb.auug.org.au>
fengguang
pushed a commit
to 0day-ci/linux
that referenced
this issue
Sep 19, 2018
Patch series "vfs: fix dedupe permission check", v6. 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: markfasheh/duperemove#129 markfasheh/duperemove#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. This patch (of 2): The permission check in vfs_dedupe_file_range_one() 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: markfasheh/duperemove#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. Link: http://lkml.kernel.org/r/20180910232118.14424-2-mfasheh@suse.de Signed-off-by: Mark Fasheh <mfasheh@suse.de> Cc: Al Viro <viro@zeniv.linux.org.uk> Cc: Darrick J. Wong <darrick.wong@oracle.com> Cc: David Sterba <dsterba@suse.com> Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
fengguang
pushed a commit
to 0day-ci/linux
that referenced
this issue
Sep 21, 2018
Patch series "vfs: fix dedupe permission check", v6. 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: markfasheh/duperemove#129 markfasheh/duperemove#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. This patch (of 2): The permission check in vfs_dedupe_file_range_one() 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: markfasheh/duperemove#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. Link: http://lkml.kernel.org/r/20180910232118.14424-2-mfasheh@suse.de Signed-off-by: Mark Fasheh <mfasheh@suse.de> Cc: Al Viro <viro@zeniv.linux.org.uk> Cc: Darrick J. Wong <darrick.wong@oracle.com> Cc: David Sterba <dsterba@suse.com> Signed-off-by: Andrew Morton <akpm@linux-foundation.org> Signed-off-by: Stephen Rothwell <sfr@canb.auug.org.au>
jic23
pushed a commit
to hisilicon/kernel-dev
that referenced
this issue
Sep 26, 2018
Patch series "vfs: fix dedupe permission check", v6. 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: markfasheh/duperemove#129 markfasheh/duperemove#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. This patch (of 2): The permission check in vfs_dedupe_file_range_one() 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: markfasheh/duperemove#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. Link: http://lkml.kernel.org/r/20180910232118.14424-2-mfasheh@suse.de Signed-off-by: Mark Fasheh <mfasheh@suse.de> Cc: Al Viro <viro@zeniv.linux.org.uk> Cc: Darrick J. Wong <darrick.wong@oracle.com> Cc: David Sterba <dsterba@suse.com> Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
sergey-senozhatsky
pushed a commit
to sergey-senozhatsky/linux-next-ss
that referenced
this issue
Oct 4, 2018
Patch series "vfs: fix dedupe permission check", v6. 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: markfasheh/duperemove#129 markfasheh/duperemove#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. This patch (of 2): The permission check in vfs_dedupe_file_range_one() 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: markfasheh/duperemove#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. Link: http://lkml.kernel.org/r/20180910232118.14424-2-mfasheh@suse.de Signed-off-by: Mark Fasheh <mfasheh@suse.de> Cc: Al Viro <viro@zeniv.linux.org.uk> Cc: Darrick J. Wong <darrick.wong@oracle.com> Cc: David Sterba <dsterba@suse.com> Signed-off-by: Andrew Morton <akpm@linux-foundation.org> Signed-off-by: Stephen Rothwell <sfr@canb.auug.org.au>
osandov
pushed a commit
to osandov/linux
that referenced
this issue
Oct 5, 2018
Patch series "vfs: fix dedupe permission check", v6. 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: markfasheh/duperemove#129 markfasheh/duperemove#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. This patch (of 2): The permission check in vfs_dedupe_file_range_one() 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: markfasheh/duperemove#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. Link: http://lkml.kernel.org/r/20180910232118.14424-2-mfasheh@suse.de Signed-off-by: Mark Fasheh <mfasheh@suse.de> Cc: Al Viro <viro@zeniv.linux.org.uk> Cc: Darrick J. Wong <darrick.wong@oracle.com> Cc: David Sterba <dsterba@suse.com> Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
ColinIanKing
pushed a commit
to ColinIanKing/linux-next-mirror
that referenced
this issue
Oct 8, 2018
Patch series "vfs: fix dedupe permission check", v6. 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: markfasheh/duperemove#129 markfasheh/duperemove#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. This patch (of 2): The permission check in vfs_dedupe_file_range_one() 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: markfasheh/duperemove#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. Link: http://lkml.kernel.org/r/20180910232118.14424-2-mfasheh@suse.de Signed-off-by: Mark Fasheh <mfasheh@suse.de> Cc: Al Viro <viro@zeniv.linux.org.uk> Cc: Darrick J. Wong <darrick.wong@oracle.com> Cc: David Sterba <dsterba@suse.com> Signed-off-by: Andrew Morton <akpm@linux-foundation.org> Signed-off-by: Stephen Rothwell <sfr@canb.auug.org.au>
ColinIanKing
pushed a commit
to ColinIanKing/linux-next-mirror
that referenced
this issue
Oct 15, 2018
Patch series "vfs: fix dedupe permission check", v6. 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: markfasheh/duperemove#129 markfasheh/duperemove#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. This patch (of 2): The permission check in vfs_dedupe_file_range_one() 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: markfasheh/duperemove#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. Link: http://lkml.kernel.org/r/20180910232118.14424-2-mfasheh@suse.de Signed-off-by: Mark Fasheh <mfasheh@suse.de> Cc: Al Viro <viro@zeniv.linux.org.uk> Cc: Darrick J. Wong <darrick.wong@oracle.com> Cc: David Sterba <dsterba@suse.com> Signed-off-by: Andrew Morton <akpm@linux-foundation.org> Signed-off-by: Stephen Rothwell <sfr@canb.auug.org.au>
sergey-senozhatsky
pushed a commit
to sergey-senozhatsky/linux-next-ss
that referenced
this issue
Oct 17, 2018
Patch series "vfs: fix dedupe permission check", v6. 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: markfasheh/duperemove#129 markfasheh/duperemove#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. This patch (of 2): The permission check in vfs_dedupe_file_range_one() 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: markfasheh/duperemove#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. Link: http://lkml.kernel.org/r/20180910232118.14424-2-mfasheh@suse.de Signed-off-by: Mark Fasheh <mfasheh@suse.de> Cc: Al Viro <viro@zeniv.linux.org.uk> Cc: Darrick J. Wong <darrick.wong@oracle.com> Cc: David Sterba <dsterba@suse.com> Signed-off-by: Andrew Morton <akpm@linux-foundation.org> Signed-off-by: Stephen Rothwell <sfr@canb.auug.org.au>
avagin
pushed a commit
to avagin/linux
that referenced
this issue
Oct 18, 2018
The permission check in vfs_dedupe_file_range_one() 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: markfasheh/duperemove#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> Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
ColinIanKing
pushed a commit
to ColinIanKing/linux-next-mirror
that referenced
this issue
Oct 29, 2018
Patch series "vfs: fix dedupe permission check", v6. 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: markfasheh/duperemove#129 markfasheh/duperemove#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. This patch (of 2): The permission check in vfs_dedupe_file_range_one() 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: markfasheh/duperemove#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. Link: http://lkml.kernel.org/r/20180910232118.14424-2-mfasheh@suse.de Signed-off-by: Mark Fasheh <mfasheh@suse.de> Cc: Al Viro <viro@zeniv.linux.org.uk> Cc: Darrick J. Wong <darrick.wong@oracle.com> Cc: David Sterba <dsterba@suse.com> Signed-off-by: Andrew Morton <akpm@linux-foundation.org> Signed-off-by: Stephen Rothwell <sfr@canb.auug.org.au>
sergey-senozhatsky
pushed a commit
to sergey-senozhatsky/linux-next-ss
that referenced
this issue
Oct 31, 2018
Patch series "vfs: fix dedupe permission check", v6. 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: markfasheh/duperemove#129 markfasheh/duperemove#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. This patch (of 2): The permission check in vfs_dedupe_file_range_one() 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: markfasheh/duperemove#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. Link: http://lkml.kernel.org/r/20180910232118.14424-2-mfasheh@suse.de Signed-off-by: Mark Fasheh <mfasheh@suse.de> Cc: Al Viro <viro@zeniv.linux.org.uk> Cc: Darrick J. Wong <darrick.wong@oracle.com> Cc: David Sterba <dsterba@suse.com> Signed-off-by: Andrew Morton <akpm@linux-foundation.org> Signed-off-by: Stephen Rothwell <sfr@canb.auug.org.au>
ColinIanKing
pushed a commit
to ColinIanKing/linux-next-mirror
that referenced
this issue
Nov 1, 2018
Patch series "vfs: fix dedupe permission check", v6. 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: markfasheh/duperemove#129 markfasheh/duperemove#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. This patch (of 2): The permission check in vfs_dedupe_file_range_one() 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: markfasheh/duperemove#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. Link: http://lkml.kernel.org/r/20180910232118.14424-2-mfasheh@suse.de Signed-off-by: Mark Fasheh <mfasheh@suse.de> Cc: Al Viro <viro@zeniv.linux.org.uk> Cc: Darrick J. Wong <darrick.wong@oracle.com> Cc: David Sterba <dsterba@suse.com> Signed-off-by: Andrew Morton <akpm@linux-foundation.org> Signed-off-by: Stephen Rothwell <sfr@canb.auug.org.au>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
It seems that duperemove (executed as normal user) cannnot dedupe read-only files owned by said user.
Error 13: Permission denied while opening "[...]" (write=1)
This is weird because it should not modify the files but just read the underlying extents.