On 01/23/2017 06:20 PM, Christoph Hellwig wrote:

On Mon, Jan 23, 2017 at 05:07:52PM +0100, Matias Bj�rling wrote:

quoted

Hi,

I could use some help verifying an use-after-free bug that I am seeing
after the new direct I/O work went in.

When issuing a direct write io using libaio, a bio is referenced in the
blkdev_direct_IO path, and then put again in the blkdev_bio_end_io path.
However, there is a case where the bio is put twice, which leads to
modules that rely on the bio after biodev_bio_end_io() to access it
prematurely.

Can you reproduce anything like this with a normal block device?

Looks like bcache has something similar with a get/put in it. I'll look
a bit more.

quoted

The KASAN error report:

[   14.645916]
==================================================================
[   14.648027] BUG: KASAN: use-after-free in
blkdev_direct_IO+0x50c/0x600 at addr ffff8801ef30ea14

Can you resolve that address for me, please?

*(gdb) list *blkdev_direct_IO+0x50c
0xffffffff8142ab8c is in blkdev_direct_IO (fs/block_dev.c:401).
396			submit_bio(bio);
397			bio = bio_alloc(GFP_KERNEL, nr_pages);
398		}
399		blk_finish_plug(&plug);
400	
401		if (!dio->is_sync)
402			return -EIOCBQUEUED;

It looks like the qc = submit_bio() completes the I/O before the
blkdev_direct_IO completes, which then leads to the use after free case,
when the private dio struct is accessed.

quoted

Which boils down to the bio already being freed, when we hit the
module's endio function.

The double bio_put() occurs when dio->should_dirty = 0, and dio->is_sync
= 0. The flow follows:

Issuing:
  blkdev_direct_IO
     get_bio(bio)

bio refcounting in __blkdev_direct_IO is the following

first bio is allocated using bio_alloc_bioset to embedd the dio structure
into it.  We then grab a second reference to that bio and only that one.
Each other new bio just gets it's single reference from bio_alloc.

blkdev_bio_end_io then checks if we hit the last reference on the dio, in
which case it then drops the additional reference on the first bio after
calling the aio completion handler.  Once that is done it always drops the
reference for the current bio - either explicitly or through
bio_check_pages_dirty in the should_dirty case.

quoted

     submit_io()
       rrpc make_request(bio)
         get_bio(bio)
Completion:
  blkdev_bio_end_io
    bio_put(bio)
    bio_put(bio) - bio is freed

Yes, and that's how it's intended.

quoted

  rrpc_end_io
    bio_put(bio) (use-after-free access)

Can you check this is the same bio that got submitted and it didn't
get bounced somewhere?

Yup, the same:

[11.329950] blkdev_direct_io: get_bio     (bio=ffff8801f1e7a018) get ref
[11.331557] blkdev_direct_io: (!nr_pages) (bio=ffff8801f1e7a018) submit
[11.333603] rrpc bio_get:                 (bio=ffff8801f1e7a018) get ref
[11.335004] blkdev_bio_end_io:!dio->is_syn(bio=ffff8801f1e7a018) put ref
[11.335009] rrpc bio_put:                 (bio=ffff8801f1e7a018) put ref

It could look like the first get_bio() ref is decremented prematurely in
the blkdev_bio_end_io() path, where it should first have been
decremented at the end of blkdev_direct_IO() path.