Re: [PATCH v3 8/9] drm/rockchip: gem: Don't alloc/free gem buf when dev_private is invalid

From: Daniel Vetter <daniel@ffwll.ch>
To: jeffy <jeffy.chen@rock-chips.com>
Cc: Brian Norris <briannorris@chromium.org>,
	Douglas Anderson <dianders@chromium.org>,
	dri-devel <dri-devel@lists.freedesktop.org>,
	Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
	"open list:ARM/Rockchip SoC..."
	<linux-rockchip@lists.infradead.org>,
	Chris Zhong <zyw@rock-chips.com>,
	Tomasz Figa <tfiga@chromium.org>,
	"linux-arm-kernel@lists.infradead.org"
	<linux-arm-kernel@lists.infradead.org>
Subject: Re: [PATCH v3 8/9] drm/rockchip: gem: Don't alloc/free gem buf when dev_private is invalid
Date: Fri, 7 Apr 2017 09:15:31 +0200	[thread overview]
Message-ID: <20170407071531.2siqzefoddys6lut@phenom.ffwll.local> (raw)
In-Reply-To: <58E73535.4000409@rock-chips.com>

On Fri, Apr 07, 2017 at 02:44:05PM +0800, jeffy wrote:
> Hi Daniel,
> 
> On 04/07/2017 02:30 PM, Daniel Vetter wrote:
> > On Thu, Apr 6, 2017 at 1:09 PM, jeffy <jeffy.chen@rock-chips.com> wrote:
> > > 
> > > On 04/06/2017 04:26 PM, Daniel Vetter wrote:
> > > > 
> > > > On Wed, Apr 05, 2017 at 12:28:40PM -0400, Sean Paul wrote:
> > > > > 
> > > > > On Wed, Apr 05, 2017 at 04:29:26PM +0800, Jeffy Chen wrote:
> > > > > > 
> > > > > > After unbinding drm, the userspace may still has a chance to access
> > > > > > gem buf.
> > > > > > 
> > > > > > Add a sanity check for a NULL dev_private to prevent that from
> > > > > > happening.
> > > > > 
> > > > > 
> > > > > I still don't understand how this is happening. You're saying that these
> > > > > hooks
> > > > > can be called after rockchip_drm_unbind() has finished?
> > > > 
> > > > 
> > > > Yeah this is supposed to be impossible. If it isn't, we need to debug and
> > > > fix this properly. This smells like pretty bad duct-tape ...
> > > 
> > > 
> > > it looks like after unbind, the user space may still own drm dev fd, and
> > > could be able to call ioctl:
> > > lrwx------. 1 chronos chronos 64 Mar 15 12:53 28 -> /dev/dri/card1 (deleted)
> > > 
> > > and the drm_unplug_dev may help it, maybe we should call it in unbind? or
> > > just break drm_ioctl when drm_dev not registered?
> > 
> > Yes, by default unbind while userspace is running is totally broken in
> > drm. drm_unplug_dev would be the fix, but it's only used by udl and
> > not many use that. You might need to fix infrastructure up a bit.
> please check this patch:
> 9667071 New          [v5,12/12] drm/drm_ioctl.c: Break ioctl when drm device
> not registered
> > 
> > For normal module unload the module reference will prevent unloading.
> > So why exactly do you care about the unbind use-case?
> sometimes we use unbind/bind for testing ;)

Then make sure you stop your userspace first. Fixing unbind to be
completely race-free requires a bit of work in the drm core.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel