| Message ID | 20220317232408.202636-1-catherine.hoang@oracle.com (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [v1] xfs/019: extend protofile test | expand |
On Thu, Mar 17, 2022 at 11:24:08PM +0000, Catherine Hoang wrote: > This test creates an xfs filesystem and verifies that the filesystem > matches what is specified by the protofile. > > This patch extends the current test to check that a protofile can specify > setgid mode on directories. Also, check that the created symlink isn’t > broken. > > Signed-off-by: Catherine Hoang <catherine.hoang@oracle.com> > --- Any specific reason to add this test? Likes uncovering some one known bug/fix? Thanks, Zorro > tests/xfs/019 | 6 ++++++ > tests/xfs/019.out | 12 +++++++++++- > 2 files changed, 17 insertions(+), 1 deletion(-) > > diff --git a/tests/xfs/019 b/tests/xfs/019 > index 3dfd5408..535b7af1 100755 > --- a/tests/xfs/019 > +++ b/tests/xfs/019 > @@ -73,6 +73,10 @@ $ > setuid -u-666 0 0 $tempfile > setgid --g666 0 0 $tempfile > setugid -ug666 0 0 $tempfile > +directory_setgid d-g755 3 2 > +file_xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx_5 ---755 3 1 $tempfile > +$ > +: back in the root > block_device b--012 3 1 161 162 > char_device c--345 3 1 177 178 > pipe p--670 0 0 > @@ -114,6 +118,8 @@ _verify_fs() > | xargs $here/src/lstat64 | _filter_stat) > diff -q $SCRATCH_MNT/bigfile $tempfile.2 \ > || _fail "bigfile corrupted" > + diff -q $SCRATCH_MNT/symlink $tempfile.2 \ > + || _fail "symlink broken" > > echo "*** unmount FS" > _full "umount" > diff --git a/tests/xfs/019.out b/tests/xfs/019.out > index 19614d9d..8584f593 100644 > --- a/tests/xfs/019.out > +++ b/tests/xfs/019.out > @@ -7,7 +7,7 @@ Wrote 2048.00Kb (value 0x2c) > File: "." > Size: <DSIZE> Filetype: Directory > Mode: (0777/drwxrwxrwx) Uid: (3) Gid: (1) > -Device: <DEVICE> Inode: <INODE> Links: 3 > +Device: <DEVICE> Inode: <INODE> Links: 4 > > File: "./bigfile" > Size: 2097152 Filetype: Regular File > @@ -54,6 +54,16 @@ Device: <DEVICE> Inode: <INODE> Links: 1 > Mode: (0755/-rwxr-xr-x) Uid: (3) Gid: (1) > Device: <DEVICE> Inode: <INODE> Links: 1 > > + File: "./directory_setgid" > + Size: <DSIZE> Filetype: Directory > + Mode: (2755/drwxr-sr-x) Uid: (3) Gid: (2) > +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) > +Device: <DEVICE> Inode: <INODE> Links: 1 > + > File: "./pipe" > Size: 0 Filetype: Fifo File > Mode: (0670/frw-rwx---) Uid: (0) Gid: (0) > -- > 2.25.1 >
> On Mar 22, 2022, at 6:36 PM, Zorro Lang <zlang@redhat.com> wrote: > > On Thu, Mar 17, 2022 at 11:24:08PM +0000, Catherine Hoang wrote: >> This test creates an xfs filesystem and verifies that the filesystem >> matches what is specified by the protofile. >> >> This patch extends the current test to check that a protofile can specify >> setgid mode on directories. Also, check that the created symlink isn’t >> broken. >> >> Signed-off-by: Catherine Hoang <catherine.hoang@oracle.com> >> --- > > Any specific reason to add this test? Likes uncovering some one known > bug/fix? > > Thanks, > Zorro Hi Zorro, We’ve been exploring alternate uses for protofiles and noticed a few holes in the testing. Thanks, Catherine > >> tests/xfs/019 | 6 ++++++ >> tests/xfs/019.out | 12 +++++++++++- >> 2 files changed, 17 insertions(+), 1 deletion(-) >> >> diff --git a/tests/xfs/019 b/tests/xfs/019 >> index 3dfd5408..535b7af1 100755 >> --- a/tests/xfs/019 >> +++ b/tests/xfs/019 >> @@ -73,6 +73,10 @@ $ >> setuid -u-666 0 0 $tempfile >> setgid --g666 0 0 $tempfile >> setugid -ug666 0 0 $tempfile >> +directory_setgid d-g755 3 2 >> +file_xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx_5 ---755 3 1 $tempfile >> +$ >> +: back in the root >> block_device b--012 3 1 161 162 >> char_device c--345 3 1 177 178 >> pipe p--670 0 0 >> @@ -114,6 +118,8 @@ _verify_fs() >> | xargs $here/src/lstat64 | _filter_stat) >> diff -q $SCRATCH_MNT/bigfile $tempfile.2 \ >> || _fail "bigfile corrupted" >> + diff -q $SCRATCH_MNT/symlink $tempfile.2 \ >> + || _fail "symlink broken" >> >> echo "*** unmount FS" >> _full "umount" >> diff --git a/tests/xfs/019.out b/tests/xfs/019.out >> index 19614d9d..8584f593 100644 >> --- a/tests/xfs/019.out >> +++ b/tests/xfs/019.out >> @@ -7,7 +7,7 @@ Wrote 2048.00Kb (value 0x2c) >> File: "." >> Size: <DSIZE> Filetype: Directory >> Mode: (0777/drwxrwxrwx) Uid: (3) Gid: (1) >> -Device: <DEVICE> Inode: <INODE> Links: 3 >> +Device: <DEVICE> Inode: <INODE> Links: 4 >> >> File: "./bigfile" >> Size: 2097152 Filetype: Regular File >> @@ -54,6 +54,16 @@ Device: <DEVICE> Inode: <INODE> Links: 1 >> Mode: (0755/-rwxr-xr-x) Uid: (3) Gid: (1) >> Device: <DEVICE> Inode: <INODE> Links: 1 >> >> + File: "./directory_setgid" >> + Size: <DSIZE> Filetype: Directory >> + Mode: (2755/drwxr-sr-x) Uid: (3) Gid: (2) >> +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) >> +Device: <DEVICE> Inode: <INODE> Links: 1 >> + >> File: "./pipe" >> Size: 0 Filetype: Fifo File >> Mode: (0670/frw-rwx---) Uid: (0) Gid: (0) >> -- >> 2.25.1 >> >
On Thu, Mar 24, 2022 at 03:44:00PM +0000, Catherine Hoang wrote: > > On Mar 22, 2022, at 6:36 PM, Zorro Lang <zlang@redhat.com> wrote: > > > > On Thu, Mar 17, 2022 at 11:24:08PM +0000, Catherine Hoang wrote: > >> This test creates an xfs filesystem and verifies that the filesystem > >> matches what is specified by the protofile. > >> > >> This patch extends the current test to check that a protofile can specify > >> setgid mode on directories. Also, check that the created symlink isn’t > >> broken. > >> > >> Signed-off-by: Catherine Hoang <catherine.hoang@oracle.com> > >> --- > > > > Any specific reason to add this test? Likes uncovering some one known > > bug/fix? > > > > Thanks, > > Zorro > > Hi Zorro, > > We’ve been exploring alternate uses for protofiles and noticed a few holes > in the testing. That's great, but better to show some details in the patch/commit, likes a commit id of xfsprogs?/kernel? (if there's one) which fix the bug you metioned, to help others to know what's this change trying to cover. Thanks, Zorro > > Thanks, > Catherine > > > >> tests/xfs/019 | 6 ++++++ > >> tests/xfs/019.out | 12 +++++++++++- > >> 2 files changed, 17 insertions(+), 1 deletion(-) > >> > >> diff --git a/tests/xfs/019 b/tests/xfs/019 > >> index 3dfd5408..535b7af1 100755 > >> --- a/tests/xfs/019 > >> +++ b/tests/xfs/019 > >> @@ -73,6 +73,10 @@ $ > >> setuid -u-666 0 0 $tempfile > >> setgid --g666 0 0 $tempfile > >> setugid -ug666 0 0 $tempfile > >> +directory_setgid d-g755 3 2 > >> +file_xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx_5 ---755 3 1 $tempfile > >> +$ > >> +: back in the root > >> block_device b--012 3 1 161 162 > >> char_device c--345 3 1 177 178 > >> pipe p--670 0 0 > >> @@ -114,6 +118,8 @@ _verify_fs() > >> | xargs $here/src/lstat64 | _filter_stat) > >> diff -q $SCRATCH_MNT/bigfile $tempfile.2 \ > >> || _fail "bigfile corrupted" > >> + diff -q $SCRATCH_MNT/symlink $tempfile.2 \ > >> + || _fail "symlink broken" > >> > >> echo "*** unmount FS" > >> _full "umount" > >> diff --git a/tests/xfs/019.out b/tests/xfs/019.out > >> index 19614d9d..8584f593 100644 > >> --- a/tests/xfs/019.out > >> +++ b/tests/xfs/019.out > >> @@ -7,7 +7,7 @@ Wrote 2048.00Kb (value 0x2c) > >> File: "." > >> Size: <DSIZE> Filetype: Directory > >> Mode: (0777/drwxrwxrwx) Uid: (3) Gid: (1) > >> -Device: <DEVICE> Inode: <INODE> Links: 3 > >> +Device: <DEVICE> Inode: <INODE> Links: 4 > >> > >> File: "./bigfile" > >> Size: 2097152 Filetype: Regular File > >> @@ -54,6 +54,16 @@ Device: <DEVICE> Inode: <INODE> Links: 1 > >> Mode: (0755/-rwxr-xr-x) Uid: (3) Gid: (1) > >> Device: <DEVICE> Inode: <INODE> Links: 1 > >> > >> + File: "./directory_setgid" > >> + Size: <DSIZE> Filetype: Directory > >> + Mode: (2755/drwxr-sr-x) Uid: (3) Gid: (2) > >> +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) > >> +Device: <DEVICE> Inode: <INODE> Links: 1 > >> + > >> File: "./pipe" > >> Size: 0 Filetype: Fifo File > >> Mode: (0670/frw-rwx---) Uid: (0) Gid: (0) > >> -- > >> 2.25.1 > >> > > >
On Fri, Mar 25, 2022 at 03:26:00AM +0800, Zorro Lang wrote: > On Thu, Mar 24, 2022 at 03:44:00PM +0000, Catherine Hoang wrote: > > > On Mar 22, 2022, at 6:36 PM, Zorro Lang <zlang@redhat.com> wrote: > > > > > > On Thu, Mar 17, 2022 at 11:24:08PM +0000, Catherine Hoang wrote: > > >> This test creates an xfs filesystem and verifies that the filesystem > > >> matches what is specified by the protofile. > > >> > > >> This patch extends the current test to check that a protofile can specify > > >> setgid mode on directories. Also, check that the created symlink isn’t > > >> broken. > > >> > > >> Signed-off-by: Catherine Hoang <catherine.hoang@oracle.com> > > >> --- > > > > > > Any specific reason to add this test? Likes uncovering some one known > > > bug/fix? > > > > > > Thanks, > > > Zorro > > > > Hi Zorro, > > > > We’ve been exploring alternate uses for protofiles and noticed a few holes > > in the testing. > > That's great, but better to show some details in the patch/commit, likes > a commit id of xfsprogs?/kernel? (if there's one) which fix the bug you > metioned, to help others to know what's this change trying to cover. I think this patch is adding a check that protofile lines are actually being honored (in the case of the symlink file) and checking that setgid on a directory is not carried over into new children unless the protofile explicitly marks the children setgid. IOWs, this isn't adding a regression test for a fix in xfsprogs, it's increasing test coverage. --D > Thanks, > Zorro > > > > > Thanks, > > Catherine > > > > > >> tests/xfs/019 | 6 ++++++ > > >> tests/xfs/019.out | 12 +++++++++++- > > >> 2 files changed, 17 insertions(+), 1 deletion(-) > > >> > > >> diff --git a/tests/xfs/019 b/tests/xfs/019 > > >> index 3dfd5408..535b7af1 100755 > > >> --- a/tests/xfs/019 > > >> +++ b/tests/xfs/019 > > >> @@ -73,6 +73,10 @@ $ > > >> setuid -u-666 0 0 $tempfile > > >> setgid --g666 0 0 $tempfile > > >> setugid -ug666 0 0 $tempfile > > >> +directory_setgid d-g755 3 2 > > >> +file_xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx_5 ---755 3 1 $tempfile > > >> +$ > > >> +: back in the root > > >> block_device b--012 3 1 161 162 > > >> char_device c--345 3 1 177 178 > > >> pipe p--670 0 0 > > >> @@ -114,6 +118,8 @@ _verify_fs() > > >> | xargs $here/src/lstat64 | _filter_stat) > > >> diff -q $SCRATCH_MNT/bigfile $tempfile.2 \ > > >> || _fail "bigfile corrupted" > > >> + diff -q $SCRATCH_MNT/symlink $tempfile.2 \ > > >> + || _fail "symlink broken" > > >> > > >> echo "*** unmount FS" > > >> _full "umount" > > >> diff --git a/tests/xfs/019.out b/tests/xfs/019.out > > >> index 19614d9d..8584f593 100644 > > >> --- a/tests/xfs/019.out > > >> +++ b/tests/xfs/019.out > > >> @@ -7,7 +7,7 @@ Wrote 2048.00Kb (value 0x2c) > > >> File: "." > > >> Size: <DSIZE> Filetype: Directory > > >> Mode: (0777/drwxrwxrwx) Uid: (3) Gid: (1) > > >> -Device: <DEVICE> Inode: <INODE> Links: 3 > > >> +Device: <DEVICE> Inode: <INODE> Links: 4 > > >> > > >> File: "./bigfile" > > >> Size: 2097152 Filetype: Regular File > > >> @@ -54,6 +54,16 @@ Device: <DEVICE> Inode: <INODE> Links: 1 > > >> Mode: (0755/-rwxr-xr-x) Uid: (3) Gid: (1) > > >> Device: <DEVICE> Inode: <INODE> Links: 1 > > >> > > >> + File: "./directory_setgid" > > >> + Size: <DSIZE> Filetype: Directory > > >> + Mode: (2755/drwxr-sr-x) Uid: (3) Gid: (2) > > >> +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) > > >> +Device: <DEVICE> Inode: <INODE> Links: 1 > > >> + > > >> File: "./pipe" > > >> Size: 0 Filetype: Fifo File > > >> Mode: (0670/frw-rwx---) Uid: (0) Gid: (0) > > >> -- > > >> 2.25.1 > > >> > > > > > >
On Thu, Mar 24, 2022 at 01:17:30PM -0700, Darrick J. Wong wrote: > On Fri, Mar 25, 2022 at 03:26:00AM +0800, Zorro Lang wrote: > > On Thu, Mar 24, 2022 at 03:44:00PM +0000, Catherine Hoang wrote: > > > > On Mar 22, 2022, at 6:36 PM, Zorro Lang <zlang@redhat.com> wrote: > > > > > > > > On Thu, Mar 17, 2022 at 11:24:08PM +0000, Catherine Hoang wrote: > > > >> This test creates an xfs filesystem and verifies that the filesystem > > > >> matches what is specified by the protofile. > > > >> > > > >> This patch extends the current test to check that a protofile can specify > > > >> setgid mode on directories. Also, check that the created symlink isn’t > > > >> broken. > > > >> > > > >> Signed-off-by: Catherine Hoang <catherine.hoang@oracle.com> > > > >> --- > > > > > > > > Any specific reason to add this test? Likes uncovering some one known > > > > bug/fix? > > > > > > > > Thanks, > > > > Zorro > > > > > > Hi Zorro, > > > > > > We’ve been exploring alternate uses for protofiles and noticed a few holes > > > in the testing. > > > > That's great, but better to show some details in the patch/commit, likes > > a commit id of xfsprogs?/kernel? (if there's one) which fix the bug you > > metioned, to help others to know what's this change trying to cover. > > I think this patch is adding a check that protofile lines are actually > being honored (in the case of the symlink file) and checking that setgid > on a directory is not carried over into new children unless the > protofile explicitly marks the children setgid. > > IOWs, this isn't adding a regression test for a fix in xfsprogs, it's > increasing test coverage. Oh, understand, I have no objection with this patch, just thought it covers a known bug :) If it's good to you too, let's ACK it. Thanks, Zorro > > --D > > > Thanks, > > Zorro > > > > > > > > Thanks, > > > Catherine > > > > > > > >> tests/xfs/019 | 6 ++++++ > > > >> tests/xfs/019.out | 12 +++++++++++- > > > >> 2 files changed, 17 insertions(+), 1 deletion(-) > > > >> > > > >> diff --git a/tests/xfs/019 b/tests/xfs/019 > > > >> index 3dfd5408..535b7af1 100755 > > > >> --- a/tests/xfs/019 > > > >> +++ b/tests/xfs/019 > > > >> @@ -73,6 +73,10 @@ $ > > > >> setuid -u-666 0 0 $tempfile > > > >> setgid --g666 0 0 $tempfile > > > >> setugid -ug666 0 0 $tempfile > > > >> +directory_setgid d-g755 3 2 > > > >> +file_xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx_5 ---755 3 1 $tempfile > > > >> +$ > > > >> +: back in the root > > > >> block_device b--012 3 1 161 162 > > > >> char_device c--345 3 1 177 178 > > > >> pipe p--670 0 0 > > > >> @@ -114,6 +118,8 @@ _verify_fs() > > > >> | xargs $here/src/lstat64 | _filter_stat) > > > >> diff -q $SCRATCH_MNT/bigfile $tempfile.2 \ > > > >> || _fail "bigfile corrupted" > > > >> + diff -q $SCRATCH_MNT/symlink $tempfile.2 \ > > > >> + || _fail "symlink broken" > > > >> > > > >> echo "*** unmount FS" > > > >> _full "umount" > > > >> diff --git a/tests/xfs/019.out b/tests/xfs/019.out > > > >> index 19614d9d..8584f593 100644 > > > >> --- a/tests/xfs/019.out > > > >> +++ b/tests/xfs/019.out > > > >> @@ -7,7 +7,7 @@ Wrote 2048.00Kb (value 0x2c) > > > >> File: "." > > > >> Size: <DSIZE> Filetype: Directory > > > >> Mode: (0777/drwxrwxrwx) Uid: (3) Gid: (1) > > > >> -Device: <DEVICE> Inode: <INODE> Links: 3 > > > >> +Device: <DEVICE> Inode: <INODE> Links: 4 > > > >> > > > >> File: "./bigfile" > > > >> Size: 2097152 Filetype: Regular File > > > >> @@ -54,6 +54,16 @@ Device: <DEVICE> Inode: <INODE> Links: 1 > > > >> Mode: (0755/-rwxr-xr-x) Uid: (3) Gid: (1) > > > >> Device: <DEVICE> Inode: <INODE> Links: 1 > > > >> > > > >> + File: "./directory_setgid" > > > >> + Size: <DSIZE> Filetype: Directory > > > >> + Mode: (2755/drwxr-sr-x) Uid: (3) Gid: (2) > > > >> +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) > > > >> +Device: <DEVICE> Inode: <INODE> Links: 1 > > > >> + > > > >> File: "./pipe" > > > >> Size: 0 Filetype: Fifo File > > > >> Mode: (0670/frw-rwx---) Uid: (0) Gid: (0) > > > >> -- > > > >> 2.25.1 > > > >> > > > > > > > > > >
On Fri, Mar 25, 2022 at 09:33:56PM +0800, Zorro Lang wrote: > On Thu, Mar 24, 2022 at 01:17:30PM -0700, Darrick J. Wong wrote: > > On Fri, Mar 25, 2022 at 03:26:00AM +0800, Zorro Lang wrote: > > > On Thu, Mar 24, 2022 at 03:44:00PM +0000, Catherine Hoang wrote: > > > > > On Mar 22, 2022, at 6:36 PM, Zorro Lang <zlang@redhat.com> wrote: > > > > > > > > > > On Thu, Mar 17, 2022 at 11:24:08PM +0000, Catherine Hoang wrote: > > > > >> This test creates an xfs filesystem and verifies that the filesystem > > > > >> matches what is specified by the protofile. > > > > >> > > > > >> This patch extends the current test to check that a protofile can specify > > > > >> setgid mode on directories. Also, check that the created symlink isn’t > > > > >> broken. > > > > >> > > > > >> Signed-off-by: Catherine Hoang <catherine.hoang@oracle.com> > > > > >> --- > > > > > > > > > > Any specific reason to add this test? Likes uncovering some one known > > > > > bug/fix? > > > > > > > > > > Thanks, > > > > > Zorro > > > > > > > > Hi Zorro, > > > > > > > > We’ve been exploring alternate uses for protofiles and noticed a few holes > > > > in the testing. > > > > > > That's great, but better to show some details in the patch/commit, likes > > > a commit id of xfsprogs?/kernel? (if there's one) which fix the bug you > > > metioned, to help others to know what's this change trying to cover. > > > > I think this patch is adding a check that protofile lines are actually > > being honored (in the case of the symlink file) and checking that setgid > > on a directory is not carried over into new children unless the > > protofile explicitly marks the children setgid. > > > > IOWs, this isn't adding a regression test for a fix in xfsprogs, it's > > increasing test coverage. > > Oh, understand, I have no objection with this patch, just thought it covers > a known bug :) If it's good to you too, let's ACK it. Yes! Reviewed-by: Darrick J. Wong <djwong@kernel.org> --D > > Thanks, > Zorro > > > > > --D > > > > > Thanks, > > > Zorro > > > > > > > > > > > Thanks, > > > > Catherine > > > > > > > > > >> tests/xfs/019 | 6 ++++++ > > > > >> tests/xfs/019.out | 12 +++++++++++- > > > > >> 2 files changed, 17 insertions(+), 1 deletion(-) > > > > >> > > > > >> diff --git a/tests/xfs/019 b/tests/xfs/019 > > > > >> index 3dfd5408..535b7af1 100755 > > > > >> --- a/tests/xfs/019 > > > > >> +++ b/tests/xfs/019 > > > > >> @@ -73,6 +73,10 @@ $ > > > > >> setuid -u-666 0 0 $tempfile > > > > >> setgid --g666 0 0 $tempfile > > > > >> setugid -ug666 0 0 $tempfile > > > > >> +directory_setgid d-g755 3 2 > > > > >> +file_xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx_5 ---755 3 1 $tempfile > > > > >> +$ > > > > >> +: back in the root > > > > >> block_device b--012 3 1 161 162 > > > > >> char_device c--345 3 1 177 178 > > > > >> pipe p--670 0 0 > > > > >> @@ -114,6 +118,8 @@ _verify_fs() > > > > >> | xargs $here/src/lstat64 | _filter_stat) > > > > >> diff -q $SCRATCH_MNT/bigfile $tempfile.2 \ > > > > >> || _fail "bigfile corrupted" > > > > >> + diff -q $SCRATCH_MNT/symlink $tempfile.2 \ > > > > >> + || _fail "symlink broken" > > > > >> > > > > >> echo "*** unmount FS" > > > > >> _full "umount" > > > > >> diff --git a/tests/xfs/019.out b/tests/xfs/019.out > > > > >> index 19614d9d..8584f593 100644 > > > > >> --- a/tests/xfs/019.out > > > > >> +++ b/tests/xfs/019.out > > > > >> @@ -7,7 +7,7 @@ Wrote 2048.00Kb (value 0x2c) > > > > >> File: "." > > > > >> Size: <DSIZE> Filetype: Directory > > > > >> Mode: (0777/drwxrwxrwx) Uid: (3) Gid: (1) > > > > >> -Device: <DEVICE> Inode: <INODE> Links: 3 > > > > >> +Device: <DEVICE> Inode: <INODE> Links: 4 > > > > >> > > > > >> File: "./bigfile" > > > > >> Size: 2097152 Filetype: Regular File > > > > >> @@ -54,6 +54,16 @@ Device: <DEVICE> Inode: <INODE> Links: 1 > > > > >> Mode: (0755/-rwxr-xr-x) Uid: (3) Gid: (1) > > > > >> Device: <DEVICE> Inode: <INODE> Links: 1 > > > > >> > > > > >> + File: "./directory_setgid" > > > > >> + Size: <DSIZE> Filetype: Directory > > > > >> + Mode: (2755/drwxr-sr-x) Uid: (3) Gid: (2) > > > > >> +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) > > > > >> +Device: <DEVICE> Inode: <INODE> Links: 1 > > > > >> + > > > > >> File: "./pipe" > > > > >> Size: 0 Filetype: Fifo File > > > > >> Mode: (0670/frw-rwx---) Uid: (0) Gid: (0) > > > > >> -- > > > > >> 2.25.1 > > > > >> > > > > > > > > > > > > > > >
> On Mar 25, 2022, at 10:59 AM, Darrick J. Wong <djwong@kernel.org> wrote: > > On Fri, Mar 25, 2022 at 09:33:56PM +0800, Zorro Lang wrote: >> On Thu, Mar 24, 2022 at 01:17:30PM -0700, Darrick J. Wong wrote: >>> On Fri, Mar 25, 2022 at 03:26:00AM +0800, Zorro Lang wrote: >>>> On Thu, Mar 24, 2022 at 03:44:00PM +0000, Catherine Hoang wrote: >>>>>> On Mar 22, 2022, at 6:36 PM, Zorro Lang <zlang@redhat.com> wrote: >>>>>> >>>>>> On Thu, Mar 17, 2022 at 11:24:08PM +0000, Catherine Hoang wrote: >>>>>>> This test creates an xfs filesystem and verifies that the filesystem >>>>>>> matches what is specified by the protofile. >>>>>>> >>>>>>> This patch extends the current test to check that a protofile can specify >>>>>>> setgid mode on directories. Also, check that the created symlink isn’t >>>>>>> broken. >>>>>>> >>>>>>> Signed-off-by: Catherine Hoang <catherine.hoang@oracle.com> >>>>>>> --- >>>>>> >>>>>> Any specific reason to add this test? Likes uncovering some one known >>>>>> bug/fix? >>>>>> >>>>>> Thanks, >>>>>> Zorro >>>>> >>>>> Hi Zorro, >>>>> >>>>> We’ve been exploring alternate uses for protofiles and noticed a few holes >>>>> in the testing. >>>> >>>> That's great, but better to show some details in the patch/commit, likes >>>> a commit id of xfsprogs?/kernel? (if there's one) which fix the bug you >>>> metioned, to help others to know what's this change trying to cover. >>> >>> I think this patch is adding a check that protofile lines are actually >>> being honored (in the case of the symlink file) and checking that setgid >>> on a directory is not carried over into new children unless the >>> protofile explicitly marks the children setgid. >>> >>> IOWs, this isn't adding a regression test for a fix in xfsprogs, it's >>> increasing test coverage. >> >> Oh, understand, I have no objection with this patch, just thought it covers >> a known bug :) If it's good to you too, let's ACK it. > > Yes! > Reviewed-by: Darrick J. Wong <djwong@kernel.org> > > --D Thanks Darrick! > >> >> Thanks, >> Zorro >> >>> >>> --D >>> >>>> Thanks, >>>> Zorro >>>> >>>>> >>>>> Thanks, >>>>> Catherine >>>>>> >>>>>>> tests/xfs/019 | 6 ++++++ >>>>>>> tests/xfs/019.out | 12 +++++++++++- >>>>>>> 2 files changed, 17 insertions(+), 1 deletion(-) >>>>>>> >>>>>>> diff --git a/tests/xfs/019 b/tests/xfs/019 >>>>>>> index 3dfd5408..535b7af1 100755 >>>>>>> --- a/tests/xfs/019 >>>>>>> +++ b/tests/xfs/019 >>>>>>> @@ -73,6 +73,10 @@ $ >>>>>>> setuid -u-666 0 0 $tempfile >>>>>>> setgid --g666 0 0 $tempfile >>>>>>> setugid -ug666 0 0 $tempfile >>>>>>> +directory_setgid d-g755 3 2 >>>>>>> +file_xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx_5 ---755 3 1 $tempfile >>>>>>> +$ >>>>>>> +: back in the root >>>>>>> block_device b--012 3 1 161 162 >>>>>>> char_device c--345 3 1 177 178 >>>>>>> pipe p--670 0 0 >>>>>>> @@ -114,6 +118,8 @@ _verify_fs() >>>>>>> | xargs $here/src/lstat64 | _filter_stat) >>>>>>> diff -q $SCRATCH_MNT/bigfile $tempfile.2 \ >>>>>>> || _fail "bigfile corrupted" >>>>>>> + diff -q $SCRATCH_MNT/symlink $tempfile.2 \ >>>>>>> + || _fail "symlink broken" >>>>>>> >>>>>>> echo "*** unmount FS" >>>>>>> _full "umount" >>>>>>> diff --git a/tests/xfs/019.out b/tests/xfs/019.out >>>>>>> index 19614d9d..8584f593 100644 >>>>>>> --- a/tests/xfs/019.out >>>>>>> +++ b/tests/xfs/019.out >>>>>>> @@ -7,7 +7,7 @@ Wrote 2048.00Kb (value 0x2c) >>>>>>> File: "." >>>>>>> Size: <DSIZE> Filetype: Directory >>>>>>> Mode: (0777/drwxrwxrwx) Uid: (3) Gid: (1) >>>>>>> -Device: <DEVICE> Inode: <INODE> Links: 3 >>>>>>> +Device: <DEVICE> Inode: <INODE> Links: 4 >>>>>>> >>>>>>> File: "./bigfile" >>>>>>> Size: 2097152 Filetype: Regular File >>>>>>> @@ -54,6 +54,16 @@ Device: <DEVICE> Inode: <INODE> Links: 1 >>>>>>> Mode: (0755/-rwxr-xr-x) Uid: (3) Gid: (1) >>>>>>> Device: <DEVICE> Inode: <INODE> Links: 1 >>>>>>> >>>>>>> + File: "./directory_setgid" >>>>>>> + Size: <DSIZE> Filetype: Directory >>>>>>> + Mode: (2755/drwxr-sr-x) Uid: (3) Gid: (2) >>>>>>> +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) >>>>>>> +Device: <DEVICE> Inode: <INODE> Links: 1 >>>>>>> + >>>>>>> File: "./pipe" >>>>>>> Size: 0 Filetype: Fifo File >>>>>>> Mode: (0670/frw-rwx---) Uid: (0) Gid: (0) >>>>>>> -- >>>>>>> 2.25.1 >>>>>>> >>>>>> >>>>> >>>> >>> >>
On Fri, 2022-03-25 at 10:59 -0700, Darrick J. Wong wrote: > On Fri, Mar 25, 2022 at 09:33:56PM +0800, Zorro Lang wrote: > > On Thu, Mar 24, 2022 at 01:17:30PM -0700, Darrick J. Wong wrote: > > > On Fri, Mar 25, 2022 at 03:26:00AM +0800, Zorro Lang wrote: > > > > On Thu, Mar 24, 2022 at 03:44:00PM +0000, Catherine Hoang > > > > wrote: > > > > > > On Mar 22, 2022, at 6:36 PM, Zorro Lang <zlang@redhat.com> > > > > > > wrote: > > > > > > > > > > > > On Thu, Mar 17, 2022 at 11:24:08PM +0000, Catherine Hoang > > > > > > wrote: > > > > > > > This test creates an xfs filesystem and verifies that the > > > > > > > filesystem > > > > > > > matches what is specified by the protofile. > > > > > > > > > > > > > > This patch extends the current test to check that a > > > > > > > protofile can specify > > > > > > > setgid mode on directories. Also, check that the created > > > > > > > symlink isn’t > > > > > > > broken. > > > > > > > > > > > > > > Signed-off-by: Catherine Hoang < > > > > > > > catherine.hoang@oracle.com> > > > > > > > --- > > > > > > > > > > > > Any specific reason to add this test? Likes uncovering some > > > > > > one known > > > > > > bug/fix? > > > > > > > > > > > > Thanks, > > > > > > Zorro > > > > > > > > > > Hi Zorro, > > > > > > > > > > We’ve been exploring alternate uses for protofiles and > > > > > noticed a few holes > > > > > in the testing. > > > > > > > > That's great, but better to show some details in the > > > > patch/commit, likes > > > > a commit id of xfsprogs?/kernel? (if there's one) which fix the > > > > bug you > > > > metioned, to help others to know what's this change trying to > > > > cover. > > > > > > I think this patch is adding a check that protofile lines are > > > actually > > > being honored (in the case of the symlink file) and checking that > > > setgid > > > on a directory is not carried over into new children unless the > > > protofile explicitly marks the children setgid. > > > > > > IOWs, this isn't adding a regression test for a fix in xfsprogs, > > > it's > > > increasing test coverage. > > > > Oh, understand, I have no objection with this patch, just thought > > it covers > > a known bug :) If it's good to you too, let's ACK it. > > Yes! > Reviewed-by: Darrick J. Wong <djwong@kernel.org> > > --D > This looks good to me as well. Feel free to add my rvb: Reviewed-by: Allison Henderson <allison.henderson@oracle.org> Thanks! Allison > > Thanks, > > Zorro > > > > > --D > > > > > > > Thanks, > > > > Zorro > > > > > > > > > Thanks, > > > > > Catherine > > > > > > > tests/xfs/019 | 6 ++++++ > > > > > > > tests/xfs/019.out | 12 +++++++++++- > > > > > > > 2 files changed, 17 insertions(+), 1 deletion(-) > > > > > > > > > > > > > > diff --git a/tests/xfs/019 b/tests/xfs/019 > > > > > > > index 3dfd5408..535b7af1 100755 > > > > > > > --- a/tests/xfs/019 > > > > > > > +++ b/tests/xfs/019 > > > > > > > @@ -73,6 +73,10 @@ $ > > > > > > > setuid -u-666 0 0 $tempfile > > > > > > > setgid --g666 0 0 $tempfile > > > > > > > setugid -ug666 0 0 $tempfile > > > > > > > +directory_setgid d-g755 3 2 > > > > > > > +file_xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx_5 - > > > > > > > --755 3 1 $tempfile > > > > > > > +$ > > > > > > > +: back in the root > > > > > > > block_device b--012 3 1 161 162 > > > > > > > char_device c--345 3 1 177 178 > > > > > > > pipe p--670 0 0 > > > > > > > @@ -114,6 +118,8 @@ _verify_fs() > > > > > > > | xargs $here/src/lstat64 | _filter_stat) > > > > > > > diff -q $SCRATCH_MNT/bigfile $tempfile.2 \ > > > > > > > || _fail "bigfile corrupted" > > > > > > > + diff -q $SCRATCH_MNT/symlink $tempfile.2 \ > > > > > > > + || _fail "symlink broken" > > > > > > > > > > > > > > echo "*** unmount FS" > > > > > > > _full "umount" > > > > > > > diff --git a/tests/xfs/019.out b/tests/xfs/019.out > > > > > > > index 19614d9d..8584f593 100644 > > > > > > > --- a/tests/xfs/019.out > > > > > > > +++ b/tests/xfs/019.out > > > > > > > @@ -7,7 +7,7 @@ Wrote 2048.00Kb (value 0x2c) > > > > > > > File: "." > > > > > > > Size: <DSIZE> Filetype: Directory > > > > > > > Mode: (0777/drwxrwxrwx) Uid: (3) Gid: (1) > > > > > > > -Device: <DEVICE> Inode: <INODE> Links: 3 > > > > > > > +Device: <DEVICE> Inode: <INODE> Links: 4 > > > > > > > > > > > > > > File: "./bigfile" > > > > > > > Size: 2097152 Filetype: Regular File > > > > > > > @@ -54,6 +54,16 @@ Device: <DEVICE> Inode: <INODE> Links: > > > > > > > 1 > > > > > > > Mode: (0755/-rwxr-xr-x) Uid: (3) Gid: (1) > > > > > > > Device: <DEVICE> Inode: <INODE> Links: 1 > > > > > > > > > > > > > > + File: "./directory_setgid" > > > > > > > + Size: <DSIZE> Filetype: Directory > > > > > > > + Mode: (2755/drwxr-sr-x) Uid: (3) Gid: (2) > > > > > > > +Device: <DEVICE> Inode: <INODE> Links: 2 > > > > > > > + > > > > > > > + File: > > > > > > > "./directory_setgid/file_xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx > > > > > > > xxxxxxxxxxx_5" > > > > > > > + Size: 5 Filetype: Regular File > > > > > > > + Mode: (0755/-rwxr-xr-x) Uid: (3) Gid: (2) > > > > > > > +Device: <DEVICE> Inode: <INODE> Links: 1 > > > > > > > + > > > > > > > File: "./pipe" > > > > > > > Size: 0 Filetype: Fifo File > > > > > > > Mode: (0670/frw-rwx---) Uid: (0) Gid: (0) > > > > > > > -- > > > > > > > 2.25.1 > > > > > > >
diff --git a/tests/xfs/019 b/tests/xfs/019 index 3dfd5408..535b7af1 100755 --- a/tests/xfs/019 +++ b/tests/xfs/019 @@ -73,6 +73,10 @@ $ setuid -u-666 0 0 $tempfile setgid --g666 0 0 $tempfile setugid -ug666 0 0 $tempfile +directory_setgid d-g755 3 2 +file_xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx_5 ---755 3 1 $tempfile +$ +: back in the root block_device b--012 3 1 161 162 char_device c--345 3 1 177 178 pipe p--670 0 0 @@ -114,6 +118,8 @@ _verify_fs() | xargs $here/src/lstat64 | _filter_stat) diff -q $SCRATCH_MNT/bigfile $tempfile.2 \ || _fail "bigfile corrupted" + diff -q $SCRATCH_MNT/symlink $tempfile.2 \ + || _fail "symlink broken" echo "*** unmount FS" _full "umount" diff --git a/tests/xfs/019.out b/tests/xfs/019.out index 19614d9d..8584f593 100644 --- a/tests/xfs/019.out +++ b/tests/xfs/019.out @@ -7,7 +7,7 @@ Wrote 2048.00Kb (value 0x2c) File: "." Size: <DSIZE> Filetype: Directory Mode: (0777/drwxrwxrwx) Uid: (3) Gid: (1) -Device: <DEVICE> Inode: <INODE> Links: 3 +Device: <DEVICE> Inode: <INODE> Links: 4 File: "./bigfile" Size: 2097152 Filetype: Regular File @@ -54,6 +54,16 @@ Device: <DEVICE> Inode: <INODE> Links: 1 Mode: (0755/-rwxr-xr-x) Uid: (3) Gid: (1) Device: <DEVICE> Inode: <INODE> Links: 1 + File: "./directory_setgid" + Size: <DSIZE> Filetype: Directory + Mode: (2755/drwxr-sr-x) Uid: (3) Gid: (2) +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) +Device: <DEVICE> Inode: <INODE> Links: 1 + File: "./pipe" Size: 0 Filetype: Fifo File Mode: (0670/frw-rwx---) Uid: (0) Gid: (0)
This test creates an xfs filesystem and verifies that the filesystem matches what is specified by the protofile. This patch extends the current test to check that a protofile can specify setgid mode on directories. Also, check that the created symlink isn’t broken. Signed-off-by: Catherine Hoang <catherine.hoang@oracle.com> --- tests/xfs/019 | 6 ++++++ tests/xfs/019.out | 12 +++++++++++- 2 files changed, 17 insertions(+), 1 deletion(-)