Skip to contentSkip to conversation

xfsprogs: some things aren't all that special

PATCH In review Alex Elder 2 participants 3 messages Dec 30, 2010 – Jan 10, 2011

lore
Share & tools
lore

Alex Elder

author
#0 lore raw

PATCH 1/12 xfsprogs: some things aren't all that special

Move the check for short file names out of is_special_dirent() and into generate_obfuscated_name(). That way the check is more directly associated with the algorithm that requires it.

Similarly, move the check for inode == 0, since that case has to do with storing extended attributes (not files) in the name table.

As a result, is_special_dirent() is really only focused on whether a given file is in the lost+found directory.

Rename is_special_dirent() to reflect its more specific purpose.

And use a cast to elminate a compile warning in calls to libxfs_da_hashname().

---
 db/metadump.c |   51 +++++++++++++++++++++++++++++----------------------
 1 file changed, 29 insertions(+), 22 deletions(-)
db/metadump.c +29 -22
Index: b/db/metadump.c
===================================================================
@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2007 Silicon Graphics, Inc.
+ * Copyright (c) 2007,2010 SGI
  * All Rights Reserved.
  *
  * This program is free software; you can redistribute it and/or
@@ -377,8 +377,15 @@ random_filename_char(void)
 	return c;
 }
 
+/*
+ * We won't obfuscate "lost+found" nor any inodes within it.
+ *
+ * Record the "lost+found" directory's inode number when it's found.
+ * While processing that directory, any file whose name matches
+ * its inode number is left as-is (no obfuscation).
+ */
 static int
-is_special_dirent(
+in_lost_found(
 	xfs_ino_t		ino,
 	int			namelen,
 	uchar_t			*name)
@@ -387,22 +394,8 @@ is_special_dirent(
 	char			s[32];
 	int			slen;
 
-	/*
-	 * due to the XFS name hashing algorithm, we cannot obfuscate
-	 * names with 4 chars or less.
-	 */
-	if (namelen <= 4)
-		return 1;
-
-	if (ino == 0)
-		return 0;
-
-	/*
-	 * don't obfuscate lost+found nor any inodes within lost+found with
-	 * the inode number
-	 */
-	if (cur_ino == mp->m_sb.sb_rootino && namelen == 10 &&
-			memcmp(name, "lost+found", 10) == 0) {
+	if (!orphanage_ino && cur_ino == mp->m_sb.sb_rootino &&
+			namelen == 10 && !memcmp(name, "lost+found", 10)) {
 		orphanage_ino = ino;
 		return 1;
 	}
@@ -410,7 +403,8 @@ is_special_dirent(
 		return 0;
 
 	slen = sprintf(s, "%lld", (long long)ino);
-	return (slen == namelen && memcmp(name, s, namelen) == 0);
+
+	return slen == namelen && !memcmp(name, s, namelen);
 }
 
 static void
@@ -426,10 +420,23 @@ generate_obfuscated_name(
 	xfs_dahash_t		newhash;
 	uchar_t			newname[NAME_MAX];
 
-	if (is_special_dirent(ino, namelen, name))
+	/*
+	 * Our obfuscation algorithm requires at least 5-character
+	 * names, so don't bother if the name is too short.
+	 */
+	if (namelen < 5)
+	    	return;
+
+	/*
+	 * We don't obfuscate "lost+found" or any of the files
+	 * therein.  When the name table is used for extended
+	 * attributes, the inode number provided is 0, in which
+	 * case we don't need to make this check.
+	 */
+	if (ino && in_lost_found(ino, namelen, name))
 		return;
 
-	hash = libxfs_da_hashname(name, namelen);
+	hash = libxfs_da_hashname((char *) name, namelen);
 
 	/* create a random name with the same hash value */
 
@@ -468,7 +475,7 @@ generate_obfuscated_name(
 			break;
 		}
 
-		ASSERT(libxfs_da_hashname(newname, namelen) == hash);
+		ASSERT(libxfs_da_hashname((char *) newname, namelen) == hash);
 
 		for (p = nametable[hash % NAME_TABLE_SIZE]; p; p = p->next) {
 			if (p->hash == hash && p->namelen == namelen &&

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

Reviewed-by on v1 1/12
#1 lore raw
Quoted 7 lines · Alex Elder

Move the check for short file names out of is_special_dirent() and into generate_obfuscated_name(). That way the check is more directly associated with the algorithm that requires it.

Similarly, move the check for inode == 0, since that case has to do with storing extended attributes (not files) in the name table.

Quoted 7 lines

As a result, is_special_dirent() is really only focused on whether a given file is in the lost+found directory.

Rename is_special_dirent() to reflect its more specific purpose.

And use a cast to elminate a compile warning in calls to libxfs_da_hashname().

I think Dave's resync of libxfs takes care of that bit. Either way it really should be a separate patch.

Also you've stopped overwriting orphanage_ino if it already exists. I don't think this really matters as we won't have two inodes with the same name below the root inode. Either way it should be documented in the changelog.

Otherwise the change looks good,

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

author
#2 lore raw
Quoted 1 line · Christoph Hellwig

Also you've stopped overwriting orphanage_ino if it already exists.

Actually, I had noticed I did more (or worse) than just that. Once orphanage_ino has been saved, my change no longer says it should avoid obfuscating it. (Or is that what you mean by "overwriting"?)

Anyway, I noticed that problem the other day and thought I'd fix it if I had a chance to re-post.

Thanks.

-Alex

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