Re: [PATCH] ubifs: add test for race between listxattr and setxatr

[Zorro Lang <zlang@redhat.com>]

On Sat, Jan 08, 2022 at 10:24:31AM +0800, Chen Long wrote:
> Add reproducer for a bug on ubifs where listxattr() copies
> the newly created xattr names regardless of the remaining
> buffer size, fails the assertion of used buffer size, and
> may corrupt buffer memory.
> 
> This is a regression test for three kernel commit:
> 1. f4e3634a3b642 (ubifs: Fix races between xattr_{set|get} and
> 	listxattr operations)
> 2. 819f9ab430a44 (ubifs: Remove ui_mutex in ubifs_xattr_get and
> 	change_xattr)
> 
> Signed-off-by: Chen Long <chenlongcl.chen@huawei.com>
> ---
>  tests/ubifs/001      | 54 ++++++++++++++++++++++++++++++++++++++++++++
>  tests/ubifs/001.out  |  2 ++
>  tests/ubifs/Makefile | 24 ++++++++++++++++++++
>  3 files changed, 80 insertions(+)
>  create mode 100755 tests/ubifs/001
>  create mode 100644 tests/ubifs/001.out
>  create mode 100644 tests/ubifs/Makefile
> 
> diff --git a/tests/ubifs/001 b/tests/ubifs/001
> new file mode 100755
> index 00000000..1e9a4de2
> --- /dev/null
> +++ b/tests/ubifs/001
> @@ -0,0 +1,54 @@
> +#! /bin/bash
> +# SPDX-License-Identifier: GPL-2.0
> +# Copyright (c) 2022 Huawei.  All Rights Reserved.
> +#
> +# FS QA Test 001
> +#
> +# Regression test for kernel commit:
> +# 1. f4e3634a3b642 (ubifs: Fix races between xattr_{set|get} and
> +#	listxattr operations)
> +# 2. 819f9ab430a44 (ubifs: Remove ui_mutex in ubifs_xattr_get and
> +#	change_xattr)
> +#
> +. ./common/preamble
> +_begin_fstest auto quick attr
> +
> +_cleanup()
> +{
> +    cd /
> +    rm -r -f $tmp.*
> +}
> +
> +# Import common functions.
> +. ./common/filter
> +. ./common/attr
> +
> +# real QA test starts here
> +_supported_fs ubifs

From the whole test case, I don't think it's an ubifs special case. So we can
make it to be a generic case.

> +_require_test
> +_require_attrs
> +
> +target=$TEST_DIR/$seq
> +touch $target
> +
> +# start a background listxattr
> +runfile="$tmp.listxattr"
> +touch $runfile
> +while [ -e $runfile ]; do

We might can explain at here, that if run getfattr without "-n" option,
it uses listxattr system call, or it uses getxattr system call. It's
the code logic of getfattr.c

        if (opt_name)
                err = print_attribute(path, opt_name, &header_printed);
        else
                err = list_attributes(path, &header_printed);

> +    ${GETFATTR_PROG} $target >/dev/null 2>&1

JFYI: There's a wrapper of ${GETFATTR_PROG} _getfattr().
But if you filter all output of getfattr, I think it's fine to use
${GETFATTR_PROG} directly :)

> +done &
> +
> +# add new xattr continuously
> +largename=`for i in $(seq 0 128); do echo -n a; done`
> +for i in $(seq 0 99); do
> +    ${SETFATTR_PROG} -n user.${largename}.$i -v $i $target
> +done
> +
> +rm -f $runfile
> +wait > /dev/null 2>&1
> +rm -f $target
> +
> +echo "Silence is golden"
> +# success, all done
> +status=0
> +exit
> diff --git a/tests/ubifs/001.out b/tests/ubifs/001.out
> new file mode 100644
> index 00000000..88678b8e
> --- /dev/null
> +++ b/tests/ubifs/001.out
> @@ -0,0 +1,2 @@
> +QA output created by 001
> +Silence is golden
> diff --git a/tests/ubifs/Makefile b/tests/ubifs/Makefile
> new file mode 100644
> index 00000000..b464b22b
> --- /dev/null
> +++ b/tests/ubifs/Makefile
> @@ -0,0 +1,24 @@
> +#
> +# Copyright (c) 2003-2005 Silicon Graphics, Inc.  All Rights Reserved.
> +#
> +
> +TOPDIR = ../..
> +include $(TOPDIR)/include/builddefs
> +include $(TOPDIR)/include/buildgrouplist
> +
> +GENERIC_DIR = generic
> +TARGET_DIR = $(PKG_LIB_DIR)/$(TESTS_DIR)/$(GENERIC_DIR)
> +DIRT = group.list
> +
> +default: $(DIRT)
> +
> +include $(BUILDRULES)
> +
> +install:
> +	$(INSTALL) -m 755 -d $(TARGET_DIR)
> +	$(INSTALL) -m 755 $(TESTS) $(TARGET_DIR)
> +	$(INSTALL) -m 644 group.list $(TARGET_DIR)
> +	$(INSTALL) -m 644 $(OUTFILES) $(TARGET_DIR)
> +
> +# Nothing.
> +install-dev install-lib:

Make this case to be a generic case, will avoid doing this :)

> -- 
> 2.17.1
>