diff mbox series

[1/2] xfs/019: fix golden output for files created in setgid dir

Message ID 165038952072.1677615.13209407698123810165.stgit@magnolia (mailing list archive)
State New, archived
Headers show
Series fstests: random fixes | expand

Commit Message

Darrick J. Wong April 19, 2022, 5:32 p.m. UTC 
From: Darrick J. Wong <djwong@kernel.org>

A recent change to xfs/019 exposed a long-standing bug in mkfs where
it would always set the gid of a new child created in a setgid directory
to match the gid parent directory instead of what's in the protofile.

Ignoring the user's directions is not the correct behavior, so update
this test to reflect that.  Also don't erase the $seqres.full file,
because that makes forensic analysis pointlessly difficult.

Cc: Catherine Hoang <catherine.hoang@oracle.com>
Fixes: 7834a740 ("xfs/019: extend protofile test")
Signed-off-by: Darrick J. Wong <djwong@kernel.org>
---
 tests/xfs/019     |    3 +--
 tests/xfs/019.out |    2 +-
 2 files changed, 2 insertions(+), 3 deletions(-)

Comments

Zorro Lang April 19, 2022, 6:33 p.m. UTC | #1 
On Tue, Apr 19, 2022 at 10:32:00AM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@kernel.org>
> 
> A recent change to xfs/019 exposed a long-standing bug in mkfs where
> it would always set the gid of a new child created in a setgid directory
> to match the gid parent directory instead of what's in the protofile.
> 
> Ignoring the user's directions is not the correct behavior, so update
> this test to reflect that.  Also don't erase the $seqres.full file,
> because that makes forensic analysis pointlessly difficult.
> 
> Cc: Catherine Hoang <catherine.hoang@oracle.com>
> Fixes: 7834a740 ("xfs/019: extend protofile test")
> Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> ---

After merge this patch, xfs/019 fails on my system. So this test will cover
a new bug of xfsprogs which hasn't been fixed? If so, this change is good to me.
But we'd better to take a look at the patch you used to fix xfsprogs, and make
sure it's reviewed.

Thanks,
Zorro

>  tests/xfs/019     |    3 +--
>  tests/xfs/019.out |    2 +-
>  2 files changed, 2 insertions(+), 3 deletions(-)
> 
> 
> diff --git a/tests/xfs/019 b/tests/xfs/019
> index 535b7af1..790a6821 100755
> --- a/tests/xfs/019
> +++ b/tests/xfs/019
> @@ -10,6 +10,7 @@
>  _begin_fstest mkfs auto quick
>  
>  seqfull="$seqres.full"
> +rm -f $seqfull
>  # Import common functions.
>  . ./common/filter
>  
> @@ -97,7 +98,6 @@ _verify_fs()
>  	echo "*** create FS version $1"
>  	VERSION="-n version=$1"
>  
> -	rm -f $seqfull
>  	_scratch_unmount >/dev/null 2>&1
>  
>  	_full "mkfs"
> @@ -131,6 +131,5 @@ _verify_fs()
>  _verify_fs 2
>  
>  echo "*** done"
> -rm $seqfull
>  status=0
>  exit
> diff --git a/tests/xfs/019.out b/tests/xfs/019.out
> index 8584f593..9db157f9 100644
> --- a/tests/xfs/019.out
> +++ b/tests/xfs/019.out
> @@ -61,7 +61,7 @@ Device: <DEVICE> Inode: <INODE> Links: 2
>  
>   File: "./directory_setgid/file_xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx_5"
>   Size: 5 Filetype: Regular File
> - Mode: (0755/-rwxr-xr-x) Uid: (3) Gid: (2)
> + Mode: (0755/-rwxr-xr-x) Uid: (3) Gid: (1)
>  Device: <DEVICE> Inode: <INODE> Links: 1 
>  
>   File: "./pipe"
>
Darrick J. Wong April 19, 2022, 6:38 p.m. UTC | #2 
On Wed, Apr 20, 2022 at 02:33:47AM +0800, Zorro Lang wrote:
> On Tue, Apr 19, 2022 at 10:32:00AM -0700, Darrick J. Wong wrote:
> > From: Darrick J. Wong <djwong@kernel.org>
> > 
> > A recent change to xfs/019 exposed a long-standing bug in mkfs where
> > it would always set the gid of a new child created in a setgid directory
> > to match the gid parent directory instead of what's in the protofile.
> > 
> > Ignoring the user's directions is not the correct behavior, so update
> > this test to reflect that.  Also don't erase the $seqres.full file,
> > because that makes forensic analysis pointlessly difficult.
> > 
> > Cc: Catherine Hoang <catherine.hoang@oracle.com>
> > Fixes: 7834a740 ("xfs/019: extend protofile test")
> > Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> > ---
> 
> After merge this patch, xfs/019 fails on my system. So this test will cover
> a new bug of xfsprogs which hasn't been fixed? If so, this change is good to me.
> But we'd better to take a look at the patch you used to fix xfsprogs, and make
> sure it's reviewed.

Yep.  Already reviewed, just waiting for xfsprogs 5.15.1:

https://lore.kernel.org/linux-xfs/B28D1D24-2E23-4F60-AA50-C199392BBE4F@oracle.com/T/#u

--D

> 
> Thanks,
> Zorro
> 
> >  tests/xfs/019     |    3 +--
> >  tests/xfs/019.out |    2 +-
> >  2 files changed, 2 insertions(+), 3 deletions(-)
> > 
> > 
> > diff --git a/tests/xfs/019 b/tests/xfs/019
> > index 535b7af1..790a6821 100755
> > --- a/tests/xfs/019
> > +++ b/tests/xfs/019
> > @@ -10,6 +10,7 @@
> >  _begin_fstest mkfs auto quick
> >  
> >  seqfull="$seqres.full"
> > +rm -f $seqfull
> >  # Import common functions.
> >  . ./common/filter
> >  
> > @@ -97,7 +98,6 @@ _verify_fs()
> >  	echo "*** create FS version $1"
> >  	VERSION="-n version=$1"
> >  
> > -	rm -f $seqfull
> >  	_scratch_unmount >/dev/null 2>&1
> >  
> >  	_full "mkfs"
> > @@ -131,6 +131,5 @@ _verify_fs()
> >  _verify_fs 2
> >  
> >  echo "*** done"
> > -rm $seqfull
> >  status=0
> >  exit
> > diff --git a/tests/xfs/019.out b/tests/xfs/019.out
> > index 8584f593..9db157f9 100644
> > --- a/tests/xfs/019.out
> > +++ b/tests/xfs/019.out
> > @@ -61,7 +61,7 @@ Device: <DEVICE> Inode: <INODE> Links: 2
> >  
> >   File: "./directory_setgid/file_xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx_5"
> >   Size: 5 Filetype: Regular File
> > - Mode: (0755/-rwxr-xr-x) Uid: (3) Gid: (2)
> > + Mode: (0755/-rwxr-xr-x) Uid: (3) Gid: (1)
> >  Device: <DEVICE> Inode: <INODE> Links: 1 
> >  
> >   File: "./pipe"
> > 
>
Zorro Lang April 20, 2022, 2:47 p.m. UTC | #3 
On Tue, Apr 19, 2022 at 11:38:55AM -0700, Darrick J. Wong wrote:
> On Wed, Apr 20, 2022 at 02:33:47AM +0800, Zorro Lang wrote:
> > On Tue, Apr 19, 2022 at 10:32:00AM -0700, Darrick J. Wong wrote:
> > > From: Darrick J. Wong <djwong@kernel.org>
> > > 
> > > A recent change to xfs/019 exposed a long-standing bug in mkfs where
> > > it would always set the gid of a new child created in a setgid directory
> > > to match the gid parent directory instead of what's in the protofile.
> > > 
> > > Ignoring the user's directions is not the correct behavior, so update
> > > this test to reflect that.  Also don't erase the $seqres.full file,
> > > because that makes forensic analysis pointlessly difficult.
> > > 
> > > Cc: Catherine Hoang <catherine.hoang@oracle.com>
> > > Fixes: 7834a740 ("xfs/019: extend protofile test")
> > > Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> > > ---
> > 
> > After merge this patch, xfs/019 fails on my system. So this test will cover
> > a new bug of xfsprogs which hasn't been fixed? If so, this change is good to me.
> > But we'd better to take a look at the patch you used to fix xfsprogs, and make
> > sure it's reviewed.
> 
> Yep.  Already reviewed, just waiting for xfsprogs 5.15.1:
> 
> https://lore.kernel.org/linux-xfs/B28D1D24-2E23-4F60-AA50-C199392BBE4F@oracle.com/T/#u

Thanks for this information, it makes sense to me.

Reviewed-by: Zorro Lang <zlang@redhat.com>

> 
> --D
> 
> > 
> > Thanks,
> > Zorro
> > 
> > >  tests/xfs/019     |    3 +--
> > >  tests/xfs/019.out |    2 +-
> > >  2 files changed, 2 insertions(+), 3 deletions(-)
> > > 
> > > 
> > > diff --git a/tests/xfs/019 b/tests/xfs/019
> > > index 535b7af1..790a6821 100755
> > > --- a/tests/xfs/019
> > > +++ b/tests/xfs/019
> > > @@ -10,6 +10,7 @@
> > >  _begin_fstest mkfs auto quick
> > >  
> > >  seqfull="$seqres.full"
> > > +rm -f $seqfull
> > >  # Import common functions.
> > >  . ./common/filter
> > >  
> > > @@ -97,7 +98,6 @@ _verify_fs()
> > >  	echo "*** create FS version $1"
> > >  	VERSION="-n version=$1"
> > >  
> > > -	rm -f $seqfull
> > >  	_scratch_unmount >/dev/null 2>&1
> > >  
> > >  	_full "mkfs"
> > > @@ -131,6 +131,5 @@ _verify_fs()
> > >  _verify_fs 2
> > >  
> > >  echo "*** done"
> > > -rm $seqfull
> > >  status=0
> > >  exit
> > > diff --git a/tests/xfs/019.out b/tests/xfs/019.out
> > > index 8584f593..9db157f9 100644
> > > --- a/tests/xfs/019.out
> > > +++ b/tests/xfs/019.out
> > > @@ -61,7 +61,7 @@ Device: <DEVICE> Inode: <INODE> Links: 2
> > >  
> > >   File: "./directory_setgid/file_xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx_5"
> > >   Size: 5 Filetype: Regular File
> > > - Mode: (0755/-rwxr-xr-x) Uid: (3) Gid: (2)
> > > + Mode: (0755/-rwxr-xr-x) Uid: (3) Gid: (1)
> > >  Device: <DEVICE> Inode: <INODE> Links: 1 
> > >  
> > >   File: "./pipe"
> > > 
> > 
>
Catherine Hoang April 20, 2022, 4:03 p.m. UTC | #4 
> On Apr 19, 2022, at 10:32 AM, Darrick J. Wong <djwong@kernel.org> wrote:
> 
> From: Darrick J. Wong <djwong@kernel.org>
> 
> A recent change to xfs/019 exposed a long-standing bug in mkfs where
> it would always set the gid of a new child created in a setgid directory
> to match the gid parent directory instead of what's in the protofile.
> 
> Ignoring the user's directions is not the correct behavior, so update
> this test to reflect that.  Also don't erase the $seqres.full file,
> because that makes forensic analysis pointlessly difficult.

Looks good
Reviewed-by: Catherine Hoang <catherine.hoang@oracle.com>
> 
> Cc: Catherine Hoang <catherine.hoang@oracle.com>
> Fixes: 7834a740 ("xfs/019: extend protofile test")
> Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> ---
> tests/xfs/019     |    3 +--
> tests/xfs/019.out |    2 +-
> 2 files changed, 2 insertions(+), 3 deletions(-)
> 
> 
> diff --git a/tests/xfs/019 b/tests/xfs/019
> index 535b7af1..790a6821 100755
> --- a/tests/xfs/019
> +++ b/tests/xfs/019
> @@ -10,6 +10,7 @@
> _begin_fstest mkfs auto quick
> 
> seqfull="$seqres.full"
> +rm -f $seqfull
> # Import common functions.
> . ./common/filter
> 
> @@ -97,7 +98,6 @@ _verify_fs()
> 	echo "*** create FS version $1"
> 	VERSION="-n version=$1"
> 
> -	rm -f $seqfull
> 	_scratch_unmount >/dev/null 2>&1
> 
> 	_full "mkfs"
> @@ -131,6 +131,5 @@ _verify_fs()
> _verify_fs 2
> 
> echo "*** done"
> -rm $seqfull
> status=0
> exit
> diff --git a/tests/xfs/019.out b/tests/xfs/019.out
> index 8584f593..9db157f9 100644
> --- a/tests/xfs/019.out
> +++ b/tests/xfs/019.out
> @@ -61,7 +61,7 @@ Device: <DEVICE> Inode: <INODE> Links: 2
> 
>  File: "./directory_setgid/file_xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx_5"
>  Size: 5 Filetype: Regular File
> - Mode: (0755/-rwxr-xr-x) Uid: (3) Gid: (2)
> + Mode: (0755/-rwxr-xr-x) Uid: (3) Gid: (1)
> Device: <DEVICE> Inode: <INODE> Links: 1 
> 
>  File: "./pipe"
>
diff mbox series

Patch

diff --git a/tests/xfs/019 b/tests/xfs/019
index 535b7af1..790a6821 100755
--- a/tests/xfs/019
+++ b/tests/xfs/019
@@ -10,6 +10,7 @@ 
 _begin_fstest mkfs auto quick
 
 seqfull="$seqres.full"
+rm -f $seqfull
 # Import common functions.
 . ./common/filter
 
@@ -97,7 +98,6 @@  _verify_fs()
 	echo "*** create FS version $1"
 	VERSION="-n version=$1"
 
-	rm -f $seqfull
 	_scratch_unmount >/dev/null 2>&1
 
 	_full "mkfs"
@@ -131,6 +131,5 @@  _verify_fs()
 _verify_fs 2
 
 echo "*** done"
-rm $seqfull
 status=0
 exit
diff --git a/tests/xfs/019.out b/tests/xfs/019.out
index 8584f593..9db157f9 100644
--- a/tests/xfs/019.out
+++ b/tests/xfs/019.out
@@ -61,7 +61,7 @@  Device: <DEVICE> Inode: <INODE> Links: 2
 
  File: "./directory_setgid/file_xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx_5"
  Size: 5 Filetype: Regular File
- Mode: (0755/-rwxr-xr-x) Uid: (3) Gid: (2)
+ Mode: (0755/-rwxr-xr-x) Uid: (3) Gid: (1)
 Device: <DEVICE> Inode: <INODE> Links: 1 
 
  File: "./pipe"