Date: Sun, 6 Jul 2008 05:05:19 +0100
From: Al Viro <viro@ZenIV.linux.org.uk>
To: Marcin Slusarz <marcin.slusarz@gmail.com>
Cc: LKML <linux-kernel@vger.kernel.org>, Frans Pop <elendil@planet.nl>,
       "Eric W. Biederman" <ebiederm@xmission.com>
Subject: Re: [PATCH] parport/ppdev: fix registration of sysctl entries
Message-ID: <20080706040519.GZ28946@ZenIV.linux.org.uk>
References: <20080705131924.GA2083@joi> <20080705235148.GW28946@ZenIV.linux.org.uk> <20080706001148.GX28946@ZenIV.linux.org.uk>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20080706001148.GX28946@ZenIV.linux.org.uk>
User-Agent: Mutt/1.5.17 (2007-11-01)
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 1925
Lines: 37

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.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/