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

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

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(...);
}

>  	_("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?

>  		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