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

[Allison Henderson <allison.henderson@xxxxxxxxxx>]

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@xxxxxxxxxx>
> > ---
> >   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?


> >   		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@xxxxxxxxxxxxxxx
> > 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@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html