On Mon, 2020-06-01 at 14:48 +0100, Filipe Manana wrote:
> On Mon, May 11, 2020 at 12:52 PM Filipe Manana <fdmanana@xxxxxxxxx>
> wrote:
> >
> > On Fri, May 8, 2020 at 9:14 PM Marcos Paulo de Souza
> > <marcos@xxxxxxxxxxxxx> wrote:
> > >
> > > From: Marcos Paulo de Souza <mpdesouza@xxxxxxxx>
> > >
> > > 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@xxxxxxxx>
> > > ---
> > > 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?
Sorry, the test was in my radar but somehow I forgot about it. I hope
to send a new version later today, addressing all your comments.
> We should really have this test case in fstests.
Sure, I agree.
>
>
> Thanks.
>
> >
> > > --
> > > 2.25.1
> > >
> >
> >
> > --
> > Filipe David Manana,
> >
> > “Whether you think you can, or you think you can't — you're right.”
>
>
>
