Re: [PATCH] btrfs: test if the capability is kept on incremental send

[Filipe Manana <fdmanana@gmail.com>]

On Mon, May 11, 2020 at 12:52 PM Filipe Manana <fdmanana@gmail.com> wrote:
>
> On Fri, May 8, 2020 at 9:14 PM Marcos Paulo de Souza
> <marcos@mpdesouza.com> wrote:
> >
> > From: Marcos Paulo de Souza <mpdesouza@suse.com>
> >
> > There is a situation where the incremental send can drop the capability
> > from the receiving side. If you have a file that changed the group, but
> > the capability was the same of before the group changed, the current
> > kernel code will only emit a chown, since the capability is the same.
> >
> > The steps bellow can reproduce 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
>
> It's redundant to put here the steps to trigger the problem, that's
> what the test does.
>
> You just want to say this test exercises full send and incremental
> send operations for cases where files have capabilities, to verify the
> capabilities aren't lost.
>
> >
> > At this point, 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
>
> Explaining in a changelog for a test case why exactly the bug happens
> is out of scope.
> We just want to know what bug are we testing for.
>
> The gory details about why the bug happens usually go the in the
> changelog of the patch that fixes btrfs.
>
> >
> > This test fails on current kernel, the fix was submitted to the btrfs
> > mailing list titled:
> >
> > "btrfs: send: Emit file capabilities after chown"
> >
> > Signed-off-by: Marcos Paulo de Souza <mpdesouza@suse.com>
> > ---
> >  tests/btrfs/211     | 153 ++++++++++++++++++++++++++++++++++++++++++++
> >  tests/btrfs/211.out |   6 ++
> >  tests/btrfs/group   |   1 +
> >  3 files changed, 160 insertions(+)
> >  create mode 100755 tests/btrfs/211
> >  create mode 100644 tests/btrfs/211.out
> >
> > diff --git a/tests/btrfs/211 b/tests/btrfs/211
> > new file mode 100755
> > index 00000000..e9aeaef3
> > --- /dev/null
> > +++ b/tests/btrfs/211
> > @@ -0,0 +1,153 @@
> > +#! /bin/bash
> > +# SPDX-License-Identifier: GPL-2.0
> > +# Copyright (C) 2020 SUSE Linux Products GmbH. All Rights Reserved.
> > +#
> > +# FS QA Test 211
> > +#
> > +# Test if the file capabilities aren't lost after full and incremental send
> > +#
> > +seq=`basename $0`
> > +seqres=$RESULT_DIR/$seq
> > +echo "QA output created by $seq"
> > +
> > +here=`pwd`
> > +tmp=/tmp/$$
> > +status=1       # failure is the default!
> > +trap "_cleanup; exit \$status" 0 1 2 3 15
> > +
> > +# get standard environment, filters and checks
> > +. ./common/rc
> > +. ./common/filter
> > +
> > +# remove previous $seqres.full before test
> > +rm -f $seqres.full
> > +
> > +_supported_fs btrfs
> > +_supported_os Linux
> > +_require_scratch
> > +_require_command "$SETCAP_PROG" setcap
> > +_require_command "$GETCAP_PROG" getcap
> > +
> > +_cleanup()
> > +{
> > +       cd /
> > +       rm -f $tmp.*
> > +}
> > +
> > +_check_xattr()
>
> Function names starting with '_' are usually reserved for functions
> provided by fstests, global functions (like the ones in common/*).
> Same comment applies to all the other function names.
>
> Also, a better name would be "check_capabilities" - that they are
> stored in a xattr it's irrelevant as we are using the getcap and
> setcap utilities to read and set them, and not accessing xattrs
> directly.
>
> > +{
> > +       local RET
> > +       local FILE
> > +       local CAP
> > +       FILE="$1"
> > +       CAP="$2"
>
> Variables in uppercase are meant to be used for global variables. Same
> comment applies to all the other functions.
>
> > +       RET=$($GETCAP_PROG "$FILE")
> > +       if [ -z "$RET" ]; then
> > +               echo "$RET"
> > +               _fail "missing capability in file $FILE"
>
> This is a practice generally discouraged, we should avoid "_fail"
> unless absolutely necessary.
> The right approach here is just to "echo" the message. That's enough
> to make the test fail as that message is not part of the golden
> output.
> Furthermore, not using _fail allows the test to proceed and
> potentially find other problems.
>
> > +       fi
> > +       if [[ "$RET" != *$CAP* ]]; then
> > +               echo "$RET"
> > +               echo "$CAP"
> > +               _fail "Capability do not match. Output: $RET"
>
> Capability -> Capabilities
>
> > +       fi
> > +}
> > +
> > +_setup()
> > +{
> > +       _scratch_mkfs >/dev/null
> > +       _scratch_mount
> > +
> > +       FS1="$SCRATCH_MNT/fs1"
> > +       FS2="$SCRATCH_MNT/fs2"
>
> To make it easier to follow, perhaps declare this outside this
> function, as they are used by the other functions.
>
> > +
> > +       $BTRFS_UTIL_PROG subvolume create "$FS1" > /dev/null
> > +       $BTRFS_UTIL_PROG subvolume create "$FS2" > /dev/null
> > +}
> > +
> > +_full_nocap_inc_withcap_send()
> > +{
> > +       _setup
> > +
> > +       # Test full send containing a file with no capability
> > +       touch "$FS1/foo.bar"
> > +       $BTRFS_UTIL_PROG subvolume snapshot -r "$FS1" "$FS1/snap_init" >/dev/null
> > +       $BTRFS_UTIL_PROG send "$FS1/snap_init" -q | $BTRFS_UTIL_PROG receive "$FS2" -q
> > +       # ensure that we don't have caps set
> > +       RET=$($GETCAP_PROG "$FS2/snap_init/foo.bar")
>
> Should be declared local (and lower case name).
>
> > +       if [ -n "$RET" ]; then
> > +               _fail "File contain capabilities when it shouldn't"
>
> echo and contain -> contains
>
> > +       fi
> > +
> > +       # Test if  incremental send bring the newly added capability
> > +       $SETCAP_PROG "cap_sys_ptrace+ep cap_sys_nice+ep" "$FS1/foo.bar"
> > +       $BTRFS_UTIL_PROG subvolume snapshot -r "$FS1" "$FS1/snap_inc" >/dev/null
> > +       $BTRFS_UTIL_PROG send -p "$FS1/snap_init" "$FS1/snap_inc" -q | \
> > +                                       $BTRFS_UTIL_PROG receive "$FS2" -q
> > +       _check_xattr "$FS2/snap_inc/foo.bar" "cap_sys_ptrace,cap_sys_nice+ep"
> > +
> > +       _scratch_unmount
> > +}
> > +
> > +_roundtrip_send()
> > +{
> > +       local FILES
> > +       local FS1
> > +       local FS2
>
> So FS1 and FS2 are declared local but never set. And they were
> previously set as global by "_setup".
> So those two declarations should be removed here.
>
> > +
> > +       # FILES should include foo.bar
> > +       FILES="$1"
> > +
> > +       _setup
> > +
> > +       # create files on fs1, must contain foo.bar
> > +       for f in $FILES; do
> > +               touch "$FS1/$f"
> > +       done
> > +
> > +       # Test full send, checking if the receiving side keeps the capability
>
> capability -> capabilities (as the test sets 2)
>
> > +       $SETCAP_PROG "cap_sys_ptrace+ep cap_sys_nice+ep" "$FS1/foo.bar"
> > +       $BTRFS_UTIL_PROG subvolume snapshot -r "$FS1" "$FS1/snap_init" >/dev/null
> > +       $BTRFS_UTIL_PROG send "$FS1/snap_init" -q | $BTRFS_UTIL_PROG receive "$FS2" -q
> > +       _check_xattr "$FS2/snap_init/foo.bar" "cap_sys_ptrace,cap_sys_nice+ep"
> > +
> > +       # Test incremental send with different owner/group but same capability
>
> capability -> capabilities (as the test sets 2)
>
> > +       chgrp 100 "$FS1/foo.bar"
> > +       $SETCAP_PROG "cap_sys_ptrace+ep cap_sys_nice+ep" "$FS1/foo.bar"
> > +       $BTRFS_UTIL_PROG subvolume snapshot -r "$FS1" "$FS1/snap_inc" >/dev/null
> > +       _check_xattr "$FS1/snap_inc/foo.bar" "cap_sys_ptrace,cap_sys_nice+ep"
> > +       $BTRFS_UTIL_PROG send -p "$FS1/snap_init" "$FS1/snap_inc" -q | \
> > +                               $BTRFS_UTIL_PROG receive "$FS2" -q
> > +       _check_xattr "$FS2/snap_inc/foo.bar" "cap_sys_ptrace,cap_sys_nice+ep"
> > +
> > +       # Test capability after incremental send with different capability and group
>
> capability -> capabilities (as the test sets 2)
>
> > +       chgrp 0 "$FS1/foo.bar"
> > +       $SETCAP_PROG "cap_sys_time+ep cap_syslog+ep" "$FS1/foo.bar"
> > +       $BTRFS_UTIL_PROG subvolume snapshot -r "$FS1" "$FS1/snap_inc2" >/dev/null
> > +       _check_xattr "$FS1/snap_inc2/foo.bar" "cap_sys_time,cap_syslog+ep"
> > +       $BTRFS_UTIL_PROG send -p "$FS1/snap_inc" "$FS1/snap_inc2" -q | \
> > +                               $BTRFS_UTIL_PROG receive "$FS2"  -q
> > +       _check_xattr "$FS2/snap_inc2/foo.bar" "cap_sys_time,cap_syslog+ep"
> > +
> > +       _scratch_unmount
> > +}
> > +
> > +# real QA test starts here
> > +
> > +echo "Test full send + file without caps, then inc send bringing a new cap"
>
> inc -> incremental
>
> Also, the abbreviation "caps" is used but in other messages (below)
> the non-abbreviated name "capabilities" is used.
> We should be consistent and use only one form.
>
> > +_full_nocap_inc_withcap_send
> > +
> > +echo "Testing if foo.bar alone can keep it's capability"
>
> it's -> its
> capability -> capabilities (as the test sets 2)
>
> > +_roundtrip_send "foo.bar"
> > +
> > +echo "Test foo.bar being the first item among other files"
> > +_roundtrip_send "foo.bar foo.bax foo.baz"
> > +
> > +echo "Test foo.bar with objectid between two other files"
> > +_roundtrip_send "foo1 foo.bar foo3"
> > +
> > +echo "Test foo.bar being the last item among other files"
> > +_roundtrip_send "foo1 foo2 foo.bar"
> > +
> > +status=0
> > +exit
> > diff --git a/tests/btrfs/211.out b/tests/btrfs/211.out
> > new file mode 100644
> > index 00000000..fc50c923
> > --- /dev/null
> > +++ b/tests/btrfs/211.out
> > @@ -0,0 +1,6 @@
> > +QA output created by 211
> > +Test full send + file without caps, then inc send bringing a new cap
> > +Testing if foo.bar alone can keep it's capability
> > +Test foo.bar being the first item among other files
> > +Test foo.bar with objectid between two other files
> > +Test foo.bar being the last item among other files
> > diff --git a/tests/btrfs/group b/tests/btrfs/group
> > index 9426fb17..437d4431 100644
> > --- a/tests/btrfs/group
> > +++ b/tests/btrfs/group
> > @@ -213,3 +213,4 @@
> >  208 auto quick subvol
> >  209 auto quick log
> >  210 auto quick qgroup snapshot
> > +211 auto quick snapshot
>
> Missing the 'send' group.
>
> Other that is looks good and it works as expected (with patched and
> unpatched btrfs)
> Thanks.

Marcos, ping. Anything I can help with?
We should really have this test case in fstests.

Thanks.

>
> > --
> > 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.”