| Message ID | Pine.GSO.4.64.1210251622110.394@umail (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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. /Thomas
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. 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. thanks, Ilija
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
diff --git a/drivers/gpu/drm/drm_fops.c b/drivers/gpu/drm/drm_fops.c index 7ef1b67..50b7b47 100644 --- a/drivers/gpu/drm/drm_fops.c +++ b/drivers/gpu/drm/drm_fops.c @@ -121,6 +121,8 @@ int drm_open(struct inode *inode, struct file *filp) int minor_id = iminor(inode); struct drm_minor *minor; int retcode = 0; + int need_setup = 0; + struct address_space *old_mapping; minor = idr_find(&drm_minors_idr, minor_id); if (!minor) @@ -132,23 +134,36 @@ int drm_open(struct inode *inode, struct file *filp) if (drm_device_is_unplugged(dev)) return -ENODEV; + if (!dev->open_count++) + need_setup = 1; + mutex_lock(&dev->struct_mutex); + old_mapping = dev->dev_mapping; + if (old_mapping == NULL) + dev->dev_mapping = &inode->i_data; + mutex_unlock(&dev->struct_mutex); + retcode = drm_open_helper(inode, filp, dev); - if (!retcode) { - atomic_inc(&dev->counts[_DRM_STAT_OPENS]); - if (!dev->open_count++) - retcode = drm_setup(dev); - } - if (!retcode) { - mutex_lock(&dev->struct_mutex); - if (dev->dev_mapping == NULL) - dev->dev_mapping = &inode->i_data; - /* ihold ensures nobody can remove inode with our i_data */ - ihold(container_of(dev->dev_mapping, struct inode, i_data)); - inode->i_mapping = dev->dev_mapping; - filp->f_mapping = dev->dev_mapping; - mutex_unlock(&dev->struct_mutex); + if (retcode) + goto err_undo; + atomic_inc(&dev->counts[_DRM_STAT_OPENS]); + if (need_setup) { + retcode = drm_setup(dev); + if (retcode) + goto err_undo; } + /* ihold ensures nobody can remove inode with our i_data */ + mutex_lock(&dev->struct_mutex); + ihold(container_of(dev->dev_mapping, struct inode, i_data)); + inode->i_mapping = dev->dev_mapping; + filp->f_mapping = dev->dev_mapping; + mutex_unlock(&dev->struct_mutex); + return 0; +err_undo: + dev->open_count--; + mutex_lock(&dev->struct_mutex); + dev->dev_mapping = old_mapping; + mutex_unlock(&dev->struct_mutex); return retcode; } EXPORT_SYMBOL(drm_open);