On Jul 12, 2018, at 7:30 AM, Dan Carpenter <dan.carpenter@xxxxxxxxxx> wrote:
>
> Hello Andreas Dilger,
>
> The patch e50e5129f384: "ext4: xattr-in-inode support" from Jun 21,
> 2017, leads to the following static checker warning:
>
> fs/ext4/xattr.c:1393 ext4_xattr_inode_write()
> warn: 'bh' can also be NULL
Initially I thought "IS_ERR_OR_NULL()" or a separate NULL check was
in order.
Looking at this more closely, it doesn't appear that this can actually
happen. Immediately before this code, there is a loop that is calling
ext4_map_blocks() to allocate all of the blocks, and this is only
fetching the bh references again, so the "err == 0" case (no block)
therein cannot be hit.
It is done this way so that all of the allocations can be done at once,
rather than interleaving 1-block allocations and writes.
That said, this is definitely not easily determined by static code
analysis, nor necessarily by the reader.
Possible solutions:
1) add a comment here explaining why bh can't be NULL. In theory, it
might be possible for the block to be deallocated before the call
to ext4_getblk() (e.g. corruption in the small window), but unlikely.
2) add a comment to tell the static analyzer to ignore this case (not
sure if this is possible or not, depends on the tool)
3) add "map_flags = EXT4_GET_BLOCKS_CREATE" argument to ext4_getblk(),
even though it would be redundant, so that the static checker cannot
get into the "return NULL" condition. It would also potentially
avoid the unlikely oops if the filesystem state were corrupted at
the wrong time, though I'm not sure if that is good or bad.
4) have static analysis track filesystem state across ext4_map_blocks()
and ext4_getblk(), but very unlikely (though there *are* some tools
that will do this kind of thing, but they are very different beasts).
I'd be inclined to do #3 and add a comment that EXT4_GET_BLOCKS_CREATE
is just for cosmetic/static checker reasons and isn't actually needed.
Ted, do you have an opinion on this?
Cheers, Andreas
> fs/ext4/xattr.c
> 1380 block = 0;
> 1381 while (wsize < bufsize) {
> 1382 if (bh != NULL)
> 1383 brelse(bh);
> 1384 csize = (bufsize - wsize) > blocksize ? blocksize :
> 1385 bufsize - wsize;
> 1386 bh = ext4_getblk(handle, ea_inode, block, 0);
> 1387 if (IS_ERR(bh))
> 1388 return PTR_ERR(bh);
> 1389 ret = ext4_journal_get_write_access(handle, bh);
> 1390 if (ret)
> 1391 goto out;
> 1392
> 1393 memcpy(bh->b_data, buf, csize);
>
> I don't know the code well enought to say if it's an issue. I really
> wish there were comments when functions return a mix of error pointers
> and NULL.
>
> 1394 set_buffer_uptodate(bh);
> 1395 ext4_handle_dirty_metadata(handle, ea_inode, bh);
> 1396
> 1397 buf += csize;
> 1398 wsize += csize;
> 1399 block += 1;
> 1400 }
>
> regards,
> dan carpenter
Cheers, Andreas
Attachment:
signature.asc
Description: Message signed with OpenPGP
