devfs: BKL *not* taken while opening devices
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.
Signature · 2 lines
Russell King ([email protected]) The developer of ARM Linux http://www.arm.linux.org.uk/personal/aboutme.html
Quoted 6 lines · Russell King
Kernel 2.5.8... Devfs
devfs_open()bypasses the normalchrdev_openandblkdev_openfunctions, 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?
Signature · 2 lines
Dave Hansen [email protected]
Quoted 11 lines · Dave Hansen
Quoted 6 lines · Russell King
Kernel 2.5.8... Devfs
devfs_open()bypasses the normalchrdev_openandblkdev_openfunctions, 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,
Quoted 3 lines · Arjan van de Ven
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
Quoted 2 lines · Roman Zippel
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.
Signature · 2 lines
Dave Hansen [email protected]
Quoted 10 lines · Roman Zippel
Hi,
Quoted 3 lines · Arjan van de Ven
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
Quoted 5 lines
Quoted 2 lines
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
Quoted 3 lines · Arjan van de Ven
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?
Signature · 2 lines
Russell King ([email protected]) The developer of ARM Linux http://www.arm.linux.org.uk/personal/aboutme.html
Quoted 11 lines · Russell King
Quoted 3 lines · Arjan van de Ven
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.
Signature · 2 lines
Dave Hansen [email protected]
Quoted 10 lines · Dave Hansen
Quoted 9 lines · Russell King
Quoted 3 lines · Arjan van de Ven
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.
Quoted 7 lines
Quoted 2 lines
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
Quoted 8 lines
Quoted 4 lines
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.
Quoted 4 lines · Arjan van de Ven
Quoted 1 line
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.
Quoted 2 lines
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"?
Signature · 2 lines
Dave Hansen [email protected]
Quoted 4 lines · Dave Hansen
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".
Signature · 2 lines
Russell King ([email protected]) The developer of ARM Linux http://www.arm.linux.org.uk/personal/aboutme.html
Quoted 10 lines · Russell King
Hi,
Nice simple bug report.
Kernel 2.5.8... Devfs
devfs_open()bypasses the normalchrdev_openandblkdev_openfunctions, 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.
Quoted 18 lines
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;
Quoted 2 lines
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: [email protected] Current: [email protected]