| Message ID | 20200507201804.1623-1-marcos@mpdesouza.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v1] btrfs: send: Emit file capabilities after chown | expand |
On Thu, May 7, 2020 at 9:16 PM Marcos Paulo de Souza <marcos@mpdesouza.com> wrote: > > From: Marcos Paulo de Souza <mpdesouza@suse.com> > > [PROBLEM] > Whenever a chown is executed, all capabilities of the file being touched are > lost. When doing incremental send with a file with capabilities, there is a > situation where the capability can be lost in the receiving side. The > sequence of actions bellow shows the problem: > > $ mount /dev/sda fs1 > $ mount /dev/sdb fs2 > > $ touch fs1/foo.bar > $ setcap cap_sys_nice+ep fs1/foo.bar > $ btrfs subvol snap -r fs1 fs1/snap_init > $ btrfs send fs1/snap_init | btrfs receive fs2 > > $ chgrp adm fs1/foo.bar > $ setcap cap_sys_nice+ep fs1/foo.bar > > $ btrfs subvol snap -r fs1 fs1/snap_complete > $ btrfs subvol snap -r fs1 fs1/snap_incremental > > $ btrfs send fs1/snap_complete | btrfs receive fs2 > $ btrfs send -p fs1/snap_init fs1/snap_incremental | btrfs receive fs2 > > At this point, only a chown was emitted by "btrfs send" since only the group > was changed. This makes the cap_sys_nice capability to be dropped from > fs2/snap_incremental/foo.bar > > [FIX] > Only emit capabilities after chown is emitted. The current code > first checks for xattrs that are new/changed, emits them, and later emit > the chown. Now, __process_new_xattr skips capabilities, letting only > finish_inode_if_needed to emit them, if they exist, for the inode being > processed. > > Also, this patch also fixes a longstanding problem that was issuing an xattr > _before_ chown. This sentence seems an unnecessary duplication of what the previous paragraph said. > This behavior was being worked around in "btrfs receive" > side by caching the capability and only applying it after chown. Now, > xattrs are only emmited _after_ chown, making that hack not needed > anymore. Right, lets add: CC: stable@vger.kernel.org With that and after a few kernels releases with the fix (lets in 6 months maybe), we can remove the btrfs-progs stuff. Also please send the test case for fstests, to test that particular scenario in the changelog and others for which the btrfs-progs hack worked but we never got any test case: 1) test that after a full send of a snapshot with a file with capabilities, the receiver still has the capabilities; 2) test that if we add capabilities to a file which had none in the parent snapshot, the receiver gets the capabilities after an incremental send; 3) test that if the capabilities and owner/group change in the second snapshot for a file, after an incremental send the receiver has the correct capabilities; 4) the case for the changelog - if the onwer/group changes but the capabilities remain the same, after an incremental send the capabilities aren't lost > > Link: https://github.com/kdave/btrfs-progs/issues/202 > Suggested-by: Filipe Manana <fdmanana@suse.com> > Signed-off-by: Marcos Paulo de Souza <mpdesouza@suse.com> Reviewed-by: Filipe Manana <fdmanana@suse.com> > --- > The first version of the patch was an RFC > > Changes from RFC: > * Explained about chown + drop capabilities problem in the commit message (suggested > by Filipe and David) > * Changed the commit message to show describe the fix (suggested by Filipe) > * Skip the xattr in __process_new_xattr if it's a capability, since it'll be > handled in finish_inode_if_needed now (suggested by Filipe). > * Created function send_capabilities to query if the inode has caps, and if > yes, emit them. > * Call send_capabilities in finish_inode_if_needed _after_ the needs_chown > check. (suggested by Filipe) > > fs/btrfs/send.c | 69 +++++++++++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 69 insertions(+) > > diff --git a/fs/btrfs/send.c b/fs/btrfs/send.c > index 6b86841315be..4f19965bdd82 100644 > --- a/fs/btrfs/send.c > +++ b/fs/btrfs/send.c > @@ -23,6 +23,7 @@ > #include "btrfs_inode.h" > #include "transaction.h" > #include "compression.h" > +#include "xattr.h" > > /* > * Maximum number of references an extent can have in order for us to attempt to > @@ -4545,6 +4546,10 @@ static int __process_new_xattr(int num, struct btrfs_key *di_key, > struct fs_path *p; > struct posix_acl_xattr_header dummy_acl; > > + /* capabilities are emitted by finish_inode_if_needed */ > + if (!strncmp(name, XATTR_NAME_CAPS, name_len)) > + return 0; > + > p = fs_path_alloc(); > if (!p) > return -ENOMEM; > @@ -5107,6 +5112,66 @@ static int send_extent_data(struct send_ctx *sctx, > return 0; > } > > +/* > + * Search for a capability xattr related to sctx->cur_ino. If the capability if > + * found, call send_set_xattr function to emit it. > + * > + * Return %0 if there isn't a capability, or when the capability was emitted > + * successfully, or < %0 if an error occurred. > + */ > +static int send_capabilities(struct send_ctx *sctx) > +{ > + struct fs_path *fspath = NULL; > + struct btrfs_path *path; > + struct btrfs_dir_item *di; > + struct extent_buffer *leaf; > + unsigned long data_ptr; > + char *name = XATTR_NAME_CAPS; Can be "const", or get rid of name and use XATTR_NAME_CAPS directly everywhere. Anyway, it's more a subjective stylistic detail, not that relevant. Thanks, good work! > + char *buf = NULL; > + int buf_len; > + int ret = 0; > + > + path = btrfs_alloc_path(); > + if (!path) > + return -ENOMEM; > + > + di = btrfs_lookup_xattr(NULL, sctx->send_root, path, sctx->cur_ino, > + name, strlen(name), 0); > + if (!di) { > + /* there is no xattr for this inode */ > + goto out; > + } else if (IS_ERR(di)) { > + ret = PTR_ERR(di); > + goto out; > + } > + > + leaf = path->nodes[0]; > + buf_len = btrfs_dir_data_len(leaf, di); > + > + fspath = fs_path_alloc(); > + buf = kmalloc(buf_len, GFP_KERNEL); > + if (!fspath || !buf) { > + ret = -ENOMEM; > + goto out; > + } > + > + ret = get_cur_path(sctx, sctx->cur_ino, sctx->cur_inode_gen, fspath); > + if (ret < 0) > + goto out; > + > + data_ptr = (unsigned long)((char *)(di + 1) + > + btrfs_dir_name_len(leaf, di)); > + read_extent_buffer(leaf, buf, data_ptr, > + btrfs_dir_data_len(leaf, di)); > + > + ret = send_set_xattr(sctx, fspath, name, strlen(name), buf, buf_len); > +out: > + kfree(buf); > + fs_path_free(fspath); > + btrfs_free_path(path); > + return ret; > +} > + > static int clone_range(struct send_ctx *sctx, > struct clone_root *clone_root, > const u64 disk_byte, > @@ -6010,6 +6075,10 @@ static int finish_inode_if_needed(struct send_ctx *sctx, int at_end) > goto out; > } > > + ret = send_capabilities(sctx); > + if (ret < 0) > + goto out; > + > /* > * If other directory inodes depended on our current directory > * inode's move/rename, now do their move/rename operations. > -- > 2.25.1 >
On Thu, May 7, 2020 at 9:16 PM Marcos Paulo de Souza <marcos@mpdesouza.com> wrote: > > From: Marcos Paulo de Souza <mpdesouza@suse.com> > > [PROBLEM] > Whenever a chown is executed, all capabilities of the file being touched are > lost. When doing incremental send with a file with capabilities, there is a > situation where the capability can be lost in the receiving side. The > sequence of actions bellow shows the problem: > > $ mount /dev/sda fs1 > $ mount /dev/sdb fs2 > > $ touch fs1/foo.bar > $ setcap cap_sys_nice+ep fs1/foo.bar > $ btrfs subvol snap -r fs1 fs1/snap_init > $ btrfs send fs1/snap_init | btrfs receive fs2 > > $ chgrp adm fs1/foo.bar > $ setcap cap_sys_nice+ep fs1/foo.bar > > $ btrfs subvol snap -r fs1 fs1/snap_complete > $ btrfs subvol snap -r fs1 fs1/snap_incremental > > $ btrfs send fs1/snap_complete | btrfs receive fs2 > $ btrfs send -p fs1/snap_init fs1/snap_incremental | btrfs receive fs2 > > At this point, only a chown was emitted by "btrfs send" since only the group > was changed. This makes the cap_sys_nice capability to be dropped from > fs2/snap_incremental/foo.bar > > [FIX] > Only emit capabilities after chown is emitted. The current code > first checks for xattrs that are new/changed, emits them, and later emit > the chown. Now, __process_new_xattr skips capabilities, letting only > finish_inode_if_needed to emit them, if they exist, for the inode being > processed. > > Also, this patch also fixes a longstanding problem that was issuing an xattr > _before_ chown. This behavior was being worked around in "btrfs receive" > side by caching the capability and only applying it after chown. Now, > xattrs are only emmited _after_ chown, making that hack not needed > anymore. > > Link: https://github.com/kdave/btrfs-progs/issues/202 > Suggested-by: Filipe Manana <fdmanana@suse.com> > Signed-off-by: Marcos Paulo de Souza <mpdesouza@suse.com> > --- > The first version of the patch was an RFC > > Changes from RFC: > * Explained about chown + drop capabilities problem in the commit message (suggested > by Filipe and David) > * Changed the commit message to show describe the fix (suggested by Filipe) > * Skip the xattr in __process_new_xattr if it's a capability, since it'll be > handled in finish_inode_if_needed now (suggested by Filipe). > * Created function send_capabilities to query if the inode has caps, and if > yes, emit them. > * Call send_capabilities in finish_inode_if_needed _after_ the needs_chown > check. (suggested by Filipe) > > fs/btrfs/send.c | 69 +++++++++++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 69 insertions(+) > > diff --git a/fs/btrfs/send.c b/fs/btrfs/send.c > index 6b86841315be..4f19965bdd82 100644 > --- a/fs/btrfs/send.c > +++ b/fs/btrfs/send.c > @@ -23,6 +23,7 @@ > #include "btrfs_inode.h" > #include "transaction.h" > #include "compression.h" > +#include "xattr.h" > > /* > * Maximum number of references an extent can have in order for us to attempt to > @@ -4545,6 +4546,10 @@ static int __process_new_xattr(int num, struct btrfs_key *di_key, > struct fs_path *p; > struct posix_acl_xattr_header dummy_acl; > > + /* capabilities are emitted by finish_inode_if_needed */ > + if (!strncmp(name, XATTR_NAME_CAPS, name_len)) > + return 0; > + > p = fs_path_alloc(); > if (!p) > return -ENOMEM; > @@ -5107,6 +5112,66 @@ static int send_extent_data(struct send_ctx *sctx, > return 0; > } > > +/* > + * Search for a capability xattr related to sctx->cur_ino. If the capability if > + * found, call send_set_xattr function to emit it. > + * > + * Return %0 if there isn't a capability, or when the capability was emitted > + * successfully, or < %0 if an error occurred. > + */ > +static int send_capabilities(struct send_ctx *sctx) > +{ > + struct fs_path *fspath = NULL; > + struct btrfs_path *path; > + struct btrfs_dir_item *di; > + struct extent_buffer *leaf; > + unsigned long data_ptr; > + char *name = XATTR_NAME_CAPS; > + char *buf = NULL; > + int buf_len; > + int ret = 0; > + > + path = btrfs_alloc_path(); So, I forgot this: use alloc_path_for_send() instead, we want a path that uses the commit root. Path allocations in send should always use that helper. Thanks. > + if (!path) > + return -ENOMEM; > + > + di = btrfs_lookup_xattr(NULL, sctx->send_root, path, sctx->cur_ino, > + name, strlen(name), 0); > + if (!di) { > + /* there is no xattr for this inode */ > + goto out; > + } else if (IS_ERR(di)) { > + ret = PTR_ERR(di); > + goto out; > + } > + > + leaf = path->nodes[0]; > + buf_len = btrfs_dir_data_len(leaf, di); > + > + fspath = fs_path_alloc(); > + buf = kmalloc(buf_len, GFP_KERNEL); > + if (!fspath || !buf) { > + ret = -ENOMEM; > + goto out; > + } > + > + ret = get_cur_path(sctx, sctx->cur_ino, sctx->cur_inode_gen, fspath); > + if (ret < 0) > + goto out; > + > + data_ptr = (unsigned long)((char *)(di + 1) + > + btrfs_dir_name_len(leaf, di)); > + read_extent_buffer(leaf, buf, data_ptr, > + btrfs_dir_data_len(leaf, di)); > + > + ret = send_set_xattr(sctx, fspath, name, strlen(name), buf, buf_len); > +out: > + kfree(buf); > + fs_path_free(fspath); > + btrfs_free_path(path); > + return ret; > +} > + > static int clone_range(struct send_ctx *sctx, > struct clone_root *clone_root, > const u64 disk_byte, > @@ -6010,6 +6075,10 @@ static int finish_inode_if_needed(struct send_ctx *sctx, int at_end) > goto out; > } > > + ret = send_capabilities(sctx); > + if (ret < 0) > + goto out; > + > /* > * If other directory inodes depended on our current directory > * inode's move/rename, now do their move/rename operations. > -- > 2.25.1 >
diff --git a/fs/btrfs/send.c b/fs/btrfs/send.c index 6b86841315be..4f19965bdd82 100644 --- a/fs/btrfs/send.c +++ b/fs/btrfs/send.c @@ -23,6 +23,7 @@ #include "btrfs_inode.h" #include "transaction.h" #include "compression.h" +#include "xattr.h" /* * Maximum number of references an extent can have in order for us to attempt to @@ -4545,6 +4546,10 @@ static int __process_new_xattr(int num, struct btrfs_key *di_key, struct fs_path *p; struct posix_acl_xattr_header dummy_acl; + /* capabilities are emitted by finish_inode_if_needed */ + if (!strncmp(name, XATTR_NAME_CAPS, name_len)) + return 0; + p = fs_path_alloc(); if (!p) return -ENOMEM; @@ -5107,6 +5112,66 @@ static int send_extent_data(struct send_ctx *sctx, return 0; } +/* + * Search for a capability xattr related to sctx->cur_ino. If the capability if + * found, call send_set_xattr function to emit it. + * + * Return %0 if there isn't a capability, or when the capability was emitted + * successfully, or < %0 if an error occurred. + */ +static int send_capabilities(struct send_ctx *sctx) +{ + struct fs_path *fspath = NULL; + struct btrfs_path *path; + struct btrfs_dir_item *di; + struct extent_buffer *leaf; + unsigned long data_ptr; + char *name = XATTR_NAME_CAPS; + char *buf = NULL; + int buf_len; + int ret = 0; + + path = btrfs_alloc_path(); + if (!path) + return -ENOMEM; + + di = btrfs_lookup_xattr(NULL, sctx->send_root, path, sctx->cur_ino, + name, strlen(name), 0); + if (!di) { + /* there is no xattr for this inode */ + goto out; + } else if (IS_ERR(di)) { + ret = PTR_ERR(di); + goto out; + } + + leaf = path->nodes[0]; + buf_len = btrfs_dir_data_len(leaf, di); + + fspath = fs_path_alloc(); + buf = kmalloc(buf_len, GFP_KERNEL); + if (!fspath || !buf) { + ret = -ENOMEM; + goto out; + } + + ret = get_cur_path(sctx, sctx->cur_ino, sctx->cur_inode_gen, fspath); + if (ret < 0) + goto out; + + data_ptr = (unsigned long)((char *)(di + 1) + + btrfs_dir_name_len(leaf, di)); + read_extent_buffer(leaf, buf, data_ptr, + btrfs_dir_data_len(leaf, di)); + + ret = send_set_xattr(sctx, fspath, name, strlen(name), buf, buf_len); +out: + kfree(buf); + fs_path_free(fspath); + btrfs_free_path(path); + return ret; +} + static int clone_range(struct send_ctx *sctx, struct clone_root *clone_root, const u64 disk_byte, @@ -6010,6 +6075,10 @@ static int finish_inode_if_needed(struct send_ctx *sctx, int at_end) goto out; } + ret = send_capabilities(sctx); + if (ret < 0) + goto out; + /* * If other directory inodes depended on our current directory * inode's move/rename, now do their move/rename operations.