diff mbox

[09/14] xfs/348: dir->symlink corruption must not be allowed

Message ID 150957284389.18388.370732842906149356.stgit@magnolia (mailing list archive)
State Accepted
Headers show

Commit Message

Darrick J. Wong Nov. 1, 2017, 9:47 p.m. UTC 
From: Darrick J. Wong <darrick.wong@oracle.com>

A directory corrupted into a symlink will be caught by the upcoming
local format ifork verifiers.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 tests/xfs/348.out |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)



--
To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Comments

Eryu Guan Nov. 2, 2017, 12:42 p.m. UTC | #1 
On Wed, Nov 01, 2017 at 02:47:23PM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> A directory corrupted into a symlink will be caught by the upcoming
> local format ifork verifiers.
> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> ---
>  tests/xfs/348.out |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> 
> diff --git a/tests/xfs/348.out b/tests/xfs/348.out
> index f4a7a71..17d9be2 100644
> --- a/tests/xfs/348.out
> +++ b/tests/xfs/348.out
> @@ -239,7 +239,7 @@ would have junked entry "DATA" in directory PARENT_INO
>  would have junked entry "DIR" in directory PARENT_INO
>  would have junked entry "EMPTY" in directory PARENT_INO
>  would have junked entry "FIFO" in directory PARENT_INO
> -stat: 'SCRATCH_MNT/test/DIR' is a symbolic link
> +stat: cannot stat 'SCRATCH_MNT/test/DIR': Structure needs cleaning

But this breaks tests on old kernels. Or with the new ifork verifiers,
old kernels can be considered as buggy?

Thanks,
Eryu

>  stat: 'SCRATCH_MNT/test/DATA' is a symbolic link
>  stat: cannot stat 'SCRATCH_MNT/test/EMPTY': Structure needs cleaning
>  stat: 'SCRATCH_MNT/test/SYMLINK' is a symbolic link
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Amir Goldstein Nov. 2, 2017, 1:14 p.m. UTC | #2 
On Thu, Nov 2, 2017 at 2:42 PM, Eryu Guan <eguan@redhat.com> wrote:
> On Wed, Nov 01, 2017 at 02:47:23PM -0700, Darrick J. Wong wrote:
>> From: Darrick J. Wong <darrick.wong@oracle.com>
>>
>> A directory corrupted into a symlink will be caught by the upcoming
>> local format ifork verifiers.
>>
>> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
>> ---
>>  tests/xfs/348.out |    2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>>
>> diff --git a/tests/xfs/348.out b/tests/xfs/348.out
>> index f4a7a71..17d9be2 100644
>> --- a/tests/xfs/348.out
>> +++ b/tests/xfs/348.out
>> @@ -239,7 +239,7 @@ would have junked entry "DATA" in directory PARENT_INO
>>  would have junked entry "DIR" in directory PARENT_INO
>>  would have junked entry "EMPTY" in directory PARENT_INO
>>  would have junked entry "FIFO" in directory PARENT_INO
>> -stat: 'SCRATCH_MNT/test/DIR' is a symbolic link
>> +stat: cannot stat 'SCRATCH_MNT/test/DIR': Structure needs cleaning
>
> But this breaks tests on old kernels. Or with the new ifork verifiers,
> old kernels can be considered as buggy?
>

Can filter new and old error into canonicalized string.
Dodgy, bug we've done it before.
See:
git grep sed | grep 'Operation not permitted\|Permission denied'

Amir.
--
To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Darrick J. Wong Nov. 2, 2017, 4:37 p.m. UTC | #3 
On Thu, Nov 02, 2017 at 08:42:14PM +0800, Eryu Guan wrote:
> On Wed, Nov 01, 2017 at 02:47:23PM -0700, Darrick J. Wong wrote:
> > From: Darrick J. Wong <darrick.wong@oracle.com>
> > 
> > A directory corrupted into a symlink will be caught by the upcoming
> > local format ifork verifiers.
> > 
> > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> > ---
> >  tests/xfs/348.out |    2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > 
> > diff --git a/tests/xfs/348.out b/tests/xfs/348.out
> > index f4a7a71..17d9be2 100644
> > --- a/tests/xfs/348.out
> > +++ b/tests/xfs/348.out
> > @@ -239,7 +239,7 @@ would have junked entry "DATA" in directory PARENT_INO
> >  would have junked entry "DIR" in directory PARENT_INO
> >  would have junked entry "EMPTY" in directory PARENT_INO
> >  would have junked entry "FIFO" in directory PARENT_INO
> > -stat: 'SCRATCH_MNT/test/DIR' is a symbolic link
> > +stat: cannot stat 'SCRATCH_MNT/test/DIR': Structure needs cleaning
> 
> But this breaks tests on old kernels. Or with the new ifork verifiers,
> old kernels can be considered as buggy?

Yes, they're buggy since we shouldn't be interpreting directory entries
as a link target given that the zero bytes in the "link target" will
screw things up.

--D

> Thanks,
> Eryu
> 
> >  stat: 'SCRATCH_MNT/test/DATA' is a symbolic link
> >  stat: cannot stat 'SCRATCH_MNT/test/EMPTY': Structure needs cleaning
> >  stat: 'SCRATCH_MNT/test/SYMLINK' is a symbolic link
> > 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Darrick J. Wong Nov. 2, 2017, 4:39 p.m. UTC | #4 
On Thu, Nov 02, 2017 at 03:14:45PM +0200, Amir Goldstein wrote:
> On Thu, Nov 2, 2017 at 2:42 PM, Eryu Guan <eguan@redhat.com> wrote:
> > On Wed, Nov 01, 2017 at 02:47:23PM -0700, Darrick J. Wong wrote:
> >> From: Darrick J. Wong <darrick.wong@oracle.com>
> >>
> >> A directory corrupted into a symlink will be caught by the upcoming
> >> local format ifork verifiers.
> >>
> >> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> >> ---
> >>  tests/xfs/348.out |    2 +-
> >>  1 file changed, 1 insertion(+), 1 deletion(-)
> >>
> >>
> >> diff --git a/tests/xfs/348.out b/tests/xfs/348.out
> >> index f4a7a71..17d9be2 100644
> >> --- a/tests/xfs/348.out
> >> +++ b/tests/xfs/348.out
> >> @@ -239,7 +239,7 @@ would have junked entry "DATA" in directory PARENT_INO
> >>  would have junked entry "DIR" in directory PARENT_INO
> >>  would have junked entry "EMPTY" in directory PARENT_INO
> >>  would have junked entry "FIFO" in directory PARENT_INO
> >> -stat: 'SCRATCH_MNT/test/DIR' is a symbolic link
> >> +stat: cannot stat 'SCRATCH_MNT/test/DIR': Structure needs cleaning
> >
> > But this breaks tests on old kernels. Or with the new ifork verifiers,
> > old kernels can be considered as buggy?
> >
> 
> Can filter new and old error into canonicalized string.
> Dodgy, bug we've done it before.
> See:
> git grep sed | grep 'Operation not permitted\|Permission denied'

I tried that, but the "stat: XXX is a symbolic link" message comes from
a different path in the test file code.

Unless... you're suggesting that I should sed the error output to make
it look like the previous (broken) output?

--D

> 
> Amir.
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Amir Goldstein Nov. 2, 2017, 6:11 p.m. UTC | #5 
On Thu, Nov 2, 2017 at 6:39 PM, Darrick J. Wong <darrick.wong@oracle.com> wrote:
> On Thu, Nov 02, 2017 at 03:14:45PM +0200, Amir Goldstein wrote:
>> On Thu, Nov 2, 2017 at 2:42 PM, Eryu Guan <eguan@redhat.com> wrote:
>> > On Wed, Nov 01, 2017 at 02:47:23PM -0700, Darrick J. Wong wrote:
>> >> From: Darrick J. Wong <darrick.wong@oracle.com>
>> >>
>> >> A directory corrupted into a symlink will be caught by the upcoming
>> >> local format ifork verifiers.
>> >>
>> >> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
>> >> ---
>> >>  tests/xfs/348.out |    2 +-
>> >>  1 file changed, 1 insertion(+), 1 deletion(-)
>> >>
>> >>
>> >> diff --git a/tests/xfs/348.out b/tests/xfs/348.out
>> >> index f4a7a71..17d9be2 100644
>> >> --- a/tests/xfs/348.out
>> >> +++ b/tests/xfs/348.out
>> >> @@ -239,7 +239,7 @@ would have junked entry "DATA" in directory PARENT_INO
>> >>  would have junked entry "DIR" in directory PARENT_INO
>> >>  would have junked entry "EMPTY" in directory PARENT_INO
>> >>  would have junked entry "FIFO" in directory PARENT_INO
>> >> -stat: 'SCRATCH_MNT/test/DIR' is a symbolic link
>> >> +stat: cannot stat 'SCRATCH_MNT/test/DIR': Structure needs cleaning
>> >
>> > But this breaks tests on old kernels. Or with the new ifork verifiers,
>> > old kernels can be considered as buggy?
>> >
>>
>> Can filter new and old error into canonicalized string.
>> Dodgy, bug we've done it before.
>> See:
>> git grep sed | grep 'Operation not permitted\|Permission denied'
>
> I tried that, but the "stat: XXX is a symbolic link" message comes from
> a different path in the test file code.
>
> Unless... you're suggesting that I should sed the error output to make
> it look like the previous (broken) output?
>

Let's say I am not *suggesting* this, but I have seen tests that do that
See generic/160 and other tests that show in this grep output:

git grep sed | grep 'Operation not permitted\|Permission denied'

Amir.
--
To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Eryu Guan Nov. 3, 2017, 4:30 a.m. UTC | #6 
On Thu, Nov 02, 2017 at 09:37:54AM -0700, Darrick J. Wong wrote:
> On Thu, Nov 02, 2017 at 08:42:14PM +0800, Eryu Guan wrote:
> > On Wed, Nov 01, 2017 at 02:47:23PM -0700, Darrick J. Wong wrote:
> > > From: Darrick J. Wong <darrick.wong@oracle.com>
> > > 
> > > A directory corrupted into a symlink will be caught by the upcoming
> > > local format ifork verifiers.
> > > 
> > > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> > > ---
> > >  tests/xfs/348.out |    2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > 
> > > 
> > > diff --git a/tests/xfs/348.out b/tests/xfs/348.out
> > > index f4a7a71..17d9be2 100644
> > > --- a/tests/xfs/348.out
> > > +++ b/tests/xfs/348.out
> > > @@ -239,7 +239,7 @@ would have junked entry "DATA" in directory PARENT_INO
> > >  would have junked entry "DIR" in directory PARENT_INO
> > >  would have junked entry "EMPTY" in directory PARENT_INO
> > >  would have junked entry "FIFO" in directory PARENT_INO
> > > -stat: 'SCRATCH_MNT/test/DIR' is a symbolic link
> > > +stat: cannot stat 'SCRATCH_MNT/test/DIR': Structure needs cleaning
> > 
> > But this breaks tests on old kernels. Or with the new ifork verifiers,
> > old kernels can be considered as buggy?
> 
> Yes, they're buggy since we shouldn't be interpreting directory entries
> as a link target given that the zero bytes in the "link target" will
> screw things up.

Then I'm fine with taking it, and probably will edit the commit log a
bit to reflect that we were missing a error case previously. Thanks!

Eryu
--
To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Theodore Ts'o Nov. 3, 2017, 4:14 p.m. UTC | #7 
On Fri, Nov 03, 2017 at 12:30:22PM +0800, Eryu Guan wrote:
> > Yes, they're buggy since we shouldn't be interpreting directory entries
> > as a link target given that the zero bytes in the "link target" will
> > screw things up.
> 
> Then I'm fine with taking it, and probably will edit the commit log a
> bit to reflect that we were missing a error case previously. Thanks!

Darrick, can we add a note indicating what commit needs to be
backported to make the failure go away?

					- Ted
--
To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Darrick J. Wong Nov. 3, 2017, 4:30 p.m. UTC | #8 
On Fri, Nov 03, 2017 at 12:14:48PM -0400, Theodore Ts'o wrote:
> On Fri, Nov 03, 2017 at 12:30:22PM +0800, Eryu Guan wrote:
> > > Yes, they're buggy since we shouldn't be interpreting directory entries
> > > as a link target given that the zero bytes in the "link target" will
> > > screw things up.
> > 
> > Then I'm fine with taking it, and probably will edit the commit log a
> > bit to reflect that we were missing a error case previously. Thanks!
> 
> Darrick, can we add a note indicating what commit needs to be
> backported to make the failure go away?

Will do when the time comes.  TBH we've not been consistent about
marking regression tests with the appropriate upstream commit id once
they're available.

--D

> 					- Ted
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/tests/xfs/348.out b/tests/xfs/348.out
index f4a7a71..17d9be2 100644
--- a/tests/xfs/348.out
+++ b/tests/xfs/348.out
@@ -239,7 +239,7 @@  would have junked entry "DATA" in directory PARENT_INO
 would have junked entry "DIR" in directory PARENT_INO
 would have junked entry "EMPTY" in directory PARENT_INO
 would have junked entry "FIFO" in directory PARENT_INO
-stat: 'SCRATCH_MNT/test/DIR' is a symbolic link
+stat: cannot stat 'SCRATCH_MNT/test/DIR': Structure needs cleaning
 stat: 'SCRATCH_MNT/test/DATA' is a symbolic link
 stat: cannot stat 'SCRATCH_MNT/test/EMPTY': Structure needs cleaning
 stat: 'SCRATCH_MNT/test/SYMLINK' is a symbolic link