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

[Raghavendra K T]

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.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/