On Thu, Mar 26, 2020 at 1:51 PM Filipe Manana <fdmanana@xxxxxxxxx> wrote: > > On Wed, Mar 25, 2020 at 1:52 AM Marcos Paulo de Souza > <marcos@xxxxxxxxxxxxx> wrote: > > > > From: Marcos Paulo de Souza <mpdesouza@xxxxxxxx> > > > > [PROBLEM] > > 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 show 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 fs/snap_increment/foo.bar lost the capability, since a > > chgrp was emitted by "btrfs send". The current code only checks for the > > items that changed, and as the XATTR kept the value only the chgrp change > > is emitted. > > So, the explanation could be a bit more clear. > > First the send stream emits a "chown" command (and not a "chgrp" > command), that's what is used to change both users and groups. > > Then mentioning that changing the group of a file causes the > capability xattr to be deleted is crucial - that's why the receiving > side ends up losing the capability, because we send an operation to > change the group but we don't send an operation to set the capability > xattr, since the value of the xattr is the same in both snapshots. > > > > > [FIX] > > In order to fix this issue, check if the uid/gid of the inode change, > > and if yes, emit all XATTR again, including the capability. > > > > Fixes: https://github.com/kdave/btrfs-progs/issues/202 > > The Fixes: tag is used to identify commits, in the kernel, that > introduced some problem. > A "Link:" tag is more appropriate to point to the btrfs-progs github issue. > > > > > Signed-off-by: Marcos Paulo de Souza <mpdesouza@xxxxxxxx> > > --- > > I'm posting this patch as a RFC because I had some questions > > * Is this the correct place to fix? > > Nop, see below. > > > * Also, emitting all XATTR of the inode seems overkill... > > Yes, but I wouldn't worry much - first it's not common for files to > have many xattrs, second they are small values and are layed out > sequentially in the btree, and above all, uids/gids are mostly static > and don't change often. > But that can be avoided, see below. > > > * Should it be fixed in userspace? > > No. > > Send should emit a sequence of operations that produces correct > results in the receiving side. It should never result in any data or > metadata loss, crashes, etc. > > This is no different from rename dependencies for example, where we > make send change the order of rename and other operations so that the > receiving side doesn't fail - otherwise we would have to add a lot of > intelligence and complicated code to btrfs receive in progs - which > brings us to another point - any consumer of a send stream would have > to be changed - btrfs receive from btrfs-progs, is the most well > known, and very few people will use anything else, but there may be > other consumers of send streams out there. > > > > > fs/btrfs/send.c | 29 +++++++++++++++++++++++++---- > > 1 file changed, 25 insertions(+), 4 deletions(-) > > > > diff --git a/fs/btrfs/send.c b/fs/btrfs/send.c > > index c5f41bd86765..5cffe5da91cf 100644 > > --- a/fs/btrfs/send.c > > +++ b/fs/btrfs/send.c > > @@ -6187,6 +6187,14 @@ static int changed_inode(struct send_ctx *sctx, > > sctx->cur_inode_mode = btrfs_inode_mode( > > sctx->right_path->nodes[0], right_ii); > > } else if (result == BTRFS_COMPARE_TREE_CHANGED) { > > + u64 left_uid = btrfs_inode_uid(sctx->left_path->nodes[0], > > + left_ii); > > + u64 left_gid = btrfs_inode_gid(sctx->left_path->nodes[0], > > + left_ii); > > + u64 right_uid = btrfs_inode_uid(sctx->right_path->nodes[0], > > + right_ii); > > + u64 right_gid = btrfs_inode_gid(sctx->right_path->nodes[0], > > + right_ii); > > /* > > * We need to do some special handling in case the inode was > > * reported as changed with a changed generation number. This > > @@ -6236,15 +6244,12 @@ static int changed_inode(struct send_ctx *sctx, > > sctx->send_progress = sctx->cur_ino + 1; > > > > /* > > - * Now process all extents and xattrs of the inode as if > > + * Now process all extents of the inode as if > > * they were all new. > > */ > > ret = process_all_extents(sctx); > > if (ret < 0) > > goto out; > > - ret = process_all_new_xattrs(sctx); > > - if (ret < 0) > > - goto out; > > } else { > > sctx->cur_inode_gen = left_gen; > > sctx->cur_inode_new = 0; > > @@ -6255,6 +6260,22 @@ static int changed_inode(struct send_ctx *sctx, > > sctx->cur_inode_mode = btrfs_inode_mode( > > sctx->left_path->nodes[0], left_ii); > > } > > + > > + /* > > + * Process all XATTR of the inode if the generation or owner > > + * changed. > > + * > > + * If the inode changed it's uid/gid, but kept a > > + * security.capability xattr, only the uid/gid will be emitted, > > + * causing the related xattr to deleted. For this reason always > > + * emit the XATTR when an inode has changed. > > + */ > > + if (sctx->cur_inode_new_gen || left_uid != right_uid || > > + left_gid != right_gid) { > > + ret = process_all_new_xattrs(sctx); > > So the correct place for fixing this issue is at > send.c:finish_inode_if_needed(), in the following place: > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/fs/btrfs/send.c?h=v5.6-rc7#n6000 > > If a chown operation is sent, then just send the xattr - and instead > if sending all xattrs, just send the xattr with the name > "security.capability" - check first if there are any other > capabilities that use other xattr names - if there are, just emit > set_xattr operations for all xattrs with a "security." prefix in their > name. > > Thanks. Also, I would change the subject of the patch, so that it mentions what problems it fixes and not how it fixes a problem. > > > > + if (ret < 0) > > + goto out; > > + } > > } > > > > out: > > -- > > 2.25.1 > > > > > -- > Filipe David Manana, > > “Whether you think you can, or you think you can't — you're right.” -- Filipe David Manana, “Whether you think you can, or you think you can't — you're right.”
