2.6.35-rc2 module reference counting broken

2.6.35-rc2 module reference counting broken

[Jari Ruusu]

Someone broke block device module reference counting. Problem occours when a
modular block device is mounted and unmounted. Not when it is directly read.
2.6.34 kernel works OK, but 2.6.35-rc2 kernel seems to increase usage count
by one for each mount + umount pair.

# uname -s -r -m
Linux 2.6.35-rc2 i686
# grep CONFIG_SMP /usr/src/linux-2.6.35-rc2/.config
# CONFIG_SMP is not set
# grep CONFIG_MODULE_UNLOAD /usr/src/linux-2.6.35-rc2/.config
CONFIG_MODULE_UNLOAD=y
# grep CONFIG_BLK_DEV_FD /usr/src/linux-2.6.35-rc2/.config
CONFIG_BLK_DEV_FD=m
# lsmod
Module                  Size  Used by
# modprobe floppy
# lsmod
Module                  Size  Used by
floppy                 40029  0
# mount -t ext2 /dev/fd0 /mnt
# umount /mnt
# lsmod
Module                  Size  Used by
floppy                 40029  1
# rmmod floppy
ERROR: Module floppy is in use
# echo $?
1
# 

(reboot)

# uname -s -r -m
Linux 2.6.35-rc2 i686
# lsmod
Module                  Size  Used by
# modprobe floppy
# lsmod
Module                  Size  Used by
floppy                 40029  0
# dd if=/dev/fd0 of=/dev/null bs=4096 count=1 conv=notrunc 2>/dev/null
# lsmod
Module                  Size  Used by
floppy                 40029  0
# rmmod floppy
# echo $?
0
# lsmod
Module                  Size  Used by
# 

-- 
Jari Ruusu  1024R/3A220F51 5B 4B F9 BB D3 3F 52 E9  DB 1D EB E3 24 0E A9 DD

Re: 2.6.35-rc2 module reference counting broken

[Al Viro]

On Mon, Jun 07, 2010 at 08:20:30AM +0300, Jari Ruusu wrote:
> Someone broke block device module reference counting. Problem occours when a
> modular block device is mounted and unmounted. Not when it is directly read.
> 2.6.34 kernel works OK, but 2.6.35-rc2 kernel seems to increase usage count
> by one for each mount + umount pair.

Very interesting...  Looks like mount() bumps refcount by 2.  umount() after
that drops refcount by 1, so it's not leaking superblocks.

Which probably means that open_bdev_exclusive() is fscked.  Interesting...
FWIW, quick look through the history seems to point to this:
commit 6b4517a7913a09d3259bb1d21c9cb300f12294bd
Author: Tejun Heo <tj@kernel.org>
Date:   Wed Apr 7 18:53:59 2010 +0900

    block: implement bd_claiming and claiming block

I'm far too sleepy right now, but I'd start with reviewing what that
thing is doing to module refcounting...

Re: 2.6.35-rc2 module reference counting broken

[Al Viro]

On Mon, Jun 07, 2010 at 07:44:12AM +0100, Al Viro wrote:
> On Mon, Jun 07, 2010 at 08:20:30AM +0300, Jari Ruusu wrote:
> > Someone broke block device module reference counting. Problem occours when a
> > modular block device is mounted and unmounted. Not when it is directly read.
> > 2.6.34 kernel works OK, but 2.6.35-rc2 kernel seems to increase usage count
> > by one for each mount + umount pair.
> 
> Very interesting...  Looks like mount() bumps refcount by 2.  umount() after
> that drops refcount by 1, so it's not leaking superblocks.
> 
> Which probably means that open_bdev_exclusive() is fscked.  Interesting...
> FWIW, quick look through the history seems to point to this:
> commit 6b4517a7913a09d3259bb1d21c9cb300f12294bd
> Author: Tejun Heo <tj@kernel.org>
> Date:   Wed Apr 7 18:53:59 2010 +0900
> 
>     block: implement bd_claiming and claiming block
> 
> I'm far too sleepy right now, but I'd start with reviewing what that
> thing is doing to module refcounting...

Yeah...  bd_start_claiming() grabs a reference to gendisk and we never
let it go.  There's your leak...

Re: 2.6.35-rc2 module reference counting broken

[Tejun Heo]

Hello,

On 06/09/2010 01:48 AM, Al Viro wrote:
>>     block: implement bd_claiming and claiming block
>>
>> I'm far too sleepy right now, but I'd start with reviewing what that
>> thing is doing to module refcounting...
> 
> Yeah...  bd_start_claiming() grabs a reference to gendisk and we never
> let it go.  There's your leak...

Eh, I thought you were cc'd.  Sorry.  This was fixed sometime back by
Nick and queued in block tree (delayed due to mail misdelivery).

  http://thread.gmane.org/gmane.linux.file-systems/40655

Thanks.

-- 
tejun

Re: 2.6.35-rc2 module reference counting broken

[Jari Ruusu]

Tejun Heo wrote:
> On 06/09/2010 01:48 AM, Al Viro wrote:
> > Yeah...  bd_start_claiming() grabs a reference to gendisk and we never
> > let it go.  There's your leak...
> 
> Eh, I thought you were cc'd.  Sorry.  This was fixed sometime back by
> Nick and queued in block tree (delayed due to mail misdelivery).
> 
>   http://thread.gmane.org/gmane.linux.file-systems/40655

That one liner patch makes module refcount mismatch go away.

However, I am not sure if that is the right place to insert that
module_put(). The problem with Nick Piggin's (2010-05-25 15:50:21 GMT) patch
is that it makes module refcount temporarily drop to zero.

I added this line right after that "module_put(disk->fops->owner);" fix:

 if(disk->fops->owner){printk("bd_start_claiming: module_refcount=%u\n", module_refcount(disk->fops->owner));}

And that said "module_refcount=0" when I tried it with my silly floppy
module mount+umount test.

Later in the mount system call handling the module refrence count is
incremented. But to me that looks like there is a window of opportunity for
things to go wrong. What is there to prevent module from being removed at
zero refcount?

-- 
Jari Ruusu  1024R/3A220F51 5B 4B F9 BB D3 3F 52 E9  DB 1D EB E3 24 0E A9 DD

Re: 2.6.35-rc2 module reference counting broken

[Tejun Heo]

Hello,

On 06/10/2010 08:34 AM, Jari Ruusu wrote:
> Later in the mount system call handling the module refrence count is
> incremented. But to me that looks like there is a window of opportunity for
> things to go wrong. What is there to prevent module from being removed at
> zero refcount?

It can be removed, in which case blkdev_get() fails and the whole open
attempt fails, which is the expected behavior.  Claiming block just
needs access to the containing struct block_device, caring for the
actual device and backing module is blkdev_get()'s job.

Thanks.

-- 
tejun