[1/3] ext4: do not allow external inodes for inline data

Message ID 20180523153714.28470-1-tytso@mit.edu
State Awaiting Upstream
Headers show
Series
  • [1/3] ext4: do not allow external inodes for inline data
Related show

Commit Message

Theodore Y. Ts'o May 23, 2018, 3:37 p.m.
The inline data feature was implemented before we added support for
external inodes for xattrs.  It makes no sense to support that
combination, but the problem is that there are a number of extended
attribute checks that are skipped if e_value_inum is non-zero.

Unfortunately, the inline data code is completely e_value_inum
unaware, and attempts to interpret the xattr fields as if it were an
inline xattr --- at which point, Hilarty Ensues.

https://bugzilla.kernel.org/show_bug.cgi?id=199803

Reported-by: Jann Horn <jannh@google.com>
Signed-off-by: Theodore Ts'o <tytso@mit.edu>
Fixes: e50e5129f384 ("ext4: xattr-in-inode support")
Cc: stable@kernel.org
---
 fs/ext4/inline.c | 6 ++++++
 1 file changed, 6 insertions(+)

Comments

Andreas Dilger May 24, 2018, 4:45 p.m. | #1
On May 23, 2018, at 9:37 AM, Theodore Ts'o <tytso@mit.edu> wrote:
> 
> The inline data feature was implemented before we added support for
> external inodes for xattrs.  It makes no sense to support that
> combination, but the problem is that there are a number of extended
> attribute checks that are skipped if e_value_inum is non-zero.
> 
> Unfortunately, the inline data code is completely e_value_inum
> unaware, and attempts to interpret the xattr fields as if it were an
> inline xattr --- at which point, Hilarty Ensues.
> 
> https://bugzilla.kernel.org/show_bug.cgi?id=199803
> 
> Reported-by: Jann Horn <jannh@google.com>
> Signed-off-by: Theodore Ts'o <tytso@mit.edu>

Reviewed-by: Andreas Dilger <adilger@dilger.ca>

> Fixes: e50e5129f384 ("ext4: xattr-in-inode support")
> Cc: stable@kernel.org
> ---
> fs/ext4/inline.c | 6 ++++++
> 1 file changed, 6 insertions(+)
> 
> diff --git a/fs/ext4/inline.c b/fs/ext4/inline.c
> index 70cf4c7b268a..44b4fcdc3755 100644
> --- a/fs/ext4/inline.c
> +++ b/fs/ext4/inline.c
> @@ -144,6 +144,12 @@ int ext4_find_inline_data_nolock(struct inode *inode)
> 		goto out;
> 
> 	if (!is.s.not_found) {
> +		if (is.s.here->e_value_inum) {
> +			EXT4_ERROR_INODE(inode, "inline data xattr refers "
> +					 "to an external xattr inode");
> +			error = -EFSCORRUPTED;
> +			goto out;
> +		}
> 		EXT4_I(inode)->i_inline_off = (u16)((void *)is.s.here -
> 					(void *)ext4_raw_inode(&is.iloc));
> 		EXT4_I(inode)->i_inline_size = EXT4_MIN_INLINE_DATA_SIZE +
> --
> 2.16.1.72.g5be1f00a9a
> 


Cheers, Andreas
Andreas Dilger May 24, 2018, 4:51 p.m. | #2
On May 23, 2018, at 9:37 AM, Theodore Ts'o <tytso@mit.edu> wrote:
> 
> The inline data feature was implemented before we added support for
> external inodes for xattrs.  It makes no sense to support that
> combination, but the problem is that there are a number of extended
> attribute checks that are skipped if e_value_inum is non-zero.
> 
> Unfortunately, the inline data code is completely e_value_inum
> unaware, and attempts to interpret the xattr fields as if it were an
> inline xattr --- at which point, Hilarty Ensues.

I guess the separate question is whether this should also check for
"inline data" in an external xattr block and disallow that also?
On the one hand, "inline data" in an external block doesn't make
sense - why not just store the data in a block directly?

On the other hand, this might be useful at some point in the future
if we have large bigalloc chunks that makes using a whole chunk for
a small file inefficient and we could pack the data from multiple
small files into a single bigalloc chunk as inline data xattrs (maybe
using the inode number as xattr name to keep the xattrs unique)?

Cheers, Andreas

> 
> https://bugzilla.kernel.org/show_bug.cgi?id=199803
> 
> Reported-by: Jann Horn <jannh@google.com>
> Signed-off-by: Theodore Ts'o <tytso@mit.edu>
> Fixes: e50e5129f384 ("ext4: xattr-in-inode support")
> Cc: stable@kernel.org
> ---
> fs/ext4/inline.c | 6 ++++++
> 1 file changed, 6 insertions(+)
> 
> diff --git a/fs/ext4/inline.c b/fs/ext4/inline.c
> index 70cf4c7b268a..44b4fcdc3755 100644
> --- a/fs/ext4/inline.c
> +++ b/fs/ext4/inline.c
> @@ -144,6 +144,12 @@ int ext4_find_inline_data_nolock(struct inode *inode)
> 		goto out;
> 
> 	if (!is.s.not_found) {
> +		if (is.s.here->e_value_inum) {
> +			EXT4_ERROR_INODE(inode, "inline data xattr refers "
> +					 "to an external xattr inode");
> +			error = -EFSCORRUPTED;
> +			goto out;
> +		}
> 		EXT4_I(inode)->i_inline_off = (u16)((void *)is.s.here -
> 					(void *)ext4_raw_inode(&is.iloc));
> 		EXT4_I(inode)->i_inline_size = EXT4_MIN_INLINE_DATA_SIZE +
> --
> 2.16.1.72.g5be1f00a9a
> 


Cheers, Andreas
Theodore Y. Ts'o May 24, 2018, 8:20 p.m. | #3
On Thu, May 24, 2018 at 10:51:21AM -0600, Andreas Dilger wrote:
> On May 23, 2018, at 9:37 AM, Theodore Ts'o <tytso@mit.edu> wrote:
> > 
> > The inline data feature was implemented before we added support for
> > external inodes for xattrs.  It makes no sense to support that
> > combination, but the problem is that there are a number of extended
> > attribute checks that are skipped if e_value_inum is non-zero.
> > 
> > Unfortunately, the inline data code is completely e_value_inum
> > unaware, and attempts to interpret the xattr fields as if it were an
> > inline xattr --- at which point, Hilarty Ensues.
> 
> I guess the separate question is whether this should also check for
> "inline data" in an external xattr block and disallow that also?
> On the one hand, "inline data" in an external block doesn't make
> sense - why not just store the data in a block directly?
> 
> On the other hand, this might be useful at some point in the future
> if we have large bigalloc chunks that makes using a whole chunk for
> a small file inefficient and we could pack the data from multiple
> small files into a single bigalloc chunk as inline data xattrs (maybeh
> using the inode number as xattr name to keep the xattrs unique)?

This would require fragment support and I really doubt we would ever
go there.

There's a larger question hiding here which is should we forbid
anything that we we would never encode.  In theory this we could
forbid storing inline data in an external xattr block.  But if we did
do this, the kernel would declare the file system to be corrupted, and
today e2fsck would say the file system is fine.  Even if we did add
the same check to e2fsck, it would mean that here would be file
systems that future kernels would reject which users running the
current versions of e2fsprogs would not be able to repair.

Given that storing inline data in an external xattr block is harmless
(albeit pointless), it's probably not worth it to forbid it.

		    	 	      	    - Ted
Andreas Dilger May 24, 2018, 8:29 p.m. | #4
On May 24, 2018, at 2:20 PM, Theodore Y. Ts'o <tytso@mit.edu> wrote:
> On Thu, May 24, 2018 at 10:51:21AM -0600, Andreas Dilger wrote:
>> On May 23, 2018, at 9:37 AM, Theodore Ts'o <tytso@mit.edu> wrote:
>>> 
>>> The inline data feature was implemented before we added support for
>>> external inodes for xattrs.  It makes no sense to support that
>>> combination, but the problem is that there are a number of extended
>>> attribute checks that are skipped if e_value_inum is non-zero.
>>> 
>>> Unfortunately, the inline data code is completely e_value_inum
>>> unaware, and attempts to interpret the xattr fields as if it were an
>>> inline xattr --- at which point, Hilarty Ensues.
>> 
>> I guess the separate question is whether this should also check for
>> "inline data" in an external xattr block and disallow that also?
>> On the one hand, "inline data" in an external block doesn't make
>> sense - why not just store the data in a block directly?
>> 
>> On the other hand, this might be useful at some point in the future
>> if we have large bigalloc chunks that makes using a whole chunk for
>> a small file inefficient and we could pack the data from multiple
>> small files into a single bigalloc chunk as inline data xattrs (maybeh
>> using the inode number as xattr name to keep the xattrs unique)?
> 
> This would require fragment support and I really doubt we would ever
> go there.

I don't think we would need fragments, just a shared xattr block (chunk)
that stores a bunch of "system.<ino>" xattrs, one for each file, similar
to how inline_data has a "system.data" xattr.  This would be better than
fragments, which are typically fixed at 1/N of the blocksize.  The xattr
size can be pretty arbitrary, and would be more like tail packing than
fragments.

This would be useful for storing small files on bigalloc filesystems,
which is an area that I think bigalloc is lacking today.

Cheers, Andreas

Patch

diff --git a/fs/ext4/inline.c b/fs/ext4/inline.c
index 70cf4c7b268a..44b4fcdc3755 100644
--- a/fs/ext4/inline.c
+++ b/fs/ext4/inline.c
@@ -144,6 +144,12 @@  int ext4_find_inline_data_nolock(struct inode *inode)
 		goto out;
 
 	if (!is.s.not_found) {
+		if (is.s.here->e_value_inum) {
+			EXT4_ERROR_INODE(inode, "inline data xattr refers "
+					 "to an external xattr inode");
+			error = -EFSCORRUPTED;
+			goto out;
+		}
 		EXT4_I(inode)->i_inline_off = (u16)((void *)is.s.here -
 					(void *)ext4_raw_inode(&is.iloc));
 		EXT4_I(inode)->i_inline_size = EXT4_MIN_INLINE_DATA_SIZE +