fs/ext4/inode.c:1953 __ext4_journalled_writepage() error: potential NULL dereference 'page_bufs'.

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.

---
 fs/ext4/inode.c |    5 ++---
 1 files changed, 2 insertions(+), 3 deletions(-)

index dbc5784..431201b 100644
@@ -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);

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

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

index 52f715e..ae253a2 100644
@@ -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);

Yeah, moving the check for (inode_bh == NULL) forward is nice. Thanks.

regards,
dan carpenter

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