| Message ID | 1563087801-7373-1-git-send-email-wang6495@umn.edu (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | blk-mq: fix a memory leak bug | expand |
On Sun, Jul 14, 2019 at 3:04 PM Wenwen Wang <wang6495@umn.edu> wrote: > > From: Wenwen Wang <wenwen@cs.uga.edu> > > In blk_mq_init_allocated_queue(), a kernel buffer is allocated through > kcalloc_node() to hold hardware dispatch queues in the request queue 'q', > i.e., 'q->queue_hw_ctx'. Later on, if the blk-mq device has no scheduler > set, a scheduler will be initialized through elevator_init_mq(). If this > initialization fails, blk_mq_init_allocated_queue() needs to be terminated > with an error code returned to indicate this failure. However, the > allocated buffer is not freed on this execution path, leading to a memory > leak bug. Moreover, the required cleanup work is also missed on this path. > > To fix the above issues, free the allocated buffer and invoke the cleanup > functions. > > Signed-off-by: Wenwen Wang <wenwen@cs.uga.edu> > --- > block/blk-mq.c | 8 ++++---- > 1 file changed, 4 insertions(+), 4 deletions(-) > > diff --git a/block/blk-mq.c b/block/blk-mq.c > index e5ef40c..04fe077 100644 > --- a/block/blk-mq.c > +++ b/block/blk-mq.c > @@ -2845,6 +2845,8 @@ static unsigned int nr_hw_queues(struct blk_mq_tag_set *set) > struct request_queue *blk_mq_init_allocated_queue(struct blk_mq_tag_set *set, > struct request_queue *q) > { > + int ret = -ENOMEM; > + The above isn't necessary because the function always returns ERR_PTR(-ENOMEM) in case of failure. > /* mark the queue as mq asap */ > q->mq_ops = set->ops; > > @@ -2906,11 +2908,9 @@ struct request_queue *blk_mq_init_allocated_queue(struct blk_mq_tag_set *set, > blk_mq_map_swqueue(q); > > if (!(set->flags & BLK_MQ_F_NO_SCHED)) { > - int ret; > - > ret = elevator_init_mq(q); > if (ret) > - return ERR_PTR(ret); > + goto err_hctxs; The above change itself is fine. However, elevator_init_mq() shouldn't return failure since none should work any time. That said 'none' should be fallback to in case that default mq-deadline can't be initialized. thanks, Ming Lei
On Sun, Jul 14, 2019 at 8:26 PM Ming Lei <tom.leiming@gmail.com> wrote: > > On Sun, Jul 14, 2019 at 3:04 PM Wenwen Wang <wang6495@umn.edu> wrote: > > > > From: Wenwen Wang <wenwen@cs.uga.edu> > > > > In blk_mq_init_allocated_queue(), a kernel buffer is allocated through > > kcalloc_node() to hold hardware dispatch queues in the request queue 'q', > > i.e., 'q->queue_hw_ctx'. Later on, if the blk-mq device has no scheduler > > set, a scheduler will be initialized through elevator_init_mq(). If this > > initialization fails, blk_mq_init_allocated_queue() needs to be terminated > > with an error code returned to indicate this failure. However, the > > allocated buffer is not freed on this execution path, leading to a memory > > leak bug. Moreover, the required cleanup work is also missed on this path. > > > > To fix the above issues, free the allocated buffer and invoke the cleanup > > functions. > > > > Signed-off-by: Wenwen Wang <wenwen@cs.uga.edu> > > --- > > block/blk-mq.c | 8 ++++---- > > 1 file changed, 4 insertions(+), 4 deletions(-) > > > > diff --git a/block/blk-mq.c b/block/blk-mq.c > > index e5ef40c..04fe077 100644 > > --- a/block/blk-mq.c > > +++ b/block/blk-mq.c > > @@ -2845,6 +2845,8 @@ static unsigned int nr_hw_queues(struct blk_mq_tag_set *set) > > struct request_queue *blk_mq_init_allocated_queue(struct blk_mq_tag_set *set, > > struct request_queue *q) > > { > > + int ret = -ENOMEM; > > + > > The above isn't necessary because the function always returns > ERR_PTR(-ENOMEM) in case of failure. > > > /* mark the queue as mq asap */ > > q->mq_ops = set->ops; > > > > @@ -2906,11 +2908,9 @@ struct request_queue *blk_mq_init_allocated_queue(struct blk_mq_tag_set *set, > > blk_mq_map_swqueue(q); > > > > if (!(set->flags & BLK_MQ_F_NO_SCHED)) { > > - int ret; > > - > > ret = elevator_init_mq(q); > > if (ret) > > - return ERR_PTR(ret); > > + goto err_hctxs; > > The above change itself is fine. > > However, elevator_init_mq() shouldn't return failure since none should > work any time. > That said 'none' should be fallback to in case that default > mq-deadline can't be initialized. Thanks for your comments! I agree that 'none' is the fallback if 'mq-deadline' cannot be initialized. But, the error-handling branch after elevator_init_mq() is still necessary, unless elevator_init_mq() always returns zero, which is not true. Thanks! Wenwen
diff --git a/block/blk-mq.c b/block/blk-mq.c index e5ef40c..04fe077 100644 --- a/block/blk-mq.c +++ b/block/blk-mq.c @@ -2845,6 +2845,8 @@ static unsigned int nr_hw_queues(struct blk_mq_tag_set *set) struct request_queue *blk_mq_init_allocated_queue(struct blk_mq_tag_set *set, struct request_queue *q) { + int ret = -ENOMEM; + /* mark the queue as mq asap */ q->mq_ops = set->ops; @@ -2906,11 +2908,9 @@ struct request_queue *blk_mq_init_allocated_queue(struct blk_mq_tag_set *set, blk_mq_map_swqueue(q); if (!(set->flags & BLK_MQ_F_NO_SCHED)) { - int ret; - ret = elevator_init_mq(q); if (ret) - return ERR_PTR(ret); + goto err_hctxs; } return q; @@ -2924,7 +2924,7 @@ struct request_queue *blk_mq_init_allocated_queue(struct blk_mq_tag_set *set, q->poll_cb = NULL; err_exit: q->mq_ops = NULL; - return ERR_PTR(-ENOMEM); + return ERR_PTR(ret); } EXPORT_SYMBOL(blk_mq_init_allocated_queue);