[PATCH] generic: new test to verify selinux label of whiteout inode

[PATCH] generic: new test to verify selinux label of whiteout inode

[Zorro Lang]

A but on XFS cause renameat2() with flags=RENAME_WHITEOUT doesn't
apply an selinux label. That's quite different with other fs (e.g.
ext4, tmpfs).

Signed-off-by: Zorro Lang <zlang@kernel.org>
---

Hi,

A test case for:
https://lore.kernel.org/linux-xfs/1655775516-8936-1-git-send-email-sandeen@redhat.com/

The patch has been reviewed, but not merged, so there's not commit ID, just send
this patch out to get review at first.

Thanks,
Zorro

 tests/generic/692     | 64 +++++++++++++++++++++++++++++++++++++++++++
 tests/generic/692.out |  2 ++
 2 files changed, 66 insertions(+)
 create mode 100755 tests/generic/692
 create mode 100644 tests/generic/692.out

diff --git a/tests/generic/692 b/tests/generic/692
new file mode 100755
index 00000000..ccf2213d
--- /dev/null
+++ b/tests/generic/692
@@ -0,0 +1,64 @@
+#! /bin/bash
+# SPDX-License-Identifier: GPL-2.0
+# Copyright (c) 2022 Red Hat, Copyright.  All Rights Reserved.
+#
+# FS QA Test 692
+#
+# Verify selinux label can be kept after RENAME_WHITEOUT. This is
+# a regression test for:
+#   XXXXXXXXXXXX ("xfs: add selinux labels to whiteout inodes")
+#
+. ./common/preamble
+_begin_fstest auto quick rename attr
+
+# Import common functions.
+. ./common/attr
+. ./common/renameat2
+
+# real QA test starts here
+_supported_fs generic
+_require_scratch
+_require_attrs
+_require_renameat2 whiteout
+
+_fixed_by_kernel_commit XXXXXXXXXXXX \
+	xfs: add selinux labels to whiteout inodes
+
+get_selinux_label()
+{
+	local label
+
+	label=`_getfattr --absolute-names -n security.selinux $@ | sed -n 's/security.selinux=\"\(.*\)\"/\1/p'`
+	if [ ${PIPESTATUS[0]} -ne 0 -o -z "$label" ];then
+		_fail "Fail to get selinux label: $label"
+	fi
+	echo $label
+}
+
+_scratch_mkfs >> $seqres.full 2>&1
+# SELINUX_MOUNT_OPTIONS will be set in common/config if selinux is enabled
+if [ -z "$SELINUX_MOUNT_OPTIONS" ]; then
+	_notrun "Require selinux to be enabled"
+fi
+# This test need to verify selinux labels in objects, so unset this selinux
+# mount option
+export SELINUX_MOUNT_OPTIONS=""
+_scratch_mount
+
+touch $SCRATCH_MNT/f1
+echo "Before RENAME_WHITEOUT" >> $seqres.full
+ls -lZ $SCRATCH_MNT >> $seqres.full 2>&1
+# Expect f1 and f2 have same label after RENAME_WHITEOUT
+$here/src/renameat2 -w $SCRATCH_MNT/f1 $SCRATCH_MNT/f2
+echo "After RENAME_WHITEOUT" >> $seqres.full
+ls -lZ $SCRATCH_MNT >> $seqres.full 2>&1
+label1=`get_selinux_label $SCRATCH_MNT/f1`
+label2=`get_selinux_label $SCRATCH_MNT/f2`
+if [ "$label1" != "$label2" ];then
+	echo "$label1 != $label2"
+fi
+
+echo "Silence is golden"
+# success, all done
+status=0
+exit
diff --git a/tests/generic/692.out b/tests/generic/692.out
new file mode 100644
index 00000000..d7521a9f
--- /dev/null
+++ b/tests/generic/692.out
@@ -0,0 +1,2 @@
+QA output created by 692
+Silence is golden
-- 
2.31.1

Re: [PATCH] generic: new test to verify selinux label of whiteout inode

[Amir Goldstein]

On Sun, Jul 24, 2022 at 5:07 PM Zorro Lang <zlang@kernel.org> wrote:
>
> On Thu, Jul 14, 2022 at 06:30:10PM +0200, Amir Goldstein wrote:
> >    Sorry for off list email
> >    Please add to whiteout group
>
> Oh, sure, thanks!
>
> The "whiteout" group name is described as (in doc/group-names.txt):
>
>   whiteout                overlayfs whiteout functionality
>
> I think it's not an overlayfs specific group, how about remove the
> "overlayfs" prefix?

The whiteout inode is a vfs construct that was introduced for overlayfs:

       RENAME_WHITEOUT (since Linux 3.18)
              This operation makes sense only for overlay/union filesystem
              implementations.

There is no other known usage for whiteout inode.
You could remove the "overlayfs" prefix, but that would just make the
whiteout context less clear IMO.

>
> BTW, is there anything wrong (about secret?) with this test case, to make
> you have to off list email. This looks like a normal review point ;)
>

Nope, just a technical issue when replying from my mobile.

Thanks,
Amir.

[PATCH v2] generic: new test to verify selinux label of whiteout inode

[Zorro Lang]

A but on XFS cause renameat2() with flags=RENAME_WHITEOUT doesn't
apply an selinux label. That's quite different with other fs (e.g.
ext4, tmpfs).

Signed-off-by: Zorro Lang <zlang@kernel.org>
---

Thanks the review points from Amir, this v2 did below changes:
1) Add "whiteout" group
2) Add commit ID from xfs-linux xfs-5.20-merge-2 (will change if need)
3) Rebase to latest fstests for-next branch

Thanks,
Zorro

 tests/generic/693     | 64 +++++++++++++++++++++++++++++++++++++++++++
 tests/generic/693.out |  2 ++
 2 files changed, 66 insertions(+)
 create mode 100755 tests/generic/693
 create mode 100644 tests/generic/693.out

diff --git a/tests/generic/693 b/tests/generic/693
new file mode 100755
index 00000000..adf191c4
--- /dev/null
+++ b/tests/generic/693
@@ -0,0 +1,64 @@
+#! /bin/bash
+# SPDX-License-Identifier: GPL-2.0
+# Copyright (c) 2022 Red Hat, Copyright.  All Rights Reserved.
+#
+# FS QA Test No. 693
+#
+# Verify selinux label can be kept after RENAME_WHITEOUT. This is
+# a regression test for:
+#   70b589a37e1a ("xfs: add selinux labels to whiteout inodes")
+#
+. ./common/preamble
+_begin_fstest auto quick rename attr whiteout
+
+# Import common functions.
+. ./common/attr
+. ./common/renameat2
+
+# real QA test starts here
+_supported_fs generic
+_require_scratch
+_require_attrs
+_require_renameat2 whiteout
+
+_fixed_by_kernel_commit 70b589a37e1a \
+	xfs: add selinux labels to whiteout inodes
+
+get_selinux_label()
+{
+	local label
+
+	label=`_getfattr --absolute-names -n security.selinux $@ | sed -n 's/security.selinux=\"\(.*\)\"/\1/p'`
+	if [ ${PIPESTATUS[0]} -ne 0 -o -z "$label" ];then
+		_fail "Fail to get selinux label: $label"
+	fi
+	echo $label
+}
+
+_scratch_mkfs >> $seqres.full 2>&1
+# SELINUX_MOUNT_OPTIONS will be set in common/config if selinux is enabled
+if [ -z "$SELINUX_MOUNT_OPTIONS" ]; then
+	_notrun "Require selinux to be enabled"
+fi
+# This test need to verify selinux labels in objects, so unset this selinux
+# mount option
+export SELINUX_MOUNT_OPTIONS=""
+_scratch_mount
+
+touch $SCRATCH_MNT/f1
+echo "Before RENAME_WHITEOUT" >> $seqres.full
+ls -lZ $SCRATCH_MNT >> $seqres.full 2>&1
+# Expect f1 and f2 have same label after RENAME_WHITEOUT
+$here/src/renameat2 -w $SCRATCH_MNT/f1 $SCRATCH_MNT/f2
+echo "After RENAME_WHITEOUT" >> $seqres.full
+ls -lZ $SCRATCH_MNT >> $seqres.full 2>&1
+label1=`get_selinux_label $SCRATCH_MNT/f1`
+label2=`get_selinux_label $SCRATCH_MNT/f2`
+if [ "$label1" != "$label2" ];then
+	echo "$label1 != $label2"
+fi
+
+echo "Silence is golden"
+# success, all done
+status=0
+exit
diff --git a/tests/generic/693.out b/tests/generic/693.out
new file mode 100644
index 00000000..01884ea5
--- /dev/null
+++ b/tests/generic/693.out
@@ -0,0 +1,2 @@
+QA output created by 693
+Silence is golden
-- 
2.31.1

Re: [PATCH v2] generic: new test to verify selinux label of whiteout inode

[Zorro Lang]

On Mon, Jul 25, 2022 at 02:13:27PM +0800, Zorro Lang wrote:
> A but on XFS cause renameat2() with flags=RENAME_WHITEOUT doesn't
> apply an selinux label. That's quite different with other fs (e.g.
> ext4, tmpfs).
> 
> Signed-off-by: Zorro Lang <zlang@kernel.org>
> ---

Ping, any review poings for this patch? The bug fix has been merged into
mainline kernel:
  70b589a37e1a ("xfs: add selinux labels to whiteout inodes")

Thanks,
Zorro

> 
> Thanks the review points from Amir, this v2 did below changes:
> 1) Add "whiteout" group
> 2) Add commit ID from xfs-linux xfs-5.20-merge-2 (will change if need)
> 3) Rebase to latest fstests for-next branch
> 
> Thanks,
> Zorro
> 
>  tests/generic/693     | 64 +++++++++++++++++++++++++++++++++++++++++++
>  tests/generic/693.out |  2 ++
>  2 files changed, 66 insertions(+)
>  create mode 100755 tests/generic/693
>  create mode 100644 tests/generic/693.out
> 
> diff --git a/tests/generic/693 b/tests/generic/693
> new file mode 100755
> index 00000000..adf191c4
> --- /dev/null
> +++ b/tests/generic/693
> @@ -0,0 +1,64 @@
> +#! /bin/bash
> +# SPDX-License-Identifier: GPL-2.0
> +# Copyright (c) 2022 Red Hat, Copyright.  All Rights Reserved.
> +#
> +# FS QA Test No. 693
> +#
> +# Verify selinux label can be kept after RENAME_WHITEOUT. This is
> +# a regression test for:
> +#   70b589a37e1a ("xfs: add selinux labels to whiteout inodes")
> +#
> +. ./common/preamble
> +_begin_fstest auto quick rename attr whiteout
> +
> +# Import common functions.
> +. ./common/attr
> +. ./common/renameat2
> +
> +# real QA test starts here
> +_supported_fs generic
> +_require_scratch
> +_require_attrs
> +_require_renameat2 whiteout
> +
> +_fixed_by_kernel_commit 70b589a37e1a \
> +	xfs: add selinux labels to whiteout inodes
> +
> +get_selinux_label()
> +{
> +	local label
> +
> +	label=`_getfattr --absolute-names -n security.selinux $@ | sed -n 's/security.selinux=\"\(.*\)\"/\1/p'`
> +	if [ ${PIPESTATUS[0]} -ne 0 -o -z "$label" ];then
> +		_fail "Fail to get selinux label: $label"
> +	fi
> +	echo $label
> +}
> +
> +_scratch_mkfs >> $seqres.full 2>&1
> +# SELINUX_MOUNT_OPTIONS will be set in common/config if selinux is enabled
> +if [ -z "$SELINUX_MOUNT_OPTIONS" ]; then
> +	_notrun "Require selinux to be enabled"
> +fi
> +# This test need to verify selinux labels in objects, so unset this selinux
> +# mount option
> +export SELINUX_MOUNT_OPTIONS=""
> +_scratch_mount
> +
> +touch $SCRATCH_MNT/f1
> +echo "Before RENAME_WHITEOUT" >> $seqres.full
> +ls -lZ $SCRATCH_MNT >> $seqres.full 2>&1
> +# Expect f1 and f2 have same label after RENAME_WHITEOUT
> +$here/src/renameat2 -w $SCRATCH_MNT/f1 $SCRATCH_MNT/f2
> +echo "After RENAME_WHITEOUT" >> $seqres.full
> +ls -lZ $SCRATCH_MNT >> $seqres.full 2>&1
> +label1=`get_selinux_label $SCRATCH_MNT/f1`
> +label2=`get_selinux_label $SCRATCH_MNT/f2`
> +if [ "$label1" != "$label2" ];then
> +	echo "$label1 != $label2"
> +fi
> +
> +echo "Silence is golden"
> +# success, all done
> +status=0
> +exit
> diff --git a/tests/generic/693.out b/tests/generic/693.out
> new file mode 100644
> index 00000000..01884ea5
> --- /dev/null
> +++ b/tests/generic/693.out
> @@ -0,0 +1,2 @@
> +QA output created by 693
> +Silence is golden
> -- 
> 2.31.1
> 

Re: [PATCH v2] generic: new test to verify selinux label of whiteout inode

[Darrick J. Wong]

On Mon, Jul 25, 2022 at 02:13:27PM +0800, Zorro Lang wrote:
> A but on XFS cause renameat2() with flags=RENAME_WHITEOUT doesn't
> apply an selinux label. That's quite different with other fs (e.g.
> ext4, tmpfs).
> 
> Signed-off-by: Zorro Lang <zlang@kernel.org>
> ---
> 
> Thanks the review points from Amir, this v2 did below changes:
> 1) Add "whiteout" group
> 2) Add commit ID from xfs-linux xfs-5.20-merge-2 (will change if need)
> 3) Rebase to latest fstests for-next branch
> 
> Thanks,
> Zorro
> 
>  tests/generic/693     | 64 +++++++++++++++++++++++++++++++++++++++++++
>  tests/generic/693.out |  2 ++
>  2 files changed, 66 insertions(+)
>  create mode 100755 tests/generic/693
>  create mode 100644 tests/generic/693.out
> 
> diff --git a/tests/generic/693 b/tests/generic/693
> new file mode 100755
> index 00000000..adf191c4
> --- /dev/null
> +++ b/tests/generic/693
> @@ -0,0 +1,64 @@
> +#! /bin/bash
> +# SPDX-License-Identifier: GPL-2.0
> +# Copyright (c) 2022 Red Hat, Copyright.  All Rights Reserved.
> +#
> +# FS QA Test No. 693
> +#
> +# Verify selinux label can be kept after RENAME_WHITEOUT. This is
> +# a regression test for:
> +#   70b589a37e1a ("xfs: add selinux labels to whiteout inodes")
> +#
> +. ./common/preamble
> +_begin_fstest auto quick rename attr whiteout
> +
> +# Import common functions.
> +. ./common/attr
> +. ./common/renameat2
> +
> +# real QA test starts here
> +_supported_fs generic
> +_require_scratch
> +_require_attrs
> +_require_renameat2 whiteout
> +
> +_fixed_by_kernel_commit 70b589a37e1a \
> +	xfs: add selinux labels to whiteout inodes
> +
> +get_selinux_label()
> +{
> +	local label
> +
> +	label=`_getfattr --absolute-names -n security.selinux $@ | sed -n 's/security.selinux=\"\(.*\)\"/\1/p'`
> +	if [ ${PIPESTATUS[0]} -ne 0 -o -z "$label" ];then
> +		_fail "Fail to get selinux label: $label"
> +	fi
> +	echo $label
> +}
> +
> +_scratch_mkfs >> $seqres.full 2>&1
> +# SELINUX_MOUNT_OPTIONS will be set in common/config if selinux is enabled
> +if [ -z "$SELINUX_MOUNT_OPTIONS" ]; then
> +	_notrun "Require selinux to be enabled"
> +fi
> +# This test need to verify selinux labels in objects, so unset this selinux
> +# mount option
> +export SELINUX_MOUNT_OPTIONS=""
> +_scratch_mount
> +
> +touch $SCRATCH_MNT/f1
> +echo "Before RENAME_WHITEOUT" >> $seqres.full
> +ls -lZ $SCRATCH_MNT >> $seqres.full 2>&1
> +# Expect f1 and f2 have same label after RENAME_WHITEOUT
> +$here/src/renameat2 -w $SCRATCH_MNT/f1 $SCRATCH_MNT/f2
> +echo "After RENAME_WHITEOUT" >> $seqres.full
> +ls -lZ $SCRATCH_MNT >> $seqres.full 2>&1
> +label1=`get_selinux_label $SCRATCH_MNT/f1`
> +label2=`get_selinux_label $SCRATCH_MNT/f2`

The operations of this test look ok to me, but the piece I do not know
is the higher level context of whether or not it's appropriate for
whiteout inodes to have selinux labels, or if the selinux developers
even care.  Perhaps they should be cc'd?  (And maybe I should've made
Eric do that for the kernel patch...sigh...)

--D

> +if [ "$label1" != "$label2" ];then
> +	echo "$label1 != $label2"
> +fi
> +
> +echo "Silence is golden"
> +# success, all done
> +status=0
> +exit
> diff --git a/tests/generic/693.out b/tests/generic/693.out
> new file mode 100644
> index 00000000..01884ea5
> --- /dev/null
> +++ b/tests/generic/693.out
> @@ -0,0 +1,2 @@
> +QA output created by 693
> +Silence is golden
> -- 
> 2.31.1
> 

Re: [PATCH v2] generic: new test to verify selinux label of whiteout inode

[Zorro Lang]

On Tue, Aug 23, 2022 at 07:43:54AM -0700, Darrick J. Wong wrote:
> On Mon, Jul 25, 2022 at 02:13:27PM +0800, Zorro Lang wrote:
> > A but on XFS cause renameat2() with flags=RENAME_WHITEOUT doesn't
> > apply an selinux label. That's quite different with other fs (e.g.
> > ext4, tmpfs).
> > 
> > Signed-off-by: Zorro Lang <zlang@kernel.org>
> > ---
> > 
> > Thanks the review points from Amir, this v2 did below changes:
> > 1) Add "whiteout" group
> > 2) Add commit ID from xfs-linux xfs-5.20-merge-2 (will change if need)
> > 3) Rebase to latest fstests for-next branch
> > 
> > Thanks,
> > Zorro
> > 
> >  tests/generic/693     | 64 +++++++++++++++++++++++++++++++++++++++++++
> >  tests/generic/693.out |  2 ++
> >  2 files changed, 66 insertions(+)
> >  create mode 100755 tests/generic/693
> >  create mode 100644 tests/generic/693.out
> > 
> > diff --git a/tests/generic/693 b/tests/generic/693
> > new file mode 100755
> > index 00000000..adf191c4
> > --- /dev/null
> > +++ b/tests/generic/693
> > @@ -0,0 +1,64 @@
> > +#! /bin/bash
> > +# SPDX-License-Identifier: GPL-2.0
> > +# Copyright (c) 2022 Red Hat, Copyright.  All Rights Reserved.
> > +#
> > +# FS QA Test No. 693
> > +#
> > +# Verify selinux label can be kept after RENAME_WHITEOUT. This is
> > +# a regression test for:
> > +#   70b589a37e1a ("xfs: add selinux labels to whiteout inodes")
> > +#
> > +. ./common/preamble
> > +_begin_fstest auto quick rename attr whiteout
> > +
> > +# Import common functions.
> > +. ./common/attr
> > +. ./common/renameat2
> > +
> > +# real QA test starts here
> > +_supported_fs generic
> > +_require_scratch
> > +_require_attrs
> > +_require_renameat2 whiteout
> > +
> > +_fixed_by_kernel_commit 70b589a37e1a \
> > +	xfs: add selinux labels to whiteout inodes
> > +
> > +get_selinux_label()
> > +{
> > +	local label
> > +
> > +	label=`_getfattr --absolute-names -n security.selinux $@ | sed -n 's/security.selinux=\"\(.*\)\"/\1/p'`
> > +	if [ ${PIPESTATUS[0]} -ne 0 -o -z "$label" ];then
> > +		_fail "Fail to get selinux label: $label"
> > +	fi
> > +	echo $label
> > +}
> > +
> > +_scratch_mkfs >> $seqres.full 2>&1
> > +# SELINUX_MOUNT_OPTIONS will be set in common/config if selinux is enabled
> > +if [ -z "$SELINUX_MOUNT_OPTIONS" ]; then
> > +	_notrun "Require selinux to be enabled"
> > +fi
> > +# This test need to verify selinux labels in objects, so unset this selinux
> > +# mount option
> > +export SELINUX_MOUNT_OPTIONS=""
> > +_scratch_mount
> > +
> > +touch $SCRATCH_MNT/f1
> > +echo "Before RENAME_WHITEOUT" >> $seqres.full
> > +ls -lZ $SCRATCH_MNT >> $seqres.full 2>&1
> > +# Expect f1 and f2 have same label after RENAME_WHITEOUT
> > +$here/src/renameat2 -w $SCRATCH_MNT/f1 $SCRATCH_MNT/f2
> > +echo "After RENAME_WHITEOUT" >> $seqres.full
> > +ls -lZ $SCRATCH_MNT >> $seqres.full 2>&1
> > +label1=`get_selinux_label $SCRATCH_MNT/f1`
> > +label2=`get_selinux_label $SCRATCH_MNT/f2`
> 
> The operations of this test look ok to me, but the piece I do not know
> is the higher level context of whether or not it's appropriate for
> whiteout inodes to have selinux labels, or if the selinux developers
> even care.  Perhaps they should be cc'd?  (And maybe I should've made
> Eric do that for the kernel patch...sigh...)

Hi Eric,

May you help to review this patch :)

Thanks,
Zorro

> 
> --D
> 
> > +if [ "$label1" != "$label2" ];then
> > +	echo "$label1 != $label2"
> > +fi
> > +
> > +echo "Silence is golden"
> > +# success, all done
> > +status=0
> > +exit
> > diff --git a/tests/generic/693.out b/tests/generic/693.out
> > new file mode 100644
> > index 00000000..01884ea5
> > --- /dev/null
> > +++ b/tests/generic/693.out
> > @@ -0,0 +1,2 @@
> > +QA output created by 693
> > +Silence is golden
> > -- 
> > 2.31.1
> > 
>