[PATCH net-next 00/12] ptr_ring fixes

This fixes a bunch of issues around ptr_ring use in net core.
One of these: "tap: fix use-after-free" is also needed on net,
but can't be backported cleanly.

I will post a net patch separately.

Lightly tested - Jason, could you pls confirm this
addresses the security issue you saw with ptr_ring?
Testing reports would be appreciated too.

Michael S. Tsirkin (12):
  ptr_ring: keep consumer_head valid at all times
  ptr_ring: clean up documentation
  ptr_ring: READ/WRITE_ONCE for __ptr_ring_empty
  tap: fix use-after-free
  ptr_ring: disallow lockless __ptr_ring_full
  Revert "net: ptr_ring: otherwise safe empty checks can overrun array
    bounds"
  skb_array: use __ptr_ring_empty
  ptr_ring: prevent queue load/store tearing
  tools/virtio: switch to __ptr_ring_empty
  tools/virtio: more stubs to fix tools build
  tools/virtio: copy READ/WRITE_ONCE
  tools/virtio: fix smp_mb on x86

 drivers/net/tap.c                |  3 --
 include/linux/ptr_ring.h         | 86 ++++++++++++++++++++++------------------
 include/linux/skb_array.h        |  2 +-
 tools/virtio/linux/kernel.h      |  2 +-
 tools/virtio/linux/thread_info.h |  1 +
 tools/virtio/ringtest/main.h     | 59 ++++++++++++++++++++++++++-
 tools/virtio/ringtest/ptr_ring.c |  2 +-
 7 files changed, 110 insertions(+), 45 deletions(-)
 create mode 100644 tools/virtio/linux/thread_info.h

-- 
MST

The comment near __ptr_ring_peek says:

 * If ring is never resized, and if the pointer is merely
 * tested, there's no need to take the lock - see e.g.  __ptr_ring_empty.

but this was in fact never possible since consumer_head would sometimes
point outside the ring. Refactor the code so that it's always
pointing within a ring.

Fixes: c5ad119fb6c09 ("net: sched: pfifo_fast use skb_array")
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 include/linux/ptr_ring.h | 25 ++++++++++++++++---------
 1 file changed, 16 insertions(+), 9 deletions(-)

diff --git a/include/linux/ptr_ring.h b/include/linux/ptr_ring.h
index 9ca1726..5ebcdd4 100644
--- a/include/linux/ptr_ring.h
+++ b/include/linux/ptr_ring.h

@@ -248,22 +248,28 @@ static inline void __ptr_ring_discard_one(struct ptr_ring *r)
 	/* Fundamentally, what we want to do is update consumer
 	 * index and zero out the entry so producer can reuse it.
 	 * Doing it naively at each consume would be as simple as:
-	 *       r->queue[r->consumer++] = NULL;
-	 *       if (unlikely(r->consumer >= r->size))
-	 *               r->consumer = 0;
+	 *       consumer = r->consumer;
+	 *       r->queue[consumer++] = NULL;
+	 *       if (unlikely(consumer >= r->size))
+	 *               consumer = 0;
+	 *       r->consumer = consumer;
 	 * but that is suboptimal when the ring is full as producer is writing
 	 * out new entries in the same cache line.  Defer these updates until a
 	 * batch of entries has been consumed.
 	 */
-	int head = r->consumer_head++;
+	/* Note: we must keep consumer_head valid at all times for __ptr_ring_empty
+	 * to work correctly.
+	 */
+	int consumer_head = r->consumer_head;
+	int head = consumer_head++;
 
 	/* Once we have processed enough entries invalidate them in
 	 * the ring all at once so producer can reuse their space in the ring.
 	 * We also do this when we reach end of the ring - not mandatory
 	 * but helps keep the implementation simple.
 	 */
-	if (unlikely(r->consumer_head - r->consumer_tail >= r->batch ||
-		     r->consumer_head >= r->size)) {
+	if (unlikely(consumer_head - r->consumer_tail >= r->batch ||
+		     consumer_head >= r->size)) {
 		/* Zero out entries in the reverse order: this way we touch the
 		 * cache line that producer might currently be reading the last;
 		 * producer won't make progress and touch other cache lines

@@ -271,12 +277,13 @@ static inline void __ptr_ring_discard_one(struct ptr_ring *r)
 		 */
 		while (likely(head >= r->consumer_tail))
 			r->queue[head--] = NULL;
-		r->consumer_tail = r->consumer_head;
+		r->consumer_tail = consumer_head;
 	}
-	if (unlikely(r->consumer_head >= r->size)) {
-		r->consumer_head = 0;
+	if (unlikely(consumer_head >= r->size)) {
+		consumer_head = 0;
 		r->consumer_tail = 0;
 	}
+	r->consumer_head = consumer_head;
 }
 
 static inline void *__ptr_ring_consume(struct ptr_ring *r)

-- 
MST

Lockless access to __ptr_ring_full is only legal if ring is
never resized, otherwise it might cause use-after free errors.
Simply drop the lockless test, we'll drop the packet
a bit later when produce fails.

Fixes: 362899b8 ("macvtap: switch to use skb array")
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 drivers/net/tap.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/drivers/net/tap.c b/drivers/net/tap.c
index 7c38659..7787269 100644
--- a/drivers/net/tap.c
+++ b/drivers/net/tap.c

@@ -330,9 +330,6 @@ rx_handler_result_t tap_handle_frame(struct sk_buff **pskb)
 	if (!q)
 		return RX_HANDLER_PASS;
 
-	if (__ptr_ring_full(&q->ring))
-		goto drop;
-
 	skb_push(skb, ETH_HLEN);
 
 	/* Apply the forward feature mask so that we perform segmentation

-- 
MST

This reverts commit bcecb4bbf88aa03171c30652bca761cf27755a6b.

If we try to allocate an extra entry as the above commit did, and when
the requested size is UINT_MAX, addition overflows causing zero size to
be passed to kmalloc().

kmalloc then returns ZERO_SIZE_PTR with a subsequent crash.

Reported-by: syzbot+87678bcf753b44c39b67@syzkaller.appspotmail.com
Cc: John Fastabend <john.fastabend@gmail.com>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 include/linux/ptr_ring.h | 7 +------
 1 file changed, 1 insertion(+), 6 deletions(-)

diff --git a/include/linux/ptr_ring.h b/include/linux/ptr_ring.h
index f175846..3a19ebd 100644
--- a/include/linux/ptr_ring.h
+++ b/include/linux/ptr_ring.h

@@ -466,12 +466,7 @@ static inline int ptr_ring_consume_batched_bh(struct ptr_ring *r,
 
 static inline void **__ptr_ring_init_queue_alloc(unsigned int size, gfp_t gfp)
 {
-	/* Allocate an extra dummy element at end of ring to avoid consumer head
-	 * or produce head access past the end of the array. Possible when
-	 * producer/consumer operations and __ptr_ring_peek operations run in
-	 * parallel.
-	 */
-	return kcalloc(size + 1, sizeof(void *), gfp);
+	return kcalloc(size, sizeof(void *), gfp);
 }
 
 static inline void __ptr_ring_set_size(struct ptr_ring *r, int size)

-- 
MST

We don't rely on lockless guarantees, but it
seems cleaner than inverting __ptr_ring_peek.

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 tools/virtio/ringtest/ptr_ring.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/virtio/ringtest/ptr_ring.c b/tools/virtio/ringtest/ptr_ring.c
index e6e8130..477899c 100644
--- a/tools/virtio/ringtest/ptr_ring.c
+++ b/tools/virtio/ringtest/ptr_ring.c

@@ -187,7 +187,7 @@ bool enable_kick()
 
 bool avail_empty()
 {
-	return !__ptr_ring_peek(&array);
+	return __ptr_ring_empty(&array);
 }
 
 bool use_buf(unsigned *lenp, void **bufp)

-- 
MST

This is to make ptr_ring test build again.

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 tools/virtio/ringtest/main.h | 57 ++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 57 insertions(+)

diff --git a/tools/virtio/ringtest/main.h b/tools/virtio/ringtest/main.h
index 5706e07..593a328 100644
--- a/tools/virtio/ringtest/main.h
+++ b/tools/virtio/ringtest/main.h

@@ -134,4 +134,61 @@ static inline void busy_wait(void)
     barrier(); \
 } while (0)
 
+#if defined(__i386__) || defined(__x86_64__) || defined(__s390x__)
+#define smp_wmb() barrier()
+#else
+#define smp_wmb() smp_release()
+#endif
+
+#ifdef __alpha__
+#define smp_read_barrier_depends() smp_acquire()
+#else
+#define smp_read_barrier_depends() do {} while(0)
+#endif
+
+static __always_inline
+void __read_once_size(const volatile void *p, void *res, int size)
+{
+        switch (size) {                                                 \
+        case 1: *(unsigned char *)res = *(volatile unsigned char *)p; break;              \
+        case 2: *(unsigned short *)res = *(volatile unsigned short *)p; break;            \
+        case 4: *(unsigned int *)res = *(volatile unsigned int *)p; break;            \
+        case 8: *(unsigned long long *)res = *(volatile unsigned long long *)p; break;            \
+        default:                                                        \
+                barrier();                                              \
+                __builtin_memcpy((void *)res, (const void *)p, size);   \
+                barrier();                                              \
+        }                                                               \
+}
+
+static __always_inline void __write_once_size(volatile void *p, void *res, int size)
+{
+	switch (size) {
+	case 1: *(volatile unsigned char *)p = *(unsigned char *)res; break;
+	case 2: *(volatile unsigned short *)p = *(unsigned short *)res; break;
+	case 4: *(volatile unsigned int *)p = *(unsigned int *)res; break;
+	case 8: *(volatile unsigned long long *)p = *(unsigned long long *)res; break;
+	default:
+		barrier();
+		__builtin_memcpy((void *)p, (const void *)res, size);
+		barrier();
+	}
+}
+
+#define READ_ONCE(x) \
+({									\
+	union { typeof(x) __val; char __c[1]; } __u;			\
+	__read_once_size(&(x), __u.__c, sizeof(x));		\
+	smp_read_barrier_depends(); /* Enforce dependency ordering from x */ \
+	__u.__val;							\
+})
+
+#define WRITE_ONCE(x, val) \
+({							\
+	union { typeof(x) __val; char __c[1]; } __u =	\
+		{ .__val = (typeof(x)) (val) }; \
+	__write_once_size(&(x), __u.__c, sizeof(x));	\
+	__u.__val;					\
+})
+
 #endif

-- 
MST

Offset 128 overlaps the last word of the redzone.
Use 132 which is always beyond that.

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 tools/virtio/ringtest/main.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/virtio/ringtest/main.h b/tools/virtio/ringtest/main.h
index 593a328..301d59b 100644
--- a/tools/virtio/ringtest/main.h
+++ b/tools/virtio/ringtest/main.h

@@ -111,7 +111,7 @@ static inline void busy_wait(void)
 } 
 
 #if defined(__x86_64__) || defined(__i386__)
-#define smp_mb()     asm volatile("lock; addl $0,-128(%%rsp)" ::: "memory", "cc")
+#define smp_mb()     asm volatile("lock; addl $0,-132(%%rsp)" ::: "memory", "cc")
 #else
 /*
  * Not using __ATOMIC_SEQ_CST since gcc docs say they are only synchronized

-- 
MST

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 tools/virtio/linux/kernel.h      | 2 +-
 tools/virtio/linux/thread_info.h | 1 +
 2 files changed, 2 insertions(+), 1 deletion(-)
 create mode 100644 tools/virtio/linux/thread_info.h

diff --git a/tools/virtio/linux/kernel.h b/tools/virtio/linux/kernel.h
index 395521a..fca8381 100644
--- a/tools/virtio/linux/kernel.h
+++ b/tools/virtio/linux/kernel.h

@@ -118,7 +118,7 @@ static inline void free_page(unsigned long addr)
 #define dev_err(dev, format, ...) fprintf (stderr, format, ## __VA_ARGS__)
 #define dev_warn(dev, format, ...) fprintf (stderr, format, ## __VA_ARGS__)
 
-#define WARN_ON_ONCE(cond) ((cond) && fprintf (stderr, "WARNING\n"))
+#define WARN_ON_ONCE(cond) ((cond) ? fprintf (stderr, "WARNING\n") : 0)
 
 #define min(x, y) ({				\
 	typeof(x) _min1 = (x);			\

diff --git a/tools/virtio/linux/thread_info.h b/tools/virtio/linux/thread_info.h
new file mode 100644
index 0000000..e0f610d
--- /dev/null
+++ b/tools/virtio/linux/thread_info.h

@@ -0,0 +1 @@
+#define check_copy_size(A, B, C) (1)

-- 
MST

In theory compiler could tear queue loads or stores in two. It does not
seem to be happening in practice but it seems easier to convert the
cases where this would be a problem to READ/WRITE_ONCE than worry about
it.

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 include/linux/ptr_ring.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/include/linux/ptr_ring.h b/include/linux/ptr_ring.h
index 3a19ebd..1883d61 100644
--- a/include/linux/ptr_ring.h
+++ b/include/linux/ptr_ring.h

@@ -114,7 +114,7 @@ static inline int __ptr_ring_produce(struct ptr_ring *r, void *ptr)
 	/* Pairs with smp_read_barrier_depends in __ptr_ring_consume. */
 	smp_wmb();
 
-	r->queue[r->producer++] = ptr;
+	WRITE_ONCE(r->queue[r->producer++], ptr);
 	if (unlikely(r->producer >= r->size))
 		r->producer = 0;
 	return 0;

@@ -173,7 +173,7 @@ static inline int ptr_ring_produce_bh(struct ptr_ring *r, void *ptr)
 static inline void *__ptr_ring_peek(struct ptr_ring *r)
 {
 	if (likely(r->size))
-		return r->queue[r->consumer_head];
+		return READ_ONCE(r->queue[r->consumer_head]);
 	return NULL;
 }
 

-- 
MST

__skb_array_empty should use __ptr_ring_empty since that's the only
legal lockless function.

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 include/linux/skb_array.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/linux/skb_array.h b/include/linux/skb_array.h
index c7addf3..a6b6e8b 100644
--- a/include/linux/skb_array.h
+++ b/include/linux/skb_array.h

@@ -69,7 +69,7 @@ static inline int skb_array_produce_any(struct skb_array *a, struct sk_buff *skb
  */
 static inline bool __skb_array_empty(struct skb_array *a)
 {
-	return !__ptr_ring_peek(&a->ring);
+	return __ptr_ring_empty(&a->ring);
 }
 
 static inline struct sk_buff *__skb_array_peek(struct skb_array *a)

-- 
MST

Similar to bcecb4bbf88a ("net: ptr_ring: otherwise safe empty checks can
overrun array bounds") a lockless use of __ptr_ring_full might
cause an out of bounds access.

We can fix this, but it's easier to just disallow lockless
__ptr_ring_full for now.

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 include/linux/ptr_ring.h | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/include/linux/ptr_ring.h b/include/linux/ptr_ring.h
index 9a72d8f..f175846 100644
--- a/include/linux/ptr_ring.h
+++ b/include/linux/ptr_ring.h

@@ -45,9 +45,10 @@ struct ptr_ring {
 };
 
 /* Note: callers invoking this in a loop must use a compiler barrier,
- * for example cpu_relax().  If ring is ever resized, callers must hold
- * producer_lock - see e.g. ptr_ring_full.  Otherwise, if callers don't hold
- * producer_lock, the next call to __ptr_ring_produce may fail.
+ * for example cpu_relax().
+ *
+ * NB: this is unlike __ptr_ring_empty in that callers must hold producer_lock:
+ * see e.g. ptr_ring_full.
  */
 static inline bool __ptr_ring_full(struct ptr_ring *r)
 {

-- 
MST

Lockless __ptr_ring_empty requires that consumer head is read and
written at once, atomically. Annotate accordingly to make sure compiler
does it correctly.  Switch locked callers to __ptr_ring_peek which does
not support the lockless operation.

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 include/linux/ptr_ring.h | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/include/linux/ptr_ring.h b/include/linux/ptr_ring.h
index 8594c7b..9a72d8f 100644
--- a/include/linux/ptr_ring.h
+++ b/include/linux/ptr_ring.h

@@ -196,7 +196,9 @@ static inline void *__ptr_ring_peek(struct ptr_ring *r)
  */
 static inline bool __ptr_ring_empty(struct ptr_ring *r)
 {
-	return !__ptr_ring_peek(r);
+	if (likely(r->size))
+		return !r->queue[READ_ONCE(r->consumer_head)];
+	return true;
 }
 
 static inline bool ptr_ring_empty(struct ptr_ring *r)

@@ -285,7 +287,8 @@ static inline void __ptr_ring_discard_one(struct ptr_ring *r)
 		consumer_head = 0;
 		r->consumer_tail = 0;
 	}
-	r->consumer_head = consumer_head;
+	/* matching READ_ONCE in __ptr_ring_empty for lockless tests */
+	WRITE_ONCE(r->consumer_head, consumer_head);
 }
 
 static inline void *__ptr_ring_consume(struct ptr_ring *r)

@@ -541,7 +544,9 @@ static inline void ptr_ring_unconsume(struct ptr_ring *r, void **batch, int n,
 			goto done;
 		}
 		r->queue[head] = batch[--n];
-		r->consumer_tail = r->consumer_head = head;
+		r->consumer_tail = head;
+		/* matching READ_ONCE in __ptr_ring_empty for lockless tests */
+		WRITE_ONCE(r->consumer_head, head);
 	}
 
 done:

-- 
MST

The only function safe to call without locks
is __ptr_ring_empty. Move documentation about
lockless use there to make sure people do not
try to use __ptr_ring_peek outside locks.

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 include/linux/ptr_ring.h | 34 ++++++++++++++++++----------------
 1 file changed, 18 insertions(+), 16 deletions(-)

diff --git a/include/linux/ptr_ring.h b/include/linux/ptr_ring.h
index 5ebcdd4..8594c7b 100644
--- a/include/linux/ptr_ring.h
+++ b/include/linux/ptr_ring.h

@@ -169,21 +169,6 @@ static inline int ptr_ring_produce_bh(struct ptr_ring *r, void *ptr)
 	return ret;
 }
 
-/* Note: callers invoking this in a loop must use a compiler barrier,
- * for example cpu_relax(). Callers must take consumer_lock
- * if they dereference the pointer - see e.g. PTR_RING_PEEK_CALL.
- * If ring is never resized, and if the pointer is merely
- * tested, there's no need to take the lock - see e.g.  __ptr_ring_empty.
- * However, if called outside the lock, and if some other CPU
- * consumes ring entries at the same time, the value returned
- * is not guaranteed to be correct.
- * In this case - to avoid incorrectly detecting the ring
- * as empty - the CPU consuming the ring entries is responsible
- * for either consuming all ring entries until the ring is empty,
- * or synchronizing with some other CPU and causing it to
- * execute __ptr_ring_peek and/or consume the ring enteries
- * after the synchronization point.
- */
 static inline void *__ptr_ring_peek(struct ptr_ring *r)
 {
 	if (likely(r->size))

@@ -191,7 +176,24 @@ static inline void *__ptr_ring_peek(struct ptr_ring *r)
 	return NULL;
 }
 
-/* See __ptr_ring_peek above for locking rules. */
+/*
+ * Test ring empty status without taking any locks.
+ *
+ * NB: This is only safe to call if ring is never resized.
+ *
+ * However, if some other CPU consumes ring entries at the same time, the value
+ * returned is not guaranteed to be correct.
+ *
+ * In this case - to avoid incorrectly detecting the ring
+ * as empty - the CPU consuming the ring entries is responsible
+ * for either consuming all ring entries until the ring is empty,
+ * or synchronizing with some other CPU and causing it to
+ * re-test __ptr_ring_empty and/or consume the ring enteries
+ * after the synchronization point.
+ *
+ * Note: callers invoking this in a loop must use a compiler barrier,
+ * for example cpu_relax().
+ */
 static inline bool __ptr_ring_empty(struct ptr_ring *r)
 {
 	return !__ptr_ring_peek(r);

-- 
MST

On 01/25/2018 03:36 PM, Michael S. Tsirkin wrote:

The comment near __ptr_ring_peek says:

 * If ring is never resized, and if the pointer is merely
 * tested, there's no need to take the lock - see e.g.  __ptr_ring_empty.

but this was in fact never possible since consumer_head would sometimes
point outside the ring. Refactor the code so that it's always
pointing within a ring.

Fixes: c5ad119fb6c09 ("net: sched: pfifo_fast use skb_array")
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 include/linux/ptr_ring.h | 25 ++++++++++++++++---------
 1 file changed, 16 insertions(+), 9 deletions(-)

Thanks for fixing this up.

Acked-by: John Fastabend <john.fastabend@gmail.com>

On 01/25/2018 03:36 PM, Michael S. Tsirkin wrote:

This reverts commit bcecb4bbf88aa03171c30652bca761cf27755a6b.

If we try to allocate an extra entry as the above commit did, and when
the requested size is UINT_MAX, addition overflows causing zero size to
be passed to kmalloc().

kmalloc then returns ZERO_SIZE_PTR with a subsequent crash.

Reported-by: syzbot+87678bcf753b44c39b67@syzkaller.appspotmail.com
Cc: John Fastabend <john.fastabend@gmail.com>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---

Dang, I missed this case. Thanks.

Acked-by: John Fastabend <john.fastabend@gmail.com>

On 2018年01月26日 07:36, Michael S. Tsirkin wrote:

So after patch 8, __ptr_ring_peek() did:

static inline void *__ptr_ring_peek(struct ptr_ring *r)
{
     if (likely(r->size))
         return READ_ONCE(r->queue[r->consumer_head]);
     return NULL;
}

Looks like a duplication.

Thanks

On 2018年01月26日 07:36, Michael S. Tsirkin wrote:

Similar to bcecb4bbf88a ("net: ptr_ring: otherwise safe empty checks can
overrun array bounds") a lockless use of __ptr_ring_full might
cause an out of bounds access.

We can fix this, but it's easier to just disallow lockless
__ptr_ring_full for now.

It looks to me that just fix this is better than disallow through doc 
(which is easily to be ignored ...).

Thanks

On 2018年01月26日 07:36, Michael S. Tsirkin wrote:

You may want WRITE_ONCE() here? And if we just fix the out of bound 
r->producer, we may just need one WRITE_ONCE().

Thanks

On Fri, Jan 26, 2018 at 10:37:58AM +0800, Jason Wang wrote:

On 2018年01月26日 07:36, Michael S. Tsirkin wrote:

quoted

Lockless __ptr_ring_empty requires that consumer head is read and
written at once, atomically. Annotate accordingly to make sure compiler
does it correctly.  Switch locked callers to __ptr_ring_peek which does
not support the lockless operation.

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
  include/linux/ptr_ring.h | 11 ++++++++---
  1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/include/linux/ptr_ring.h b/include/linux/ptr_ring.h
index 8594c7b..9a72d8f 100644
--- a/include/linux/ptr_ring.h
+++ b/include/linux/ptr_ring.h

@@ -196,7 +196,9 @@ static inline void *__ptr_ring_peek(struct ptr_ring *r)
   */
  static inline bool __ptr_ring_empty(struct ptr_ring *r)
  {
-	return !__ptr_ring_peek(r);
+	if (likely(r->size))
+		return !r->queue[READ_ONCE(r->consumer_head)];
+	return true;
  }

So after patch 8, __ptr_ring_peek() did:

static inline void *__ptr_ring_peek(struct ptr_ring *r)
{
    if (likely(r->size))
        return READ_ONCE(r->queue[r->consumer_head]);
    return NULL;
}

Looks like a duplication.

Thanks

Nope - they are different.

The reason is that __ptr_ring_peek does not need to read the consumer_head once
since callers have a lock, and __ptr_ring_empty does not need to read
the queue once since it merely compares it to 0.

-- 
MST

On Fri, Jan 26, 2018 at 10:38:05AM +0800, Jason Wang wrote:

On 2018年01月26日 07:36, Michael S. Tsirkin wrote:

quoted

Similar to bcecb4bbf88a ("net: ptr_ring: otherwise safe empty checks can
overrun array bounds") a lockless use of __ptr_ring_full might
cause an out of bounds access.

We can fix this, but it's easier to just disallow lockless
__ptr_ring_full for now.

It looks to me that just fix this is better than disallow through doc (which
is easily to be ignored ...).

Thanks

lockless is tricky, and I'd rather not sprinkle READ/WRITE_ONCE where
they aren't necessary.

quoted

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
  include/linux/ptr_ring.h | 7 ++++---
  1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/include/linux/ptr_ring.h b/include/linux/ptr_ring.h
index 9a72d8f..f175846 100644
--- a/include/linux/ptr_ring.h
+++ b/include/linux/ptr_ring.h

@@ -45,9 +45,10 @@ struct ptr_ring {
  };
  /* Note: callers invoking this in a loop must use a compiler barrier,
- * for example cpu_relax().  If ring is ever resized, callers must hold
- * producer_lock - see e.g. ptr_ring_full.  Otherwise, if callers don't hold
- * producer_lock, the next call to __ptr_ring_produce may fail.
+ * for example cpu_relax().
+ *
+ * NB: this is unlike __ptr_ring_empty in that callers must hold producer_lock:
+ * see e.g. ptr_ring_full.
   */
  static inline bool __ptr_ring_full(struct ptr_ring *r)
  {

On Fri, Jan 26, 2018 at 10:38:12AM +0800, Jason Wang wrote:

On 2018年01月26日 07:36, Michael S. Tsirkin wrote:

quoted

In theory compiler could tear queue loads or stores in two. It does not
seem to be happening in practice but it seems easier to convert the
cases where this would be a problem to READ/WRITE_ONCE than worry about
it.

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
  include/linux/ptr_ring.h | 4 ++--
  1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/include/linux/ptr_ring.h b/include/linux/ptr_ring.h
index 3a19ebd..1883d61 100644
--- a/include/linux/ptr_ring.h
+++ b/include/linux/ptr_ring.h

@@ -114,7 +114,7 @@ static inline int __ptr_ring_produce(struct ptr_ring *r, void *ptr)
  	/* Pairs with smp_read_barrier_depends in __ptr_ring_consume. */
  	smp_wmb();
-	r->queue[r->producer++] = ptr;
+	WRITE_ONCE(r->queue[r->producer++], ptr);
  	if (unlikely(r->producer >= r->size))
  		r->producer = 0;

You may want WRITE_ONCE() here? And if we just fix the out of bound
r->producer, we may just need one WRITE_ONCE().

Thanks

No because producers are serialized.

If we were going to sprinkle write/read once all over the place
we should just make it all volatile and drop the annotations.

I don't care much either way but for better or worse linux has volatile
considered harmful doc which says that you are supposed to think and
only add these things were they are necessary.

quoted

  	return 0;

@@ -173,7 +173,7 @@ static inline int ptr_ring_produce_bh(struct ptr_ring *r, void *ptr)
  static inline void *__ptr_ring_peek(struct ptr_ring *r)
  {
  	if (likely(r->size))
-		return r->queue[r->consumer_head];
+		return READ_ONCE(r->queue[r->consumer_head]);
  	return NULL;
  }

On 2018年01月26日 10:44, Michael S. Tsirkin wrote:

On Fri, Jan 26, 2018 at 10:37:58AM +0800, Jason Wang wrote:

quoted

On 2018年01月26日 07:36, Michael S. Tsirkin wrote:

quoted

Lockless __ptr_ring_empty requires that consumer head is read and
written at once, atomically. Annotate accordingly to make sure compiler
does it correctly.  Switch locked callers to __ptr_ring_peek which does
not support the lockless operation.

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
   include/linux/ptr_ring.h | 11 ++++++++---
   1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/include/linux/ptr_ring.h b/include/linux/ptr_ring.h
index 8594c7b..9a72d8f 100644
--- a/include/linux/ptr_ring.h
+++ b/include/linux/ptr_ring.h

@@ -196,7 +196,9 @@ static inline void *__ptr_ring_peek(struct ptr_ring *r)
    */
   static inline bool __ptr_ring_empty(struct ptr_ring *r)
   {
-	return !__ptr_ring_peek(r);
+	if (likely(r->size))
+		return !r->queue[READ_ONCE(r->consumer_head)];
+	return true;
   }

So after patch 8, __ptr_ring_peek() did:

static inline void *__ptr_ring_peek(struct ptr_ring *r)
{
     if (likely(r->size))
         return READ_ONCE(r->queue[r->consumer_head]);
     return NULL;
}

Looks like a duplication.

Thanks

Nope - they are different.

The reason is that __ptr_ring_peek does not need to read the consumer_head once
since callers have a lock,

I get this.

  and __ptr_ring_empty does not need to read
the queue once since it merely compares it to 0.

Do this still work if it was called inside a loop?

Thanks

On 2018年01月26日 07:36, Michael S. Tsirkin wrote:

This fixes a bunch of issues around ptr_ring use in net core.
One of these: "tap: fix use-after-free" is also needed on net,
but can't be backported cleanly.

I will post a net patch separately.

Lightly tested - Jason, could you pls confirm this
addresses the security issue you saw with ptr_ring?
Testing reports would be appreciated too.

With the reproducer provided by syzbot, the problem has been fixed.

Thanks

On 2018年01月26日 07:36, Michael S. Tsirkin wrote:

Just wonder did "rsp" work for __i386__ ?

Thanks

+#define smp_mb()     asm volatile("lock; addl $0,-132(%%rsp)" ::: "memory", "cc")
  #else
  /*
   * Not using __ATOMIC_SEQ_CST since gcc docs say they are only synchronized

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

On Fri, Jan 26, 2018 at 11:19:58AM +0800, Jason Wang wrote:

On 2018年01月26日 10:44, Michael S. Tsirkin wrote:

quoted

On Fri, Jan 26, 2018 at 10:37:58AM +0800, Jason Wang wrote:

quoted

On 2018年01月26日 07:36, Michael S. Tsirkin wrote:

quoted

Lockless __ptr_ring_empty requires that consumer head is read and
written at once, atomically. Annotate accordingly to make sure compiler
does it correctly.  Switch locked callers to __ptr_ring_peek which does
not support the lockless operation.

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
   include/linux/ptr_ring.h | 11 ++++++++---
   1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/include/linux/ptr_ring.h b/include/linux/ptr_ring.h
index 8594c7b..9a72d8f 100644
--- a/include/linux/ptr_ring.h
+++ b/include/linux/ptr_ring.h

@@ -196,7 +196,9 @@ static inline void *__ptr_ring_peek(struct ptr_ring *r)
    */
   static inline bool __ptr_ring_empty(struct ptr_ring *r)
   {
-	return !__ptr_ring_peek(r);
+	if (likely(r->size))
+		return !r->queue[READ_ONCE(r->consumer_head)];
+	return true;
   }

So after patch 8, __ptr_ring_peek() did:

static inline void *__ptr_ring_peek(struct ptr_ring *r)
{
     if (likely(r->size))
         return READ_ONCE(r->queue[r->consumer_head]);
     return NULL;
}

Looks like a duplication.

Thanks

Nope - they are different.

The reason is that __ptr_ring_peek does not need to read the consumer_head once
since callers have a lock,

I get this.

quoted

  and __ptr_ring_empty does not need to read
the queue once since it merely compares it to 0.

Do this still work if it was called inside a loop?

Thanks

Sure because compiler does not know head didn't change.

-- 
MST

On Fri, Jan 26, 2018 at 11:56:14AM +0800, Jason Wang wrote:

On 2018年01月26日 07:36, Michael S. Tsirkin wrote:

quoted

Offset 128 overlaps the last word of the redzone.
Use 132 which is always beyond that.

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
  tools/virtio/ringtest/main.h | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/virtio/ringtest/main.h b/tools/virtio/ringtest/main.h
index 593a328..301d59b 100644
--- a/tools/virtio/ringtest/main.h
+++ b/tools/virtio/ringtest/main.h

@@ -111,7 +111,7 @@ static inline void busy_wait(void)
  }
  #if defined(__x86_64__) || defined(__i386__)
-#define smp_mb()     asm volatile("lock; addl $0,-128(%%rsp)" ::: "memory", "cc")

Just wonder did "rsp" work for __i386__ ?

Thanks

Oh you are right of course. Probably no one ever run this one on i386 :)
I'll add a patch on top as this is not a new bug.

quoted

+#define smp_mb()     asm volatile("lock; addl $0,-132(%%rsp)" ::: "memory", "cc")
  #else
  /*
   * Not using __ATOMIC_SEQ_CST since gcc docs say they are only synchronized

On 2018年01月26日 10:46, Michael S. Tsirkin wrote:

quoted

On 2018年01月26日 07:36, Michael S. Tsirkin wrote:

quoted

Similar to bcecb4bbf88a ("net: ptr_ring: otherwise safe empty checks can
overrun array bounds") a lockless use of __ptr_ring_full might
cause an out of bounds access.

We can fix this, but it's easier to just disallow lockless
__ptr_ring_full for now.

It looks to me that just fix this is better than disallow through doc (which
is easily to be ignored ...).

Thanks

lockless is tricky, and I'd rather not sprinkle READ/WRITE_ONCE where
they aren't necessary.

The problem is then API looks a little bit strange. Lockless were only 
allowed to be done at __ptr_ring_empty() but not __ptr_ring_full().

Thanks

On Mon, Jan 29, 2018 at 11:36:09AM +0800, Jason Wang wrote:

On 2018年01月26日 10:46, Michael S. Tsirkin wrote:

quoted

quoted

On 2018年01月26日 07:36, Michael S. Tsirkin wrote:

quoted

Similar to bcecb4bbf88a ("net: ptr_ring: otherwise safe empty checks can
overrun array bounds") a lockless use of __ptr_ring_full might
cause an out of bounds access.

We can fix this, but it's easier to just disallow lockless
__ptr_ring_full for now.

It looks to me that just fix this is better than disallow through doc (which
is easily to be ignored ...).

Thanks

lockless is tricky, and I'd rather not sprinkle READ/WRITE_ONCE where
they aren't necessary.

The problem is then API looks a little bit strange. Lockless were only
allowed to be done at __ptr_ring_empty() but not __ptr_ring_full().

Thanks

So __ptr_ring_empty doesn't really work lockless. It merely does not crash.
I don't believe we can do anything to remove the need to read the
docs unless people use the safe non __ variants.

-- 
MST

On 2018年01月29日 12:41, Michael S. Tsirkin wrote:

On Mon, Jan 29, 2018 at 11:36:09AM +0800, Jason Wang wrote:

quoted

On 2018年01月26日 10:46, Michael S. Tsirkin wrote:

quoted

quoted

On 2018年01月26日 07:36, Michael S. Tsirkin wrote:

quoted

Similar to bcecb4bbf88a ("net: ptr_ring: otherwise safe empty checks can
overrun array bounds") a lockless use of __ptr_ring_full might
cause an out of bounds access.

We can fix this, but it's easier to just disallow lockless
__ptr_ring_full for now.

It looks to me that just fix this is better than disallow through doc (which
is easily to be ignored ...).

Thanks

lockless is tricky, and I'd rather not sprinkle READ/WRITE_ONCE where
they aren't necessary.

The problem is then API looks a little bit strange. Lockless were only
allowed to be done at __ptr_ring_empty() but not __ptr_ring_full().

Thanks

So __ptr_ring_empty doesn't really work lockless. It merely does not crash.
I don't believe we can do anything to remove the need to read the
docs unless people use the safe non __ variants.

Ok, then I will ack the series.

Thanks

On 2018年01月26日 07:36, Michael S. Tsirkin wrote:

This fixes a bunch of issues around ptr_ring use in net core.
One of these: "tap: fix use-after-free" is also needed on net,
but can't be backported cleanly.

I will post a net patch separately.

Lightly tested - Jason, could you pls confirm this
addresses the security issue you saw with ptr_ring?
Testing reports would be appreciated too.

Michael S. Tsirkin (12):
   ptr_ring: keep consumer_head valid at all times
   ptr_ring: clean up documentation
   ptr_ring: READ/WRITE_ONCE for __ptr_ring_empty
   tap: fix use-after-free
   ptr_ring: disallow lockless __ptr_ring_full
   Revert "net: ptr_ring: otherwise safe empty checks can overrun array
     bounds"
   skb_array: use __ptr_ring_empty
   ptr_ring: prevent queue load/store tearing
   tools/virtio: switch to __ptr_ring_empty
   tools/virtio: more stubs to fix tools build
   tools/virtio: copy READ/WRITE_ONCE
   tools/virtio: fix smp_mb on x86

  drivers/net/tap.c                |  3 --
  include/linux/ptr_ring.h         | 86 ++++++++++++++++++++++------------------
  include/linux/skb_array.h        |  2 +-
  tools/virtio/linux/kernel.h      |  2 +-
  tools/virtio/linux/thread_info.h |  1 +
  tools/virtio/ringtest/main.h     | 59 ++++++++++++++++++++++++++-
  tools/virtio/ringtest/ptr_ring.c |  2 +-
  7 files changed, 110 insertions(+), 45 deletions(-)
  create mode 100644 tools/virtio/linux/thread_info.h

For the series:

Tested-by: Jason Wang <redacted>
Acked-by: Jason Wang <redacted>

Thanks

From: Jason Wang <redacted>
Date: Mon, 29 Jan 2018 15:10:37 +0800

On 2018年01月26日 07:36, Michael S. Tsirkin wrote:

quoted

This fixes a bunch of issues around ptr_ring use in net core.
One of these: "tap: fix use-after-free" is also needed on net,
but can't be backported cleanly.

I will post a net patch separately.

Lightly tested - Jason, could you pls confirm this
addresses the security issue you saw with ptr_ring?
Testing reports would be appreciated too.

Michael S. Tsirkin (12):
   ptr_ring: keep consumer_head valid at all times
   ptr_ring: clean up documentation
   ptr_ring: READ/WRITE_ONCE for __ptr_ring_empty
   tap: fix use-after-free
   ptr_ring: disallow lockless __ptr_ring_full
   Revert "net: ptr_ring: otherwise safe empty checks can overrun array
     bounds"
   skb_array: use __ptr_ring_empty
   ptr_ring: prevent queue load/store tearing
   tools/virtio: switch to __ptr_ring_empty
   tools/virtio: more stubs to fix tools build
   tools/virtio: copy READ/WRITE_ONCE
   tools/virtio: fix smp_mb on x86

  drivers/net/tap.c                |  3 --
  include/linux/ptr_ring.h | 86 ++++++++++++++++++++++------------------
  include/linux/skb_array.h        |  2 +-
  tools/virtio/linux/kernel.h      |  2 +-
  tools/virtio/linux/thread_info.h |  1 +
  tools/virtio/ringtest/main.h     | 59 ++++++++++++++++++++++++++-
  tools/virtio/ringtest/ptr_ring.c |  2 +-
  7 files changed, 110 insertions(+), 45 deletions(-)
  create mode 100644 tools/virtio/linux/thread_info.h

For the series:

Tested-by: Jason Wang <redacted>
Acked-by: Jason Wang <redacted>

Series applied, thanks everyone.