Hi Tao,
FYI, there are new smatch warnings show up in
tree: git://git.kernel.org/pub/scm/linux/kernel/git/tytso/ext4.git dev
head: e9f49e454264c1cdd793552bfeb44f337508201b
commit: 97bb0f99e9d2b047c7a6010771c41d5f0d2ed80c [40/60] ext4: add journalled write support for inline data
+ fs/ext4/inode.c:1953 __ext4_journalled_writepage() error: potential NULL dereference 'page_bufs'.
git remote update ext4
git checkout 97bb0f99e9d2b047c7a6010771c41d5f0d2ed80c
vim +1953 +/page_bufs fs/ext4/inode.c
62e086be Aneesh Kumar K.V 2009-06-14 1937 unlock_page(page);
62e086be Aneesh Kumar K.V 2009-06-14 1938
62e086be Aneesh Kumar K.V 2009-06-14 1939 handle = ext4_journal_start(inode, ext4_writepage_trans_blocks(inode));
62e086be Aneesh Kumar K.V 2009-06-14 1940 if (IS_ERR(handle)) {
62e086be Aneesh Kumar K.V 2009-06-14 1941 ret = PTR_ERR(handle);
62e086be Aneesh Kumar K.V 2009-06-14 1942 goto out;
62e086be Aneesh Kumar K.V 2009-06-14 1943 }
62e086be Aneesh Kumar K.V 2009-06-14 1944
441c8508 Curt Wohlgemuth 2011-08-13 1945 BUG_ON(!ext4_handle_valid(handle));
441c8508 Curt Wohlgemuth 2011-08-13 1946
97bb0f99 Tao Ma 2012-12-02 1947 if (ext4_has_inline_data(inode) && inode_bh) {
If we have inline data but inode_bh is NULL then it will lead to a
NULL dereference. Btw, smatch will still complain about this even
after we fix the code.
97bb0f99 Tao Ma 2012-12-02 1948 ret = ext4_journal_get_write_access(handle, inode_bh);
97bb0f99 Tao Ma 2012-12-02 1949
97bb0f99 Tao Ma 2012-12-02 1950 err = ext4_handle_dirty_metadata(handle, inode, inode_bh);
97bb0f99 Tao Ma 2012-12-02 1951
97bb0f99 Tao Ma 2012-12-02 1952 } else {
97bb0f99 Tao Ma 2012-12-02 @1953 ret = walk_page_buffers(handle, page_bufs, 0, len, NULL,
97bb0f99 Tao Ma 2012-12-02 1954 do_journal_get_write_access);
97bb0f99 Tao Ma 2012-12-02 1955
97bb0f99 Tao Ma 2012-12-02 1956 err = walk_page_buffers(handle, page_bufs, 0, len, NULL,
97bb0f99 Tao Ma 2012-12-02 1957 write_end_fn);
97bb0f99 Tao Ma 2012-12-02 1958 }
62e086be Aneesh Kumar K.V 2009-06-14 1959
62e086be Aneesh Kumar K.V 2009-06-14 1960 if (ret == 0)
62e086be Aneesh Kumar K.V 2009-06-14 1961 ret = err;
---
0-DAY kernel build testing backend Open Source Technology Center
Fengguang Wu, Yuanhan Liu Intel Corporation
Hi Dan,
Thanks for the report. Can you check whether this patch
works for you?
Thanks
Tao
From: Tao Ma <[email protected]>
Subject: [PATCH] ext4: Fix a build warning in __ext4_journalled_writepage.
smatch complains:
fs/ext4/inode.c:1953 __ext4_journalled_writepage() error: potential NULL dereference 'page_bufs'.
So add the check for it.
Reported-by: Dan Carpenter <[email protected]>
Signed-off-by: Tao Ma <[email protected]>
---
fs/ext4/inode.c | 5 ++---
1 files changed, 2 insertions(+), 3 deletions(-)
diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
index dbc5784..431201b 100644
--- a/fs/ext4/inode.c
+++ b/fs/ext4/inode.c
@@ -1956,7 +1956,7 @@ static int __ext4_journalled_writepage(struct page *page,
struct buffer_head *page_bufs = NULL;
handle_t *handle = NULL;
int ret = 0;
- int err;
+ int err = 0;
struct buffer_head *inode_bh = NULL;
ClearPageChecked(page);
@@ -1987,8 +1987,7 @@ static int __ext4_journalled_writepage(struct page *page,
ret = ext4_journal_get_write_access(handle, inode_bh);
err = ext4_handle_dirty_metadata(handle, inode, inode_bh);
On Tue, Dec 04, 2012 at 09:25:45PM +0800, Tao Ma wrote:
> Hi Dan,
> Thanks for the report. Can you check whether this patch
> works for you?
>
It looks good.
Like I mentioned before Smatch doesn't understand
ext4_has_inline_data() so it still complains later on in the
function. But it's now obvious to a human reader that there won't
be a NULL dereference.
regards,
dan carpenter
On Tue, Dec 04, 2012 at 04:56:02PM +0300, Dan Carpenter wrote:
> It looks good.
>
> Like I mentioned before Smatch doesn't understand
> ext4_has_inline_data() so it still complains later on in the
> function. But it's now obvious to a human reader that there won't
> be a NULL dereference.
This is what I plan to fold into the patch. It should make it easier
for gcc to produce optimized code, as well as being easier to
understand. Hopefully this should also keep smatch happy.
- Ted
diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
index 52f715e..ae253a2 100644
--- a/fs/ext4/inode.c
+++ b/fs/ext4/inode.c
@@ -1917,19 +1917,24 @@ static int __ext4_journalled_writepage(struct page *page,
struct inode *inode = mapping->host;
struct buffer_head *page_bufs = NULL;
handle_t *handle = NULL;
- int ret = 0;
- int err;
+ int ret = 0, err = 0;
+ int inline_data = ext4_has_inline_data(inode);
struct buffer_head *inode_bh = NULL;
ClearPageChecked(page);
- if (ext4_has_inline_data(inode)) {
+ if (inline_data) {
BUG_ON(page->index != 0);
BUG_ON(len > ext4_get_max_inline_size(inode));
inode_bh = ext4_journalled_write_inline_data(inode, len, page);
+ if (inode_bh == NULL)
+ goto out;
} else {
page_bufs = page_buffers(page);
- BUG_ON(!page_bufs);
+ if (!page_bufs) {
+ BUG();
+ goto out;
+ }
walk_page_buffers(handle, page_bufs, 0, len, NULL, bget_one);
}
/* As soon as we unlock the page, it can go away, but we have
@@ -1944,7 +1949,7 @@ static int __ext4_journalled_writepage(struct page *page,
BUG_ON(!ext4_handle_valid(handle));
- if (ext4_has_inline_data(inode) && inode_bh) {
+ if (inline_data) {
ret = ext4_journal_get_write_access(handle, inode_bh);
err = ext4_handle_dirty_metadata(handle, inode, inode_bh);
On Wed, Dec 05, 2012 at 12:41:25AM -0500, Theodore Ts'o wrote:
> On Tue, Dec 04, 2012 at 04:56:02PM +0300, Dan Carpenter wrote:
> > It looks good.
> >
> > Like I mentioned before Smatch doesn't understand
> > ext4_has_inline_data() so it still complains later on in the
> > function. But it's now obvious to a human reader that there won't
> > be a NULL dereference.
>
> This is what I plan to fold into the patch. It should make it easier
> for gcc to produce optimized code, as well as being easier to
> understand. Hopefully this should also keep smatch happy.
>
Yeah, moving the check for (inode_bh == NULL) forward is nice.
Thanks.
regards,
dan carpenter
On Wed, Dec 05, 2012 at 11:31:41AM +0300, Dan Carpenter wrote:
>
> Yeah, moving the check for (inode_bh == NULL) forward is nice.
> Thanks.
Not that I really care about optimizing the error path that much, but
getting a starting a handle only to immediately stop it again if
inode_bh == NULL was just silly. :-)
- Ted