xattr corruption issue on ext2fs generated filesystems

xattr corruption issue on ext2fs generated filesystems

[Richard Purdie]

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:

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

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:

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;
+
+	return strcmp(xa->name, xb->name);
 }
 
 static const char *find_ea_prefix(int index)

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?

Cheers,

Richard

Re: xattr corruption issue on ext2fs generated filesystems

[Darren Hart]

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
> 
> 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

Re: xattr corruption issue on ext2fs generated filesystems

[Darrick J. Wong]

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

Re: xattr corruption issue on ext2fs generated filesystems

[Darrick J. Wong]

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@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
--
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

Re: xattr corruption issue on ext2fs generated filesystems

[Darren Hart]

On Sat, Feb 13, 2016 at 02:34:59PM -0800, Darrick J. Wong wrote:
> 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?

Thank you very much Darrick!

-- 
Darren Hart
Intel Open Source Technology Center
--
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