dma-buf/fence: Avoid use of uninitialised timestamp

[  236.821534] WARNING: kmemcheck: Caught 64-bit read from uninitialized memory (ffff8802538683d0)
[  236.828642] 420000001e7f0000000000000000000000080000000000000000000000000000
[  236.839543]  i i i i u u u u i i i i i i i i u u u u u u u u u u u u u u u u
[  236.850420]                                  ^
[  236.854123] RIP: 0010:[<ffffffff81396f07>]  [<ffffffff81396f07>] fence_signal+0x17/0xd0
[  236.861313] RSP: 0018:ffff88024acd7ba0  EFLAGS: 00010282
[  236.865027] RAX: ffffffff812f6a90 RBX: ffff8802527ca800 RCX: ffff880252cb30e0
[  236.868801] RDX: ffff88024ac5d918 RSI: ffff880252f780e0 RDI: ffff880253868380
[  236.872579] RBP: ffff88024acd7bc0 R08: ffff88024acd7be0 R09: 0000000000000000
[  236.876407] R10: 0000000000000000 R11: 0000000000000000 R12: ffff880253868380
[  236.880185] R13: ffff8802538684d0 R14: ffff880253868380 R15: ffff88024cd48e00
[  236.883983] FS:  00007f1646d1a740(0000) GS:ffff88025d000000(0000) knlGS:0000000000000000
[  236.890959] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[  236.894702] CR2: ffff880251360318 CR3: 000000024ad21000 CR4: 00000000001406f0
[  236.898481]  [<ffffffff8130d1ad>] i915_gem_request_retire+0x1cd/0x230
[  236.902439]  [<ffffffff8130e2b3>] i915_gem_request_alloc+0xa3/0x2f0
[  236.906435]  [<ffffffff812fb1bd>] i915_gem_do_execbuffer.isra.41+0xb6d/0x18b0
[  236.910434]  [<ffffffff812fc265>] i915_gem_execbuffer2+0x95/0x1e0
[  236.914390]  [<ffffffff812ad625>] drm_ioctl+0x1e5/0x460
[  236.918275]  [<ffffffff8110d4cf>] do_vfs_ioctl+0x8f/0x5c0
[  236.922168]  [<ffffffff8110da3c>] SyS_ioctl+0x3c/0x70
[  236.926090]  [<ffffffff814b7a5f>] entry_SYSCALL_64_fastpath+0x17/0x93
[  236.930045]  [<ffffffffffffffff>] 0xffffffffffffffff

We only set the timestamp before we mark the fence as signaled. It is
done before to avoid observers having a window in which they may see the
fence as complete but no timestamp. Having it does incur a potential for
the timestamp to be written twice, and even for it to be corrupted if
the u64 write is not atomic. Instead use a new bit to record the
presence of the timestamp, and teach the readers to wait until it is set
if the fence is complete. There still remains a race where the timestamp
for the signaled fence may be shown before the fence is reported as
signaled, but that's a pre-existing error.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Sumit Semwal <sumit.semwal@linaro.org>
Cc: Gustavo Padovan <gustavo@padovan.org>
Cc: Daniel Vetter <daniel.vetter@intel.com>
Reported-by: Rafael Antognolli <rafael.antognolli@intel.com>
Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.com>
Link: http://patchwork.freedesktop.org/patch/msgid/20170214124001.1930-1-chris@chris-wilson.co.uk

diff --git a/drivers/dma-buf/dma-fence.c b/drivers/dma-buf/dma-fence.c
index 0918d3f003d65d633a6e06a2c8d41a47fc42f9b5..13556fdda2a52836b4ec38af48d54a48aa518bdd 100644 (file)
--- a/drivers/dma-buf/dma-fence.c
+++ b/drivers/dma-buf/dma-fence.c
@@ -75,11 +75,6 @@ int dma_fence_signal_locked(struct dma_fence *fence)
        if (WARN_ON(!fence))
                return -EINVAL;
 
-       if (!ktime_to_ns(fence->timestamp)) {
-               fence->timestamp = ktime_get();
-               smp_mb__before_atomic();
-       }
-
        if (test_and_set_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &fence->flags)) {
                ret = -EINVAL;
 
@@ -87,8 +82,11 @@ int dma_fence_signal_locked(struct dma_fence *fence)
                 * we might have raced with the unlocked dma_fence_signal,
                 * still run through all callbacks
                 */
-       } else
+       } else {
+               fence->timestamp = ktime_get();
+               set_bit(DMA_FENCE_FLAG_TIMESTAMP_BIT, &fence->flags);
                trace_dma_fence_signaled(fence);
+       }
 
        list_for_each_entry_safe(cur, tmp, &fence->cb_list, node) {
                list_del_init(&cur->node);
@@ -115,14 +113,11 @@ int dma_fence_signal(struct dma_fence *fence)
        if (!fence)
                return -EINVAL;
 
-       if (!ktime_to_ns(fence->timestamp)) {
-               fence->timestamp = ktime_get();
-               smp_mb__before_atomic();
-       }
-
        if (test_and_set_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &fence->flags))
                return -EINVAL;
 
+       fence->timestamp = ktime_get();
+       set_bit(DMA_FENCE_FLAG_TIMESTAMP_BIT, &fence->flags);
        trace_dma_fence_signaled(fence);
 
        if (test_bit(DMA_FENCE_FLAG_ENABLE_SIGNAL_BIT, &fence->flags)) {

diff --git a/drivers/dma-buf/sync_debug.c b/drivers/dma-buf/sync_debug.c
index c769dc653b344ac23fe5a99a36b99a31dc2fceba..bfead12390f245954e42d28d7bfd9efb3d47ab6e 100644 (file)
--- a/drivers/dma-buf/sync_debug.c
+++ b/drivers/dma-buf/sync_debug.c
@@ -84,7 +84,7 @@ static void sync_print_fence(struct seq_file *s,
                   show ? "_" : "",
                   sync_status_str(status));
 
-       if (status) {
+       if (test_bit(DMA_FENCE_FLAG_TIMESTAMP_BIT, &fence->flags)) {
                struct timespec64 ts64 =
                        ktime_to_timespec64(fence->timestamp);
 

diff --git a/drivers/dma-buf/sync_file.c b/drivers/dma-buf/sync_file.c
index 2321035f6204bc4e0f5c84980becb22bc534e328..95f259b719fc4b6831dc3249affc35510c6bb30b 100644 (file)
--- a/drivers/dma-buf/sync_file.c
+++ b/drivers/dma-buf/sync_file.c
@@ -375,7 +375,13 @@ static void sync_fill_fence_info(struct dma_fence *fence,
                sizeof(info->driver_name));
 
        info->status = dma_fence_get_status(fence);
-       info->timestamp_ns = ktime_to_ns(fence->timestamp);
+       while (test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &fence->flags) &&
+              !test_bit(DMA_FENCE_FLAG_TIMESTAMP_BIT, &fence->flags))
+               cpu_relax();
+       info->timestamp_ns =
+               test_bit(DMA_FENCE_FLAG_TIMESTAMP_BIT, &fence->flags) ?
+               ktime_to_ns(fence->timestamp) :
+               ktime_set(0, 0);
 }
 
 static long sync_file_ioctl_fence_info(struct sync_file *sync_file,

diff --git a/include/linux/dma-fence.h b/include/linux/dma-fence.h
index a5195a7d6f77e40d23d29ba2793c7b393f4eb0bb..0a186c4f3981c8738dec34ed558a10f47f9fc026 100644 (file)
--- a/include/linux/dma-fence.h
+++ b/include/linux/dma-fence.h
@@ -55,6 +55,7 @@ struct dma_fence_cb;
  * of the time.
  *
  * DMA_FENCE_FLAG_SIGNALED_BIT - fence is already signaled
+ * DMA_FENCE_FLAG_TIMESTAMP_BIT - timestamp recorded for fence signaling
  * DMA_FENCE_FLAG_ENABLE_SIGNAL_BIT - enable_signaling might have been called
  * DMA_FENCE_FLAG_USER_BITS - start of the unused bits, can be used by the
  * implementer of the fence for its own purposes. Can be used in different
@@ -84,6 +85,7 @@ struct dma_fence {
 
 enum dma_fence_flag_bits {
        DMA_FENCE_FLAG_SIGNALED_BIT,
+       DMA_FENCE_FLAG_TIMESTAMP_BIT,
        DMA_FENCE_FLAG_ENABLE_SIGNAL_BIT,
        DMA_FENCE_FLAG_USER_BITS, /* must always be last member */
 };