linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] ext4: missing !bh check in ext4_xattr_inode_write()
@ 2018-11-04 17:29 Vasily Averin
  2018-11-07 16:33 ` Theodore Y. Ts'o
  0 siblings, 1 reply; 6+ messages in thread
From: Vasily Averin @ 2018-11-04 17:29 UTC (permalink / raw)
  To: linux-ext4, Theodore Ts'o, Andreas Dilger; +Cc: linux-kernel

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



^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] ext4: missing !bh check in ext4_xattr_inode_write()
  2018-11-04 17:29 [PATCH] ext4: missing !bh check in ext4_xattr_inode_write() Vasily Averin
@ 2018-11-07 16:33 ` Theodore Y. Ts'o
  2018-11-08  6:46   ` [PATCH v2] " Vasily Averin
  0 siblings, 1 reply; 6+ messages in thread
From: Theodore Y. Ts'o @ 2018-11-07 16:33 UTC (permalink / raw)
  To: Vasily Averin; +Cc: linux-ext4, Andreas Dilger, linux-kernel

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

^ permalink raw reply	[flat|nested] 6+ messages in thread

* [PATCH v2] ext4: missing !bh check in ext4_xattr_inode_write()
  2018-11-07 16:33 ` Theodore Y. Ts'o
@ 2018-11-08  6:46   ` Vasily Averin
  2018-11-08 14:54     ` Theodore Y. Ts'o
  0 siblings, 1 reply; 6+ messages in thread
From: Vasily Averin @ 2018-11-08  6:46 UTC (permalink / raw)
  To: linux-ext4, Theodore Ts'o, Andreas Dilger; +Cc: linux-kernel

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


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH v2] ext4: missing !bh check in ext4_xattr_inode_write()
  2018-11-08  6:46   ` [PATCH v2] " Vasily Averin
@ 2018-11-08 14:54     ` Theodore Y. Ts'o
  2018-11-09  5:49       ` [PATCH v3] " Vasily Averin
  0 siblings, 1 reply; 6+ messages in thread
From: Theodore Y. Ts'o @ 2018-11-08 14:54 UTC (permalink / raw)
  To: Vasily Averin; +Cc: linux-ext4, Andreas Dilger, linux-kernel

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

^ permalink raw reply	[flat|nested] 6+ messages in thread

* [PATCH v3] ext4: missing !bh check in ext4_xattr_inode_write()
  2018-11-08 14:54     ` Theodore Y. Ts'o
@ 2018-11-09  5:49       ` Vasily Averin
  2018-11-09 16:42         ` Theodore Y. Ts'o
  0 siblings, 1 reply; 6+ messages in thread
From: Vasily Averin @ 2018-11-09  5:49 UTC (permalink / raw)
  To: linux-ext4, Theodore Ts'o, Andreas Dilger; +Cc: linux-kernel

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


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH v3] ext4: missing !bh check in ext4_xattr_inode_write()
  2018-11-09  5:49       ` [PATCH v3] " Vasily Averin
@ 2018-11-09 16:42         ` Theodore Y. Ts'o
  0 siblings, 0 replies; 6+ messages in thread
From: Theodore Y. Ts'o @ 2018-11-09 16:42 UTC (permalink / raw)
  To: Vasily Averin; +Cc: linux-ext4, Andreas Dilger, linux-kernel

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

^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2018-11-09 16:42 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-11-04 17:29 [PATCH] ext4: missing !bh check in ext4_xattr_inode_write() Vasily Averin
2018-11-07 16:33 ` Theodore Y. Ts'o
2018-11-08  6:46   ` [PATCH v2] " Vasily Averin
2018-11-08 14:54     ` Theodore Y. Ts'o
2018-11-09  5:49       ` [PATCH v3] " Vasily Averin
2018-11-09 16:42         ` Theodore Y. Ts'o

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).