From patchwork Mon Apr 1 18:14:50 2013 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Ilija Hadzic X-Patchwork-Id: 2372121 Return-Path: X-Original-To: patchwork-dri-devel@patchwork.kernel.org Delivered-To: patchwork-process-083081@patchwork1.kernel.org Received: from gabe.freedesktop.org (gabe.freedesktop.org [131.252.210.177]) by patchwork1.kernel.org (Postfix) with ESMTP id 6D7333FDDA for ; Mon, 1 Apr 2013 18:39:22 +0000 (UTC) Received: from gabe.freedesktop.org (localhost [127.0.0.1]) by gabe.freedesktop.org (Postfix) with ESMTP id 29355E6259 for ; Mon, 1 Apr 2013 11:39:22 -0700 (PDT) X-Original-To: dri-devel@lists.freedesktop.org Delivered-To: dri-devel@lists.freedesktop.org X-Greylist: delayed 1281 seconds by postgrey-1.32 at gabe; Mon, 01 Apr 2013 11:36:18 PDT Received: from ihemail4.lucent.com (ihemail4.lucent.com [135.245.0.39]) by gabe.freedesktop.org (Postfix) with ESMTP id 43E80E6274 for ; Mon, 1 Apr 2013 11:36:18 -0700 (PDT) Received: from usnavsmail1.ndc.alcatel-lucent.com (usnavsmail1.ndc.alcatel-lucent.com [135.3.39.9]) by ihemail4.lucent.com (8.13.8/IER-o) with ESMTP id r31IEqLg017757 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=FAIL); Mon, 1 Apr 2013 13:14:52 -0500 (CDT) Received: from umail.lucent.com (umail-ce2.ndc.lucent.com [135.3.40.63]) by usnavsmail1.ndc.alcatel-lucent.com (8.14.3/8.14.3/GMO) with ESMTP id r31IEp0j009927 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=NOT); Mon, 1 Apr 2013 13:14:52 -0500 Received: from umail-ce2 (umail-ce2 [135.3.40.63]) by umail.lucent.com (8.13.8/TPES) with ESMTP id r31IEo7Z014743; Mon, 1 Apr 2013 13:14:50 -0500 (CDT) Date: Mon, 1 Apr 2013 13:14:50 -0500 (CDT) From: Ilija Hadzic X-X-Sender: ihadzic@umail To: Michal Hocko Subject: Re: [PATCH] drm: fix i_mapping and f_mapping initialization in drm_open in error path In-Reply-To: <20130331103418.GA18476@dhcp22.suse.cz> Message-ID: References: <20130326195601.GA5124@dhcp22.suse.cz> <20130331103418.GA18476@dhcp22.suse.cz> MIME-Version: 1.0 X-Scanned-By: MIMEDefang 2.57 on 135.245.2.39 X-Scanned-By: MIMEDefang 2.64 on 135.3.39.9 Cc: dri-devel@lists.freedesktop.org, Thomas Hellstrom , Marco Munderloh , linux-kernel@vger.kernel.org X-BeenThere: dri-devel@lists.freedesktop.org X-Mailman-Version: 2.1.13 Precedence: list List-Id: Direct Rendering Infrastructure - Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: dri-devel-bounces+patchwork-dri-devel=patchwork.kernel.org@lists.freedesktop.org Errors-To: dri-devel-bounces+patchwork-dri-devel=patchwork.kernel.org@lists.freedesktop.org On Sun, 31 Mar 2013, Michal Hocko wrote: > On Sat 30-03-13 18:26:53, Ilija Hadzic wrote: >> This looks a bit like a hack and it doesn't look right, >> conceptually. If the call fails, it should restore things as if >> nothing has ever happened and overwriting old_mapping is not going to >> do the trick. > > OK, I thought this is what the patch does as it falls back to > &inode->i_data which is the default mapping for all inodes or it uses > what used to be in device mapping. > > I am obviously not familiar with the drm code but it feels a bit strange > that the device mapping can be different than inode's resp. file's one The reason for this is explained in commit message associated with 949c4a34. In summary, the device's mapping is that of the inode associated with the first opener. Before 949c4a34, subsequent openers would have to come in through exactly the same inode that the first opener came in (otherwise the open call would fail). So if a user did something like: start X, remove /dev/dri/cardN file, mknod the same file again, the applications started after such an action would stop working. Also, using the GPU from chroot-ed environment was not possible if there was another opener from different root. The 949c4a34, removed this restriction, but introduced a problem with VmWare GPU drivers, which fdb40a08. However, fdb40a08 introduced the bug that you have reported. The problem that I have with your proposed fix is that if the first opener fails, it can set the device's mapping to that of the inode that was never used and never opened (and could even be removed later down the road). > and even more confusing that inode and file are saved separately. > I was trying to quickly get out the patch that was safe in terms of introducing new breakage. So the "conservative" thing to do (without having to think through all possible scenarios) was to restore each of the three pointers from their own temporary variable. Thinking about it, you are probably right that file descriptor's and inode's mapping pointer are equal when open call is entered so we could use one variable. However, you still need a separate variable to store the device's mapping pointer because that one can be different. Attached is a v2 of the patch, for reference. I would appreciate if the original reporter or you tested it in lieu of your proposed patch and let me know if it fixes your issue. -- Ilija From 7e3c832158e2552e5e106a588e2b9e61c35b68f2 Mon Sep 17 00:00:00 2001 From: Ilija Hadzic Date: Sat, 30 Mar 2013 18:20:35 -0400 Subject: [PATCH] drm: correctly restore mappings if drm_open fails If first drm_open fails, the error-handling path will incorrectly restore inode's mapping to NULL. This can cause the crash later on. Fix by separately storing away mapping pointers that drm_open can touch and restore each from its own respective variable if the call fails. Reference: http://lists.freedesktop.org/archives/dri-devel/2013-March/036564.html v2: use one variable to store file and inode mapping since they are the same at the function entry; also fix spelling mistakes in commit message. Reported-by: Marco Munderloh Signed-off-by: Ilija Hadzic Cc: Michal Hocko Cc: stable@vger.kernel.org Signed-off-by: Ilija Hadzic --- drivers/gpu/drm/drm_fops.c | 6 ++++-- 1 files changed, 4 insertions(+), 2 deletions(-) -- 1.7.4.1 Reviewed-by: Michal Hocko diff --git a/drivers/gpu/drm/drm_fops.c b/drivers/gpu/drm/drm_fops.c index 13fdcd1..429e07d 100644 --- a/drivers/gpu/drm/drm_fops.c +++ b/drivers/gpu/drm/drm_fops.c @@ -123,6 +123,7 @@ int drm_open(struct inode *inode, struct file *filp) int retcode = 0; int need_setup = 0; struct address_space *old_mapping; + struct address_space *old_imapping; minor = idr_find(&drm_minors_idr, minor_id); if (!minor) @@ -137,6 +138,7 @@ int drm_open(struct inode *inode, struct file *filp) if (!dev->open_count++) need_setup = 1; mutex_lock(&dev->struct_mutex); + old_imapping = inode->i_mapping; old_mapping = dev->dev_mapping; if (old_mapping == NULL) dev->dev_mapping = &inode->i_data; @@ -159,8 +161,8 @@ int drm_open(struct inode *inode, struct file *filp) err_undo: mutex_lock(&dev->struct_mutex); - filp->f_mapping = old_mapping; - inode->i_mapping = old_mapping; + filp->f_mapping = old_imapping; + inode->i_mapping = old_imapping; iput(container_of(dev->dev_mapping, struct inode, i_data)); dev->dev_mapping = old_mapping; mutex_unlock(&dev->struct_mutex);