* [PATCH net-next 01/12] ptr_ring: keep consumer_head valid at all times
2018-01-25 23:36 [PATCH net-next 00/12] ptr_ring fixes Michael S. Tsirkin
@ 2018-01-25 23:36 ` Michael S. Tsirkin
2018-01-26 0:11 ` John Fastabend
2018-01-25 23:36 ` [PATCH net-next 02/12] ptr_ring: clean up documentation Michael S. Tsirkin
` (15 subsequent siblings)
16 siblings, 1 reply; 37+ messages in thread
From: Michael S. Tsirkin @ 2018-01-25 23:36 UTC (permalink / raw)
To: linux-kernel; +Cc: netdev, Jason Wang, John Fastabend, David Miller
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
^ permalink raw reply related [flat|nested] 37+ messages in thread* Re: [PATCH net-next 01/12] ptr_ring: keep consumer_head valid at all times
2018-01-25 23:36 ` [PATCH net-next 01/12] ptr_ring: keep consumer_head valid at all times Michael S. Tsirkin
@ 2018-01-26 0:11 ` John Fastabend
0 siblings, 0 replies; 37+ messages in thread
From: John Fastabend @ 2018-01-26 0:11 UTC (permalink / raw)
To: Michael S. Tsirkin, linux-kernel; +Cc: netdev, Jason Wang, David Miller
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>
^ permalink raw reply [flat|nested] 37+ messages in thread
* [PATCH net-next 02/12] ptr_ring: clean up documentation
2018-01-25 23:36 [PATCH net-next 00/12] ptr_ring fixes Michael S. Tsirkin
2018-01-25 23:36 ` [PATCH net-next 01/12] ptr_ring: keep consumer_head valid at all times Michael S. Tsirkin
@ 2018-01-25 23:36 ` Michael S. Tsirkin
2018-01-25 23:36 ` [PATCH net-next 03/12] ptr_ring: READ/WRITE_ONCE for __ptr_ring_empty Michael S. Tsirkin
` (14 subsequent siblings)
16 siblings, 0 replies; 37+ messages in thread
From: Michael S. Tsirkin @ 2018-01-25 23:36 UTC (permalink / raw)
To: linux-kernel; +Cc: netdev, Jason Wang, John Fastabend, David Miller
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
^ permalink raw reply related [flat|nested] 37+ messages in thread* [PATCH net-next 03/12] ptr_ring: READ/WRITE_ONCE for __ptr_ring_empty
2018-01-25 23:36 [PATCH net-next 00/12] ptr_ring fixes Michael S. Tsirkin
2018-01-25 23:36 ` [PATCH net-next 01/12] ptr_ring: keep consumer_head valid at all times Michael S. Tsirkin
2018-01-25 23:36 ` [PATCH net-next 02/12] ptr_ring: clean up documentation Michael S. Tsirkin
@ 2018-01-25 23:36 ` Michael S. Tsirkin
2018-01-26 2:37 ` Jason Wang
2018-01-25 23:36 ` [PATCH net-next 04/12] tap: fix use-after-free Michael S. Tsirkin
` (13 subsequent siblings)
16 siblings, 1 reply; 37+ messages in thread
From: Michael S. Tsirkin @ 2018-01-25 23:36 UTC (permalink / raw)
To: linux-kernel; +Cc: netdev, Jason Wang, John Fastabend, David Miller
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
^ permalink raw reply related [flat|nested] 37+ messages in thread* Re: [PATCH net-next 03/12] ptr_ring: READ/WRITE_ONCE for __ptr_ring_empty
2018-01-25 23:36 ` [PATCH net-next 03/12] ptr_ring: READ/WRITE_ONCE for __ptr_ring_empty Michael S. Tsirkin
@ 2018-01-26 2:37 ` Jason Wang
2018-01-26 2:44 ` Michael S. Tsirkin
0 siblings, 1 reply; 37+ messages in thread
From: Jason Wang @ 2018-01-26 2:37 UTC (permalink / raw)
To: Michael S. Tsirkin, linux-kernel; +Cc: netdev, John Fastabend, David Miller
On 2018年01月26日 07:36, Michael S. Tsirkin wrote:
> 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
^ permalink raw reply [flat|nested] 37+ messages in thread* Re: [PATCH net-next 03/12] ptr_ring: READ/WRITE_ONCE for __ptr_ring_empty
2018-01-26 2:37 ` Jason Wang
@ 2018-01-26 2:44 ` Michael S. Tsirkin
2018-01-26 3:19 ` Jason Wang
0 siblings, 1 reply; 37+ messages in thread
From: Michael S. Tsirkin @ 2018-01-26 2:44 UTC (permalink / raw)
To: Jason Wang; +Cc: linux-kernel, netdev, John Fastabend, David Miller
On Fri, Jan 26, 2018 at 10:37:58AM +0800, Jason Wang wrote:
>
>
> On 2018年01月26日 07:36, Michael S. Tsirkin wrote:
> > 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
^ permalink raw reply [flat|nested] 37+ messages in thread* Re: [PATCH net-next 03/12] ptr_ring: READ/WRITE_ONCE for __ptr_ring_empty
2018-01-26 2:44 ` Michael S. Tsirkin
@ 2018-01-26 3:19 ` Jason Wang
2018-01-26 13:44 ` Michael S. Tsirkin
0 siblings, 1 reply; 37+ messages in thread
From: Jason Wang @ 2018-01-26 3:19 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: linux-kernel, netdev, John Fastabend, David Miller
On 2018年01月26日 10:44, Michael S. Tsirkin wrote:
> On Fri, Jan 26, 2018 at 10:37:58AM +0800, Jason Wang wrote:
>>
>> On 2018年01月26日 07:36, Michael S. Tsirkin wrote:
>>> 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
^ permalink raw reply [flat|nested] 37+ messages in thread* Re: [PATCH net-next 03/12] ptr_ring: READ/WRITE_ONCE for __ptr_ring_empty
2018-01-26 3:19 ` Jason Wang
@ 2018-01-26 13:44 ` Michael S. Tsirkin
0 siblings, 0 replies; 37+ messages in thread
From: Michael S. Tsirkin @ 2018-01-26 13:44 UTC (permalink / raw)
To: Jason Wang; +Cc: linux-kernel, netdev, John Fastabend, David Miller
On Fri, Jan 26, 2018 at 11:19:58AM +0800, Jason Wang wrote:
>
>
> On 2018年01月26日 10:44, Michael S. Tsirkin wrote:
> > On Fri, Jan 26, 2018 at 10:37:58AM +0800, Jason Wang wrote:
> > >
> > > On 2018年01月26日 07:36, Michael S. Tsirkin wrote:
> > > > 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
Sure because compiler does not know head didn't change.
--
MST
^ permalink raw reply [flat|nested] 37+ messages in thread
* [PATCH net-next 04/12] tap: fix use-after-free
2018-01-25 23:36 [PATCH net-next 00/12] ptr_ring fixes Michael S. Tsirkin
` (2 preceding siblings ...)
2018-01-25 23:36 ` [PATCH net-next 03/12] ptr_ring: READ/WRITE_ONCE for __ptr_ring_empty Michael S. Tsirkin
@ 2018-01-25 23:36 ` Michael S. Tsirkin
2018-01-25 23:36 ` [PATCH net-next 05/12] ptr_ring: disallow lockless __ptr_ring_full Michael S. Tsirkin
` (12 subsequent siblings)
16 siblings, 0 replies; 37+ messages in thread
From: Michael S. Tsirkin @ 2018-01-25 23:36 UTC (permalink / raw)
To: linux-kernel; +Cc: netdev, Jason Wang, John Fastabend, David Miller
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
^ permalink raw reply related [flat|nested] 37+ messages in thread* [PATCH net-next 05/12] ptr_ring: disallow lockless __ptr_ring_full
2018-01-25 23:36 [PATCH net-next 00/12] ptr_ring fixes Michael S. Tsirkin
` (3 preceding siblings ...)
2018-01-25 23:36 ` [PATCH net-next 04/12] tap: fix use-after-free Michael S. Tsirkin
@ 2018-01-25 23:36 ` Michael S. Tsirkin
2018-01-26 2:38 ` Jason Wang
2018-01-25 23:36 ` [PATCH net-next 06/12] Revert "net: ptr_ring: otherwise safe empty checks can overrun array bounds" Michael S. Tsirkin
` (11 subsequent siblings)
16 siblings, 1 reply; 37+ messages in thread
From: Michael S. Tsirkin @ 2018-01-25 23:36 UTC (permalink / raw)
To: linux-kernel; +Cc: netdev, Jason Wang, John Fastabend, David Miller
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
^ permalink raw reply related [flat|nested] 37+ messages in thread* Re: [PATCH net-next 05/12] ptr_ring: disallow lockless __ptr_ring_full
2018-01-25 23:36 ` [PATCH net-next 05/12] ptr_ring: disallow lockless __ptr_ring_full Michael S. Tsirkin
@ 2018-01-26 2:38 ` Jason Wang
2018-01-26 2:46 ` Michael S. Tsirkin
0 siblings, 1 reply; 37+ messages in thread
From: Jason Wang @ 2018-01-26 2:38 UTC (permalink / raw)
To: Michael S. Tsirkin, linux-kernel; +Cc: netdev, John Fastabend, David Miller
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
>
> 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)
> {
^ permalink raw reply [flat|nested] 37+ messages in thread* Re: [PATCH net-next 05/12] ptr_ring: disallow lockless __ptr_ring_full
2018-01-26 2:38 ` Jason Wang
@ 2018-01-26 2:46 ` Michael S. Tsirkin
2018-01-29 3:36 ` Jason Wang
0 siblings, 1 reply; 37+ messages in thread
From: Michael S. Tsirkin @ 2018-01-26 2:46 UTC (permalink / raw)
To: Jason Wang; +Cc: linux-kernel, netdev, John Fastabend, David Miller
On Fri, Jan 26, 2018 at 10:38:05AM +0800, Jason Wang wrote:
>
>
> 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
lockless is tricky, and I'd rather not sprinkle READ/WRITE_ONCE where
they aren't necessary.
> >
> > 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)
> > {
^ permalink raw reply [flat|nested] 37+ messages in thread* Re: [PATCH net-next 05/12] ptr_ring: disallow lockless __ptr_ring_full
2018-01-26 2:46 ` Michael S. Tsirkin
@ 2018-01-29 3:36 ` Jason Wang
2018-01-29 4:41 ` Michael S. Tsirkin
0 siblings, 1 reply; 37+ messages in thread
From: Jason Wang @ 2018-01-29 3:36 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: linux-kernel, netdev, John Fastabend, David Miller
On 2018年01月26日 10:46, Michael S. Tsirkin wrote:
>> 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
> 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
^ permalink raw reply [flat|nested] 37+ messages in thread* Re: [PATCH net-next 05/12] ptr_ring: disallow lockless __ptr_ring_full
2018-01-29 3:36 ` Jason Wang
@ 2018-01-29 4:41 ` Michael S. Tsirkin
2018-01-29 7:09 ` Jason Wang
0 siblings, 1 reply; 37+ messages in thread
From: Michael S. Tsirkin @ 2018-01-29 4:41 UTC (permalink / raw)
To: Jason Wang; +Cc: linux-kernel, netdev, John Fastabend, David Miller
On Mon, Jan 29, 2018 at 11:36:09AM +0800, Jason Wang wrote:
>
>
> On 2018年01月26日 10:46, Michael S. Tsirkin wrote:
> > > 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
> > 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
^ permalink raw reply [flat|nested] 37+ messages in thread* Re: [PATCH net-next 05/12] ptr_ring: disallow lockless __ptr_ring_full
2018-01-29 4:41 ` Michael S. Tsirkin
@ 2018-01-29 7:09 ` Jason Wang
0 siblings, 0 replies; 37+ messages in thread
From: Jason Wang @ 2018-01-29 7:09 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: linux-kernel, netdev, John Fastabend, David Miller
On 2018年01月29日 12:41, Michael S. Tsirkin wrote:
> On Mon, Jan 29, 2018 at 11:36:09AM +0800, Jason Wang wrote:
>>
>> On 2018年01月26日 10:46, Michael S. Tsirkin wrote:
>>>> 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
>>> 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
^ permalink raw reply [flat|nested] 37+ messages in thread
* [PATCH net-next 06/12] Revert "net: ptr_ring: otherwise safe empty checks can overrun array bounds"
2018-01-25 23:36 [PATCH net-next 00/12] ptr_ring fixes Michael S. Tsirkin
` (4 preceding siblings ...)
2018-01-25 23:36 ` [PATCH net-next 05/12] ptr_ring: disallow lockless __ptr_ring_full Michael S. Tsirkin
@ 2018-01-25 23:36 ` Michael S. Tsirkin
2018-01-26 0:12 ` John Fastabend
2018-01-25 23:36 ` [PATCH net-next 07/12] skb_array: use __ptr_ring_empty Michael S. Tsirkin
` (10 subsequent siblings)
16 siblings, 1 reply; 37+ messages in thread
From: Michael S. Tsirkin @ 2018-01-25 23:36 UTC (permalink / raw)
To: linux-kernel
Cc: netdev, Jason Wang, John Fastabend, David Miller,
syzbot+87678bcf753b44c39b67
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
^ permalink raw reply related [flat|nested] 37+ messages in thread* Re: [PATCH net-next 06/12] Revert "net: ptr_ring: otherwise safe empty checks can overrun array bounds"
2018-01-25 23:36 ` [PATCH net-next 06/12] Revert "net: ptr_ring: otherwise safe empty checks can overrun array bounds" Michael S. Tsirkin
@ 2018-01-26 0:12 ` John Fastabend
0 siblings, 0 replies; 37+ messages in thread
From: John Fastabend @ 2018-01-26 0:12 UTC (permalink / raw)
To: Michael S. Tsirkin, linux-kernel
Cc: netdev, Jason Wang, David Miller, syzbot+87678bcf753b44c39b67
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>
> 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)
>
^ permalink raw reply [flat|nested] 37+ messages in thread
* [PATCH net-next 07/12] skb_array: use __ptr_ring_empty
2018-01-25 23:36 [PATCH net-next 00/12] ptr_ring fixes Michael S. Tsirkin
` (5 preceding siblings ...)
2018-01-25 23:36 ` [PATCH net-next 06/12] Revert "net: ptr_ring: otherwise safe empty checks can overrun array bounds" Michael S. Tsirkin
@ 2018-01-25 23:36 ` Michael S. Tsirkin
2018-01-25 23:36 ` [PATCH net-next 08/12] ptr_ring: prevent queue load/store tearing Michael S. Tsirkin
` (9 subsequent siblings)
16 siblings, 0 replies; 37+ messages in thread
From: Michael S. Tsirkin @ 2018-01-25 23:36 UTC (permalink / raw)
To: linux-kernel; +Cc: netdev, Jason Wang, John Fastabend, David Miller
__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
^ permalink raw reply related [flat|nested] 37+ messages in thread* [PATCH net-next 08/12] ptr_ring: prevent queue load/store tearing
2018-01-25 23:36 [PATCH net-next 00/12] ptr_ring fixes Michael S. Tsirkin
` (6 preceding siblings ...)
2018-01-25 23:36 ` [PATCH net-next 07/12] skb_array: use __ptr_ring_empty Michael S. Tsirkin
@ 2018-01-25 23:36 ` Michael S. Tsirkin
2018-01-26 2:38 ` Jason Wang
2018-01-25 23:36 ` [PATCH net-next 09/12] tools/virtio: switch to __ptr_ring_empty Michael S. Tsirkin
` (8 subsequent siblings)
16 siblings, 1 reply; 37+ messages in thread
From: Michael S. Tsirkin @ 2018-01-25 23:36 UTC (permalink / raw)
To: linux-kernel; +Cc: netdev, Jason Wang, John Fastabend, David Miller
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
^ permalink raw reply related [flat|nested] 37+ messages in thread* Re: [PATCH net-next 08/12] ptr_ring: prevent queue load/store tearing
2018-01-25 23:36 ` [PATCH net-next 08/12] ptr_ring: prevent queue load/store tearing Michael S. Tsirkin
@ 2018-01-26 2:38 ` Jason Wang
2018-01-26 2:49 ` Michael S. Tsirkin
0 siblings, 1 reply; 37+ messages in thread
From: Jason Wang @ 2018-01-26 2:38 UTC (permalink / raw)
To: Michael S. Tsirkin, linux-kernel; +Cc: netdev, John Fastabend, David Miller
On 2018年01月26日 07:36, Michael S. Tsirkin wrote:
> 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
> 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;
> }
>
^ permalink raw reply [flat|nested] 37+ messages in thread* Re: [PATCH net-next 08/12] ptr_ring: prevent queue load/store tearing
2018-01-26 2:38 ` Jason Wang
@ 2018-01-26 2:49 ` Michael S. Tsirkin
0 siblings, 0 replies; 37+ messages in thread
From: Michael S. Tsirkin @ 2018-01-26 2:49 UTC (permalink / raw)
To: Jason Wang; +Cc: linux-kernel, netdev, John Fastabend, David Miller
On Fri, Jan 26, 2018 at 10:38:12AM +0800, Jason Wang wrote:
>
>
> On 2018年01月26日 07:36, Michael S. Tsirkin wrote:
> > 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.
> > 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;
> > }
^ permalink raw reply [flat|nested] 37+ messages in thread
* [PATCH net-next 09/12] tools/virtio: switch to __ptr_ring_empty
2018-01-25 23:36 [PATCH net-next 00/12] ptr_ring fixes Michael S. Tsirkin
` (7 preceding siblings ...)
2018-01-25 23:36 ` [PATCH net-next 08/12] ptr_ring: prevent queue load/store tearing Michael S. Tsirkin
@ 2018-01-25 23:36 ` Michael S. Tsirkin
2018-01-25 23:36 ` Michael S. Tsirkin
` (7 subsequent siblings)
16 siblings, 0 replies; 37+ messages in thread
From: Michael S. Tsirkin @ 2018-01-25 23:36 UTC (permalink / raw)
To: linux-kernel
Cc: netdev, Jason Wang, John Fastabend, David Miller, virtualization
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
^ permalink raw reply related [flat|nested] 37+ messages in thread* [PATCH net-next 09/12] tools/virtio: switch to __ptr_ring_empty
2018-01-25 23:36 [PATCH net-next 00/12] ptr_ring fixes Michael S. Tsirkin
` (8 preceding siblings ...)
2018-01-25 23:36 ` [PATCH net-next 09/12] tools/virtio: switch to __ptr_ring_empty Michael S. Tsirkin
@ 2018-01-25 23:36 ` Michael S. Tsirkin
2018-01-25 23:36 ` Michael S. Tsirkin
` (6 subsequent siblings)
16 siblings, 0 replies; 37+ messages in thread
From: Michael S. Tsirkin @ 2018-01-25 23:36 UTC (permalink / raw)
To: linux-kernel; +Cc: netdev, John Fastabend, David Miller, virtualization
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
^ permalink raw reply related [flat|nested] 37+ messages in thread* [PATCH net-next 10/12] tools/virtio: more stubs to fix tools build
2018-01-25 23:36 [PATCH net-next 00/12] ptr_ring fixes Michael S. Tsirkin
@ 2018-01-25 23:36 ` Michael S. Tsirkin
2018-01-25 23:36 ` [PATCH net-next 02/12] ptr_ring: clean up documentation Michael S. Tsirkin
` (15 subsequent siblings)
16 siblings, 0 replies; 37+ messages in thread
From: Michael S. Tsirkin @ 2018-01-25 23:36 UTC (permalink / raw)
To: linux-kernel
Cc: netdev, Jason Wang, John Fastabend, David Miller, virtualization
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
^ permalink raw reply related [flat|nested] 37+ messages in thread* [PATCH net-next 10/12] tools/virtio: more stubs to fix tools build
@ 2018-01-25 23:36 ` Michael S. Tsirkin
0 siblings, 0 replies; 37+ messages in thread
From: Michael S. Tsirkin @ 2018-01-25 23:36 UTC (permalink / raw)
To: linux-kernel; +Cc: netdev, John Fastabend, David Miller, virtualization
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
^ permalink raw reply related [flat|nested] 37+ messages in thread
* [PATCH net-next 11/12] tools/virtio: copy READ/WRITE_ONCE
2018-01-25 23:36 [PATCH net-next 00/12] ptr_ring fixes Michael S. Tsirkin
` (10 preceding siblings ...)
2018-01-25 23:36 ` Michael S. Tsirkin
@ 2018-01-25 23:36 ` Michael S. Tsirkin
2018-01-25 23:36 ` Michael S. Tsirkin
` (4 subsequent siblings)
16 siblings, 0 replies; 37+ messages in thread
From: Michael S. Tsirkin @ 2018-01-25 23:36 UTC (permalink / raw)
To: linux-kernel; +Cc: netdev, John Fastabend, David Miller, virtualization
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
^ permalink raw reply related [flat|nested] 37+ messages in thread* [PATCH net-next 11/12] tools/virtio: copy READ/WRITE_ONCE
2018-01-25 23:36 [PATCH net-next 00/12] ptr_ring fixes Michael S. Tsirkin
` (11 preceding siblings ...)
2018-01-25 23:36 ` [PATCH net-next 11/12] tools/virtio: copy READ/WRITE_ONCE Michael S. Tsirkin
@ 2018-01-25 23:36 ` Michael S. Tsirkin
2018-01-25 23:36 ` [PATCH net-next 12/12] tools/virtio: fix smp_mb on x86 Michael S. Tsirkin
` (3 subsequent siblings)
16 siblings, 0 replies; 37+ messages in thread
From: Michael S. Tsirkin @ 2018-01-25 23:36 UTC (permalink / raw)
To: linux-kernel
Cc: netdev, Jason Wang, John Fastabend, David Miller, virtualization
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
^ permalink raw reply related [flat|nested] 37+ messages in thread* [PATCH net-next 12/12] tools/virtio: fix smp_mb on x86
2018-01-25 23:36 [PATCH net-next 00/12] ptr_ring fixes Michael S. Tsirkin
` (12 preceding siblings ...)
2018-01-25 23:36 ` Michael S. Tsirkin
@ 2018-01-25 23:36 ` Michael S. Tsirkin
2018-01-26 3:56 ` Jason Wang
2018-01-25 23:36 ` Michael S. Tsirkin
` (2 subsequent siblings)
16 siblings, 1 reply; 37+ messages in thread
From: Michael S. Tsirkin @ 2018-01-25 23:36 UTC (permalink / raw)
To: linux-kernel
Cc: netdev, Jason Wang, John Fastabend, David Miller, virtualization
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
^ permalink raw reply related [flat|nested] 37+ messages in thread* Re: [PATCH net-next 12/12] tools/virtio: fix smp_mb on x86
2018-01-25 23:36 ` [PATCH net-next 12/12] tools/virtio: fix smp_mb on x86 Michael S. Tsirkin
@ 2018-01-26 3:56 ` Jason Wang
0 siblings, 0 replies; 37+ messages in thread
From: Jason Wang @ 2018-01-26 3:56 UTC (permalink / raw)
To: Michael S. Tsirkin, linux-kernel
Cc: netdev, John Fastabend, David Miller, virtualization
On 2018年01月26日 07:36, Michael S. Tsirkin wrote:
> 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
> +#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
^ permalink raw reply [flat|nested] 37+ messages in thread* Re: [PATCH net-next 12/12] tools/virtio: fix smp_mb on x86
@ 2018-01-26 3:56 ` Jason Wang
0 siblings, 0 replies; 37+ messages in thread
From: Jason Wang @ 2018-01-26 3:56 UTC (permalink / raw)
To: Michael S. Tsirkin, linux-kernel
Cc: netdev, John Fastabend, David Miller, virtualization
On 2018年01月26日 07:36, Michael S. Tsirkin wrote:
> 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
> +#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
^ permalink raw reply [flat|nested] 37+ messages in thread* Re: [PATCH net-next 12/12] tools/virtio: fix smp_mb on x86
2018-01-26 3:56 ` Jason Wang
(?)
@ 2018-01-26 13:45 ` Michael S. Tsirkin
-1 siblings, 0 replies; 37+ messages in thread
From: Michael S. Tsirkin @ 2018-01-26 13:45 UTC (permalink / raw)
To: Jason Wang
Cc: linux-kernel, netdev, John Fastabend, David Miller, virtualization
On Fri, Jan 26, 2018 at 11:56:14AM +0800, Jason Wang wrote:
>
>
> On 2018年01月26日 07:36, Michael S. Tsirkin wrote:
> > 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.
> > +#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
^ permalink raw reply [flat|nested] 37+ messages in thread* Re: [PATCH net-next 12/12] tools/virtio: fix smp_mb on x86
2018-01-26 3:56 ` Jason Wang
(?)
(?)
@ 2018-01-26 13:45 ` Michael S. Tsirkin
-1 siblings, 0 replies; 37+ messages in thread
From: Michael S. Tsirkin @ 2018-01-26 13:45 UTC (permalink / raw)
To: Jason Wang
Cc: netdev, virtualization, John Fastabend, linux-kernel, David Miller
On Fri, Jan 26, 2018 at 11:56:14AM +0800, Jason Wang wrote:
>
>
> On 2018年01月26日 07:36, Michael S. Tsirkin wrote:
> > 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.
> > +#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
^ permalink raw reply [flat|nested] 37+ messages in thread
* [PATCH net-next 12/12] tools/virtio: fix smp_mb on x86
2018-01-25 23:36 [PATCH net-next 00/12] ptr_ring fixes Michael S. Tsirkin
` (13 preceding siblings ...)
2018-01-25 23:36 ` [PATCH net-next 12/12] tools/virtio: fix smp_mb on x86 Michael S. Tsirkin
@ 2018-01-25 23:36 ` Michael S. Tsirkin
2018-01-26 3:20 ` [PATCH net-next 00/12] ptr_ring fixes Jason Wang
2018-01-29 7:10 ` Jason Wang
16 siblings, 0 replies; 37+ messages in thread
From: Michael S. Tsirkin @ 2018-01-25 23:36 UTC (permalink / raw)
To: linux-kernel; +Cc: netdev, John Fastabend, David Miller, virtualization
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
^ permalink raw reply related [flat|nested] 37+ messages in thread* Re: [PATCH net-next 00/12] ptr_ring fixes
2018-01-25 23:36 [PATCH net-next 00/12] ptr_ring fixes Michael S. Tsirkin
` (14 preceding siblings ...)
2018-01-25 23:36 ` Michael S. Tsirkin
@ 2018-01-26 3:20 ` Jason Wang
2018-01-29 7:10 ` Jason Wang
16 siblings, 0 replies; 37+ messages in thread
From: Jason Wang @ 2018-01-26 3:20 UTC (permalink / raw)
To: Michael S. Tsirkin, linux-kernel; +Cc: netdev, John Fastabend, David Miller
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
^ permalink raw reply [flat|nested] 37+ messages in thread* Re: [PATCH net-next 00/12] ptr_ring fixes
2018-01-25 23:36 [PATCH net-next 00/12] ptr_ring fixes Michael S. Tsirkin
` (15 preceding siblings ...)
2018-01-26 3:20 ` [PATCH net-next 00/12] ptr_ring fixes Jason Wang
@ 2018-01-29 7:10 ` Jason Wang
2018-01-29 17:03 ` David Miller
16 siblings, 1 reply; 37+ messages in thread
From: Jason Wang @ 2018-01-29 7:10 UTC (permalink / raw)
To: Michael S. Tsirkin, linux-kernel; +Cc: netdev, John Fastabend, David Miller
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 <jasowang@redhat.com>
Acked-by: Jason Wang <jasowang@redhat.com>
Thanks
^ permalink raw reply [flat|nested] 37+ messages in thread* Re: [PATCH net-next 00/12] ptr_ring fixes
2018-01-29 7:10 ` Jason Wang
@ 2018-01-29 17:03 ` David Miller
0 siblings, 0 replies; 37+ messages in thread
From: David Miller @ 2018-01-29 17:03 UTC (permalink / raw)
To: jasowang; +Cc: mst, linux-kernel, netdev, john.fastabend
From: Jason Wang <jasowang@redhat.com>
Date: Mon, 29 Jan 2018 15:10:37 +0800
>
>
> 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 <jasowang@redhat.com>
> Acked-by: Jason Wang <jasowang@redhat.com>
Series applied, thanks everyone.
^ permalink raw reply [flat|nested] 37+ messages in thread