devfs: BKL *not* taken while opening devices

devfs: BKL *not* taken while opening devices

[Russell King]

Hi,

Nice simple bug report.

Kernel 2.5.8... Devfs devfs_open() bypasses the normal chrdev_open and
blkdev_open functions, and misses out taking the BKL.  2.5.10 is the
same.

Certainly the tty layer (and probably many of the other devices as well)
require the BKL to be taken before calling the open method.

Also, the following looks wrong:

    if ( S_ISBLK (inode->i_mode) )
    {
        file->f_op = &def_blk_fops;
        if (df->ops) inode->i_bdev->bd_op = df->ops;
    }
    else file->f_op = fops_get ( (struct file_operations *) df->ops );
    if (file->f_op)
        err = file->f_op->open ? (*file->f_op->open) (inode, file) : 0;
    else
    {
        /*  Fallback to legacy scheme  */
        if ( S_ISCHR (inode->i_mode) ) err = chrdev_open (inode, file);
        else err = -ENODEV;
    }
    if (err < 0) return err;

We can return without releasing the file operations after fops_get(),
thereby effectively locking modules in memory.

-- 
Russell King (rmk@arm.linux.org.uk)                The developer of ARM Linux
             http://www.arm.linux.org.uk/personal/aboutme.html

Re: devfs: BKL *not* taken while opening devices

[Dave Hansen]

Russell King wrote:
 > Kernel 2.5.8... Devfs devfs_open() bypasses the normal chrdev_open
 > and blkdev_open functions, and misses out taking the BKL.  2.5.10 is
 > the same.
 >
 > Certainly the tty layer (and probably many of the other devices as
 > well) require the BKL to be taken before calling the open method.

Has the time come to push the BKL down into all of the driver open()s?
It's going to be a lot of work, but it has to happen eventually, right?

-- 
Dave Hansen
haveblue@us.ibm.com

Re: devfs: BKL *not* taken while opening devices

[Arjan van de Ven]

Dave Hansen wrote:
> 
> Russell King wrote:
>  > Kernel 2.5.8... Devfs devfs_open() bypasses the normal chrdev_open
>  > and blkdev_open functions, and misses out taking the BKL.  2.5.10 is
>  > the same.
>  >
>  > Certainly the tty layer (and probably many of the other devices as
>  > well) require the BKL to be taken before calling the open method.
> 
> Has the time come to push the BKL down into all of the driver open()s?
> It's going to be a lot of work, but it has to happen eventually, right?

I'm not convinced of that. It's not nearly a critical path and it's
better to get even the "dumb" drivers safe than to risk having big
security holes in there for years to come.

Re: devfs: BKL *not* taken while opening devices

[Roman Zippel]

Hi,

On Mon, 29 Apr 2002, Arjan van de Ven wrote:

> I'm not convinced of that. It's not nearly a critical path and it's
> better to get even the "dumb" drivers safe than to risk having big
> security holes in there for years to come.

The BKL doesn't make a driver safe, remember that it's released on
schedule.

bye, Roman

Re: devfs: BKL *not* taken while opening devices

[Arjan van de Ven]

On Mon, Apr 29, 2002 at 07:40:08PM +0200, Roman Zippel wrote:
> Hi,
> 
> On Mon, 29 Apr 2002, Arjan van de Ven wrote:
> 
> > I'm not convinced of that. It's not nearly a critical path and it's
> > better to get even the "dumb" drivers safe than to risk having big
> > security holes in there for years to come.
> 
> The BKL doesn't make a driver safe, remember that it's released on
> schedule.

I know. But a LOT of in kernel and out-of kernel drives don't schedule
in open and are therefore safe right now

Re: devfs: BKL *not* taken while opening devices

[Dave Hansen]

Roman Zippel wrote:
 > The BKL doesn't make a driver safe, remember that it's released on
 > schedule.

Not safe, but _safer_, and definitely safe enough for almost all uses. 
Some of the drivers rely on the fact that open() cannot be run 
concurrently.  The BKL does provide this if open never blocks.

-- 
Dave Hansen
haveblue@us.ibm.com

Re: devfs: BKL *not* taken while opening devices

[Russell King]

On Mon, Apr 29, 2002 at 06:21:34PM +0100, Arjan van de Ven wrote:
> I'm not convinced of that. It's not nearly a critical path and it's
> better to get even the "dumb" drivers safe than to risk having big
> security holes in there for years to come.

Would it be worth dropping a  BUG_ON(!kernel_locked()) in tty_open() to
catch this type of error?  The tty code heavily relies on the BKL.

This way, such locking problems would get caught early, since everyone
uses the tty code during boot, right?

-- 
Russell King (rmk@arm.linux.org.uk)                The developer of ARM Linux
             http://www.arm.linux.org.uk/personal/aboutme.html

Re: devfs: BKL *not* taken while opening devices

[Dave Hansen]

Russell King wrote:
> On Mon, Apr 29, 2002 at 06:21:34PM +0100, Arjan van de Ven wrote:
> 
>>I'm not convinced of that. It's not nearly a critical path and it's
>>better to get even the "dumb" drivers safe than to risk having big
>>security holes in there for years to come.
> 
> Would it be worth dropping a  BUG_ON(!kernel_locked()) in tty_open() to
> catch this type of error?  The tty code heavily relies on the BKL.
> 
> This way, such locking problems would get caught early, since everyone
> uses the tty code during boot, right?

I like the idea.  But, while we're at it, does anyone have a good enough 
grasp of locking the the TTY layer that we can start peeling some of the 
BKL out of there?  Somebody was doing tests over a serial console here 
and the lockmeter data showed horrible BKL contention and hold times.

-- 
Dave Hansen
haveblue@us.ibm.com

Re: devfs: BKL *not* taken while opening devices

[Arjan van de Ven]

On Tue, Apr 30, 2002 at 09:42:32AM -0700, Dave Hansen wrote:
> Russell King wrote:
> > On Mon, Apr 29, 2002 at 06:21:34PM +0100, Arjan van de Ven wrote:
> > 
> >>I'm not convinced of that. It's not nearly a critical path and it's
> >>better to get even the "dumb" drivers safe than to risk having big
> >>security holes in there for years to come.
> > 
> > Would it be worth dropping a  BUG_ON(!kernel_locked()) in tty_open() to
> > catch this type of error?  The tty code heavily relies on the BKL.
> > 
> > This way, such locking problems would get caught early, since everyone
> > uses the tty code during boot, right?
> 
> I like the idea.  But, while we're at it, does anyone have a good enough 
> grasp of locking the the TTY layer that we can start peeling some of the 
> BKL out of there?  Somebody was doing tests over a serial console here 
> and the lockmeter data showed horrible BKL contention and hold times.

I really really doubt that fixing contention will make serial ports go
faster... it'll just move to another lock since I suspect we're
just waiting for hardware

Re: devfs: BKL *not* taken while opening devices

[Dave Hansen]

Arjan van de Ven wrote:
>>I like the idea.  But, while we're at it, does anyone have a good enough 
>>grasp of locking the the TTY layer that we can start peeling some of the 
>>BKL out of there?  Somebody was doing tests over a serial console here 
>>and the lockmeter data showed horrible BKL contention and hold times.
> 
> I really really doubt that fixing contention will make serial ports go
> faster... 

I know :)  It just takes extra explaining on my part whenever someone 
sees the lockmeter data.

 > it'll just move to another lock since I suspect we're
 > just waiting for hardware

Just about any other lock is preferrable to the BKL.  Should 
ext2_update_inode() be blocked because someone hit "Enter"?

-- 
Dave Hansen
haveblue@us.ibm.com

Re: devfs: BKL *not* taken while opening devices

[Russell King]

On Tue, Apr 30, 2002 at 09:42:32AM -0700, Dave Hansen wrote:
> I like the idea.  But, while we're at it, does anyone have a good enough 
> grasp of locking the the TTY layer that we can start peeling some of the 
> BKL out of there?  Somebody was doing tests over a serial console here 
> and the lockmeter data showed horrible BKL contention and hold times.

I'm sure it isn't *that* bad for average workloads.  Sure, if you hammer
the TTY layer madly to measure the BKL it will show up, but that isn't
an average workload.

I purposely didn't mention this in the previous mail.  The tty code is
beyond any type of "peeling".  The whole thing relies on the behaviour
of the BKL - in that when you sleep the BKL is released.  Think about
someone opening /dev/cua0 while /dev/ttyS0 is trying to be opened, or
a hangup while a port is being opened, or... the list is endless.

It's not as simple as replacing the BKL with a semaphore or spinlock.

I've actually brought this up in passing with Alan back in October - his
feeling at the time was (iirc) that the effort required isn't worth the
rewards you'd get.

When I talked to Ted last, Ted was going to rewrite the whole thing to
get it into a reasonable shape, which included a BKL free tty layer.
I've not heard anything from Ted recently on this though.

However, being able to type on a laptop over a ssh connection to another
machine, and have everything freeze while the hard disk spins up for no
apparant reason (other than your typing) is an annoyance that I wouldn't
mind see "disappear".

-- 
Russell King (rmk@arm.linux.org.uk)                The developer of ARM Linux
             http://www.arm.linux.org.uk/personal/aboutme.html

Re: devfs: BKL *not* taken while opening devices

[Martin J. Bligh]

>> I like the idea.  But, while we're at it, does anyone have a good enough 
>> grasp of locking the the TTY layer that we can start peeling some of the 
>> BKL out of there?  Somebody was doing tests over a serial console here 
>> and the lockmeter data showed horrible BKL contention and hold times.
> 
> I really really doubt that fixing contention will make serial ports go
> faster... it'll just move to another lock since I suspect we're
> just waiting for hardware

No, but it might make other things who are waiting for the BKL go faster.

M.

Re: devfs: BKL *not* taken while opening devices

[Alan Cox]

> > The BKL doesn't make a driver safe, remember that it's released on
> > schedule.
> 
> I know. But a LOT of in kernel and out-of kernel drives don't schedule
> in open and are therefore safe right now

I looked at this for video4linux and for watchdog drivers. The score was
something like 80% buggy 20% correct. Remember things like request_irq
can schedule.

Fix the drivers, even if they get fixed before the lock is removed

Re: devfs: BKL *not* taken while opening devices

[Richard Gooch]

Russell King writes:
> Hi,
> 
> Nice simple bug report.
> 
> Kernel 2.5.8... Devfs devfs_open() bypasses the normal chrdev_open and
> blkdev_open functions, and misses out taking the BKL.  2.5.10 is the
> same.
> 
> Certainly the tty layer (and probably many of the other devices as well)
> require the BKL to be taken before calling the open method.

Yes, I'll be submitting a patch to fix this.

> Also, the following looks wrong:
> 
>     if ( S_ISBLK (inode->i_mode) )
>     {
>         file->f_op = &def_blk_fops;
>         if (df->ops) inode->i_bdev->bd_op = df->ops;
>     }
>     else file->f_op = fops_get ( (struct file_operations *) df->ops );
>     if (file->f_op)
>         err = file->f_op->open ? (*file->f_op->open) (inode, file) : 0;
>     else
>     {
>         /*  Fallback to legacy scheme  */
>         if ( S_ISCHR (inode->i_mode) ) err = chrdev_open (inode, file);
>         else err = -ENODEV;
>     }
>     if (err < 0) return err;
> 
> We can return without releasing the file operations after fops_get(),
> thereby effectively locking modules in memory.

How is this different from the chrdev_open() logic? It seems to me
this is taken care of in fput(), so no need to worry.

				Regards,

					Richard....
Permanent: rgooch@atnf.csiro.au
Current:   rgooch@ras.ucalgary.ca