Re: [PATCH] CDRW packet writing support for 2.6.7-bk13

[Greg KH <greg@kroah.com>]

On Fri, Jul 02, 2004 at 03:08:19PM -0700, Andrew Morton wrote:
> Peter Osterlund <petero2@telia.com> wrote:
> >
> > This code leaks a module reference. The patch below fixes it. I'm not
> > sure it's correct, but do_open() also does module_put() after
> > put_disk().
> > 
> > Signed-off-by: Peter Osterlund <petero2@telia.com>
> > 
> > ---
> > 
> >  linux-petero/fs/partitions/check.c |    1 +
> >  1 files changed, 1 insertion(+)
> > 
> > diff -puN fs/partitions/check.c~packet-refcnt fs/partitions/check.c
> > --- linux/fs/partitions/check.c~packet-refcnt	2004-07-02 23:18:22.000000000 +0200
> > +++ linux-petero/fs/partitions/check.c	2004-07-02 23:18:23.000000000 +0200
> > @@ -151,6 +151,7 @@ const char *__bdevname(dev_t dev, char *
> >  	if (disk) {
> >  		buffer = disk_name(disk, part, buffer);
> >  		put_disk(disk);
> > +		module_put(disk->fops->owner);
> >  	} else {
> >  		snprintf(buffer, BDEVNAME_SIZE, "unknown-block(%u,%u)",
> >  				MAJOR(dev), MINOR(dev));
> 
> Yes, there's certainly a leak there.  It's surprising that it hasn't been
> noted before.
> 
> But:
> 
> const char *__bdevname(dev_t dev, char *buffer)
> {
> 	struct gendisk *disk;
> 	int part;
> 
> 	disk = get_gendisk(dev, &part);
> 	if (disk) {
> 		buffer = disk_name(disk, part, buffer);
> 		put_disk(disk);
> 	} else {
> 		snprintf(buffer, BDEVNAME_SIZE, "unknown-block(%u,%u)",
> 				MAJOR(dev), MINOR(dev));
> 	}
> 
> get_gendisk() did an internal module_get() in kobj_lookup().

But if kobj_lookup() succeeds, module_put() is called before returning
(actually it's always called, right?) 

> It would seem to be logical that the module_put() should be in
> kobject_put(), called from put_disk().  But surely if we made that
> change 1000 things would explode.

I don't see how that would fix anything, as the module reference is not
held after kobj_lookup() returns.  Or am I missing something here?

thanks,

greg k-h