Various bits of the DRM deal with minor->master:
In the case of the open helper its protected by the struct mutex.
In the release path it's protected on some paths, but not this one ...
/* if the master has gone away we can't do anything with the lock */ if (file_priv->minor->master) drm_master_release(dev, filp);
and I can't see what makes this safe if the drm_release for the master and a client occur at the same time ?
The setmaster/dropmaster ioctl seems similar - the various conditional checks are not protected from parallel changes occuring during their execution.
Is this a bug or is something clever afoot ?
Alan
On Fri, Apr 23, 2010 at 8:22 PM, Alan Cox alan@lxorguk.ukuu.org.uk wrote:
Various bits of the DRM deal with minor->master:
In the case of the open helper its protected by the struct mutex.
In the release path it's protected on some paths, but not this one ...
/* if the master has gone away we can't do anything with the lock */ if (file_priv->minor->master) drm_master_release(dev, filp);
and I can't see what makes this safe if the drm_release for the master and a client occur at the same time ?
lock_kernel in drm_release. We probably need to clean that up.
The setmaster/dropmaster ioctl seems similar - the various conditional checks are not protected from parallel changes occuring during their execution.
Is this a bug or is something clever afoot ?
These ioctls are also under the BKL.
So yes its nasty, and we should probably grow a minor lock to protect that.
Dave.
and I can't see what makes this safe if the drm_release for the master and a client occur at the same time ?
lock_kernel in drm_release. We probably need to clean that up.
I don't think that works. drm_open_helper doesn't appear to be under the BKL merely the struct mutex.
The setmaster/dropmaster ioctl seems similar - the various conditional checks are not protected from parallel changes occuring during their execution.
Is this a bug or is something clever afoot ?
These ioctls are also under the BKL.
But setmaster can sleep so the BKL is dropped on contention of the struct_mutex, ditto dropmaster
Alan
On Fri, Apr 23, 2010 at 8:37 PM, Alan Cox alan@lxorguk.ukuu.org.uk wrote:
and I can't see what makes this safe if the drm_release for the master and a client occur at the same time ?
lock_kernel in drm_release. We probably need to clean that up.
I don't think that works. drm_open_helper doesn't appear to be under the BKL merely the struct mutex.
It blocks the case you specified of two releases happening together.
The setmaster/dropmaster ioctl seems similar - the various conditional checks are not protected from parallel changes occuring during their execution.
Is this a bug or is something clever afoot ?
These ioctls are also under the BKL.
But setmaster can sleep so the BKL is dropped on contention of the struct_mutex, ditto dropmaster
they should only sleep in the mutex lock nuless the driver callback is allocating memory. but yeah its a bit of a mess.
Dave.
I don't think that works. drm_open_helper doesn't appear to be under the BKL merely the struct mutex.
It blocks the case you specified of two releases happening together.
But not parallel open/release
But setmaster can sleep so the BKL is dropped on contention of the struct_mutex, ditto dropmaster
they should only sleep in the mutex lock nuless the driver callback is allocating memory. but yeah its a bit of a mess.
With the mutex alone the damage is done. Consider two setmasters and some other action which is making the mutex contend
CPU1 CPU2
file->priv->minor->master == NULL ? file_priv->minor->master != file_priv->master
mutex_lock (drop BKL) file->priv->minor->master === NULL master != file_priv->master mutex_lock [drop BKL] takes mutex minor->master = drm_master_get is_master = 1 master_set drop mutex return 0 takes mutex minor->master = drm_master_get is_master = 1 master_set drops mutex return 0
Alan
dri-devel@lists.freedesktop.org
-
Alan Cox -
Dave Airlie