On 9/17/12 9:55 AM, Eric Sandeen wrote:
> On 9/15/12 1:30 PM, Theodore Ts'o wrote:
>> On Mon, Sep 10, 2012 at 11:55:48AM -0000, Carlos Maiolino wrote:
>>> htree_dirblock_to_tree() declares a non-initialized 'err' variable,
>>> which is passed as a reference to another functions expecting them
>>> to set this variable with thei error codes. It's passed to
>>> ext4_bread(), which then passes it to ext4_getblk(). If
>>> ext4_map_blocks() returns 0 due to a lookup failure, leaving the
>>> ext4_getblk() buffer_head uninitialized, it will make ext4_getblk()
>>> return to ext4_bread() without initialize the 'err' variable, and
>>> ext4_bread() will return to htree_dirblock_to_tree() with this
>>> variable still uninitialized.
>>
>> Hi Carlos,
>>
>> Thanks for noticing this problem!
>>
>> In the case where there is no block mapping for a particular block,
>> ext4_bread() can return NULL, and with your patch, *err will now be
>> zero instead of some uninitialized value. That's an improvement, and
>> in the case of htree_dirblock_to_tree(), when we return 0 as an
>> "error", the caller will do the right thing.
>
> Hm, sorry, I had counseled Carlos to do that. I figured a bmap
> call w/o create set, returning a NULL bh was perfectly valid - it simply
> means that it's not mapped there, right? - so a 0 retval made sense
> to me.
fwiw, the uninit variable came about as part of
2ed886852adfcb070bf350e66a0da0d98b2f3ab5; before that we happily returned
0 for an unmapped block; see below. So unless something else has changed
since then, Carlos' patch shouldn't be doing any harm, at least. An
audit may be in order but anyone misunderstanding a NULL/0 return has probably
been that way for a while.
struct buffer_head *ext4_getblk(handle_t *handle, struct inode *inode,
ext4_lblk_t block, int create, int *errp)
{
struct buffer_head dummy;
int fatal = 0, err;
int flags = 0;
J_ASSERT(handle != NULL || create == 0);
dummy.b_state = 0;
dummy.b_blocknr = -1000;
buffer_trace_init(&dummy.b_history);
if (create)
flags |= EXT4_GET_BLOCKS_CREATE;
err = ext4_get_blocks(handle, inode, block, 1, &dummy, flags);
/*
* ext4_get_blocks() returns number of blocks mapped. 0 in
* case of a HOLE.
*/
if (err > 0) {
if (err > 1)
WARN_ON(1);
err = 0;
}
*errp = err;
--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
