2020-09-10 19:49:04

by Frank van der Linden

[permalink] [raw]
Subject: [PATCH 1/3] common/attr: make _require_attrs more fine-grained

Filesystems may not support all xattr types. But, _require_attr assumes
that being able to use "user" namespace xattrs means that all namespaces
("trusted", "system", etc) are supported. This breaks on NFS, that only
supports the "user" namespace, and a few cases in the "system" namespace.

Change _require_attrs to optionally take namespace arguments that specify
the namespaces to check for. The default behavior (no arguments) is still
to check for the "user" namespace only.

Signed-off-by: Frank van der Linden <[email protected]>
---
common/attr | 49 +++++++++++++++++++++++++++++++------------------
1 file changed, 31 insertions(+), 18 deletions(-)

diff --git a/common/attr b/common/attr
index 20049de0..c60cb6ed 100644
--- a/common/attr
+++ b/common/attr
@@ -175,30 +175,43 @@ _list_acl()

_require_attrs()
{
+ local args
+ local nsp
+
+ if [ $# -eq 0 ];
+ then
+ args="user"
+ else
+ args="$*"
+ fi
+
[ -n "$ATTR_PROG" ] || _notrun "attr command not found"
[ -n "$GETFATTR_PROG" ] || _notrun "getfattr command not found"
[ -n "$SETFATTR_PROG" ] || _notrun "setfattr command not found"

- #
- # Test if chacl is able to write an attribute on the target filesystems.
- # On really old kernels the system calls might not be implemented at all,
- # but the more common case is that the tested filesystem simply doesn't
- # support attributes. Note that we can't simply list attributes as
- # various security modules generate synthetic attributes not actually
- # stored on disk.
- #
- touch $TEST_DIR/syscalltest
- attr -s "user.xfstests" -V "attr" $TEST_DIR/syscalltest > $TEST_DIR/syscalltest.out 2>&1
- cat $TEST_DIR/syscalltest.out >> $seqres.full
+ for nsp in $args
+ do
+ #
+ # Test if chacl is able to write an attribute on the target filesystems.
+ # On really old kernels the system calls might not be implemented at all,
+ # but the more common case is that the tested filesystem simply doesn't
+ # support attributes. Note that we can't simply list attributes as
+ # various security modules generate synthetic attributes not actually
+ # stored on disk.
+ #
+ touch $TEST_DIR/syscalltest
+ $SETFATTR_PROG -n "$nsp.xfstests" -v "attr" $TEST_DIR/syscalltest > $TEST_DIR/syscalltest.out 2>&1
+ cat $TEST_DIR/syscalltest.out >> $seqres.full

- if grep -q 'Function not implemented' $TEST_DIR/syscalltest.out; then
- _notrun "kernel does not support attrs"
- fi
- if grep -q 'Operation not supported' $TEST_DIR/syscalltest.out; then
- _notrun "attrs not supported by this filesystem type: $FSTYP"
- fi
+ if grep -q 'Function not implemented' $TEST_DIR/syscalltest.out; then
+ _notrun "kernel does not support attrs"
+ fi
+ if grep -q 'Operation not supported' $TEST_DIR/syscalltest.out; then
+ _notrun "attr namespace $nsp not supported by this filesystem type: $FSTYP"
+ fi

- rm -f $TEST_DIR/syscalltest.out
+ rm -f $TEST_DIR/syscalltest.out
+ done
}

_require_attr_v1()
--
2.16.6


2020-09-13 16:57:55

by Eryu Guan

[permalink] [raw]
Subject: Re: [PATCH 1/3] common/attr: make _require_attrs more fine-grained

On Thu, Sep 10, 2020 at 07:43:53PM +0000, Frank van der Linden wrote:
> Filesystems may not support all xattr types. But, _require_attr assumes
> that being able to use "user" namespace xattrs means that all namespaces
> ("trusted", "system", etc) are supported. This breaks on NFS, that only
> supports the "user" namespace, and a few cases in the "system" namespace.
>
> Change _require_attrs to optionally take namespace arguments that specify
> the namespaces to check for. The default behavior (no arguments) is still
> to check for the "user" namespace only.
>
> Signed-off-by: Frank van der Linden <[email protected]>

This patchset looks great to me, thanks!

Some minor nits below (and I've fixed them on commit, so there's no need
to resend :)

> ---
> common/attr | 49 +++++++++++++++++++++++++++++++------------------
> 1 file changed, 31 insertions(+), 18 deletions(-)
>
> diff --git a/common/attr b/common/attr
> index 20049de0..c60cb6ed 100644
> --- a/common/attr
> +++ b/common/attr
> @@ -175,30 +175,43 @@ _list_acl()
>
> _require_attrs()
> {
> + local args
> + local nsp
> +
> + if [ $# -eq 0 ];
> + then

We prefer the following coding style in fstests

if [ $# -eq 0 ]; then
args="user"
else
args="$*"
fi

> + args="user"
> + else
> + args="$*"
> + fi

And you've almost re-written the whole _require_attrs(), it's better to
use tab as indention instead of 4 spaces (we're in the (very slow)
progress converting all 4-spaces indention to tab, except there're old
code using 4-spaces around).

> +
> [ -n "$ATTR_PROG" ] || _notrun "attr command not found"
> [ -n "$GETFATTR_PROG" ] || _notrun "getfattr command not found"
> [ -n "$SETFATTR_PROG" ] || _notrun "setfattr command not found"
>
> - #
> - # Test if chacl is able to write an attribute on the target filesystems.
> - # On really old kernels the system calls might not be implemented at all,
> - # but the more common case is that the tested filesystem simply doesn't
> - # support attributes. Note that we can't simply list attributes as
> - # various security modules generate synthetic attributes not actually
> - # stored on disk.
> - #
> - touch $TEST_DIR/syscalltest
> - attr -s "user.xfstests" -V "attr" $TEST_DIR/syscalltest > $TEST_DIR/syscalltest.out 2>&1
> - cat $TEST_DIR/syscalltest.out >> $seqres.full
> + for nsp in $args
> + do

Same here, use the format below

for nsp in $args; do
<do things here>
done

Thanks,
Eryu

> + #
> + # Test if chacl is able to write an attribute on the target filesystems.
> + # On really old kernels the system calls might not be implemented at all,
> + # but the more common case is that the tested filesystem simply doesn't
> + # support attributes. Note that we can't simply list attributes as
> + # various security modules generate synthetic attributes not actually
> + # stored on disk.
> + #
> + touch $TEST_DIR/syscalltest
> + $SETFATTR_PROG -n "$nsp.xfstests" -v "attr" $TEST_DIR/syscalltest > $TEST_DIR/syscalltest.out 2>&1
> + cat $TEST_DIR/syscalltest.out >> $seqres.full
>
> - if grep -q 'Function not implemented' $TEST_DIR/syscalltest.out; then
> - _notrun "kernel does not support attrs"
> - fi
> - if grep -q 'Operation not supported' $TEST_DIR/syscalltest.out; then
> - _notrun "attrs not supported by this filesystem type: $FSTYP"
> - fi
> + if grep -q 'Function not implemented' $TEST_DIR/syscalltest.out; then
> + _notrun "kernel does not support attrs"
> + fi
> + if grep -q 'Operation not supported' $TEST_DIR/syscalltest.out; then
> + _notrun "attr namespace $nsp not supported by this filesystem type: $FSTYP"
> + fi
>
> - rm -f $TEST_DIR/syscalltest.out
> + rm -f $TEST_DIR/syscalltest.out
> + done
> }
>
> _require_attr_v1()
> --
> 2.16.6