Re: xattr corruption issue on ext2fs generated filesystems

["Darrick J. Wong" <darrick.wong@oracle.com>]

On Wed, Feb 10, 2016 at 10:20:52AM -0800, Darren Hart wrote:
> On Sat, 2016-02-06 at 11:23 +0000, Richard Purdie wrote:
> > I'm using the -d option of mke2fs to construct a filesystem, I'm
> > seeing
> > that some xattrs are being corrupted. The filesystem builds with no
> > errors but when mounted by the kernel, I see errors like
> > "security.ima:
> > No such attribute". The strace from such a failure is:
> 
> 
> Interesting. +Ted and +Darrick who helped us merge the -d argument
> originally.
> 
> 
> > mmap(NULL, 26258, PROT_READ, MAP_SHARED, 3, 0) = 0x7fdb36a8c000
> > close(3)                    = 0
> > getrlimit(RLIMIT_NOFILE, {rlim_cur=1024, rlim_max=64*1024}) = 0
> > lstat("mnt/foobar", {st_mode=S_IFREG|0755, st_size=1, ...}) = 0
> > listxattr("mnt/foobar", NULL, 0) = 30
> > listxattr("mnt/foobar", "security.SMACK64\0security.ima\0", 256) = 30
> > getxattr("mnt/foobar", "security.SMACK64", 0x0, 0) = 1
> > getxattr("mnt/foobar", "security.SMACK64", "_", 256) = 1
> > fstat(1, {st_mode=S_IFCHR|0620, st_rdev=makedev(136, 13), ...}) = 0
> > mmap(NULL, 4096, PROT_READ|PROT_WRITE, MAP_PRIVATE|MAP_ANONYMOUS, -1,
> > 0) = 0x7fdb36a8b000
> > write(1, "# file: mnt/foobar\n", 19# file: mnt/foobar) = 19
> > write(1, "security.SMACK64=\"_\"\n", 21security.SMACK64="_") = 21
> > getxattr("mnt/foobar", "security.ima", 0x0, 0) = -1 ENODATA (No data
> > available)
> > write(2, "mnt/foobar: ", 12mnt/foobar: ) = 12
> > write(2, "security.ima: No such attribute\n", 32security.ima: No such
> > attribute) = 32= 32

Aha, you're right, the trick is that EAs in an external block have to be sorted
by index number, then by strlen(name), and then by strcmp(name).  Unlike inode
attributes, which can be in any order.

e2fsprogs inserts them in whatever order you happened to set them, which is
whatever order llistxattr provides them.

So, Mr. Purdie's is correct -- attr_compare needs to do more work, but it needs
to grab the index number and the suffix text (via find_ea_index()) and
replicate the same comparison operators as the kernel code.

(Not sure why we bother to sort the keys in the xattr block since there can
only be one block, but whatever...)

--D

> > 
> > so the attribute is there but the kernel gives ENODATA when trying
> > to read it.
> > 
> > http://www.nongnu.org/ext2-doc/ext2.html#CONTRIB-EXTENDED-ATTRIBUTES
> > co
> > ntains the small snippet that " The entry descriptors are sorted by
> > attribute name, so that two extended attribute blocks can be compared
> > efficiently. ". It doesn't specify what kind of sort.
> > 
> > Looking at ext2fs, there is some sorting code through the qsort call
> > using attr_compare() but it doesn't match what the kernel is doing in
> >  ext4_xattr_find_entry().
> >
> > I put together this quick patch to test my theory that this causing
> > the
> > problem:
> > 
> > 
> > This makes my filesystems work.
> > 
> > Is this a bug? I'm assuming ext2fs shouldn't generate filesystems the
> > kernel can't read? Is the above the correct fix?
> > 
> 
> Reviewing the kernel ext4_attr_find_entry():
> 
> ...
> 		if (cmp <= 0 && (sorted || cmp == 0))
> 			break;
> 	}
> 	*pentry = entry;
> 	if (!cmp && ext4_xattr_check_entry(entry, size))
> 		return -EFSCORRUPTED;
> 	return cmp ? -ENODATA : 0;
> ...
> 
> It would seem that a different sorting algorithm would result in the
> kernel interpreting the FS to be corrupted.
> 
> 
> > Cheers,
> > 
> > Richard
> > ---
> > To unsubscribe from this list: send the line "unsubscribe linux-ext4" 
> > in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> > 
> > Index: git/lib/ext2fs/ext_attr.c
> > ===================================================================
> > --- git.orig/lib/ext2fs/ext_attr.c
> > +++ git/lib/ext2fs/ext_attr.c
> > @@ -258,6 +258,7 @@ static struct ea_name_index ea_names[] =
> >  static int attr_compare(const void *a, const void *b)
> >  {
> >  	const struct ext2_xattr *xa = a, *xb = b;
> > +	size_t len;
> >  
> >  	if (xa->name == NULL)
> >  		return +1;
> > @@ -267,7 +268,11 @@ static int attr_compare(const void *a, c
> >  		return -1;
> >  	else if (!strcmp(xb->name, "system.data"))
> >  		return +1;
> > -	return 0;
> > +	len = strlen(xa->name) - strlen(xb->name);
> > +	if (len)
> > +		return len;
> 
> I *think* the index and len comparisons in the kernel are simply
> optimizations to avoid the memcmp, but to properly sort them here, I
> think you can drop the len block above and just return the strcmp
> below.
> 
> Ted, Darrick?
> 
> > +
> > +	return strcmp(xa->name, xb->name);
> >  }
> >  
> >  static const char *find_ea_prefix(int index)
> --
> To unsubscribe from this list: send the line "unsubscribe linux-ext4" 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-ext4" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html