Date:	Sat, 13 Feb 2016 14:34:59 -0800
From:	"Darrick J. Wong" <darrick.wong@...cle.com>
To:	Richard Purdie <richard.purdie@...uxfoundation.org>
Cc:	Darren Hart <dvhart@...ux.intel.com>, linux-ext4@...r.kernel.org,
	"Ted Ts'o" <tytso@....edu>
Subject: Re: xattr corruption issue on ext2fs generated filesystems

On Sat, Feb 13, 2016 at 01:29:55PM -0800, Darrick J. Wong wrote:
> 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...)

A patch to (I hope) fix this issue will appear shortly as patch #9 in
my e2fsprogs patchbomb.  When it appears, can you please give it a spin?

--D

> 
> --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@...r.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@...r.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@...r.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@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Powered by blists - more mailing lists