Re: [PATCH RFC] btrfs: send: Emit all xattr of an inode if the uid/gid changed

[Filipe Manana <fdmanana@gmail.com>]

On Thu, Mar 26, 2020 at 1:51 PM Filipe Manana <fdmanana@gmail.com> wrote:
>
> On Wed, Mar 25, 2020 at 1:52 AM Marcos Paulo de Souza
> <marcos@mpdesouza.com> wrote:
> >
> > From: Marcos Paulo de Souza <mpdesouza@suse.com>
> >
> > [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@suse.com>
> > ---
> >  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.”