From mboxrd@z Thu Jan 1 00:00:00 1970 From: Thomas Hellstrom Subject: Re: Breakage in "track dev_mapping in more robust and flexible way" Date: Mon, 29 Oct 2012 09:39:09 +0100 Message-ID: <508E40AD.9060009@shipmail.org> References: <50894671.2070803@vmware.com> <20121025144136.GB2062@gmail.com> <50895672.7070706@vmware.com> <5089847B.50808@vmware.com> <508A4599.80507@vmware.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: Received: from GOTHNET-SMTP2.gothnet.se (ns2.gothnet.se [82.193.160.251]) by gabe.freedesktop.org (Postfix) with ESMTP id B99FB9E9C2 for ; Mon, 29 Oct 2012 01:39:13 -0700 (PDT) Received: from mail.shipmail.org (lin0.kontor.shipmail.org [127.0.0.1]) by mail.shipmail.org (Postfix) with ESMTP id 1663333807C for ; Mon, 29 Oct 2012 09:39:10 +0100 (CET) Received: from linlap1.kontor.shipmail.org (lin0.kontor.shipmail.org [127.0.0.1]) by mail.shipmail.org (Postfix) with ESMTP id E129433806F for ; Mon, 29 Oct 2012 09:39:09 +0100 (CET) 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: dri-devel@lists.freedesktop.org List-Id: dri-devel@lists.freedesktop.org On 10/26/2012 03:14 PM, Ilija Hadzic wrote: > > > On Fri, 26 Oct 2012, Thomas Hellstrom wrote: > >> Hi, >> >> On 10/25/2012 11:27 PM, Ilija Hadzic wrote: >>> >>> Can you give the attached patch a whirl and let me know if it fixes >>> the problem? >>> >>> As I indicated in my previous note, vmwgfx should be the only >>> affected driver because it looks at dev_mapping in the open hook >>> (others do it when they create an object, so they are not affected). >>> >>> I'll probably revise it (and I'll have some general questions about >>> drm_open syscall) before officially send the patch, but I wanted to >>> get something quickly to you to check if it fixes your problem. I >>> hope that your vmwgfx test environment is such that you can >>> reproduce the original >>> problem. >>> >>> thanks, >>> >>> -- Ilija >> >> Yes, it appears like this patch fixes the problem. It'd be good to >> have it in 3.7 (drm-fixes) with a cc to stable. >> > > OK great. Thanks for testing. Before I send out an "official" patch, I > have a few questions for those who have been around longer and can > possibly reflect better than me on the history of drm_open syscall. > > Currently, before touching dev->dev_mapping field we grab dev->struct > mutex. This has been introduced by Dave Airlie a long time ago in > a2c0a97b784f837300f7b0869c82ab712c600952. I tried to preserve that in > all patches where I touched dev_open, but looking at the code I don't > think the mutex is necessary. Namely, drm_open is only set in > drm_open, and concurrent openers are protected with drm_global_mutex. > Other places (drivers) where dev->dev_mapping is accessed is read-only > and dev_mapping is written at first open when there are no file > descriptors around to issue any other call. Also, it doesn't look to > me that any driver locks dev->struct_mutex before accessing > dev->dev_mapping anyway. So I am thinking of dropping the mutex > completely, but I would like to hear a second thought. Without having looked a the code, with your current changes dev->dev_mapping should be immutable and initialized before any consumers reference it, and as such would need no mutex, so dropping the protection of dev->dev_mapping from that point of view should be fine. I think people sooner or later want to get rid of drm_global_mutex, though, but at that point we probably want another mutex that protects open-time initialization of immutable members only, so from my point of view this is OK, but you might want to double-check with Dave. > > The other issue, I noticed is that of the drm_setup() call fails, the > open_count counter would remain incremented and I think we need to > restore it back (or if I am missing something, would someone please > enlighten me). This was also in the kernel all this time (and I have > not noticed until now), so I "smuggled" that fix in the patch that I > sent you. However, wonder if I should cut the separate patch for > open_count fix. > > Actually, I think that I should cut three patches: one to drop the > mutex, one to fix the open_count and one to fix your problem with > dev_mapping and that probably all three should CC stable. Before I do > that, I'd like to hear opinions of others. I think you should, However stable doesn't want fixes for theoretical stuff that have never been triggered in real life, so the patch to drop mutex protection doesn't belong there. That's a patch for drm-next, so people get a decent chance to see if it breaks something. The dev_mapping thing opens up a quite severe security issue and should got into drm-fixes with Cc to stable as soon as ever possible. The open_count stuff should go into drm-fixes, possibly cc'd to stable. Thanks, Thomas > > thanks, > > Ilija > > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/dri-devel