Re: [RFC PATCH V2] fs null_blk: Null pointer deference problem in alloc_page_buffers

[Raghavendra K T <raghavendra.kt@linux.vnet.ibm.com>]

On 01/20/2014 10:02 PM, Matias Bjorling wrote:
> On 01/20/2014 04:58 AM, Raghavendra K T wrote:
>>   If we load the null_blk module with bs=8k we get following oops:
>> [ 3819.812190] BUG: unable to handle kernel NULL pointer dereference at 0000000000000008
>> [ 3819.812387] IP: [<ffffffff81170aa5>] create_empty_buffers+0x28/0xaf
>> [ 3819.812527] PGD 219244067 PUD 215a06067 PMD 0
>> [ 3819.812640] Oops: 0000 [#1] SMP
>> [ 3819.812772] Modules linked in: null_blk(+)
>>
>>   Fix that by resetting block size to PAGE_SIZE if it is greater than PAGE_SIZE
>>   Also add sanity checks for block size > PAGE_SIZE.
>
> We should probably split the patch into two. Giving a better description
> to each of the changes.
>

Agreed.

[...]
>> diff --git a/drivers/block/null_blk.c b/drivers/block/null_blk.c
>> index a2e69d2..bcae726 100644
>> --- a/drivers/block/null_blk.c
>> +++ b/drivers/block/null_blk.c
>> @@ -622,6 +622,10 @@ static int __init null_init(void)
>>   		irqmode = NULL_IRQ_NONE;
>>   	}
>>   #endif
>> +	if (bs > PAGE_SIZE) {
>> +		pr_warn("Invalid block size. Setting it to %lu\n", PAGE_SIZE);
>> +		bs = PAGE_SIZE;
>> +	}
>
> Could the warning say something like:
>
> pr_warn("null_blk: invalid block size\n");
> pr_warn("null_blk: defaults block size to &lu\n");
>
> then it follows the same patterns as the other errors.

Agree.

>
>>
>>   	if (queue_mode == NULL_Q_MQ && use_per_node_hctx) {
>>   		if (submit_queues < nr_online_nodes) {
>> diff --git a/fs/block_dev.c b/fs/block_dev.c
>> index 1e86823..2481d42 100644
>> --- a/fs/block_dev.c
>> +++ b/fs/block_dev.c
>> @@ -1027,6 +1027,7 @@ void bd_set_size(struct block_device *bdev, loff_t size)
>>   			break;
>>   		bsize <<= 1;
>>   	}
>> +	BUG_ON(bsize > PAGE_SIZE);
>>   	bdev->bd_block_size = bsize;
>>   	bdev->bd_inode->i_blkbits = blksize_bits(bsize);
>>   }
>> diff --git a/fs/buffer.c b/fs/buffer.c
>> index 6024877..8b7ada1 100644
>> --- a/fs/buffer.c
>> +++ b/fs/buffer.c
>> @@ -1571,6 +1571,7 @@ void create_empty_buffers(struct page *page,
>>   	struct buffer_head *bh, *head, *tail;
>>
>>   	head = alloc_page_buffers(page, blocksize, 1);
>> +	BUG_ON(!head);
>>   	bh = head;
>>   	do {
>>   		bh->b_state |= b_state;
>>
>
> For the check patch, the description could mention some text on why we
> hit this error case and why we check for it.
>

Thanks for the suggestions. Will post V3 with the changes.