[PATCH] parport/ppdev: fix registration of sysctl entries

[PATCH] parport/ppdev: fix registration of sysctl entries

[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).

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

Reported-by: Frans Pop <elendil@planet.nl>
Signed-off-by: Marcin Slusarz <marcin.slusarz@gmail.com>
Cc: Eric W. Biederman <ebiederm@xmission.com>
---
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(-)

diff --git a/drivers/char/ppdev.c b/drivers/char/ppdev.c
index 3aab837..e60111a 100644
--- a/drivers/char/ppdev.c
+++ b/drivers/char/ppdev.c
@@ -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;
 
diff --git a/drivers/parport/procfs.c b/drivers/parport/procfs.c
index d950fc3..5a6ac1d 100644
--- a/drivers/parport/procfs.c
+++ b/drivers/parport/procfs.c
@@ -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)
 {
diff --git a/drivers/parport/share.c b/drivers/parport/share.c
index a8a62bb..3a777be 100644
--- a/drivers/parport/share.c
+++ b/drivers/parport/share.c
@@ -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:
diff --git a/include/linux/parport.h b/include/linux/parport.h
index dcb9e01..f871031 100644
--- a/include/linux/parport.h
+++ b/include/linux/parport.h
@@ -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 *);
-- 
1.5.4.5

Re: [PATCH] parport/ppdev: fix registration of sysctl entries

[Al Viro]

On Sat, Jul 05, 2008 at 03:21:06PM +0200, Marcin Slusarz wrote:
> 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.

Re: [PATCH] parport/ppdev: fix registration of sysctl entries

[Al Viro]

On Sun, Jul 06, 2008 at 12:51:48AM +0100, Al Viro wrote:

> 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.

BTW, with your patch you'll have 100% reproducible double registration if
you do PPCLAIM/PPRELEASE/PPCLAIN on one file descriptor.

Re: [PATCH] parport/ppdev: fix registration of sysctl entries

[Al Viro]

On Sun, Jul 06, 2008 at 01:11:48AM +0100, Al Viro wrote:
> On Sun, Jul 06, 2008 at 12:51:48AM +0100, Al Viro wrote:
> 
> > 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.
> 
> 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.

Re: [PATCH] parport/ppdev: fix registration of sysctl entries

[Eric W. Biederman]

Al Viro <viro@ZenIV.linux.org.uk> writes:

> On Sun, Jul 06, 2008 at 01:11:48AM +0100, Al Viro wrote:
>> On Sun, Jul 06, 2008 at 12:51:48AM +0100, Al Viro wrote:
>> 
>> > 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.
>> 
>> 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.

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

Re: [PATCH] parport/ppdev: fix registration of sysctl entries

[Al Viro]

On Sat, Jul 05, 2008 at 11:49:26PM -0700, Eric W. Biederman wrote:
> 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).

> - 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...

> - 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.

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.

Re: [PATCH] parport/ppdev: fix registration of sysctl entries

[Eric W. Biederman]

Al Viro <viro@ZenIV.linux.org.uk> writes:

> On Sat, Jul 05, 2008 at 11:49:26PM -0700, Eric W. Biederman wrote:
>> 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.
>
>> - 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.

>> - 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.
>
> 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.

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.

> 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.

> _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

Re: [PATCH] parport/ppdev: fix registration of sysctl entries

[Eduard - Gabriel Munteanu]

On Sun, 6 Jul 2008 09:11:26 +0100
Al Viro <viro@ZenIV.linux.org.uk> wrote:

> On Sat, Jul 05, 2008 at 11:49:26PM -0700, Eric W. Biederman wrote:
> > 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

Re: [PATCH] parport/ppdev: fix registration of sysctl entries

[Alan Cox]

> 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.

Re: [PATCH] parport/ppdev: fix registration of sysctl entries

[Eduard - Gabriel Munteanu]

On Sun, 6 Jul 2008 17:08:02 +0100
Alan Cox <alan@lxorguk.ukuu.org.uk> wrote:

> > 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

Re: [PATCH] parport/ppdev: fix registration of sysctl entries

[Alan Cox]

> 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 ?

> 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.

Re: [PATCH] parport/ppdev: fix registration of sysctl entries

[Marcin Slusarz]

On Sun, Jul 06, 2008 at 05:05:19AM +0100, Al Viro wrote:
> 	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)?

Re: [PATCH] parport/ppdev: fix registration of sysctl entries

[Marcin Slusarz]

On Sun, Jul 06, 2008 at 01:11:48AM +0100, Al Viro wrote:
> On Sun, Jul 06, 2008 at 12:51:48AM +0100, Al Viro wrote:
> 
> > 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).

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.

> 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()).

Re: [PATCH] parport/ppdev: fix registration of sysctl entries

[Arjan van de Ven]

On Sun, 6 Jul 2008 17:07:02 +0200
Marcin Slusarz <marcin.slusarz@gmail.com> wrote:

> On Sun, Jul 06, 2008 at 01:11:48AM +0100, Al Viro wrote:
> > On Sun, Jul 06, 2008 at 12:51:48AM +0100, Al Viro wrote:
> > 
> > > 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).
> 
> 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 ;)(


-- 
If you want to reach me at my work email, use arjan@linux.intel.com
For development, discussion and tips for power savings, 
visit http://www.lesswatts.org

Re: [PATCH] parport/ppdev: fix registration of sysctl entries

[Al Viro]

On Sun, Jul 06, 2008 at 05:07:02PM +0200, Marcin Slusarz wrote:
> On Sun, Jul 06, 2008 at 01:11:48AM +0100, Al Viro wrote:
> > On Sun, Jul 06, 2008 at 12:51:48AM +0100, Al Viro wrote:
> > 
> > > 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).
> 
> 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.