-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1
I've attached a new version of the patch. This should fix a minor bug: I put
the call to init_timer() too late, which resulted in a kernel warning when
the module was loaded/unloaded without actually being used.
On Sunday 23 May 2004 14:37, Michel DÃnzer wrote:
> > 2. The timeout cannot be configured yet. I didn't find "prior art" as to
how
> > something like it should be configured, so I'm open for input. For a
Linux
> > driver, adding to the /proc entries seems to be the logical way to go,
but
> > the DRI is very ioctl-centric. Maybe both?
>
> What's the goal of making it configurable at all, to allow for driver
> debugging? Maybe that could be dealt with better, see below.
This is actually a good point :)
> Is there a way to tell that a process is being debugged? If so, maybe it
> could be handled sanely by default? E.g., release the lock while the
> process is stopped? (That might wreak havoc once execution is resumed
> though) ...
Could be possible, but it *is* bound to wreak havoc. Now that you talk about
it... in the far future, it would be *very* useful if clients could deal
with temporary loss of access to the DRM. I'm thinking of the recent
discussions about the possible future of fbdev, DRI, etc. where all
graphics access eventually goes through the DRM, or something similar.
In that scenario, we need to have a way to establish a secure terminal that
is safe against
a) fake messages / dialogs created by DRI clients running in the background
and
b) screen scraping by background clients
I don't see how this could be done without revoking authorization
temporarily, including unmapping memory regions.
Once DRI clients can deal with this, running them in a debugger should be a
piece of cake, really :)
cu,
Nicolai
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.2.4 (GNU/Linux)
iD8DBQFAs6BxsxPozBga0lwRAglnAKC9ldd4n/KbE1cDSLPapkRHMx2O0QCZAdMC
Ab4c9daD4WyRZWyGPRJmSyw=
=yLV/
-----END PGP SIGNATURE-----
diff -ur drm-base/linux/drm_drv.h drm/linux/drm_drv.h
--- drm-base/linux/drm_drv.h 2004-05-22 21:41:28.000000000 +0200
+++ drm/linux/drm_drv.h 2004-05-25 19:51:21.000000000 +0200
@@ -273,6 +273,8 @@
MODULE_PARM( drm_opts, "s" );
MODULE_LICENSE("GPL and additional rights");
+static void drm_lock_watchdog( unsigned long __data );
+
static int DRM(setup)( drm_device_t *dev )
{
int i;
@@ -415,6 +417,7 @@
down( &dev->struct_sem );
del_timer( &dev->timer );
+ del_timer_sync( &dev->lock.watchdog );
if ( dev->devname ) {
DRM(free)( dev->devname, strlen( dev->devname ) + 1,
@@ -545,6 +548,7 @@
if ( dev->lock.hw_lock ) {
dev->sigdata.lock = dev->lock.hw_lock = NULL; /* SHM removed */
dev->lock.filp = 0;
+ dev->lock.dontbreak = 1;
wake_up_interruptible( &dev->lock.lock_queue );
}
@@ -581,6 +585,10 @@
sema_init( &dev->struct_sem, 1 );
sema_init( &dev->ctxlist_sem, 1 );
+ init_timer( &dev->lock.watchdog );
+ dev->lock.watchdog.data = (unsigned long) dev;
+ dev->lock.watchdog.function = drm_lock_watchdog;
+
if ((dev->minor = DRM(stub_register)(DRIVER_NAME, &DRM(fops),dev)) < 0)
{
retcode = -EPERM;
@@ -928,6 +936,11 @@
#if __HAVE_RELEASE
DRIVER_RELEASE();
#endif
+ /* Avoid potential race where the watchdog callback is still
+ * running when filp is freed.
+ */
+ del_timer_sync( &dev->lock.watchdog );
+
DRM(lock_free)( dev, &dev->lock.hw_lock->lock,
_DRM_LOCKING_CONTEXT(dev->lock.hw_lock->lock) );
@@ -951,6 +964,7 @@
}
if ( DRM(lock_take)( &dev->lock.hw_lock->lock,
DRM_KERNEL_CONTEXT ) ) {
+ dev->lock.dontbreak = 1;
dev->lock.filp = filp;
dev->lock.lock_time = jiffies;
atomic_inc( &dev->counts[_DRM_STAT_LOCKS] );
@@ -1096,6 +1110,40 @@
return retcode;
}
+
+/**
+ * Lock watchdog callback function.
+ *
+ * Whenever a privileged client must sleep on the lock waitqueue
+ * in the LOCK ioctl, the watchdog timer is started.
+ * When the UNLOCK ioctl is called, the timer is stopped.
+ *
+ * When the watchdog timer expires, the process holding the lock
+ * is killed. Privileged clients set lock.dontbreak and are exempt
+ * from this rule.
+ */
+static void drm_lock_watchdog( unsigned long __data )
+{
+ drm_device_t *dev = (drm_device_t *)__data;
+ drm_file_t *priv;
+
+ if ( !dev->lock.filp ) {
+ DRM_DEBUG( "held by kernel\n" );
+ return;
+ }
+
+ if ( dev->lock.dontbreak ) {
+ DRM_DEBUG( "privileged lock\n" );
+ return;
+ }
+
+ priv = dev->lock.filp->private_data;
+ DRM_DEBUG( "Kill pid=%d\n", priv->pid );
+
+ kill_proc( priv->pid, SIGKILL, 1 );
+}
+
+
/**
* Lock ioctl.
*
@@ -1115,6 +1163,7 @@
DECLARE_WAITQUEUE( entry, current );
drm_lock_t lock;
int ret = 0;
+ int privileged = capable( CAP_SYS_ADMIN );
#if __HAVE_MULTIPLE_DMA_QUEUES
drm_queue_t *q;
#endif
@@ -1157,6 +1206,7 @@
}
if ( DRM(lock_take)( &dev->lock.hw_lock->lock,
lock.context ) ) {
+ dev->lock.dontbreak = privileged;
dev->lock.filp = filp;
dev->lock.lock_time = jiffies;
atomic_inc( &dev->counts[_DRM_STAT_LOCKS] );
@@ -1164,6 +1214,13 @@
}
/* Contention */
+ if ( privileged ) {
+ if ( !timer_pending( &dev->lock.watchdog ) ) {
+ DRM_DEBUG( "Starting lock watchdog\n" );
+ mod_timer( &dev->lock.watchdog, jiffies + 5 * HZ );
+ }
+ }
+
schedule();
if ( signal_pending( current ) ) {
ret = -ERESTARTSYS;
@@ -1243,8 +1300,12 @@
return -EINVAL;
}
+ DRM_DEBUG( "\n" );
+
atomic_inc( &dev->counts[_DRM_STAT_UNLOCKS] );
+ del_timer_sync( &dev->lock.watchdog );
+
/* __HAVE_KERNEL_CTX_SWITCH isn't used by any of the drm
* modules in the DRI cvs tree, but it is required by the
* Sparc driver.
diff -ur drm-base/linux/drmP.h drm/linux/drmP.h
--- drm-base/linux/drmP.h 2004-05-22 21:41:28.000000000 +0200
+++ drm/linux/drmP.h 2004-05-22 19:52:41.000000000 +0200
@@ -574,6 +574,8 @@
struct file *filp; /**< File descr of lock holder (0=kernel) */
wait_queue_head_t lock_queue; /**< Queue of blocked processes */
unsigned long lock_time; /**< Time of last lock in jiffies */
+ struct timer_list watchdog; /**< Watchdog timer to kill runaway processes */
+ int dontbreak; /**< Even watchdog honours the current lock */
} drm_lock_data_t;
/**
