[PATCH] ext4: missing !bh check in ext4_xattr_inode_write()

[PATCH] ext4: missing !bh check in ext4_xattr_inode_write()

[Vasily Averin]

ext4_getblk() called with map_flags=0 can return NULL,
it can lead to oops on bh dereferemce

Fixes e50e5129f384 ("ext4: xattr-in-inode support")
Cc: stable@kernel.org # 4.13

Signed-off-by: Vasily Averin <vvs@virtuozzo.com>
---
 fs/ext4/xattr.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/fs/ext4/xattr.c b/fs/ext4/xattr.c
index 0b9688683526..6dc6c70828f0 100644
--- a/fs/ext4/xattr.c
+++ b/fs/ext4/xattr.c
@@ -1384,6 +1384,8 @@ static int ext4_xattr_inode_write(handle_t *handle, struct inode *ea_inode,
 		bh = ext4_getblk(handle, ea_inode, block, 0);
 		if (IS_ERR(bh))
 			return PTR_ERR(bh);
+		if (!bh)
+			return -ENOMEM;
 		ret = ext4_journal_get_write_access(handle, bh);
 		if (ret)
 			goto out;
-- 
2.17.1

Re: [PATCH] ext4: missing !bh check in ext4_xattr_inode_write()

[Theodore Y. Ts'o]

On Sun, Nov 04, 2018 at 08:29:39PM +0300, Vasily Averin wrote:
> ext4_getblk() called with map_flags=0 can return NULL,
> it can lead to oops on bh dereferemce
> 
> Fixes e50e5129f384 ("ext4: xattr-in-inode support")
> Cc: stable@kernel.org # 4.13
> 
> Signed-off-by: Vasily Averin <vvs@virtuozzo.com>
> ---
>  fs/ext4/xattr.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/fs/ext4/xattr.c b/fs/ext4/xattr.c
> index 0b9688683526..6dc6c70828f0 100644
> --- a/fs/ext4/xattr.c
> +++ b/fs/ext4/xattr.c
> @@ -1384,6 +1384,8 @@ static int ext4_xattr_inode_write(handle_t *handle, struct inode *ea_inode,
>  		bh = ext4_getblk(handle, ea_inode, block, 0);
>  		if (IS_ERR(bh))
>  			return PTR_ERR(bh);
> +		if (!bh)
> +			return -ENOMEM;

ext4_getblk() should never return NULL here; and if it does, it won't
be because of ENOMEM.  If we did have a memory allocation problem, we
would have returned ERR_PTR(-ENOMEM). 

In the while loop above this point, the blocks in ea_inode would have
been allocated, and so when ext4_getblk() is called with map_flags ==
0, it will return NULL if there is a hole in the inode --- but we had
*just* allocated the blocks in question.

The only time that bh could be NULL, then, would be in the case of
something really going wrong; a programming error elsewhere (perhaps a
wild pointer dereference) or I/O error causing on-disk file system
corruption (although that would be highly unlikely given that we had
*just* allocated the blocks and so the metadata blocks in question
probably would still be in the cache).

If we do want to handle this case, what we should do is something like
call WARN_ON_ONCE(1), call ext4_error() since the file system may have
gotten corrupted, and then return -EFSCORRUPTED.

(Linus has said that there should be no new BUG_ON's, so the
WARN_ON_ONE is to help debugging, and flaging the file system as
corrupted is probably the best we could do here, and is marginally
better than what we do now, which is just let the Oops take place.)

						- Ted

[PATCH v2] ext4: missing !bh check in ext4_xattr_inode_write()

[Vasily Averin]

According to Ted Ts'o ext4_getblk() called in ext4_xattr_inode_write()
should not return bh = NULL

The only time that bh could be NULL, then, would be in the case of
something really going wrong; a programming error elsewhere (perhaps a
wild pointer dereference) or I/O error causing on-disk file system
corruption (although that would be highly unlikely given that we had
*just* allocated the blocks and so the metadata blocks in question
probably would still be in the cache).

Fixes e50e5129f384 ("ext4: xattr-in-inode support")
Cc: stable@kernel.org # 4.13

Signed-off-by: Vasily Averin <vvs@virtuozzo.com>
---
 fs/ext4/xattr.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/fs/ext4/xattr.c b/fs/ext4/xattr.c
index 0b9688683526..415f73d4c9e6 100644
--- a/fs/ext4/xattr.c
+++ b/fs/ext4/xattr.c
@@ -1384,6 +1384,12 @@ static int ext4_xattr_inode_write(handle_t *handle, struct inode *ea_inode,
 		bh = ext4_getblk(handle, ea_inode, block, 0);
 		if (IS_ERR(bh))
 			return PTR_ERR(bh);
+		if (!bh) {
+			WARN_ON_ONCE(1);
+			__ext4_error_inode(ea_inode, __func__, __LINE__, 0,
+					   "ext4_getblk() return bh = NULL");
+			return -EFSCORRUPTED;
+		}
 		ret = ext4_journal_get_write_access(handle, bh);
 		if (ret)
 			goto out;
-- 
2.17.1

Re: [PATCH v2] ext4: missing !bh check in ext4_xattr_inode_write()

[Theodore Y. Ts'o]

On Thu, Nov 08, 2018 at 09:46:30AM +0300, Vasily Averin wrote:
> diff --git a/fs/ext4/xattr.c b/fs/ext4/xattr.c
> index 0b9688683526..415f73d4c9e6 100644
> --- a/fs/ext4/xattr.c
> +++ b/fs/ext4/xattr.c
> @@ -1384,6 +1384,12 @@ static int ext4_xattr_inode_write(handle_t *handle, struct inode *ea_inode,
>  		bh = ext4_getblk(handle, ea_inode, block, 0);
>  		if (IS_ERR(bh))
>  			return PTR_ERR(bh);
> +		if (!bh) {
> +			WARN_ON_ONCE(1);
> +			__ext4_error_inode(ea_inode, __func__, __LINE__, 0,
> +					   "ext4_getblk() return bh = NULL");

You should use EXT4_ERROR_INODE(), defined in ext4.h, not
__ext4_error_inode().  That way you don't need to explicitly specify
__func__ and __LINE__, and we the compiler will correctly do printf
format checking even when CONFIG_PRINTK is not set.

						- Ted

[PATCH v3] ext4: missing !bh check in ext4_xattr_inode_write()

[Vasily Averin]

According to Ted Ts'o ext4_getblk() called in ext4_xattr_inode_write()
should not return bh = NULL

The only time that bh could be NULL, then, would be in the case of
something really going wrong; a programming error elsewhere (perhaps a
wild pointer dereference) or I/O error causing on-disk file system
corruption (although that would be highly unlikely given that we had
*just* allocated the blocks and so the metadata blocks in question
probably would still be in the cache).

Fixes e50e5129f384 ("ext4: xattr-in-inode support")
Cc: stable@kernel.org # 4.13

Signed-off-by: Vasily Averin <vvs@virtuozzo.com>
---
 fs/ext4/xattr.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/fs/ext4/xattr.c b/fs/ext4/xattr.c
index 0b9688683526..7643d52c776c 100644
--- a/fs/ext4/xattr.c
+++ b/fs/ext4/xattr.c
@@ -1384,6 +1384,12 @@ static int ext4_xattr_inode_write(handle_t *handle, struct inode *ea_inode,
 		bh = ext4_getblk(handle, ea_inode, block, 0);
 		if (IS_ERR(bh))
 			return PTR_ERR(bh);
+		if (!bh) {
+			WARN_ON_ONCE(1);
+			EXT4_ERROR_INODE(ea_inode,
+					 "ext4_getblk() return bh = NULL");
+			return -EFSCORRUPTED;
+		}
 		ret = ext4_journal_get_write_access(handle, bh);
 		if (ret)
 			goto out;
-- 
2.17.1

Re: [PATCH v3] ext4: missing !bh check in ext4_xattr_inode_write()

[Theodore Y. Ts'o]

On Fri, Nov 09, 2018 at 08:49:48AM +0300, Vasily Averin wrote:
> According to Ted Ts'o ext4_getblk() called in ext4_xattr_inode_write()
> should not return bh = NULL
> 
> The only time that bh could be NULL, then, would be in the case of
> something really going wrong; a programming error elsewhere (perhaps a
> wild pointer dereference) or I/O error causing on-disk file system
> corruption (although that would be highly unlikely given that we had
> *just* allocated the blocks and so the metadata blocks in question
> probably would still be in the cache).
> 
> Fixes e50e5129f384 ("ext4: xattr-in-inode support")
> Cc: stable@kernel.org # 4.13
> 
> Signed-off-by: Vasily Averin <vvs@virtuozzo.com>

Thanks, applied.

						- Ted