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@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 *)¤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@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
--
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
