Re: [GIT PULL] block fixes for 2.6.32-rc

[Jens Axboe <jens.axboe@oracle.com>]

On Wed, Oct 28 2009, Linus Torvalds wrote:
> 
> 
> On Wed, 28 Oct 2009, Jens Axboe wrote:
> > 
> > Neil Brown (1):
> >       block: use after free bug in __blkdev_get
> >
> > diff --git a/fs/block_dev.c b/fs/block_dev.c
> > index 9cf4b92..8bed055 100644
> > --- a/fs/block_dev.c
> > +++ b/fs/block_dev.c
> > @@ -1248,8 +1248,8 @@ static int __blkdev_get(struct block_device *bdev, fmode_t mode, int for_part)
> >  			bd_set_size(bdev, (loff_t)bdev->bd_part->nr_sects << 9);
> >  		}
> >  	} else {
> > -		put_disk(disk);
> >  		module_put(disk->fops->owner);
> > +		put_disk(disk);
> >  		disk = NULL;
> >  		if (bdev->bd_contains == bdev) {
> >  			if (bdev->bd_disk->fops->open) {
> 
> Is this really right? You do the module-put while the disk is still 
> available..
> 
> I get the feeling that it might have been better to do
> 
> 	struct module *mod = disk->fops->owner;
> 	put_disk(disk);
> 	module_put(mod);
> 
> instead, which tries to make sure that the module is put only after we've 
> gotten rid of the disk entirely.
> 
> But I dunno. Maybe there is some reason why it's safe either way. You're 
> sure the kobject_put() in put_disk will never call to the module?

Hmm good point. The general use case in block_dev.c is indeed to put the
module after the disk, which does seem a bit backwards (at least
logically). I'd say pull the patch since it fixes Neil's problem and
follows the general pattern, then I'll investigate whether that use
pattern is indeed safe. It wont make things worse and the current usage
being fixed is definitely wrong.

-- 
Jens Axboe