parport/ppdev: fix registration of sysctl entries
Subsystems (2)
PATCH 1/1 parport/ppdev: fix registration of sysctl entries
ppdev (which provides support for user-space parallel port device drivers) is slightly different from other parallel port drivers. It allows to open the device by more than one process, but locks the device on ioctl (PPCLAIM). Unfortunately registration of sysctl entries were done before locking the device, so 2 processes could open it and try to register sysctl (it ignored error on registration, so it didn't block access to port).
So move registration of sysctl after locking (parport_claim_or_block).
Warning:
ppdev0: registered pardevice
sysctl table check failed: /dev/parport/parport0/devices/ppdev0/timeslice Sysctl already exists
Pid: 14491, comm: hpijs Not tainted 2.6.24-rc5 #1
Call Trace: [<ffffffff8024c0b0>] set_fail+0x3f/0x47 [<ffffffff8024c566>] sysctl_check_table+0x4ae/0x4fb [<ffffffff8024c062>] sysctl_check_lookup+0xc1/0xd0 [<ffffffff8024c57c>] sysctl_check_table+0x4c4/0x4fb [<ffffffff8024c062>] sysctl_check_lookup+0xc1/0xd0 [<ffffffff8024c57c>] sysctl_check_table+0x4c4/0x4fb [<ffffffff8024c062>] sysctl_check_lookup+0xc1/0xd0 [<ffffffff8024c57c>] sysctl_check_table+0x4c4/0x4fb [<ffffffff8024c062>] sysctl_check_lookup+0xc1/0xd0 [<ffffffff8024c57c>] sysctl_check_table+0x4c4/0x4fb [<ffffffff8024c062>] sysctl_check_lookup+0xc1/0xd0 [<ffffffff8024c57c>] sysctl_check_table+0x4c4/0x4fb [<ffffffff8023ca15>] register_sysctl_table+0x52/0x9d [<ffffffff881d97cb>] :parport:parport_device_proc_register+0xc3/0xe3 [<ffffffff881d7da7>] :parport:parport_register_device+0x206/0x267 [<ffffffff884171ae>] :ppdev:pp_irq+0x0/0x40 [<ffffffff8841774f>] :ppdev:pp_ioctl+0x13f/0x77c [<ffffffff80296a11>] do_ioctl+0x55/0x6b [<ffffffff80296c6a>] vfs_ioctl+0x243/0x25c [<ffffffff80296cd4>] sys_ioctl+0x51/0x71 [<ffffffff8020beee>] system_call+0x7e/0x83
http://lkml.org/lkml/2007/12/16/121
http://kerneloops.org/guilty.php?guilty=parport_device_proc_register&version=2.6.25-release&start=1671168&end=1703935&class=warn
https://bugzilla.redhat.com/show_bug.cgi?id=449112
---
Frans: Could you test it on real hardware?
--- drivers/char/ppdev.c | 4 +++- drivers/parport/procfs.c | 1 + drivers/parport/share.c | 6 ++++-- include/linux/parport.h | 1 + 4 files changed, 9 insertions(+), 3 deletions(-)
drivers/char/ppdev.c +3 -1
@@ -302,7 +302,8 @@ static int register_device (int minor, struct pp_struct *pp) fl = (pp->flags &PP_EXCL ) ?PARPORT_FLAG_EXCL :0 ; pdev =parport_register_device (port, name,NULL , -NULL , pp_irq, fl, pp); +NULL , pp_irq, + fl |PARPORT_DONT_REG_PROC , pp);parport_put_port (port);if (!pdev) {
@@ -359,6 +360,7 @@ static int pp_ioctl(struct inode *inode, struct file *file, ret =parport_claim_or_block (pp->pdev);if (ret <0 )return ret; +parport_device_proc_register (pp->pdev); pp->flags |=PP_CLAIMED ;
drivers/parport/procfs.c +1 -0
@@ -554,6 +554,7 @@ int parport_device_proc_register(struct pardevice *device) device->sysctl_table = t;return 0 ; } +EXPORT_SYMBOL (parport_device_proc_register);int parport_device_proc_unregister (struct pardevice *device) {
drivers/parport/share.c +4 -2
@@ -572,7 +572,7 @@ parport_register_device(struct parport *port, const char *name, tmp->preempt = pf; tmp->wakeup = kf; tmp->private = handle; - tmp->flags = flags; + tmp->flags = flags & ~PARPORT_DONT_REG_PROC ; tmp->irq_func = irq_func; tmp->waiting =0 ; tmp->timeout =5 *HZ ;
@@ -614,7 +614,9 @@ parport_register_device(struct parport *port, const char *name,* pardevice fields. -arca */ port->ops->init_state (tmp, tmp->state); -parport_device_proc_register (tmp); +if (!(flags &PARPORT_DONT_REG_PROC )) +parport_device_proc_register (tmp); +return tmp; out_free_all:
include/linux/parport.h +1 -0
@@ -463,6 +463,7 @@ static __inline__ int parport_yield_blocking(struct pardevice *dev)#define PARPORT_DEV_EXCL (1<<1)/* Need exclusive access. */ #define PARPORT_FLAG_EXCL (1<<1)/* EXCL driver registered. */ +#define PARPORT_DONT_REG_PROC (1<<2)/* IEEE1284 functions */ extern void parport_ieee1284_interrupt (void *);
Signature · 1 line
1.5.4.5
Quoted 8 lines · Marcin Slusarz
ppdev (which provides support for user-space parallel port device drivers) is slightly different from other parallel port drivers. It allows to open the device by more than one process, but locks the device on ioctl (PPCLAIM). Unfortunately registration of sysctl entries were done before locking the device, so 2 processes could open it and try to register sysctl (it ignored error on registration, so it didn't block access to port).
So move registration of sysctl after locking (
parport_claim_or_block).
I don't believe that it's right. Note that if you *do* race there, you are fucked regardless of sysctls - ppdev.c::register_device() racing with itself will do tons of fun things all by itself (starting with two threads allocating different pdev and both setting pp->pdev).
IOW, *if* that's what we are hitting here, you've only papered over the visible symptom.
Quoted 7 lines · Al Viro
I don't believe that it's right. Note that if you *do* race there, you are fucked regardless of sysctls -
ppdev.c::register_device()racing with itself will do tons of fun things all by itself (starting with two threads allocating different pdev and both settingpp->pdev).IOW, *if* that's what we are hitting here, you've only papered over the visible symptom.
BTW, with your patch you'll have 100% reproducible double registration if you do PPCLAIM/PPRELEASE/PPCLAIN on one file descriptor.
Quoted 7 lines · Al Viro
Quoted 5 lines · Al Viro
I don't believe that it's right. Note that if you *do* race there, you are fucked regardless of sysctls -
ppdev.c::register_device()racing with itself will do tons of fun things all by itself (starting with two threads allocating different pdev and both settingpp->pdev).
Quoted 5 lines
Quoted 2 lines
IOW, *if* that's what we are hitting here, you've only papered over the visible symptom.
BTW, with your patch you'll have 100% reproducible double registration if you do PPCLAIM/PPRELEASE/PPCLAIN on one file descriptor.
FWIW, here's what's going on in ppdev:
a) we *are* allowed to create several pardevice over the same
port, one per each open(). Each is essentially a parport scheduling entity. So far, so good.
b) creation is actually delayed until an ioclt (PPCLAIM). That
appears to be a result of shitty API (another ioctl (PPEXCL) instead of just using O_EXCL at open() time, as any normal driver would). In any case, it's badly racy - two tasks doing PPCLAIM on the same struct file (e.g. one had opened it, then called fork(), then both child and parent had called ioctl(fd, PPCLAIM, 0)) can race, leading to rather nasty effects. Check for delayed registration + register_device() call should be atomic. That's solvable by a mutex.
c) *HOWEVER*, all races aside, we have a genuinely fscked up
API. Each of these parport scheduling entities has a parameter - timeslice. That parameter is exposed as sysctl. And we definitely want these per-open, not per-port. And we get everything for the same port mapped to the same sysctl.
Quoted 23 lines · Al Viro
Quoted 7 lines · Al Viro
Quoted 5 lines · Al Viro
I don't believe that it's right. Note that if you *do* race there, you are fucked regardless of sysctls -
ppdev.c::register_device()racing with itself will do tons of fun things all by itself (starting with two threads allocating different pdev and both settingpp->pdev).Quoted 5 lines
Quoted 2 lines
IOW, *if* that's what we are hitting here, you've only papered over the visible symptom.
BTW, with your patch you'll have 100% reproducible double registration if you do PPCLAIM/PPRELEASE/PPCLAIN on one file descriptor.
FWIW, here's what's going on in ppdev:
a) we *are* allowed to create several pardevice over the same
port, one per eachopen(). Each is essentially a parport scheduling entity. So far, so good.
b) creation is actually delayed until an ioclt (PPCLAIM). That
appears to be a result of shitty API (another ioctl (PPEXCL) instead of just usingO_EXCLatopen()time, as any normal driver would). In any case, it's badly racy - two tasks doing PPCLAIM on the same struct file (e.g. one had opened it, then calledfork(), then both child and parent
Quoted 8 lines
had called
ioctl(fd, PPCLAIM, 0)) can race, leading to rather nasty effects. Check for delayed registration +register_device()call should be atomic. That's solvable by a mutex.
c) *HOWEVER*, all races aside, we have a genuinely fscked up
API. Each of these parport scheduling entities has a parameter - timeslice. That parameter is exposed as sysctl. And we definitely want these per-open, not per-port. And we get everything for the same port mapped to the same sysctl.
It isn't quite that bad. Every other user of parport_register_device uses a compile time unique name. Only ppdev allows multiple callers to reuse the same name.
So our choices appear to be.
- Change the name in sysctl so each parport device always has a unique name.
- Only allow one opener of ppdev for a given port.
- Take the approach of the initial patch and export to sysctl when we claim the port and unexport when we release the port.
- Give up and simply don't register with sysctl for ppdev.
I did a quick google search and I could not find any hits (except for this bug report on devices/ppdev) so I am inclined just to special case ppdev and not even bother registering with sysctl. I did not see any other fields that would have problems with a duplicate name.
The only other backwards compatible and viable approach appears to be registering ppdev parport devices when they are claimed.
The only reason we would be able to change the name without breakage is if no one uses the /proc interface in which case I don't see a point in continuing to provide it for ppdev.
Eric
Quoted 3 lines · Eric W. Biederman
So our choices appear to be.
- Change the name in sysctl so each parport device always has a unique name.
- Only allow one opener of ppdev for a given port.
Can't do - it's a legitimate use of ppdev (several userland programs multiplexing the sucker).
Quoted 2 lines
- Take the approach of the initial patch and export to sysctl when we claim the port and unexport when we release the port.
You do realize that we need exclusion around that lazy registration in any case? sysctl is not the only problem there...
Quoted 7 lines
- Give up and simply don't register with sysctl for ppdev.
I did a quick google search and I could not find any hits (except for this bug report on devices/ppdev) so I am inclined just to special case ppdev and not even bother registering with sysctl. I did not see any other fields that would have problems with a duplicate name.
Quoted 6 lines
The only other backwards compatible and viable approach appears to be registering ppdev parport devices when they are claimed.
The only reason we would be able to change the name without breakage is if no one uses the /proc interface in which case I don't see a point in continuing to provide it for ppdev.
Not quite. /proc/sys/.../timeslice is a generically documented way to tune the damn thing when we have several things on the same port. Note that while one of those might be in userland, the rest might be in kernel and very different. In this case the parameter is both relevant *and* currently usable.
Frankly, I'd go for IDR and rename in cases when we have additional openers. _And_ add a mutex around delayed allocation - that's a separate problem.
Quoted 7 lines · Al Viro
Quoted 3 lines · Eric W. Biederman
So our choices appear to be.
- Change the name in sysctl so each parport device always has a unique name.
- Only allow one opener of ppdev for a given port.Can't do - it's a legitimate use of ppdev (several userland programs multiplexing the sucker).
Yep. And if didn't happen we wouldn't have the bug report.
Quoted 6 lines
Quoted 3 lines
- Take the approach of the initial patch and export to sysctl when we claim the port and unexport when we release the port.
You do realize that we need exclusion around that lazy registration in any case? sysctl is not the only problem there...
Totally. I was giving credit to the general idea rather then refering to specific implementation details.
Quoted 14 lines
Quoted 7 lines
- Give up and simply don't register with sysctl for ppdev.
I did a quick google search and I could not find any hits (except for this bug report on devices/ppdev) so I am inclined just to special case ppdev and not even bother registering with sysctl. I did not see any other fields that would have problems with a duplicate name.
Quoted 6 lines
The only other backwards compatible and viable approach appears to be registering ppdev parport devices when they are claimed.
The only reason we would be able to change the name without breakage is if no one uses the /proc interface in which case I don't see a point in continuing to provide it for ppdev.
Quoted 5 lines
Not quite.
/proc/sys/.../timesliceis a generically documented way to tune the damn thing when we have several things on the same port. Note that while one of those might be in userland, the rest might be in kernel and very different. In this case the parameter is both relevant *and* currently usable.
Yes. I was only thinking about killing it off for ppdev. You do have a point
something that is tuning this based on all openers could be looking at it generically.
Quoted 1 line
Frankly, I'd go for IDR and rename in cases when we have additional openers.
It looks like we can walk port->physport->devices to see if we are the first ppdev to register. So that should not be too hard.
That will provide maximum compatibility. Right now the first ppdev on a minor shows up in sysctl and the rest error out sysctl wise. Having the others show up at a different name is exactly equivalent except that they show up.
The unfortunate thing is that we won't have a good way to tie those additional sysctl entries back to whoever opened them. Oh well.
Quoted 1 line
_And_ add a mutex around delayed allocation - that's a separate problem.
Yes. We need locking so that only one process can set or clear PP_CLAIMED.
Eric
Quoted 8 lines · On Sun, 6 Jul 2008 09:11:26 +0100 Al Viro <[email protected]> wrote:
Quoted 4 lines · Eric W. Biederman
So our choices appear to be.
- Change the name in sysctl so each parport device always has a unique name.
- Only allow one opener of ppdev for a given port.Can't do - it's a legitimate use of ppdev (several userland programs multiplexing the sucker).
How in the world would such a thing work?! I imagine one could read()/write() there and multiplex data, but isn't the whole point of ppdev to allow non-standard communication (manually flipping control and data lines)? I've used it for custom hardware.
IMHO, there is one reason ppdev exists: allow both _safe_ and _raw_ access to the parallel port, and a much better alternative to the iopl stuff.
Eduard
Quoted 3 lines
IMHO, there is one reason ppdev exists: allow both _safe_ and _raw_ access to the parallel port, and a much better alternative to the iopl stuff.
And various devices support sharing protocols on the parallel port.
Quoted 5 lines · On Sun, 6 Jul 2008 17:08:02 +0100 Alan Cox <[email protected]> wrote:
Quoted 3 lines
IMHO, there is one reason ppdev exists: allow both _safe_ and _raw_ access to the parallel port, and a much better alternative to the iopl stuff.
And various devices support sharing protocols on the parallel port.
But isn't it incorrect to assume that sharing protocols allow multiple separate users? Why not have an userspace server mux/demux requests? Told more properly, such devices are multi-context, not multi-user.
AFAICT, this is similar to software mixing on soundcards, where we have an additional layer mixing (~ multiplexing) outgoing PCM streams.
The parallel port itself is not designed to be shared, so even if the device supports it, it does not mean we should allow independent access. We really should have an userspace lib/server to do this.
Eduard
Quoted 2 lines
But isn't it incorrect to assume that sharing protocols allow multiple separate users? Why not have an userspace server mux/demux requests?
Why do that ?
Quoted 3 lines
The parallel port itself is not designed to be shared, so even if the device supports it, it does not mean we should allow independent access. We really should have an userspace lib/server to do this.
I suggest you look at the actual hardware examples in more detail. Most of them are happy to share but need to own the port temporarily. The manner in which they lock/unlock their use of the port tends to be very specific to the device so it makes sense to have the current desgign.
Quoted 5 lines · Al Viro
c) *HOWEVER*, all races aside, we have a genuinely fscked up
API. Each of these parport scheduling entities has a parameter - timeslice. That parameter is exposed as sysctl. And we definitely want these per-open, not per-port. And we get everything for the same port mapped to the same sysctl.
Why we want it per-open? What's a timeslice (in this context)?
Quoted 6 lines · Al Viro
Quoted 4 lines · Al Viro
I don't believe that it's right. Note that if you *do* race there, you are fucked regardless of sysctls -
ppdev.c::register_device()racing with itself will do tons of fun things all by itself (starting with two threads allocating different pdev and both settingpp->pdev).
I wouldn't call it a race - BKL is protecting multiple ioctl calls, so we won't ever claim the device from two different threads.
Quoted 2 lines
BTW, with your patch you'll have 100% reproducible double registration if you do PPCLAIM/PPRELEASE/PPCLAIN on one file descriptor.
Yes, I didn't notice PPRELEASE ioctl (I thought releasing the device is done during close()).
Quoted 11 lines · On Sun, 6 Jul 2008 17:07:02 +0200 Marcin Slusarz <[email protected]>…
Quoted 7 lines · Al Viro
Quoted 5 lines · Al Viro
I don't believe that it's right. Note that if you *do* race there, you are fucked regardless of sysctls -
ppdev.c::register_device()racing with itself will do tons of fun things all by itself (starting with two threads allocating different pdev and both settingpp->pdev).I wouldn't call it a race - BKL is protecting multiple ioctl calls, so we won't ever claim the device from two different threads.
eh... there's sleeping functions in there.. the BKL doesn't protect you squat ;)(
Signature · 3 lines
If you want to reach me at my work email, use [email protected] For development, discussion and tips for power savings, visit http://www.lesswatts.org
Quoted 10 lines · Marcin Slusarz
Quoted 6 lines · Al Viro
Quoted 4 lines · Al Viro
I don't believe that it's right. Note that if you *do* race there, you are fucked regardless of sysctls -
ppdev.c::register_device()racing with itself will do tons of fun things all by itself (starting with two threads allocating different pdev and both settingpp->pdev).I wouldn't call it a race - BKL is protecting multiple ioctl calls, so we won't ever claim the device from two different threads.
Yes, we can - register_device() does kmalloc() with GFP_KERNEL, which kills any protection from BKL. BKL is _not_ providing any kind of exclusion while you sleep.