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 ([email protected]) The developer of ARM Linux
http://www.arm.linux.org.uk/personal/aboutme.html
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
[email protected]
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.
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
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
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
[email protected]
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 ([email protected]) The developer of ARM Linux
http://www.arm.linux.org.uk/personal/aboutme.html
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
[email protected]
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
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
[email protected]
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 ([email protected]) The developer of ARM Linux
http://www.arm.linux.org.uk/personal/aboutme.html
>> 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.