From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752799AbdCMOXU (ORCPT ); Mon, 13 Mar 2017 10:23:20 -0400 Received: from mx2.suse.de ([195.135.220.15]:38807 "EHLO mx2.suse.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751050AbdCMOXM (ORCPT ); Mon, 13 Mar 2017 10:23:12 -0400 Date: Mon, 13 Mar 2017 15:23:07 +0100 From: Jan Kara To: Tahsin Erdogan Cc: Tejun Heo , Alexander Viro , Jan Kara , Jens Axboe , John Sperbeck , Peter Chang , linux-fsdevel@vger.kernel.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH] writeback: fix memory leak in wb_queue_work() Message-ID: <20170313142307.GB3885@quack2.suse.cz> References: <20170310200949.26359-1-tahsin@google.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20170310200949.26359-1-tahsin@google.com> User-Agent: Mutt/1.5.24 (2015-08-30) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Fri 10-03-17 12:09:49, Tahsin Erdogan wrote: > When WB_registered flag is not set, wb_queue_work() skips queuing the > work, but does not perform the necessary clean up. In particular, if > work->auto_free is true, it should free the memory. > > The leak condition can be reprouced by following these steps: > > mount /dev/sdb /mnt/sdb > /* In qemu console: device_del sdb */ > umount /dev/sdb > > Above will result in a wb_queue_work() call on an unregistered wb and > thus leak memory. > > Reported-by: John Sperbeck > Signed-off-by: Tahsin Erdogan The patch looks good. You can add: Reviewed-by: Jan Kara Honza > --- > fs/fs-writeback.c | 35 +++++++++++++++++++++-------------- > 1 file changed, 21 insertions(+), 14 deletions(-) > > diff --git a/fs/fs-writeback.c b/fs/fs-writeback.c > index ef600591d96f..63ee2940775c 100644 > --- a/fs/fs-writeback.c > +++ b/fs/fs-writeback.c > @@ -173,19 +173,33 @@ static void wb_wakeup(struct bdi_writeback *wb) > spin_unlock_bh(&wb->work_lock); > } > > +static void finish_writeback_work(struct bdi_writeback *wb, > + struct wb_writeback_work *work) > +{ > + struct wb_completion *done = work->done; > + > + if (work->auto_free) > + kfree(work); > + if (done && atomic_dec_and_test(&done->cnt)) > + wake_up_all(&wb->bdi->wb_waitq); > +} > + > static void wb_queue_work(struct bdi_writeback *wb, > struct wb_writeback_work *work) > { > trace_writeback_queue(wb, work); > > - spin_lock_bh(&wb->work_lock); > - if (!test_bit(WB_registered, &wb->state)) > - goto out_unlock; > if (work->done) > atomic_inc(&work->done->cnt); > - list_add_tail(&work->list, &wb->work_list); > - mod_delayed_work(bdi_wq, &wb->dwork, 0); > -out_unlock: > + > + spin_lock_bh(&wb->work_lock); > + > + if (test_bit(WB_registered, &wb->state)) { > + list_add_tail(&work->list, &wb->work_list); > + mod_delayed_work(bdi_wq, &wb->dwork, 0); > + } else > + finish_writeback_work(wb, work); > + > spin_unlock_bh(&wb->work_lock); > } > > @@ -1873,16 +1887,9 @@ static long wb_do_writeback(struct bdi_writeback *wb) > > set_bit(WB_writeback_running, &wb->state); > while ((work = get_next_work_item(wb)) != NULL) { > - struct wb_completion *done = work->done; > - > trace_writeback_exec(wb, work); > - > wrote += wb_writeback(wb, work); > - > - if (work->auto_free) > - kfree(work); > - if (done && atomic_dec_and_test(&done->cnt)) > - wake_up_all(&wb->bdi->wb_waitq); > + finish_writeback_work(wb, work); > } > > /* > -- > 2.12.0.246.ga2ecc84866-goog > -- Jan Kara SUSE Labs, CR