Message ID | e35e5d556cd5964a4ab80bdd997856ee5be8b888.1612870936.git.fdmanana@suse.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | btrfs-progs: remove workaround for setting capabilities in the receive command | expand |
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 >
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.
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,