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
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/dri-devel
