Re: [PATCH] drm: fix i_mapping and f_mapping initialization indrm_open in error path

[Ilija Hadzic]

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 <ihadzic@xxxxxxxxxxxxxxxxxxxxxx>
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 <munderl@xxxxxxxxxxxxxxxxxxx>
Signed-off-by: Ilija Hadzic <ihadzic@xxxxxxxxxxxxxxxxxxxxxx>
Cc: Michal Hocko <mhocko@xxxxxxx>
Cc: stable@xxxxxxxxxxxxxxx

Signed-off-by: Ilija Hadzic <ihadzic@xxxxxxxxxxxxxxxxxxxxxx>
---
 drivers/gpu/drm/drm_fops.c |    6 ++++--
 1 files changed, 4 insertions(+), 2 deletions(-)

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);
-- 
1.7.4.1