Effects of -A option on read/write enabled subvolumes? #86
Comments
Claes1981
changed the title
|
Hi this is because the kernel will only allow dedupe of files open for read if you are the root user, so by default we open for write. On a readonly snapshot though, this can be a problem so -A forces duperemove to always open for read. |
|
There's more detail on the problem in #129, I'm going to close this issue and keep that one open until we have a resolution. Thanks. |
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/[email protected] Signed-off-by: Mark Fasheh <[email protected]> Cc: Al Viro <[email protected]> Cc: Darrick J. Wong <[email protected]> Cc: David Sterba <[email protected]> Signed-off-by: Andrew Morton <[email protected]>
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/[email protected] Signed-off-by: Mark Fasheh <[email protected]> Cc: Al Viro <[email protected]> Cc: Darrick J. Wong <[email protected]> Cc: David Sterba <[email protected]> Signed-off-by: Andrew Morton <[email protected]> Signed-off-by: Stephen Rothwell <[email protected]>
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/[email protected] Signed-off-by: Mark Fasheh <[email protected]> Cc: Al Viro <[email protected]> Cc: Darrick J. Wong <[email protected]> Cc: David Sterba <[email protected]> Signed-off-by: Andrew Morton <[email protected]>
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/[email protected] Signed-off-by: Mark Fasheh <[email protected]> Cc: Al Viro <[email protected]> Cc: Darrick J. Wong <[email protected]> Cc: David Sterba <[email protected]> Signed-off-by: Andrew Morton <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Hi,
I have a question about the "-A" option of Duperemove. According to the manual it "Opens files readonly when deduping. Primarily for use by privileged users on readonly snapshots." It seems like you have to use this option if you want to dedupe readonly snapshots.
Are there any difference in the result or effects on the files of read/write enabled subvolumes when running Duperemove with the "-A" option, versus running Duperemove without the "-A" option? (For example dedupe over both read/write enabled /subvolumeA and readonly /subvolumeA/.snapshots/snapshot1.)
If there are no difference in the results, why are files not always open readonly when deduping? (No need of an "-A" option.) Perhaps is it slower?
The text was updated successfully, but these errors were encountered: