[PATCH] dm-bufio: fix memleak when using a dm_buffer's inline bio

[PATCH] dm-bufio: fix memleak when using a dm_buffer's inline bio

[Darrick J. Wong]

When dm-bufio sets out to use the bio built into a struct dm_buffer to
issue an IO, it needs to call bio_reset after it's done with the bio
so that we can free things attached to the bio such as the integrity
payload.  Therefore, inject our own endio callback to take care of
the bio_reset after calling submit_io's end_io callback.

Test case:
1. modprobe scsi_debug delay=0 dif=1 dix=199 ato=1 dev_size_mb=300
2. Set up a dm-bufio client, e.g. dm-verity, on the scsi_debug device
3. Repeatedly read metadata and watch kmalloc-192 leak!

Fix is against 3.18-rc6.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 drivers/md/dm-bufio.c |   13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

diff --git a/drivers/md/dm-bufio.c b/drivers/md/dm-bufio.c
index afe7971..2967ecc 100644
--- a/drivers/md/dm-bufio.c
+++ b/drivers/md/dm-bufio.c
@@ -532,6 +532,16 @@ static void use_dmio(struct dm_buffer *b, int rw, sector_t block,
 		end_io(&b->bio, r);
 }
 
+/* Reset the bio to free attached bio integrity profiles when we're done */
+static void inline_endio(struct bio *bio, int error)
+{
+	bio_end_io_t *end_fn;
+
+	end_fn = bio->bi_private;
+	end_fn(bio, error);
+	bio_reset(bio);
+}
+
 static void use_inline_bio(struct dm_buffer *b, int rw, sector_t block,
 			   bio_end_io_t *end_io)
 {
@@ -543,7 +553,8 @@ static void use_inline_bio(struct dm_buffer *b, int rw, sector_t block,
 	b->bio.bi_max_vecs = DM_BUFIO_INLINE_VECS;
 	b->bio.bi_iter.bi_sector = block << b->c->sectors_per_block_bits;
 	b->bio.bi_bdev = b->c->bdev;
-	b->bio.bi_end_io = end_io;
+	b->bio.bi_end_io = inline_endio;
+	b->bio.bi_private = end_io;
 
 	/*
 	 * We assume that if len >= PAGE_SIZE ptr is page-aligned.

Re: dm-bufio: fix memleak when using a dm_buffer's inline bio

[Mike Snitzer]

On Tue, Nov 25 2014 at  8:45pm -0500,
Darrick J. Wong <darrick.wong@oracle.com> wrote:

> When dm-bufio sets out to use the bio built into a struct dm_buffer to
> issue an IO, it needs to call bio_reset after it's done with the bio
> so that we can free things attached to the bio such as the integrity
> payload.  Therefore, inject our own endio callback to take care of
> the bio_reset after calling submit_io's end_io callback.
> 
> Test case:
> 1. modprobe scsi_debug delay=0 dif=1 dix=199 ato=1 dev_size_mb=300
> 2. Set up a dm-bufio client, e.g. dm-verity, on the scsi_debug device
> 3. Repeatedly read metadata and watch kmalloc-192 leak!
> 
> Fix is against 3.18-rc6.
> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>

Thanks for reporting/fixing this.

Alternatively I think we could just call bio_reset() in submit_io(),
e.g.:

diff --git a/drivers/md/dm-bufio.c b/drivers/md/dm-bufio.c
index afe7971..e7036e3 100644
--- a/drivers/md/dm-bufio.c
+++ b/drivers/md/dm-bufio.c
@@ -579,6 +579,8 @@ static void submit_io(struct dm_buffer *b, int rw, sector_t block,
 	if (rw == WRITE && b->c->write_callback)
 		b->c->write_callback(b);
 
+	bio_reset(&b->bio);
+
 	if (b->c->block_size <= DM_BUFIO_INLINE_VECS * PAGE_SIZE &&
 	    b->data_mode != DATA_MODE_VMALLOC)
 		use_inline_bio(b, rw, block, end_io);

What do you think?

Re: [dm-devel] dm-bufio: fix memleak when using a dm_buffer's inline bio

[Darrick J. Wong]

On Tue, Nov 25, 2014 at 10:41:04PM -0500, Mike Snitzer wrote:
> On Tue, Nov 25 2014 at  8:45pm -0500,
> Darrick J. Wong <darrick.wong@oracle.com> wrote:
> 
> > When dm-bufio sets out to use the bio built into a struct dm_buffer to
> > issue an IO, it needs to call bio_reset after it's done with the bio
> > so that we can free things attached to the bio such as the integrity
> > payload.  Therefore, inject our own endio callback to take care of
> > the bio_reset after calling submit_io's end_io callback.
> > 
> > Test case:
> > 1. modprobe scsi_debug delay=0 dif=1 dix=199 ato=1 dev_size_mb=300
> > 2. Set up a dm-bufio client, e.g. dm-verity, on the scsi_debug device
> > 3. Repeatedly read metadata and watch kmalloc-192 leak!
> > 
> > Fix is against 3.18-rc6.
> > 
> > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> 
> Thanks for reporting/fixing this.
> 
> Alternatively I think we could just call bio_reset() in submit_io(),
> e.g.:
> 
> diff --git a/drivers/md/dm-bufio.c b/drivers/md/dm-bufio.c
> index afe7971..e7036e3 100644
> --- a/drivers/md/dm-bufio.c
> +++ b/drivers/md/dm-bufio.c
> @@ -579,6 +579,8 @@ static void submit_io(struct dm_buffer *b, int rw, sector_t block,
>  	if (rw == WRITE && b->c->write_callback)
>  		b->c->write_callback(b);
>  
> +	bio_reset(&b->bio);
> +
>  	if (b->c->block_size <= DM_BUFIO_INLINE_VECS * PAGE_SIZE &&
>  	    b->data_mode != DATA_MODE_VMALLOC)
>  		use_inline_bio(b, rw, block, end_io);
> 
> What do you think?

I decided to call bio_reset after calling end_io so that we can free
the integrity buffer as soon as we're done with the bio.  Calling
bio_reset just prior to the next submit_bio as this snippet does means
that the integrity buffer remains attached to the bio until just
before the next submit_bio call, which could be a while.

Also, I think use_dmio results in a new bio being used instead of the
bio embedded in the dm_buffer, so it shouldn't be necessary to reset
the bio if the previous IO had use_dmio'd.

--D

> 
> --
> dm-devel mailing list
> dm-devel@redhat.com
> https://www.redhat.com/mailman/listinfo/dm-devel

Re: dm-bufio: fix memleak when using a dm_buffer's inline bio

[Mike Snitzer]

On Tue, Nov 25 2014 at 11:00pm -0500,
Darrick J. Wong <darrick.wong@oracle.com> wrote:

> On Tue, Nov 25, 2014 at 10:41:04PM -0500, Mike Snitzer wrote:
> > On Tue, Nov 25 2014 at  8:45pm -0500,
> > Darrick J. Wong <darrick.wong@oracle.com> wrote:
> > 
> > > When dm-bufio sets out to use the bio built into a struct dm_buffer to
> > > issue an IO, it needs to call bio_reset after it's done with the bio
> > > so that we can free things attached to the bio such as the integrity
> > > payload.  Therefore, inject our own endio callback to take care of
> > > the bio_reset after calling submit_io's end_io callback.
> > > 
> > > Test case:
> > > 1. modprobe scsi_debug delay=0 dif=1 dix=199 ato=1 dev_size_mb=300
> > > 2. Set up a dm-bufio client, e.g. dm-verity, on the scsi_debug device
> > > 3. Repeatedly read metadata and watch kmalloc-192 leak!
> > > 
> > > Fix is against 3.18-rc6.
> > > 
> > > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> > 
> > Thanks for reporting/fixing this.
> > 
> > Alternatively I think we could just call bio_reset() in submit_io(),
> > e.g.:
> > 
> > diff --git a/drivers/md/dm-bufio.c b/drivers/md/dm-bufio.c
> > index afe7971..e7036e3 100644
> > --- a/drivers/md/dm-bufio.c
> > +++ b/drivers/md/dm-bufio.c
> > @@ -579,6 +579,8 @@ static void submit_io(struct dm_buffer *b, int rw, sector_t block,
> >  	if (rw == WRITE && b->c->write_callback)
> >  		b->c->write_callback(b);
> >  
> > +	bio_reset(&b->bio);
> > +
> >  	if (b->c->block_size <= DM_BUFIO_INLINE_VECS * PAGE_SIZE &&
> >  	    b->data_mode != DATA_MODE_VMALLOC)
> >  		use_inline_bio(b, rw, block, end_io);
> > 
> > What do you think?
> 
> I decided to call bio_reset after calling end_io so that we can free
> the integrity buffer as soon as we're done with the bio.  Calling
> bio_reset just prior to the next submit_bio as this snippet does means
> that the integrity buffer remains attached to the bio until just
> before the next submit_bio call, which could be a while.
> 
> Also, I think use_dmio results in a new bio being used instead of the
> bio embedded in the dm_buffer, so it shouldn't be necessary to reset
> the bio if the previous IO had use_dmio'd.

OK, yeah, a new embedded bio is created as a side-effect of allocating a
new dm_buffer.  So we have to use bi_end_io like you've done.

I didn't like seeing your use of .bi_private (because in the context of
bios that are passed into DM: .bi_private must always be preserved so as
not to break upper layers of the IO stack that might be using it).

But in the context of bufio's embedded bio, using .bi_private seems
fine.  Just needs a comment.  I'll fixup and get your patch staged (and
will CC stable).

Thanks again,
Mike

Re: dm-bufio: fix memleak when using a dm_buffer's inline bio

[Mike Snitzer]

On Wed, Nov 26 2014 at  9:28am -0500,
Mike Snitzer <snitzer@redhat.com> wrote:

> 
> But in the context of bufio's embedded bio, using .bi_private seems
> fine.  Just needs a comment.  I'll fixup and get your patch staged (and
> will CC stable).

Staged for 3.19, see:
https://git.kernel.org/cgit/linux/kernel/git/device-mapper/linux-dm.git/commit/?h=dm-for-3.19&id=c370192f925cf34280c7f6257e86dd79f79529d6

Re: [dm-devel] dm-bufio: fix memleak when using a dm_buffer's inline bio

[Darrick J. Wong]

On Wed, Nov 26, 2014 at 09:28:32AM -0500, Mike Snitzer wrote:
> On Tue, Nov 25 2014 at 11:00pm -0500,
> Darrick J. Wong <darrick.wong@oracle.com> wrote:
> 
> > On Tue, Nov 25, 2014 at 10:41:04PM -0500, Mike Snitzer wrote:
> > > On Tue, Nov 25 2014 at  8:45pm -0500,
> > > Darrick J. Wong <darrick.wong@oracle.com> wrote:
> > > 
> > > > When dm-bufio sets out to use the bio built into a struct dm_buffer to
> > > > issue an IO, it needs to call bio_reset after it's done with the bio
> > > > so that we can free things attached to the bio such as the integrity
> > > > payload.  Therefore, inject our own endio callback to take care of
> > > > the bio_reset after calling submit_io's end_io callback.
> > > > 
> > > > Test case:
> > > > 1. modprobe scsi_debug delay=0 dif=1 dix=199 ato=1 dev_size_mb=300
> > > > 2. Set up a dm-bufio client, e.g. dm-verity, on the scsi_debug device
> > > > 3. Repeatedly read metadata and watch kmalloc-192 leak!
> > > > 
> > > > Fix is against 3.18-rc6.
> > > > 
> > > > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> > > 
> > > Thanks for reporting/fixing this.
> > > 
> > > Alternatively I think we could just call bio_reset() in submit_io(),
> > > e.g.:
> > > 
> > > diff --git a/drivers/md/dm-bufio.c b/drivers/md/dm-bufio.c
> > > index afe7971..e7036e3 100644
> > > --- a/drivers/md/dm-bufio.c
> > > +++ b/drivers/md/dm-bufio.c
> > > @@ -579,6 +579,8 @@ static void submit_io(struct dm_buffer *b, int rw, sector_t block,
> > >  	if (rw == WRITE && b->c->write_callback)
> > >  		b->c->write_callback(b);
> > >  
> > > +	bio_reset(&b->bio);
> > > +
> > >  	if (b->c->block_size <= DM_BUFIO_INLINE_VECS * PAGE_SIZE &&
> > >  	    b->data_mode != DATA_MODE_VMALLOC)
> > >  		use_inline_bio(b, rw, block, end_io);
> > > 
> > > What do you think?
> > 
> > I decided to call bio_reset after calling end_io so that we can free
> > the integrity buffer as soon as we're done with the bio.  Calling
> > bio_reset just prior to the next submit_bio as this snippet does means
> > that the integrity buffer remains attached to the bio until just
> > before the next submit_bio call, which could be a while.
> > 
> > Also, I think use_dmio results in a new bio being used instead of the
> > bio embedded in the dm_buffer, so it shouldn't be necessary to reset
> > the bio if the previous IO had use_dmio'd.
> 
> OK, yeah, a new embedded bio is created as a side-effect of allocating a
> new dm_buffer.  So we have to use bi_end_io like you've done.
> 
> I didn't like seeing your use of .bi_private (because in the context of
> bios that are passed into DM: .bi_private must always be preserved so as
> not to break upper layers of the IO stack that might be using it).

<nod> I hadn't considered that not restoring .bi_private without
comment would raise eyebrows in a space where the norm is that they
must always be preserved.  :)

> But in the context of bufio's embedded bio, using .bi_private seems
> fine.  Just needs a comment.  I'll fixup and get your patch staged (and
> will CC stable).

The staged patch looks good, thank you!

--D

> 
> Thanks again,
> Mike
> 
> --
> dm-devel mailing list
> dm-devel@redhat.com
> https://www.redhat.com/mailman/listinfo/dm-devel

Re: [dm-devel] [PATCH] dm-bufio: fix memleak when using a dm_buffer's inline bio

[Mikulas Patocka]

On Tue, 25 Nov 2014, Darrick J. Wong wrote:

> When dm-bufio sets out to use the bio built into a struct dm_buffer to
> issue an IO, it needs to call bio_reset after it's done with the bio
> so that we can free things attached to the bio such as the integrity
> payload.  Therefore, inject our own endio callback to take care of
> the bio_reset after calling submit_io's end_io callback.
> 
> Test case:
> 1. modprobe scsi_debug delay=0 dif=1 dix=199 ato=1 dev_size_mb=300
> 2. Set up a dm-bufio client, e.g. dm-verity, on the scsi_debug device
> 3. Repeatedly read metadata and watch kmalloc-192 leak!
> 
> Fix is against 3.18-rc6.
> 
> +/* Reset the bio to free attached bio integrity profiles when we're done */
> +static void inline_endio(struct bio *bio, int error)
> +{
> +	bio_end_io_t *end_fn;
> +
> +	end_fn = bio->bi_private;
> +	end_fn(bio, error);
> +	bio_reset(bio);
> +}

This is wrong - when end_fn clears the B_READING or B_WRITING flag, the 
buffer may be freed by the background cleanup - so bio_reset may be 
modifying freed memory here. We need to call bio_reset before end_fn.



From: Mikulas Patocka <mpatocka@redhat.com>

When dm-bufio sets out to use the bio built into a struct dm_buffer to
issue an IO, it needs to call bio_reset after it's done with the bio
so that we can free things attached to the bio such as the integrity
payload.  Therefore, inject our own endio callback to take care of
the bio_reset after calling submit_io's end_io callback.

Test case:
1. modprobe scsi_debug delay=0 dif=1 dix=199 ato=1 dev_size_mb=300
2. Set up a dm-bufio client, e.g. dm-verity, on the scsi_debug device
3. Repeatedly read metadata and watch kmalloc-192 leak!

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
Signed-off-by: Mike Snitzer <snitzer@redhat.com>
Signed-off-by: Mikulas Patocka <mpatocka@redhat.com>
Cc: stable@vger.kernel.org

---
 drivers/md/dm-bufio.c |   19 ++++++++++++++++++-
 1 file changed, 18 insertions(+), 1 deletion(-)

Index: linux-3.18-rc6/drivers/md/dm-bufio.c
===================================================================
--- linux-3.18-rc6.orig/drivers/md/dm-bufio.c	2014-12-01 14:52:35.000000000 +0100
+++ linux-3.18-rc6/drivers/md/dm-bufio.c	2014-12-01 14:52:37.000000000 +0100
@@ -565,6 +565,18 @@ static void use_dmio(struct dm_buffer *b
 		end_io(&b->bio, r);
 }
 
+static void inline_endio(struct bio *bio, int error)
+{
+	bio_end_io_t *end_fn = bio->bi_private;
+	/*
+	 * Reset the bio to free any attached resources
+	 * (e.g. bio integrity profiles).
+	 */
+	bio_reset(bio);
+
+	end_fn(bio, error);
+}
+
 static void use_inline_bio(struct dm_buffer *b, int rw, sector_t block,
 			   bio_end_io_t *end_io)
 {
@@ -576,7 +588,12 @@ static void use_inline_bio(struct dm_buf
 	b->bio.bi_max_vecs = DM_BUFIO_INLINE_VECS;
 	b->bio.bi_iter.bi_sector = block << b->c->sectors_per_block_bits;
 	b->bio.bi_bdev = b->c->bdev;
-	b->bio.bi_end_io = end_io;
+	b->bio.bi_end_io = inline_endio;
+	/*
+	 * Use of .bi_private isn't a problem here because
+	 * the dm_buffer's inline bio is local to bufio.
+	 */
+	b->bio.bi_private = end_io;
 
 	/*
 	 * We assume that if len >= PAGE_SIZE ptr is page-aligned.

Re: dm-bufio: fix memleak when using a dm_buffer's inline bio

[Mike Snitzer]

On Mon, Dec 01 2014 at 11:23am -0500,
Mikulas Patocka <mpatocka@redhat.com> wrote:

> 
> 
> On Tue, 25 Nov 2014, Darrick J. Wong wrote:
> 
> > When dm-bufio sets out to use the bio built into a struct dm_buffer to
> > issue an IO, it needs to call bio_reset after it's done with the bio
> > so that we can free things attached to the bio such as the integrity
> > payload.  Therefore, inject our own endio callback to take care of
> > the bio_reset after calling submit_io's end_io callback.
> > 
> > Test case:
> > 1. modprobe scsi_debug delay=0 dif=1 dix=199 ato=1 dev_size_mb=300
> > 2. Set up a dm-bufio client, e.g. dm-verity, on the scsi_debug device
> > 3. Repeatedly read metadata and watch kmalloc-192 leak!
> > 
> > Fix is against 3.18-rc6.
> > 
> > +/* Reset the bio to free attached bio integrity profiles when we're done */
> > +static void inline_endio(struct bio *bio, int error)
> > +{
> > +	bio_end_io_t *end_fn;
> > +
> > +	end_fn = bio->bi_private;
> > +	end_fn(bio, error);
> > +	bio_reset(bio);
> > +}
> 
> This is wrong - when end_fn clears the B_READING or B_WRITING flag, the 
> buffer may be freed by the background cleanup - so bio_reset may be 
> modifying freed memory here. We need to call bio_reset before end_fn.

OK, I'll fold your fix in.