Re: [RFC PATCH v3 4/4] tests/drv_module_reload: add ipvr support

From: Thierry Reding <thierry.reding@gmail.com>
To: Daniel Vetter <daniel@ffwll.ch>
Cc: daniel.vetter@ffwll.ch, emil.l.velikov@gmail.com,
	dri-devel@lists.freedesktop.org, john.chehab@intel.com,
	fei.jiang@intel.com, intel-gfx@lists.freedesktop.org
Subject: Re: [RFC PATCH v3 4/4] tests/drv_module_reload: add ipvr support
Date: Wed, 17 Dec 2014 09:02:53 +0100	[thread overview]
Message-ID: <20141217080252.GA3303@ulmo> (raw)
In-Reply-To: <20141124131448.GP25711@phenom.ffwll.local>

[-- Attachment #1.1: Type: text/plain, Size: 2557 bytes --]

On Mon, Nov 24, 2014 at 02:14:48PM +0100, Daniel Vetter wrote:
> On Mon, Nov 24, 2014 at 10:55:46AM +0100, Thierry Reding wrote:
> > On Fri, Nov 21, 2014 at 09:36:33PM +0100, Daniel Vetter wrote:
> > > On Fri, Nov 21, 2014 at 09:27:04PM +0100, Thierry Reding wrote:
> > > > On Sat, Nov 22, 2014 at 03:10:01AM +0800, Yao Cheng wrote:
> > > > > on vlv, if ipvr is installed, it need be manually unloaded before
> > > > > i915, otherwise user might run into use-after-free issue.
> > > > 
> > > > Huh? That doesn't sound right. What exactly is it that's going wrong?
> > > > You should never have to do this. If you do you're almost certainly
> > > > doing something wrong in the kernel module.
> > > 
> > > It's the hilarity called platform devices. Removing them is somewhat racy,
> > > so doing that upfront makes the entire thing a bit safer. The use after
> > > free is on the text, since grabbing a module refcount for the platform
> > > device doesn't work (it would pin the module forever).
> > 
> > I don't understand what the issue is here. I've used platform devices
> > quite extensively on ARM and I've never encountered a situation where
> > they were insufficient (or racy for that matter).
> > 
> > If I understand correctly what this commit tries to achieve, then it
> > unloads one module before another module that it depends on so that the
> > dependency can be removed subsequently without causing a crash. That
> > sounds really brittle to me. How are you going to document this for
> > users so that they don't accidentally go and unload the i915 module and
> > crash their system?
> 
> Module unloading taints your kernel and isn't an end-user supported
> feature. That simple ;-)
> 
> Also afaik the problem is that you actually can't unload i915 until you've
> unloaded the subordinate driver, since i915 registering the platform
> driver prevents unload.

That doesn't sound at all like use-after-free, so if that's really the
only problem then the commit description should be more accurate.

> Or at least that was my understanding, I didn't test this myself. I
> just asked whether the unload script still works and apparently it
> breaks.
> 
> I guess what's different with ARM is that DT creates all the platform
> devices, and not modules themselves?

No, I don't think that has anything to do with it. I'm pretty sure I've
seen this work reliably with something like MFD where one module can
create a number of platform devices, and remove them again, just as
well.

Thierry

[-- Attachment #1.2: Type: application/pgp-signature, Size: 819 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel