dtrace: should not sleep in idr code paths

The idr_preload() causes thread to continue in atomic context.
Taking mutex in this code path may lead to deadlock or scheduler
problems.  This fix alters locking scheme in a way that may cause
some latency issues in DTrace framework but will keep host safe.

Orabug: 26680802

Signed-off-by: Tomas Jedlicka <tomas.jedlicka@oracle.com>
Reviewed-by: Nick Alcock <nick.alcock@oracle.com>

diff --git a/dtrace/dtrace_ecb.c b/dtrace/dtrace_ecb.c
index 52b04badc3f036afce32494a1ea14c1376fb46b1..4e551a4ea318cc82acb794a97961145d4f314181 100644 (file)
--- a/dtrace/dtrace_ecb.c
+++ b/dtrace/dtrace_ecb.c
@@ -164,11 +164,7 @@ success:
        /*
         * Get an ID for the aggregation (add it to the idr).
         */
-       mutex_unlock(&dtrace_lock);
-
        idr_preload(GFP_KERNEL);
-       mutex_lock(&dtrace_lock);
-
        aggid = idr_alloc_cyclic(&state->dts_agg_idr, agg, 0, 0, GFP_NOWAIT);
        idr_preload_end();
        if (aggid < 0) {

diff --git a/dtrace/dtrace_probe.c b/dtrace/dtrace_probe.c
index de6e6edc34189ab72ff8a75f9bb15b111beff481..46f7db1294d75a01834d9b44e91cec7450017fe9 100644 (file)
--- a/dtrace/dtrace_probe.c
+++ b/dtrace/dtrace_probe.c
@@ -53,18 +53,24 @@ dtrace_id_t dtrace_probe_create(dtrace_provider_id_t prov, const char *mod,
        probe = kmem_cache_alloc(dtrace_probe_cachep, __GFP_NOFAIL);
 
        /*
-        * The ir_preload() function should be called without holding locks.
+        * The idr_preload() should be called without holding locks as it may
+        * block.  At the same time it is required to protect DTrace structures.
+        * We can't drop it before idr_preload() and acquire after it because
+        * we can't sleep in atomic context (until we reach idr_preload_end()).
+        *
+        * It is better to delay DTrace framework than traced host so the lock
+        * is being held for the duration of idr allocation.
+        *
         * When the provider is the DTrace core itself, dtrace_lock will be
         * held when we enter this function.
         */
        if (provider == dtrace_provider) {
                ASSERT(MUTEX_HELD(&dtrace_lock));
-               mutex_unlock(&dtrace_lock);
+       } else {
+               mutex_lock(&dtrace_lock);
        }
 
        idr_preload(GFP_KERNEL);
-
-       mutex_lock(&dtrace_lock);
        id = idr_alloc_cyclic(&dtrace_probe_idr, probe, 0, 0, GFP_NOWAIT);
        idr_preload_end();
        if (id < 0) {
@@ -1282,16 +1288,7 @@ int dtrace_probe_init(void)
         * We need to drop our locks when calling idr_preload(), so we try to
         * get them back right after.
         */
-       mutex_unlock(&dtrace_lock);
-       mutex_unlock(&dtrace_provider_lock);
-       mutex_unlock(&cpu_lock);
-
        idr_preload(GFP_KERNEL);
-
-       mutex_lock(&cpu_lock);
-       mutex_lock(&dtrace_provider_lock);
-       mutex_lock(&dtrace_lock);
-
        id = idr_alloc_cyclic(&dtrace_probe_idr, NULL, 0, 0, GFP_NOWAIT);
        idr_preload_end();
 

diff --git a/dtrace/dtrace_state.c b/dtrace/dtrace_state.c
index 249642965a2320d725c74d87de066f97b60f3cca..c04be314f8eb2ba0f09ec5ddc7142afff31b008b 100644 (file)
--- a/dtrace/dtrace_state.c
+++ b/dtrace/dtrace_state.c
@@ -352,14 +352,7 @@ dtrace_state_t *dtrace_state_create(struct file *file)
         * Create a first entry in the aggregation IDR, so that ID 0 is used as
         * that gets used as meaning 'none'.
         */
-       mutex_unlock(&dtrace_lock);
-       mutex_unlock(&cpu_lock);
-
        idr_preload(GFP_KERNEL);
-
-       mutex_lock(&cpu_lock);
-       mutex_lock(&dtrace_lock);
-
        aggid = idr_alloc_cyclic(&state->dts_agg_idr, NULL, 0, 0, GFP_NOWAIT);
        idr_preload_end();