| Message ID | 1528607272-11122-24-git-send-email-allison.henderson@oracle.com (mailing list archive) |
|---|---|
| State | Deferred, archived |
| Headers | show |
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 *)¤tentry->nameval[0], > - currententry->namelen)) { > + if (!(currententry->flags & XFS_ATTR_PARENT) && > + namecheck((char *)¤tentry->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 *)¤tentry->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@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
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 *)¤tentry->nameval[0], >> - currententry->namelen)) { >> + if (!(currententry->flags & XFS_ATTR_PARENT) && >> + namecheck((char *)¤tentry->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 *)¤tentry->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@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
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 *)¤tentry->nameval[0], > > > - currententry->namelen)) { > > > + if (!(currententry->flags & XFS_ATTR_PARENT) && > > > + namecheck((char *)¤tentry->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 *)¤tentry->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 -- 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
On 06/11/2018 01:23 PM, Darrick J. Wong wrote: > 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 *)¤tentry->nameval[0], >>>> - currententry->namelen)) { >>>> + if (!(currententry->flags & XFS_ATTR_PARENT) && >>>> + namecheck((char *)¤tentry->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 *)¤tentry->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 Lol, alrighty then that looks good. I will see if I can put together a pptr verifier. Thx! Allison > >> >> >>>> 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 -- 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
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 *)¤tentry->nameval[0], - currententry->namelen)) { + if (!(currententry->flags & XFS_ATTR_PARENT) && + namecheck((char *)¤tentry->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))) { 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_warn( _("inconsistent remote attribute entry %d in attr block %u, ino %" PRIu64 "\n"), i, da_bno, ino); return -1;
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(-)