From mboxrd@z Thu Jan 1 00:00:00 1970 From: Martin Peres Subject: Re: [PATCH v2 0/6] DRM: VMA Access Management and Render Nodes Date: Sun, 25 Aug 2013 20:22:55 +0200 Message-ID: <521A4B7F.5010201@free.fr> References: <1377256408-746-1-git-send-email-dh.herrmann@gmail.com> <52174EF3.7050302@free.fr> Mime-Version: 1.0 Content-Type: text/plain; charset="iso-8859-1"; Format="flowed" Content-Transfer-Encoding: quoted-printable Return-path: Received: from smtp5-g21.free.fr (smtp5-g21.free.fr [212.27.42.5]) by gabe.freedesktop.org (Postfix) with ESMTP id AE082E609B for ; Sun, 25 Aug 2013 11:22:40 -0700 (PDT) In-Reply-To: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: dri-devel-bounces+sf-dri-devel=m.gmane.org@lists.freedesktop.org Errors-To: dri-devel-bounces+sf-dri-devel=m.gmane.org@lists.freedesktop.org To: David Herrmann Cc: "dri-devel@lists.freedesktop.org" List-Id: dri-devel@lists.freedesktop.org On 25/08/2013 17:09, David Herrmann wrote: > Hi > > On Fri, Aug 23, 2013 at 2:00 PM, Martin Peres wrot= e: >> Le 23/08/2013 13:13, David Herrmann a =E9crit : >> >>> Hi >>> >>> I reduced the vma access-management patches to a minimum. I now do filp* >>> tracking in gem unconditionally and force drm_gem_mmap() to check this. >>> Hence, >>> all gem drivers are safe now. For TTM drivers, I now use the already >>> available >>> verify_access() callback to get access to the underlying gem-object. >>> Pretty >>> simple.. Why hadn't I thought of that before? >>> >>> Long story short: All drivers using GEM are safe now. This leaves vmwgf= x.. >>> But >>> they do their own access-management, anyway. >> >> Great! Thanks! Have you checked they are really safe with my "exploits"? >> I'll have another round of review even if it looked good the last time I >> checked. > Good you asked. I tested whether it works, I didn't actually verify > that it correctly fails in case of exploits. And in fact there is a > small bug (I return "1" instead of -EACCES, stupid verify_access()) so > user-space gets a segfault accessing the mmap when trying to exploit > this. That actually doesn't sound that bad, does it? ;) Good to know I could contribute a little to your work. You're doing a = great job! > v2 is on its way. Yep, saw it. > >>> The 3 patches on top implement render-nodes. I added a "drm_rnodes" mod= ule >>> parameter to core drm. You need to pass "drm.rnodes=3D1" on the kernel >>> command-line or via sysfs _before_ loading a driver. Otherwise, render >>> nodes >>> will not be created. >> >> By default, having the render nodes doesn't change the way the userspace >> works at all. So, what is the point of protecting it behind a parameter? >> >> Is it to make it clear this isn't part of the API yet? I would say that = as >> long >> as libdrm hasn't been updated, this isn't part of the API anyway. > Hm, I wouldn't say so. Applications like weston and kmscon no longer > use the legacy drmOpen() facility. They use udev+open(). So once it's > upstream, it's part of the API regardless of libdrm. So the sole > purpose of drm_rnodes is to mark it as "experimental". Ah, I guess I'll have to have a look at this. I basically got preempted from adding render node support to Weston and I didn't take the time to check it again, the vma patches were more important first. Thanks for saving me a giant headache with GEM/TTM, I spent two week ends trying to track a leak for radeon cards. >>> This allows us to test render-nodes and play with the API. I added FLINK >>> for >>> now so we can better test it. Not sure whether we should allow it in the >>> end, >>> though. >> >> From a security point of view, I don't think we should keep it as >> applications shouldn't >> be trusted for not doing stupid things (because of code injection). So, >> unless we >> plan on adding access control to flink via LSM, we shouldn't expose the >> feature >> on render nodes. > This is also what I think. We have a chance to get rid of all legacy > stuff, so maybe we should just drop it all. Great! > >> From a dev point of view, keeping it means that the XServer doesn't >> have to know whether mesa supports render nodes or not. This is because >> the authentication dance isn't available on render nodes so the x-server >> has to tell mesa if it should authenticate or not. The other solution is= to >> allow >> the authentication ioctls on render nodes and just return 0 if this is a >> render node. >> This way, we won't need any modification in mesa/xserver/dri2proto to pa= ss >> the information that no authentication is needed. In this solution, only >> libdrm and >> the ddx should be modified to make use of the render node. That's not ho= w I >> did it on my render node patchset, can't remember why... >> >> What do you guys think? > We discussed that a bit on IRC. Of course, we can add a lot of > wrappers and workarounds. We can make all the drmAuth stuff *just > work*. But that means, we keep all the legacy. As said, we have the > chance to introduce a new API and drop all the legacy. I think it is > worth a shot. And we also notice quite fast which user-space programs > need some rework. Well, if by "all the legacy", you mean the authentication-related = functions then yes. How do you plan on handling the case where the ddx has been updated and = passes the render node to a not-yet-updated mesa? Mesa will try to authenticate = and it will fail. Keeping the authentication IOCTLs seem to me like a lesser evil, = especially since they would basically do nothing. > >>> Maybe we can get this into 3.11? >> >> As long as we don't have to keep the interface stable (I don't want to >> expose flink >> on render nodes), I'm all for pushing the code now. Otherwise, a kernel >> branch >> somewhere is sufficient. >> >> Do you plan on checking my userspace patches too? Those are enough to ma= ke >> use >> of the render nodes on X, but I haven't tested that all the combinations= of >> version >> would still work. The libdrm work should be quite solid though (there are >> even libdrm >> tests for the added functionalities :)). > I plan on having a working user-space for XDC. Most of your patches > can be copied unchanged indeed. But servers other than Xorg don't use > that, so they need separate fixes. Brilliant :) Looking forward to it! Martin