[PATCH] xfs: xchk_xattr_listent() fix context->seen_enough to -ECANCELED

[PATCH] xfs: xchk_xattr_listent() fix context->seen_enough to -ECANCELED

[Bill O'Donnell]

Commit e7ee96dfb8c (xfs: remove all *_ITER_ABORT values)
replaced *_ITER_ABORT values with -ECANCELED. The replacement
in the case of scrub/attr.c xchk_xattr_listent() is in
error (context->seen_enough = 1;). Instead of '1', use
the intended -ECANCELED.

Fixes: e7ee96dfb8c (xfs: remove all *_ITER_ABORT values)
Signed-off-by: Bill O'Donnell <billodo@redhat.com>
---
 fs/xfs/scrub/attr.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/xfs/scrub/attr.c b/fs/xfs/scrub/attr.c
index d9f0dd444b80..5d0590f78973 100644
--- a/fs/xfs/scrub/attr.c
+++ b/fs/xfs/scrub/attr.c
@@ -171,7 +171,7 @@ xchk_xattr_listent(
 					     args.blkno);
 fail_xref:
 	if (sx->sc->sm->sm_flags & XFS_SCRUB_OFLAG_CORRUPT)
-		context->seen_enough = 1;
+		context->seen_enough = -ECANCELED;
 	return;
 }
 
-- 
2.24.1

Re: [PATCH] xfs: xchk_xattr_listent() fix context->seen_enough to -ECANCELED

[Darrick J. Wong]

On Wed, Feb 05, 2020 at 01:04:55PM -0600, Bill O'Donnell wrote:
> Commit e7ee96dfb8c (xfs: remove all *_ITER_ABORT values)
> replaced *_ITER_ABORT values with -ECANCELED. The replacement
> in the case of scrub/attr.c xchk_xattr_listent() is in
> error (context->seen_enough = 1;). Instead of '1', use
> the intended -ECANCELED.
> 
> Fixes: e7ee96dfb8c (xfs: remove all *_ITER_ABORT values)
> Signed-off-by: Bill O'Donnell <billodo@redhat.com>
> ---
>  fs/xfs/scrub/attr.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/fs/xfs/scrub/attr.c b/fs/xfs/scrub/attr.c
> index d9f0dd444b80..5d0590f78973 100644
> --- a/fs/xfs/scrub/attr.c
> +++ b/fs/xfs/scrub/attr.c
> @@ -171,7 +171,7 @@ xchk_xattr_listent(
>  					     args.blkno);
>  fail_xref:
>  	if (sx->sc->sm->sm_flags & XFS_SCRUB_OFLAG_CORRUPT)
> -		context->seen_enough = 1;

Hmm.  The attr list functions do:

	if (context->seen_enough)
		break;

to stop iteration of the attributes.  Any nonzero value will work,
positive or negative.  Further down in the scrub/attr.c, xchk_xattr
does:

	/* Did our listent function try to return any errors? */
	if (sx.context.seen_enough < 0)
		error = sx.context.seen_enough;

Which means that if seen_enough is set to a negative value, we'll return
that negative value all the way back to userspace, which means that the
userspace buffer is not updated and xfs_scrub will think there was a
runtime error.

> +		context->seen_enough = -ECANCELED;

So this will cause xfs_scrub to abort with "Operation Canceled" if it
found a corruption error.  The patch I sent to the list had -ECANCELED,
but then I noticed the scrub breakage and changed it to 1 before
committing.  Other parts of the attr code use 1 to stop an attr walk
without returning errors to userspace.

Perhaps it's time to replace that novel use of "1" (and audit all the
branching and whatnot) with -ECANCELED so that we can go on cargoculting
negative int errors in peace.

(*UGH* I remembered that I was the one who applied negative int error
semantics to seen_enough in the first place; before that, its meaning
was purely boolean.  It's still screaming for a cleanup though...)

--D

>  	return;
>  }
>  
> -- 
> 2.24.1
> 

Re: [PATCH] xfs: xchk_xattr_listent() fix context->seen_enough to -ECANCELED

[Bill O'Donnell]

On Thu, Feb 06, 2020 at 03:07:31PM -0800, Darrick J. Wong wrote:
> On Wed, Feb 05, 2020 at 01:04:55PM -0600, Bill O'Donnell wrote:
> > Commit e7ee96dfb8c (xfs: remove all *_ITER_ABORT values)
> > replaced *_ITER_ABORT values with -ECANCELED. The replacement
> > in the case of scrub/attr.c xchk_xattr_listent() is in
> > error (context->seen_enough = 1;). Instead of '1', use
> > the intended -ECANCELED.
> > 
> > Fixes: e7ee96dfb8c (xfs: remove all *_ITER_ABORT values)
> > Signed-off-by: Bill O'Donnell <billodo@redhat.com>
> > ---
> >  fs/xfs/scrub/attr.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/fs/xfs/scrub/attr.c b/fs/xfs/scrub/attr.c
> > index d9f0dd444b80..5d0590f78973 100644
> > --- a/fs/xfs/scrub/attr.c
> > +++ b/fs/xfs/scrub/attr.c
> > @@ -171,7 +171,7 @@ xchk_xattr_listent(
> >  					     args.blkno);
> >  fail_xref:
> >  	if (sx->sc->sm->sm_flags & XFS_SCRUB_OFLAG_CORRUPT)
> > -		context->seen_enough = 1;
> 
> Hmm.  The attr list functions do:
> 
> 	if (context->seen_enough)
> 		break;
> 
> to stop iteration of the attributes.  Any nonzero value will work,
> positive or negative.  Further down in the scrub/attr.c, xchk_xattr
> does:
> 
> 	/* Did our listent function try to return any errors? */
> 	if (sx.context.seen_enough < 0)
> 		error = sx.context.seen_enough;
> 
> Which means that if seen_enough is set to a negative value, we'll return
> that negative value all the way back to userspace, which means that the
> userspace buffer is not updated and xfs_scrub will think there was a
> runtime error.
> 
> > +		context->seen_enough = -ECANCELED;
> 
> So this will cause xfs_scrub to abort with "Operation Canceled" if it
> found a corruption error.  The patch I sent to the list had -ECANCELED,
> but then I noticed the scrub breakage and changed it to 1 before
> committing.  Other parts of the attr code use 1 to stop an attr walk
> without returning errors to userspace.

That is what had me confused. 

> 
> Perhaps it's time to replace that novel use of "1" (and audit all the
> branching and whatnot) with -ECANCELED so that we can go on cargoculting
> negative int errors in peace.
> 
> (*UGH* I remembered that I was the one who applied negative int error
> semantics to seen_enough in the first place; before that, its meaning
> was purely boolean.  It's still screaming for a cleanup though...)

Agreed.
Thanks-
Bill

> --D
> 
> >  	return;
> >  }
> >  
> > -- 
> > 2.24.1
> > 
>