drm/i915: Make i915_pipe_crc_read() oops proof

Currently i915_pipe_crc_read() will drop pipe_crc->lock for the entire
duration of the copy_to_user() loop, which means it'll access
pipe_crc->entries without any protection. If another thread sneaks in
and frees pipe_crc->entries the code will oops.

Reorganize the code to hold the lock around everything except
copy_to_user(). After the copy the lock is reacquired and the the number
of available entries is rechecked.

Since this is a debug feature simplify the error handling a bit by
consuming the crc entry even if copy_to_user() would fail.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index 95829ebecf21b8067dbbeb3333feac1f618fdead..252b0b2b514cad61335c5813bd570fe9b795f0bf 100644 (file)
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -2858,7 +2858,7 @@ i915_pipe_crc_read(struct file *filep, char __user *user_buf, size_t count,
        struct drm_i915_private *dev_priv = dev->dev_private;
        struct intel_pipe_crc *pipe_crc = &dev_priv->pipe_crc[info->pipe];
        char buf[PIPE_CRC_BUFFER_LEN];
-       int head, tail, n_entries, n;
+       int n_entries;
        ssize_t bytes_read;
 
        /*
@@ -2890,36 +2890,39 @@ i915_pipe_crc_read(struct file *filep, char __user *user_buf, size_t count,
        }
 
        /* We now have one or more entries to read */
-       head = pipe_crc->head;
-       tail = pipe_crc->tail;
-       n_entries = min((size_t)CIRC_CNT(head, tail, INTEL_PIPE_CRC_ENTRIES_NR),
-                       count / PIPE_CRC_LINE_LEN);
-       spin_unlock_irq(&pipe_crc->lock);
+       n_entries = count / PIPE_CRC_LINE_LEN;
 
        bytes_read = 0;
-       n = 0;
-       do {
-               struct intel_pipe_crc_entry *entry = &pipe_crc->entries[tail];
+       while (n_entries > 0) {
+               struct intel_pipe_crc_entry *entry =
+                       &pipe_crc->entries[pipe_crc->tail];
                int ret;
 
+               if (CIRC_CNT(pipe_crc->head, pipe_crc->tail,
+                            INTEL_PIPE_CRC_ENTRIES_NR) < 1)
+                       break;
+
+               BUILD_BUG_ON_NOT_POWER_OF_2(INTEL_PIPE_CRC_ENTRIES_NR);
+               pipe_crc->tail = (pipe_crc->tail + 1) & (INTEL_PIPE_CRC_ENTRIES_NR - 1);
+
                bytes_read += snprintf(buf, PIPE_CRC_BUFFER_LEN,
                                       "%8u %8x %8x %8x %8x %8x\n",
                                       entry->frame, entry->crc[0],
                                       entry->crc[1], entry->crc[2],
                                       entry->crc[3], entry->crc[4]);
 
-               ret = copy_to_user(user_buf + n * PIPE_CRC_LINE_LEN,
-                                  buf, PIPE_CRC_LINE_LEN);
+               spin_unlock_irq(&pipe_crc->lock);
+
+               ret = copy_to_user(user_buf, buf, PIPE_CRC_LINE_LEN);
                if (ret == PIPE_CRC_LINE_LEN)
                        return -EFAULT;
 
-               BUILD_BUG_ON_NOT_POWER_OF_2(INTEL_PIPE_CRC_ENTRIES_NR);
-               tail = (tail + 1) & (INTEL_PIPE_CRC_ENTRIES_NR - 1);
-               n++;
-       } while (--n_entries);
+               user_buf += PIPE_CRC_LINE_LEN;
+               n_entries--;
+
+               spin_lock_irq(&pipe_crc->lock);
+       }
 
-       spin_lock_irq(&pipe_crc->lock);
-       pipe_crc->tail = tail;
        spin_unlock_irq(&pipe_crc->lock);
 
        return bytes_read;
@@ -3458,6 +3461,8 @@ static int pipe_crc_set_source(struct drm_device *dev, enum pipe pipe,
                spin_lock_irq(&pipe_crc->lock);
                entries = pipe_crc->entries;
                pipe_crc->entries = NULL;
+               pipe_crc->head = 0;
+               pipe_crc->tail = 0;
                spin_unlock_irq(&pipe_crc->lock);
 
                kfree(entries);