Messages in this thread | | | Subject | Re: [ 02/70] floppy: Cleanup disk->queue before caling put_disk() if add_disk() was never called | From | Ben Hutchings <> | Date | Wed, 08 Aug 2012 01:40:25 +0100 |
| |
On Tue, 2012-08-07 at 19:49 -0300, Herton Ronaldo Krzesinski wrote: > On Tue, Aug 07, 2012 at 04:27:57AM +0100, Ben Hutchings wrote: > > 3.2-stable review patch. If anyone has any objections, please let me know. > > > > ------------------ > > > > From: Vivek Goyal <vgoyal@redhat.com> > > > > commit 3f9a5aabd0a9fe0e0cd308506f48963d79169aa7 upstream. > > > > add_disk() takes gendisk reference on request queue. If driver failed during > > initialization and never called add_disk() then that extra reference is not > > taken. That reference is put in put_disk(). floppy driver allocates the > > disk, allocates queue, sets disk->queue and then relizes that floppy > > controller is not present. It tries to tear down everything and tries to > > put a reference down in put_disk() which was never taken. > > > > In such error cases cleanup disk->queue before calling put_disk() so that > > we never try to put down a reference which was never taken in first place. > > > > Reported-and-tested-by: Suresh Jayaraman <sjayaraman@suse.com> > > Tested-by: Dirk Gouders <gouders@et.bocholt.fh-gelsenkirchen.de> > > Signed-off-by: Vivek Goyal <vgoyal@redhat.com> > > Acked-by: Tejun Heo <tj@kernel.org> > > Signed-off-by: Jens Axboe <axboe@kernel.dk> > > Signed-off-by: Ben Hutchings <ben@decadent.org.uk> > > --- > > drivers/block/floppy.c | 8 +++++++- > > 1 file changed, 7 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/block/floppy.c b/drivers/block/floppy.c > > index 510fb10..401ba78 100644 > > --- a/drivers/block/floppy.c > > +++ b/drivers/block/floppy.c > > @@ -4368,8 +4368,14 @@ out_unreg_blkdev: > > out_put_disk: > > while (dr--) { > > del_timer_sync(&motor_off_timer[dr]); > > - if (disks[dr]->queue) > > + if (disks[dr]->queue) { > > blk_cleanup_queue(disks[dr]->queue); > > + /* > > + * put_disk() is not paired with add_disk() and > > + * will put queue reference one extra time. fix it. > > + */ > > + disks[dr]->queue = NULL; > > + } > > put_disk(disks[dr]); > > } > > return err; > > I was taking a look at this, and noticed some issues with the error > handling: > * missing cleanup (put_disk) if blk_init_queue fails, dr is decremented > first in the error handling loop > * if something fails in the add_disk loop, there is no cleanup of > previous iterations in the error handling. > * if (disks[dr]->queue) check is bogus, when reaching there for each dr > should exist an queue allocated, and it doesn't take into account > iterations where add_disk wasn't done, if failure happens in add_disk > loop. > * floppy_module_exit doesn't reset queue pointer if add_disk wasn't > done.
I agree with this analysis.
> I think the more complete diff below (not build tested) is needed, comments? [...]
There are several off-by-one errors, and 'drive_cnt' is not a particularly clear variable name (alongside 'dr' and 'drive'), but I think this is on the right track.
Anyway, do you agree that the patch under review does fix *a* bug and doesn't introduce a regression?
Ben.
-- Ben Hutchings Theory and practice are closer in theory than in practice. - John Levine, moderator of comp.compilers [unhandled content-type:application/pgp-signature] | |