From: "Darrick J. Wong" Subject: Re: xattr corruption issue on ext2fs generated filesystems Date: Sat, 13 Feb 2016 14:34:59 -0800 Message-ID: <20160213223459.GE6338@birch.djwong.org> References: <1454757826.27087.300.camel@linuxfoundation.org> <1455128452.24036.38.camel@linux.intel.com> <20160213212955.GD6338@birch.djwong.org> Mime-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Transfer-Encoding: QUOTED-PRINTABLE Cc: Darren Hart , linux-ext4@vger.kernel.org, "Ted Ts'o" To: Richard Purdie Return-path: Received: from userp1040.oracle.com ([156.151.31.81]:35428 "EHLO userp1040.oracle.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751383AbcBMWfL (ORCPT ); Sat, 13 Feb 2016 17:35:11 -0500 Content-Disposition: inline In-Reply-To: <20160213212955.GD6338@birch.djwong.org> Sender: linux-ext4-owner@vger.kernel.org List-ID: 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: > >=20 > >=20 > > Interesting. +Ted and +Darrick who helped us merge the -d argument > > originally. > >=20 > >=20 > > > mmap(NULL, 26258, PROT_READ, MAP_SHARED, 3, 0) =3D 0x7fdb36a8c000 > > > close(3)=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0= =A0=3D 0 > > > getrlimit(RLIMIT_NOFILE, {rlim_cur=3D1024, rlim_max=3D64*1024}) =3D= 0 > > > lstat("mnt/foobar", {st_mode=3DS_IFREG|0755, st_size=3D1, ...}) =3D= 0 > > > listxattr("mnt/foobar", NULL, 0) =3D 30 > > > listxattr("mnt/foobar", "security.SMACK64\0security.ima\0", 256) = =3D 30 > > > getxattr("mnt/foobar", "security.SMACK64", 0x0, 0) =3D 1 > > > getxattr("mnt/foobar", "security.SMACK64", "_", 256) =3D 1 > > > fstat(1, {st_mode=3DS_IFCHR|0620, st_rdev=3Dmakedev(136, 13), ...= }) =3D 0 > > > mmap(NULL, 4096, PROT_READ|PROT_WRITE, MAP_PRIVATE|MAP_ANONYMOUS,= -1, > > > 0) =3D 0x7fdb36a8b000 > > > write(1, "# file: mnt/foobar\n", 19# file: mnt/foobar) =3D 19 > > > write(1, "security.SMACK64=3D\"_\"\n", 21security.SMACK64=3D"_") = =3D 21 > > > getxattr("mnt/foobar", "security.ima", 0x0, 0) =3D -1 ENODATA (No= data > > > available) > > > write(2, "mnt/foobar: ", 12mnt/foobar: ) =3D 12 > > > write(2, "security.ima: No such attribute\n", 32security.ima: No = such > > > attribute) =3D 32=3D 32 >=20 > 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). Unl= ike inode > attributes, which can be in any order. >=20 > e2fsprogs inserts them in whatever order you happened to set them, wh= ich is > whatever order llistxattr provides them. >=20 > So, Mr. Purdie's is correct -- attr_compare needs to do more work, bu= t it needs > to grab the index number and the suffix text (via find_ea_index()) an= d > replicate the same comparison operators as the kernel code. >=20 > (Not sure why we bother to sort the keys in the xattr block since the= re 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 >=20 > --D >=20 > > >=20 > > > so the attribute is there but the kernel gives ENODATA when tryin= g > > > to read it. > > >=20 > > > http://www.nongnu.org/ext2-doc/ext2.html#CONTRIB-EXTENDED-ATTRIBU= TES > > > co > > > ntains the small snippet that " The entry descriptors are sorted = by > > > attribute name, so that two extended attribute blocks can be comp= ared > > > efficiently. ". It doesn't specify what kind of sort. > > >=20 > > > Looking at ext2fs, there is some sorting code through the qsort c= all > > > using attr_compare() but it doesn't match what the kernel is doin= g in > > > =A0ext4_xattr_find_entry(). > > > > > > I put together this quick patch to test my theory that this causi= ng > > > the > > > problem: > > >=20 > > >=20 > > > This makes my filesystems work. > > >=20 > > > Is this a bug? I'm assuming ext2fs shouldn't generate filesystems= the > > > kernel can't read? Is the above the correct fix? > > >=20 > >=20 > > Reviewing the kernel ext4_attr_find_entry(): > >=20 > > ... > > if (cmp <=3D 0 && (sorted || cmp =3D=3D 0)) > > break; > > } > > *pentry =3D entry; > > if (!cmp && ext4_xattr_check_entry(entry, size)) > > return -EFSCORRUPTED; > > return cmp ? -ENODATA : 0; > > ... > >=20 > > It would seem that a different sorting algorithm would result in th= e > > kernel interpreting the FS to be corrupted. > >=20 > >=20 > > > Cheers, > > >=20 > > > Richard > > > --- > > > To unsubscribe from this list: send the line "unsubscribe linux-e= xt4"=20 > > > in > > > the body of a message to majordomo@vger.kernel.org > > > More majordomo info at=A0=A0http://vger.kernel.org/majordomo-info= =2Ehtml > > >=20 > > > Index: git/lib/ext2fs/ext_attr.c > > > =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D > > > --- git.orig/lib/ext2fs/ext_attr.c > > > +++ git/lib/ext2fs/ext_attr.c > > > @@ -258,6 +258,7 @@ static struct ea_name_index ea_names[] =3D > > > =A0static int attr_compare(const void *a, const void *b) > > > =A0{ > > > =A0 const struct ext2_xattr *xa =3D a, *xb =3D b; > > > + size_t len; > > > =A0 > > > =A0 if (xa->name =3D=3D NULL) > > > =A0 return +1; > > > @@ -267,7 +268,11 @@ static int attr_compare(const void *a, c > > > =A0 return -1; > > > =A0 else if (!strcmp(xb->name, "system.data")) > > > =A0 return +1; > > > - return 0; > > > + len =3D strlen(xa->name) - strlen(xb->name); > > > + if (len) > > > + return len; > >=20 > > 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. > >=20 > > Ted, Darrick? > >=20 > > > + > > > + return strcmp(xa->name, xb->name); > > > =A0} > > > =A0 > > > =A0static const char *find_ea_prefix(int index) > > -- > > To unsubscribe from this list: send the line "unsubscribe linux-ext= 4" 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 -- To unsubscribe from this list: send the line "unsubscribe linux-ext4" i= n the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html