Skip to contentSkip to conversation

xfs: set XFS_DA_OP_OKNOENT in xfs_attr_get

PATCH In review Eric Sandeen 2 participants 2 messages Jul 23, 2015 XFS filesystem

lore
Share & tools
lore
#0 lore raw

PATCH 1/1 xfs: set XFS_DA_OP_OKNOENT in xfs_attr_get

It's entirely possible for userspace to ask for an xattr which does not exist.

Normally, there is no problem whatsoever when we ask for such a thing, but when we look at an obfuscated metadump image on a debug kernel with selinux, we trip over this ASSERT in xfs_da3_path_shift():

*result = -ENOENT; /* we're out of our tree */
ASSERT(args->op_flags & XFS_DA_OP_OKNOENT);

It (more or less) only shows up in the above scenario, because xfs_metadump obfuscates attr names, but chooses names which keep the same hash value - and xfs_da3_node_lookup_int does:

        if (((retval == -ENOENT) || (retval == -ENOATTR)) &&
            (blk->hashval == args->hashval)) {
                error = xfs_da3_path_shift(state, &state->path, 1, 1,
                                                 &retval);

IOWS, we only get down to the xfs_da3_path_shift() ASSERT if we are looking for an xattr which doesn't exist, but we find xattrs on disk which have the same hash, and so might be a hash collision, so we try the path shift. When *that* fails to find what we're looking for, we hit the assert about XFS_DA_OP_OKNOENT.

Simply setting XFS_DA_OP_OKNOENT in xfs_attr_get solves this rather corner-case problem with no ill side effects. It's fine for an attr name lookup to fail.

---

fs/xfs/libxfs/xfs_attr.c +2 -0
index 3349c9a..ff06557 100644
@@ -139,6 +139,8 @@ xfs_attr_get(
 
 	args.value = value;
 	args.valuelen = *valuelenp;
+	/* Entirely possible to look up a name which doesn't exist */
+	args.op_flags = XFS_DA_OP_OKNOENT;
 
 	lock_mode = xfs_ilock_attr_map_shared(ip);
 	if (!xfs_inode_hasattr(ip))

_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs

#1 lore raw
Quoted 12 lines · Eric Sandeen

It's entirely possible for userspace to ask for an xattr which does not exist.

Normally, there is no problem whatsoever when we ask for such a thing, but when we look at an obfuscated metadump image on a debug kernel with selinux, we trip over this ASSERT in xfs_da3_path_shift():

*result = -ENOENT; /* we're out of our tree */
ASSERT(args->op_flags & XFS_DA_OP_OKNOENT);

Quoted 8 lines

It (more or less) only shows up in the above scenario, because xfs_metadump obfuscates attr names, but chooses names which keep the same hash value - and xfs_da3_node_lookup_int does:

       if (((retval == -ENOENT) || (retval == -ENOATTR)) &&
           (blk->hashval == args->hashval)) {
               error = xfs_da3_path_shift(state, &state->path, 1, 1,
                                                &retval);

Oh, and add: thanks to dchinner for spotting the above conditional that got us there ;)

-Eric

Quoted 28 lines

IOWS, we only get down to the xfs_da3_path_shift() ASSERT if we are looking for an xattr which doesn't exist, but we find xattrs on disk which have the same hash, and so might be a hash collision, so we try the path shift. When *that* fails to find what we're looking for, we hit the assert about XFS_DA_OP_OKNOENT.

Simply setting XFS_DA_OP_OKNOENT in xfs_attr_get solves this rather corner-case problem with no ill side effects. It's fine for an attr name lookup to fail.

---

fs/xfs/libxfs/xfs_attr.c +2 -0
index 3349c9a..ff06557 100644
@@ -139,6 +139,8 @@ xfs_attr_get(
 
   args.value = value;
   args.valuelen = *valuelenp;
+    /* Entirely possible to look up a name which doesn't exist */
+    args.op_flags = XFS_DA_OP_OKNOENT;
 
   lock_mode = xfs_ilock_attr_map_shared(ip);
   if (!xfs_inode_hasattr(ip))
Quoted 5 lines

_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs

_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs