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