* re: ext4: calculate and verify checksums of directory leaf blocks
@ 2012-04-30 11:05 Dan Carpenter
2012-04-30 11:40 ` Ted Ts'o
0 siblings, 1 reply; 2+ messages in thread
From: Dan Carpenter @ 2012-04-30 11:05 UTC (permalink / raw)
To: djwong; +Cc: linux-ext4
Hello Darrick J. Wong,
This is a semi-automatic email about new static checker warnings.
The patch b0336e8d2108: "ext4: calculate and verify checksums of
directory leaf blocks" from Apr 29, 2012, leads to the following
Smatch complaint:
fs/ext4/namei.c:1615 add_dirent_to_buf()
warn: variable dereferenced before check 'inode' (see line 1577)
fs/ext4/namei.c
1575 if (EXT4_HAS_RO_COMPAT_FEATURE(inode->i_sb,
^^^^^^^^^^^
New dereference.
1576 EXT4_FEATURE_RO_COMPAT_METADATA_CSUM))
1577 csum_size = sizeof(struct ext4_dir_entry_tail);
1578
1579 reclen = EXT4_DIR_REC_LEN(namelen);
1580 if (!de) {
1581 de = (struct ext4_dir_entry_2 *)bh->b_data;
1582 top = bh->b_data + (blocksize - csum_size) - reclen;
1583 while ((char *) de <= top) {
1584 if (ext4_check_dir_entry(dir, NULL, de, bh, offset))
1585 return -EIO;
1586 if (ext4_match(namelen, name, de))
1587 return -EEXIST;
1588 nlen = EXT4_DIR_REC_LEN(de->name_len);
1589 rlen = ext4_rec_len_from_disk(de->rec_len, blocksize);
1590 if ((de->inode? rlen - nlen: rlen) >= reclen)
1591 break;
1592 de = (struct ext4_dir_entry_2 *)((char *)de + rlen);
1593 offset += rlen;
1594 }
1595 if ((char *) de > top)
1596 return -ENOSPC;
1597 }
1598 BUFFER_TRACE(bh, "get_write_access");
1599 err = ext4_journal_get_write_access(handle, bh);
1600 if (err) {
1601 ext4_std_error(dir->i_sb, err);
1602 return err;
1603 }
1604
1605 /* By now the buffer is marked for journaling */
1606 nlen = EXT4_DIR_REC_LEN(de->name_len);
1607 rlen = ext4_rec_len_from_disk(de->rec_len, blocksize);
1608 if (de->inode) {
1609 struct ext4_dir_entry_2 *de1 = (struct ext4_dir_entry_2 *)((char *)de + nlen);
1610 de1->rec_len = ext4_rec_len_to_disk(rlen - nlen, blocksize);
1611 de->rec_len = ext4_rec_len_to_disk(nlen, blocksize);
1612 de = de1;
1613 }
1614 de->file_type = EXT4_FT_UNKNOWN;
1615 if (inode) {
^^^^^
Old check.
1616 de->inode = cpu_to_le32(inode->i_ino);
1617 ext4_set_de_type(dir->i_sb, de, inode->i_mode);
regards,
dan carpenter
^ permalink raw reply [flat|nested] 2+ messages in thread* Re: ext4: calculate and verify checksums of directory leaf blocks
2012-04-30 11:05 ext4: calculate and verify checksums of directory leaf blocks Dan Carpenter
@ 2012-04-30 11:40 ` Ted Ts'o
0 siblings, 0 replies; 2+ messages in thread
From: Ted Ts'o @ 2012-04-30 11:40 UTC (permalink / raw)
To: Dan Carpenter; +Cc: djwong, linux-ext4
On Mon, Apr 30, 2012 at 02:05:35PM +0300, Dan Carpenter wrote:
> Hello Darrick J. Wong,
>
> This is a semi-automatic email about new static checker warnings.
>
> The patch b0336e8d2108: "ext4: calculate and verify checksums of
> directory leaf blocks" from Apr 29, 2012, leads to the following
> Smatch complaint:
>
> fs/ext4/namei.c:1615 add_dirent_to_buf()
> warn: variable dereferenced before check 'inode' (see line 1577)
>
> fs/ext4/namei.c
> 1575 if (EXT4_HAS_RO_COMPAT_FEATURE(inode->i_sb,
> ^^^^^^^^^^^
> New dereference.
>
> 1615 if (inode) {
> ^^^^^
> Old check.
>
> 1616 de->inode = cpu_to_le32(inode->i_ino);
> 1617 ext4_set_de_type(dir->i_sb, de, inode->i_mode);
Dan, thanks for the heads up.
It *looks* to me like old check is unnecessary, and the else clause is
dead code that never executes. As near as I can tell none of the
callers of add_dirent_to_buf() ever pass in a NULL inode pointer. And
this tends to be confirmed by the fact that I ran Darrick's patches
through the xfs regression suite, and we never oops over the
dereference at line 1575.
Anyone see something which I missed? As always, a double check would
be appreciated. If not, I plan to add the following patch (see
below).
Thanks,
- Ted
>From dec338b4d903f16c91b588d682f2f6f52cdf795a Mon Sep 17 00:00:00 2001
From: Theodore Ts'o <tytso@mit.edu>
Date: Mon, 30 Apr 2012 07:40:00 -0400
Subject: [PATCH] ext4: remove unnecessary check in add_dirent_to_buf()
None of this function callers ever pass in a NULL inode pointer, so
this check is unnecessary, and the else clause is dead code. (This
change should make the code coverage people a little happier. :-)
Signed-off-by: "Theodore Ts'o" <tytso@mit.edu>
---
fs/ext4/namei.c | 7 ++-----
1 file changed, 2 insertions(+), 5 deletions(-)
diff --git a/fs/ext4/namei.c b/fs/ext4/namei.c
index 5861d64..a9fd5f4 100644
--- a/fs/ext4/namei.c
+++ b/fs/ext4/namei.c
@@ -1612,11 +1612,8 @@ static int add_dirent_to_buf(handle_t *handle, struct dentry *dentry,
de = de1;
}
de->file_type = EXT4_FT_UNKNOWN;
- if (inode) {
- de->inode = cpu_to_le32(inode->i_ino);
- ext4_set_de_type(dir->i_sb, de, inode->i_mode);
- } else
- de->inode = 0;
+ de->inode = cpu_to_le32(inode->i_ino);
+ ext4_set_de_type(dir->i_sb, de, inode->i_mode);
de->name_len = namelen;
memcpy(de->name, name, namelen);
/*
--
1.7.10.rc3
^ permalink raw reply related [flat|nested] 2+ messages in thread
end of thread, other threads:[~2012-04-30 11:41 UTC | newest]
Thread overview: 2+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2012-04-30 11:05 ext4: calculate and verify checksums of directory leaf blocks Dan Carpenter
2012-04-30 11:40 ` Ted 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).