Re: [PATCH v2 23/27] xfsprogs: Do not use namechecks on parent pointers

["Darrick J. Wong" <darrick.wong@oracle.com>]

On Mon, Jun 11, 2018 at 01:00:45PM -0700, Allison Henderson wrote:
> On 06/11/2018 11:00 AM, Darrick J. Wong wrote:
> > On Sat, Jun 09, 2018 at 10:07:48PM -0700, Allison Henderson wrote:
> > > Attribute names of parent pointers are not strings.  So
> > > avoid doing namechecks for these attributes.
> > > 
> > > Signed-off-by: Allison Henderson <allison.henderson@oracle.com>
> > > ---
> > >   repair/attr_repair.c | 18 +++++++++++-------
> > >   1 file changed, 11 insertions(+), 7 deletions(-)
> > > 
> > > diff --git a/repair/attr_repair.c b/repair/attr_repair.c
> > > index 8b1b8a7..b8b0768 100644
> > > --- a/repair/attr_repair.c
> > > +++ b/repair/attr_repair.c
> > > @@ -308,8 +308,9 @@ process_shortform_attr(
> > >   		/* namecheck checks for / and null terminated for file names.
> > >   		 * attributes names currently follow the same rules.
> > >   		*/
> > > -		if (namecheck((char *)&currententry->nameval[0],
> > > -						currententry->namelen))  {
> > > +		if (!(currententry->flags & XFS_ATTR_PARENT) &&
> > > +			namecheck((char *)&currententry->nameval[0],
> > > +			currententry->namelen))  {
> > >   			do_warn(
> > 
> > Please don't indent the condition tests to the same column as the code.
> > Either line them up with the if parentheses or double-tab them.
> > 
> > if (!(currententry->flags & XFS_ATTR_PARENT) &&
> >      namecheck((char *)&currententry->nameval[0],
> > 		currententry->namelen)) {
> > 	do_warn(...);
> > }
> > 
> Alrighty, will fix
> 
> > >   	_("entry contains illegal character in shortform attribute name\n"));
> > >   			junkit = 1;
> > > @@ -470,8 +471,9 @@ process_leaf_attr_local(
> > >   	xfs_attr_leaf_name_local_t *local;
> > >   	local = xfs_attr3_leaf_name_local(leaf, i);
> > > -	if (local->namelen == 0 || namecheck((char *)&local->nameval[0],
> > > -							local->namelen)) {
> > > +	if (!(entry->flags & XFS_ATTR_PARENT) &&
> > > +		(local->namelen == 0 || namecheck((char *)&local->nameval[0],
> > > +		local->namelen))) {
> > 
> > Why skip the namelen checks when it's a parent pointer?  Isn't the pptr
> > corrupt if the (ino, gen, offset) data is length zero?
> > 
> Thats true, though I suppose in the case of parent pointers it should be the
> size of the name record.  Would it maybe be cleaner to make a subroutine
> that took local and entry and did the appropriate length checking there?  It
> may make things simpler here and also in the case below?

I probably wouldn't bother for the local entry because it's fairly
short.  The remote format case below is sort of gnarly, maybe it'd be
better refactored as a functi...

...hmm, thinking further, in the (flags & PARENT) case, namelen should
be exactly sizeof(struct xfs_parent_name_rec), right?

So perhaps we just move the namelen == 0 check into namecheck and pass
in the entry->flags so that we can do....

...thinking even further ahead, if there's some sort of verifier
function for struct xfs_parent_name_rec then we should call that here
too.  What do you think of this?

/* return true if attr name is garbage */
bool namecheck(entry, nameptr, namelen)
{
	if (namelen == 0)
		return true;
	if (entry->flags & _ATTR_PARENT) {
		xfs_failaddr_t	fa;

		if (namelen != sizeof(struct xfs_parent_name_rec))
			return true;

		fa = xfs_verify_pptr(mp, (struct xfs_parent_name_rec *)nameptr);
		return fa != NULL;
	}
	/* do the other name checks */
}

--D

> 
> 
> > >   		do_warn(
> > >   	_("attribute entry %d in attr block %u, inode %" PRIu64 " has bad name (namelen = %d)\n"),
> > >   			i, da_bno, ino, local->namelen);
> > > @@ -525,13 +527,15 @@ process_leaf_attr_remote(
> > >   	remotep = xfs_attr3_leaf_name_remote(leaf, i);
> > > -	if (remotep->namelen == 0 || namecheck((char *)&remotep->name[0],
> > > -						remotep->namelen) ||
> > > +	if (!(entry->flags & XFS_ATTR_PARENT) &&
> > > +			(remotep->namelen == 0 ||
> > > +			namecheck((char *)&remotep->name[0],
> > > +				remotep->namelen) ||
> > >   			be32_to_cpu(entry->hashval) !=
> > >   				libxfs_da_hashname((unsigned char *)&remotep->name[0],
> > >   						remotep->namelen) ||
> > >   			be32_to_cpu(entry->hashval) < last_hashval ||
> > > -			be32_to_cpu(remotep->valueblk) == 0) {
> > > +			be32_to_cpu(remotep->valueblk) == 0)) {
> > 
> > Do parent pointer attrs ever end up using a remote value block to store
> > the name?  If so, I think you only want to skip the namecheck, not the
> > namelen/hashval/valueblk checks, right?
> > 
> > --D
> > 
> > >   		do_warn(
> > >   	_("inconsistent remote attribute entry %d in attr block %u, ino %" PRIu64 "\n"), i, da_bno, ino);
> > >   		return -1;
> > > -- 
> > > 2.7.4
> > > 
> > > --
> > > 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