* [PATCH] btrfs-progs: remove workaround for setting capabilities in the receive command
@ 2021-02-09 11:49 fdmanana
2021-02-10 1:38 ` Su Yue
2021-02-11 0:25 ` David Sterba
0 siblings, 2 replies; 3+ messages in thread
From: fdmanana @ 2021-02-09 11:49 UTC (permalink / raw)
To: linux-btrfs
From: Filipe Manana <fdmanana@suse.com>
We had a few bugs on the kernel side of send/receive where capabilities
ended up being lost after receiving a send stream. They all stem from the
fact that the kernel used to send all xattrs before issuing the chown
command, and the later clears any existing capabilities in a file or
directory.
Initially a workaround was added to btrfs-progs' receive command, in commit
123a2a085027e ("btrfs-progs: receive: restore capabilities after chown"),
and that fixed some instances of the problem. More recently, other instances
of the problem were found, a proper fix for the kernel was made, which fixes
the root problem by making send always emit the sexattr command for setting
capabilities after issuing a chown command. This was done in kernel commit
89efda52e6b693 ("btrfs: send: emit file capabilities after chown"), which
landed in kernel 5.8.
However, the workaround on the receive command now causes us to incorrectly
set a capability on a file that should not have it, because it assumes all
setxattr commands for a file always comes before a chown.
Example reproducer:
$ cat send-caps.sh
#!/bin/bash
DEV1=/dev/sdh
DEV2=/dev/sdi
MNT1=/mnt/sdh
MNT2=/mnt/sdi
mkfs.btrfs -f $DEV1 > /dev/null
mkfs.btrfs -f $DEV2 > /dev/null
mount $DEV1 $MNT1
mount $DEV2 $MNT2
touch $MNT1/foo
touch $MNT1/bar
setcap cap_net_raw=p $MNT1/foo
btrfs subvolume snapshot -r $MNT1 $MNT1/snap1
btrfs send $MNT1/snap1 | btrfs receive $MNT2
echo
echo "capabilities on destination filesystem:"
echo
getcap $MNT2/snap1/foo
getcap $MNT2/snap1/bar
umount $MNT1
umount $MNT2
When running the test script, we can see that both files foo and bar get
the capability set, when only file foo should have it:
$ ./send-caps.sh
Create a readonly snapshot of '/mnt/sdh' in '/mnt/sdh/snap1'
At subvol /mnt/sdh/snap1
At subvol snap1
capabilities on destination filesystem:
/mnt/sdi/snap1/foo cap_net_raw=p
/mnt/sdi/snap1/bar cap_net_raw=p
Since the kernel fix was backported to all currently supported stable
releases (5.10.x, 5.4.x, 4.19.x, 4.14.x, 4.9.x and 4.4.x), remove the
workaround from receive. Having such a workaround relying on the order
of commands in a send stream is always troublesome and doomed to break
one day.
A test case for fstests will come soon.
Reported-by: Richard Brown <rbrown@suse.de>
Signed-off-by: Filipe Manana <fdmanana@suse.com>
---
cmds/receive.c | 49 -------------------------------------------------
1 file changed, 49 deletions(-)
diff --git a/cmds/receive.c b/cmds/receive.c
index 2aaba3ff..b4099bc4 100644
--- a/cmds/receive.c
+++ b/cmds/receive.c
@@ -77,14 +77,6 @@ struct btrfs_receive
struct subvol_uuid_search sus;
int honor_end_cmd;
-
- /*
- * Buffer to store capabilities from security.capabilities xattr,
- * usually 20 bytes, but make same room for potentially larger
- * encodings. Must be set only once per file, denoted by length > 0.
- */
- char cached_capabilities[64];
- int cached_capabilities_len;
};
static int finish_subvol(struct btrfs_receive *rctx)
@@ -825,22 +817,6 @@ static int process_set_xattr(const char *path, const char *name,
goto out;
}
- if (strcmp("security.capability", name) == 0) {
- if (bconf.verbose >= 4)
- fprintf(stderr, "set_xattr: cache capabilities\n");
- if (rctx->cached_capabilities_len)
- warning("capabilities set multiple times per file: %s",
- full_path);
- if (len > sizeof(rctx->cached_capabilities)) {
- error("capabilities encoded to %d bytes, buffer too small",
- len);
- ret = -E2BIG;
- goto out;
- }
- rctx->cached_capabilities_len = len;
- memcpy(rctx->cached_capabilities, data, len);
- }
-
if (bconf.verbose >= 3) {
fprintf(stderr, "set_xattr %s - name=%s data_len=%d "
"data=%.*s\n", path, name, len,
@@ -961,23 +937,6 @@ static int process_chown(const char *path, u64 uid, u64 gid, void *user)
error("chown %s failed: %m", path);
goto out;
}
-
- if (rctx->cached_capabilities_len) {
- if (bconf.verbose >= 3)
- fprintf(stderr, "chown: restore capabilities\n");
- ret = lsetxattr(full_path, "security.capability",
- rctx->cached_capabilities,
- rctx->cached_capabilities_len, 0);
- memset(rctx->cached_capabilities, 0,
- sizeof(rctx->cached_capabilities));
- rctx->cached_capabilities_len = 0;
- if (ret < 0) {
- ret = -errno;
- error("restoring capabilities %s: %m", path);
- goto out;
- }
- }
-
out:
return ret;
}
@@ -1155,14 +1114,6 @@ static int do_receive(struct btrfs_receive *rctx, const char *tomnt,
goto out;
while (!end) {
- if (rctx->cached_capabilities_len) {
- if (bconf.verbose >= 4)
- fprintf(stderr, "clear cached capabilities\n");
- memset(rctx->cached_capabilities, 0,
- sizeof(rctx->cached_capabilities));
- rctx->cached_capabilities_len = 0;
- }
-
ret = btrfs_read_and_process_send_stream(r_fd, &send_ops,
rctx,
rctx->honor_end_cmd,
--
2.28.0
^ permalink raw reply related [flat|nested] 3+ messages in thread* Re: [PATCH] btrfs-progs: remove workaround for setting capabilities in the receive command
2021-02-09 11:49 [PATCH] btrfs-progs: remove workaround for setting capabilities in the receive command fdmanana
@ 2021-02-10 1:38 ` Su Yue
2021-02-11 0:25 ` David Sterba
1 sibling, 0 replies; 3+ messages in thread
From: Su Yue @ 2021-02-10 1:38 UTC (permalink / raw)
To: fdmanana; +Cc: linux-btrfs
On Tue, Feb 9, 2021 at 7:53 PM <fdmanana@kernel.org> wrote:
>
> From: Filipe Manana <fdmanana@suse.com>
>
> We had a few bugs on the kernel side of send/receive where capabilities
> ended up being lost after receiving a send stream. They all stem from the
> fact that the kernel used to send all xattrs before issuing the chown
> command, and the later clears any existing capabilities in a file or
> directory.
>
> Initially a workaround was added to btrfs-progs' receive command, in commit
> 123a2a085027e ("btrfs-progs: receive: restore capabilities after chown"),
> and that fixed some instances of the problem. More recently, other instances
> of the problem were found, a proper fix for the kernel was made, which fixes
> the root problem by making send always emit the sexattr command for setting
> capabilities after issuing a chown command. This was done in kernel commit
> 89efda52e6b693 ("btrfs: send: emit file capabilities after chown"), which
> landed in kernel 5.8.
>
> However, the workaround on the receive command now causes us to incorrectly
> set a capability on a file that should not have it, because it assumes all
> setxattr commands for a file always comes before a chown.
>
> Example reproducer:
>
> $ cat send-caps.sh
> #!/bin/bash
>
> DEV1=/dev/sdh
> DEV2=/dev/sdi
>
> MNT1=/mnt/sdh
> MNT2=/mnt/sdi
>
> mkfs.btrfs -f $DEV1 > /dev/null
> mkfs.btrfs -f $DEV2 > /dev/null
>
> mount $DEV1 $MNT1
> mount $DEV2 $MNT2
>
> touch $MNT1/foo
> touch $MNT1/bar
> setcap cap_net_raw=p $MNT1/foo
>
> btrfs subvolume snapshot -r $MNT1 $MNT1/snap1
>
> btrfs send $MNT1/snap1 | btrfs receive $MNT2
>
> echo
> echo "capabilities on destination filesystem:"
> echo
> getcap $MNT2/snap1/foo
> getcap $MNT2/snap1/bar
>
> umount $MNT1
> umount $MNT2
>
> When running the test script, we can see that both files foo and bar get
> the capability set, when only file foo should have it:
>
> $ ./send-caps.sh
> Create a readonly snapshot of '/mnt/sdh' in '/mnt/sdh/snap1'
> At subvol /mnt/sdh/snap1
> At subvol snap1
>
> capabilities on destination filesystem:
>
> /mnt/sdi/snap1/foo cap_net_raw=p
> /mnt/sdi/snap1/bar cap_net_raw=p
>
> Since the kernel fix was backported to all currently supported stable
> releases (5.10.x, 5.4.x, 4.19.x, 4.14.x, 4.9.x and 4.4.x), remove the
> workaround from receive. Having such a workaround relying on the order
> of commands in a send stream is always troublesome and doomed to break
> one day.
>
Since the kernel fix was backported properly. It looks good to me now.
Reviewed-by: Su Yue <l@damenly.su>
> A test case for fstests will come soon.
>
> Reported-by: Richard Brown <rbrown@suse.de>
> Signed-off-by: Filipe Manana <fdmanana@suse.com>
> ---
> cmds/receive.c | 49 -------------------------------------------------
> 1 file changed, 49 deletions(-)
>
> diff --git a/cmds/receive.c b/cmds/receive.c
> index 2aaba3ff..b4099bc4 100644
> --- a/cmds/receive.c
> +++ b/cmds/receive.c
> @@ -77,14 +77,6 @@ struct btrfs_receive
> struct subvol_uuid_search sus;
>
> int honor_end_cmd;
> -
> - /*
> - * Buffer to store capabilities from security.capabilities xattr,
> - * usually 20 bytes, but make same room for potentially larger
> - * encodings. Must be set only once per file, denoted by length > 0.
> - */
> - char cached_capabilities[64];
> - int cached_capabilities_len;
> };
>
> static int finish_subvol(struct btrfs_receive *rctx)
> @@ -825,22 +817,6 @@ static int process_set_xattr(const char *path, const char *name,
> goto out;
> }
>
> - if (strcmp("security.capability", name) == 0) {
> - if (bconf.verbose >= 4)
> - fprintf(stderr, "set_xattr: cache capabilities\n");
> - if (rctx->cached_capabilities_len)
> - warning("capabilities set multiple times per file: %s",
> - full_path);
> - if (len > sizeof(rctx->cached_capabilities)) {
> - error("capabilities encoded to %d bytes, buffer too small",
> - len);
> - ret = -E2BIG;
> - goto out;
> - }
> - rctx->cached_capabilities_len = len;
> - memcpy(rctx->cached_capabilities, data, len);
> - }
> -
> if (bconf.verbose >= 3) {
> fprintf(stderr, "set_xattr %s - name=%s data_len=%d "
> "data=%.*s\n", path, name, len,
> @@ -961,23 +937,6 @@ static int process_chown(const char *path, u64 uid, u64 gid, void *user)
> error("chown %s failed: %m", path);
> goto out;
> }
> -
> - if (rctx->cached_capabilities_len) {
> - if (bconf.verbose >= 3)
> - fprintf(stderr, "chown: restore capabilities\n");
> - ret = lsetxattr(full_path, "security.capability",
> - rctx->cached_capabilities,
> - rctx->cached_capabilities_len, 0);
> - memset(rctx->cached_capabilities, 0,
> - sizeof(rctx->cached_capabilities));
> - rctx->cached_capabilities_len = 0;
> - if (ret < 0) {
> - ret = -errno;
> - error("restoring capabilities %s: %m", path);
> - goto out;
> - }
> - }
> -
> out:
> return ret;
> }
> @@ -1155,14 +1114,6 @@ static int do_receive(struct btrfs_receive *rctx, const char *tomnt,
> goto out;
>
> while (!end) {
> - if (rctx->cached_capabilities_len) {
> - if (bconf.verbose >= 4)
> - fprintf(stderr, "clear cached capabilities\n");
> - memset(rctx->cached_capabilities, 0,
> - sizeof(rctx->cached_capabilities));
> - rctx->cached_capabilities_len = 0;
> - }
> -
> ret = btrfs_read_and_process_send_stream(r_fd, &send_ops,
> rctx,
> rctx->honor_end_cmd,
> --
> 2.28.0
>
^ permalink raw reply [flat|nested] 3+ messages in thread* Re: [PATCH] btrfs-progs: remove workaround for setting capabilities in the receive command
2021-02-09 11:49 [PATCH] btrfs-progs: remove workaround for setting capabilities in the receive command fdmanana
2021-02-10 1:38 ` Su Yue
@ 2021-02-11 0:25 ` David Sterba
1 sibling, 0 replies; 3+ messages in thread
From: David Sterba @ 2021-02-11 0:25 UTC (permalink / raw)
To: fdmanana; +Cc: linux-btrfs
On Tue, Feb 09, 2021 at 11:49:12AM +0000, fdmanana@kernel.org wrote:
> From: Filipe Manana <fdmanana@suse.com>
>
> We had a few bugs on the kernel side of send/receive where capabilities
> ended up being lost after receiving a send stream. They all stem from the
> fact that the kernel used to send all xattrs before issuing the chown
> command, and the later clears any existing capabilities in a file or
> directory.
>
> Initially a workaround was added to btrfs-progs' receive command, in commit
> 123a2a085027e ("btrfs-progs: receive: restore capabilities after chown"),
> and that fixed some instances of the problem. More recently, other instances
> of the problem were found, a proper fix for the kernel was made, which fixes
> the root problem by making send always emit the sexattr command for setting
> capabilities after issuing a chown command. This was done in kernel commit
> 89efda52e6b693 ("btrfs: send: emit file capabilities after chown"), which
> landed in kernel 5.8.
>
> However, the workaround on the receive command now causes us to incorrectly
> set a capability on a file that should not have it, because it assumes all
> setxattr commands for a file always comes before a chown.
>
> Example reproducer:
>
> $ cat send-caps.sh
> #!/bin/bash
>
> DEV1=/dev/sdh
> DEV2=/dev/sdi
>
> MNT1=/mnt/sdh
> MNT2=/mnt/sdi
>
> mkfs.btrfs -f $DEV1 > /dev/null
> mkfs.btrfs -f $DEV2 > /dev/null
>
> mount $DEV1 $MNT1
> mount $DEV2 $MNT2
>
> touch $MNT1/foo
> touch $MNT1/bar
> setcap cap_net_raw=p $MNT1/foo
>
> btrfs subvolume snapshot -r $MNT1 $MNT1/snap1
>
> btrfs send $MNT1/snap1 | btrfs receive $MNT2
>
> echo
> echo "capabilities on destination filesystem:"
> echo
> getcap $MNT2/snap1/foo
> getcap $MNT2/snap1/bar
>
> umount $MNT1
> umount $MNT2
>
> When running the test script, we can see that both files foo and bar get
> the capability set, when only file foo should have it:
>
> $ ./send-caps.sh
> Create a readonly snapshot of '/mnt/sdh' in '/mnt/sdh/snap1'
> At subvol /mnt/sdh/snap1
> At subvol snap1
>
> capabilities on destination filesystem:
>
> /mnt/sdi/snap1/foo cap_net_raw=p
> /mnt/sdi/snap1/bar cap_net_raw=p
>
> Since the kernel fix was backported to all currently supported stable
> releases (5.10.x, 5.4.x, 4.19.x, 4.14.x, 4.9.x and 4.4.x), remove the
> workaround from receive. Having such a workaround relying on the order
> of commands in a send stream is always troublesome and doomed to break
> one day.
>
> A test case for fstests will come soon.
>
> Reported-by: Richard Brown <rbrown@suse.de>
> Signed-off-by: Filipe Manana <fdmanana@suse.com>
Thanks. I'm going to add a btrfs-progs test case as well, based on the
script in the changelog.
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2021-02-11 0:27 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-02-09 11:49 [PATCH] btrfs-progs: remove workaround for setting capabilities in the receive command fdmanana
2021-02-10 1:38 ` Su Yue
2021-02-11 0:25 ` David Sterba
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.