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

[Bill O'Donnell <billodo@redhat.com>]

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
> > 
>