| Message ID | 1562830033-24239-1-git-send-email-wang6495@umn.edu (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | block/bio-integrity: fix a memory leak bug | expand |
On Thu, Jul 11, 2019 at 3:36 PM Wenwen Wang <wang6495@umn.edu> wrote: > > From: Wenwen Wang <wenwen@cs.uga.edu> > > In bio_integrity_prep(), a kernel buffer is allocated through kmalloc() to > hold integrity metadata. Later on, the buffer will be attached to the bio > structure through bio_integrity_add_page(), which returns the number of > bytes of integrity metadata attached. Due to unexpected situations, > bio_integrity_add_page() may return 0. As a result, bio_integrity_prep() > needs to be terminated with 'false' returned to indicate this error. > However, the allocated kernel buffer is not freed on this execution path, > leading to a memory leak. > > To fix this issue, free the allocated buffer before returning from > bio_integrity_prep(). > > Signed-off-by: Wenwen Wang <wenwen@cs.uga.edu> > --- > block/bio-integrity.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > diff --git a/block/bio-integrity.c b/block/bio-integrity.c > index 4db6208..bfae10c 100644 > --- a/block/bio-integrity.c > +++ b/block/bio-integrity.c > @@ -276,8 +276,10 @@ bool bio_integrity_prep(struct bio *bio) > ret = bio_integrity_add_page(bio, virt_to_page(buf), > bytes, offset); > > - if (ret == 0) > + if (ret == 0) { > + kfree(buf); > return false; > + } This way may not be enough, and the bio payload needs to be freed. And you may refer to the error handling for 'IS_ERR(bip)', and bio->bi_status needs to be set, and bio_endio() needs to be called too. Thanks, Ming Lei
On Thu, Jul 11, 2019 at 4:22 AM Ming Lei <tom.leiming@gmail.com> wrote: > > On Thu, Jul 11, 2019 at 3:36 PM Wenwen Wang <wang6495@umn.edu> wrote: > > > > From: Wenwen Wang <wenwen@cs.uga.edu> > > > > In bio_integrity_prep(), a kernel buffer is allocated through kmalloc() to > > hold integrity metadata. Later on, the buffer will be attached to the bio > > structure through bio_integrity_add_page(), which returns the number of > > bytes of integrity metadata attached. Due to unexpected situations, > > bio_integrity_add_page() may return 0. As a result, bio_integrity_prep() > > needs to be terminated with 'false' returned to indicate this error. > > However, the allocated kernel buffer is not freed on this execution path, > > leading to a memory leak. > > > > To fix this issue, free the allocated buffer before returning from > > bio_integrity_prep(). > > > > Signed-off-by: Wenwen Wang <wenwen@cs.uga.edu> > > --- > > block/bio-integrity.c | 4 +++- > > 1 file changed, 3 insertions(+), 1 deletion(-) > > > > diff --git a/block/bio-integrity.c b/block/bio-integrity.c > > index 4db6208..bfae10c 100644 > > --- a/block/bio-integrity.c > > +++ b/block/bio-integrity.c > > @@ -276,8 +276,10 @@ bool bio_integrity_prep(struct bio *bio) > > ret = bio_integrity_add_page(bio, virt_to_page(buf), > > bytes, offset); > > > > - if (ret == 0) > > + if (ret == 0) { > > + kfree(buf); > > return false; > > + } > > This way may not be enough, and the bio payload needs to be freed. > > And you may refer to the error handling for 'IS_ERR(bip)', and bio->bi_status > needs to be set, and bio_endio() needs to be called too. Thanks for your comments! I will rework the patch. Wenwen
diff --git a/block/bio-integrity.c b/block/bio-integrity.c index 4db6208..bfae10c 100644 --- a/block/bio-integrity.c +++ b/block/bio-integrity.c @@ -276,8 +276,10 @@ bool bio_integrity_prep(struct bio *bio) ret = bio_integrity_add_page(bio, virt_to_page(buf), bytes, offset); - if (ret == 0) + if (ret == 0) { + kfree(buf); return false; + } if (ret < bytes) break;