Re: [PATCH] ptr_ring: add barriers
Hi David, On 12/11/2017 09:23 PM, David Miller wrote: From: "Michael S. Tsirkin" Date: Tue, 5 Dec 2017 21:29:37 +0200 Users of ptr_ring expect that it's safe to give the data structure a pointer and have it be available to consumers, but that actually requires an smb_wmb or a stronger barrier. In absence of such barriers and on architectures that reorder writes, consumer might read an un=initialized value from an skb pointer stored in the skb array. This was observed causing crashes. To fix, add memory barriers. The barrier we use is a wmb, the assumption being that producers do not need to read the value so we do not need to order these reads. Reported-by: George Cherian Suggested-by: Jason Wang Signed-off-by: Michael S. Tsirkin I'm asked for asking for testing feedback and did not get it in a reasonable amount of time. The tests have completed more than 48 hours without any failures. I won't interrupt the same and run for longer time. In case of any issue I will report the same. So I'm applying this as-is, and queueing it up for -stable. Thank you. Regards, -George
Re: [PATCH] ptr_ring: add barriers
From: "Michael S. Tsirkin" Date: Tue, 5 Dec 2017 21:29:37 +0200 > Users of ptr_ring expect that it's safe to give the > data structure a pointer and have it be available > to consumers, but that actually requires an smb_wmb > or a stronger barrier. > > In absence of such barriers and on architectures that reorder writes, > consumer might read an un=initialized value from an skb pointer stored > in the skb array. This was observed causing crashes. > > To fix, add memory barriers. The barrier we use is a wmb, the > assumption being that producers do not need to read the value so we do > not need to order these reads. > > Reported-by: George Cherian > Suggested-by: Jason Wang > Signed-off-by: Michael S. Tsirkin I'm asked for asking for testing feedback and did not get it in a reasonable amount of time. So I'm applying this as-is, and queueing it up for -stable. Thank you.
Re: [PATCH] ptr_ring: add barriers
From: "Michael S. Tsirkin" Date: Tue, 5 Dec 2017 21:29:37 +0200 > Users of ptr_ring expect that it's safe to give the > data structure a pointer and have it be available > to consumers, but that actually requires an smb_wmb > or a stronger barrier. > > In absence of such barriers and on architectures that reorder writes, > consumer might read an un=initialized value from an skb pointer stored > in the skb array. This was observed causing crashes. > > To fix, add memory barriers. The barrier we use is a wmb, the > assumption being that producers do not need to read the value so we do > not need to order these reads. > > Reported-by: George Cherian > Suggested-by: Jason Wang > Signed-off-by: Michael S. Tsirkin > --- > > George, could you pls report whether this patch fixes > the issue for you? > > This seems to be needed in stable as well. I really need some testing feedback for this before I apply it and queue it up for -stable. George?
Re: [PATCH] ptr_ring: Add barriers to fix NULL-pointer exception
On Wed, Dec 06, 2017 at 02:08:54PM +, Cherian, George wrote: > > @@ -275,6 +281,13 @@ static inline void *__ptr_ring_consume(struct ptr_ring > *r) > > if (ptr) > > __ptr_ring_discard_one(r); > > > > + /* > > + * This barrier is necessary in order to prevent race condition with > > + * with __ptr_ring_produce(). Make sure all the elements of ptr is > > + * in sync with the earlier writes which was done prior to pushing > > + * it to ring > > + */ > > + rmb(); > > return ptr; > > } > > You are trying to synchronise two CPUs so non-smp barriers make no > sense. wmb/rmb are for synchronising with MMIO. > > What happens when CONFIG_SMP is not set. smp_wmb/rmb becomes compiler barriers > (atleast for arm64). And that is because all read and writes always appear in order when done from the same CPU. In case of reads, we do not need a barrier at all (except on dec alpha) because a read through a pointer can't bypass a read of a pointer. > I guess that is not what we need. Maybe, but I don't yet see why not. > An SMP barrier cannot > replace a mandatory barrier, but a mandatory barrier can replace an SMP > barrier. This does imply that you can always replace a weak barrier with a strong one, but does not mean you should. > I will try out your patch too and update the results. > But I would need couple of days time. Sorry for the delay. Thanks for the testing. -- MST
Re: [PATCH] ptr_ring: Add barriers to fix NULL-pointer exception
On Wed, Dec 06, 2017 at 09:57:41AM +, George Cherian wrote: > While running a multiple VM testscase with each VM running iperf > traffic between others the following kernel NULL pointer exception > was seen. > > Race appears when the tun driver instance of one VM calls skb_array_produce > (from tun_net_xmit) and the the destined VM's skb_array_consume > (from tun_ring_recv), which could run concurrently on another core. Due to > which the sock_wfree gets called again from the tun_ring_recv context. > > The fix is to add write/read barrier calls to be sure that we get proper > values in the tun_ring_recv context. > > Crash log > [35321.580227] Unable to handle kernel NULL pointer dereference at virtual > address 0060 > [35321.596720] pgd = 809ee552f000 > [35321.603723] [0060] *pgd=009f514ac003, *pud=009f54f7c003, > *pmd= > [35321.620588] Internal error: Oops: 9606 1 SMP > [35321.630461] Modules linked in: xt_CHECKSUM iptable_mangle ipt_MASQUERADE > nf_nat_masquerade_ipv4 iptable_nat nf_nat_ipv4 nf_nat nf_conntrack_ipv4 > nf_defrag_ipv4 xt_4 > [35321.728501] CPU: 114 PID: 5560 Comm: qemu-system-aar Not tainted > 4.11.8-4k-vhe-lse+ #3 > [35321.744510] Hardware name: Cavium Inc. Unknown/Unknown, BIOS 1.0 07/24/2017 > [35321.758602] task: 80bed7fca880 task.stack: 80beb5128000 > [35321.770600] PC is at sock_wfree+0x24/0x80 > [35321.778746] LR is at skb_release_head_state+0x68/0xf8 > [35321.788979] pc : [] lr : [] pstate: > 40400149 > [35321.803930] sp : 80beb512bc30 > [35321.810648] x29: 80beb512bc30 x28: 80bed7fca880 > [35321.821400] x27: 004e x26: > [35321.832156] x25: 000c x24: > [35321.842947] x23: 809ece3e4900 x22: 80beb512be00 > [35321.853729] x21: 09138000 x20: 0144 > [35321.864507] x19: x18: 0014 > [35321.875276] x17: 9d9689a0 x16: 0828b3f8 > [35321.886048] x15: 504d7b00 x14: e90ab50c48680a08 > [35321.896824] x13: 010117773f52 x12: 1080d422c00e5db6 > [35321.907595] x11: 68c322bd3930cf7a x10: a8c0d07aa8c0ad16 > [35321.918352] x9 : 1da4ed90 x8 : b50c48680a080101 > [35321.929099] x7 : 17770c521080 x6 : 1d6c13f2 > [35321.939865] x5 : 1d6c13f2 x4 : 000e > [35321.950619] x3 : 00085ff97d82 x2 : > [35321.961376] x1 : 08a772d8 x0 : 0500 > [35321.975193] Process qemu-system-aar (pid: 5560, stack limit = > 0x80beb5128000) > [35321.990347] Stack: (0x80beb512bc30 to 0x80beb512c000) > [35322.001982] bc20: 80beb512bc50 08a79238 > [35322.017817] bc40: 809e8fd7be00 004e 80beb512bc70 > 08a79488 > [35322.033651] bc60: 809e8fd7be00 0881307c 80beb512bc90 > 08a79678 > [35322.049489] bc80: 809e8fd7be00 80beb512be00 80beb512bcb0 > 08812f24 > [35322.065321] bca0: 809e8fd7be00 004e 80beb512bd50 > 088133f0 > [35322.081165] bcc0: 809ece3e4900 00011000 80beb512bdd8 > 80beb512be00 > [35322.097001] bce0: 1d6c13a4 0015 0124 > 003f > [35322.112866] bd00: 08bc2000 0847b5ac 0002 > 80be0008 > [35322.128701] bd20: 00220001 80bed7fc0010 08100c38 > > [35322.144539] bd40: 00040b08 80beb512bd80 > 08288f80 > [35322.160395] bd60: 09138000 809ee7cd3500 00011000 > 80beb512beb0 > [35322.176255] bd80: 80beb512be30 0828a224 00011000 > 809ee7cd3500 > [35322.192109] bda0: 1d6c13a4 80beb512beb0 00011000 > > [35322.207974] bdc0: 1d6c13a4 00011000 > 809ee7cd3500 > [35322.223822] bde0: 004e > > [35322.239661] be00: 80be 004e 00010fb2 > 80beb512bdc8 > [35322.255519] be20: 0001 00040b08 80beb512be70 > 0828b464 > [35322.271392] be40: 09138000 809ee7cd3501 809ee7cd3500 > 1d6c13a4 > [35322.287255] be60: 00011000 0015 > 080833f0 > [35322.303090] be80: 80bef0071000 > 9d9689cc > [35322.318951] bea0: 8000 80bef0071000 004e > 00040b08 > [35322.334771] bec0: 000e 1d6c13a4 00011000 > 9cc89108 > [35322.350640] bee0: 0002 9cc89000 9cc896f0 > > [35322.366500] bf00: 003f 1da4ed90 a8c0d07aa8c0ad16 > 68c322bd3930cf7a > [35322.382358] bf20: 1080d422c00e5db6 010117773f52 e90ab50c48680a08 > 504d7b00 > [35322.398209] bf40: 9d9689a0 000
Re: [PATCH] ptr_ring: add barriers
On Wed, Dec 06, 2017 at 02:51:41PM +0530, George Cherian wrote: > Hi Michael, > > > On 12/06/2017 12:59 AM, Michael S. Tsirkin wrote: > > Users of ptr_ring expect that it's safe to give the > > data structure a pointer and have it be available > > to consumers, but that actually requires an smb_wmb > > or a stronger barrier. > This is not the exact situation we are seeing. Could you test the patch pls? > Let me try to explain the situation > > Affected on ARM64 platform. > 1) tun_net_xmit calls skb_array_produce, which pushes the skb to the > ptr_ring, this push is protected by a producer_lock. > > 2)Prior to this call the tun_net_xmit calls skb_orphan which calls the > skb->destructor and sets skb->destructor and skb->sk as NULL. > > 2.a) These 2 writes are getting reordered > > 3) At the same time in the receive side (tun_ring_recv), which gets executed > in another core calls skb_array_consume which pulls the skb from ptr ring, > this pull is protected by a consumer lock. > > 4) eventually calling the skb->destructor (sock_wfree) with stale values. > > Also note that this issue is reproducible in a long run and doesn't happen > immediately after the launch of multiple VM's (infact the particular test > cases launches 56 VM's which does iperf back and forth) > > > > > In absence of such barriers and on architectures that reorder writes, > > consumer might read an un=initialized value from an skb pointer stored > > in the skb array. This was observed causing crashes. > > > > To fix, add memory barriers. The barrier we use is a wmb, the > > assumption being that producers do not need to read the value so we do > > not need to order these reads. > It is not the case that producer is reading the value, but the consumer > reading stale value. So we need to have a strict rmb in place . > > > > > Reported-by: George Cherian > > Suggested-by: Jason Wang > > Signed-off-by: Michael S. Tsirkin > > --- > > > > George, could you pls report whether this patch fixes > > the issue for you? > > > > This seems to be needed in stable as well. > > > > > > > > > > include/linux/ptr_ring.h | 9 + > > 1 file changed, 9 insertions(+) > > > > diff --git a/include/linux/ptr_ring.h b/include/linux/ptr_ring.h > > index 37b4bb2..6866df4 100644 > > --- a/include/linux/ptr_ring.h > > +++ b/include/linux/ptr_ring.h > > @@ -101,12 +101,18 @@ static inline bool ptr_ring_full_bh(struct ptr_ring > > *r) > > /* Note: callers invoking this in a loop must use a compiler barrier, > >* for example cpu_relax(). Callers must hold producer_lock. > > + * Callers are responsible for making sure pointer that is being queued > > + * points to a valid data. > >*/ > > static inline int __ptr_ring_produce(struct ptr_ring *r, void *ptr) > > { > > if (unlikely(!r->size) || r->queue[r->producer]) > > return -ENOSPC; > > + /* Make sure the pointer we are storing points to a valid data. */ > > + /* Pairs with smp_read_barrier_depends in __ptr_ring_consume. */ > > + smp_wmb(); > > + > > r->queue[r->producer++] = ptr; > > if (unlikely(r->producer >= r->size)) > > r->producer = 0; > > @@ -275,6 +281,9 @@ static inline void *__ptr_ring_consume(struct ptr_ring > > *r) > > if (ptr) > > __ptr_ring_discard_one(r); > > + /* Make sure anyone accessing data through the pointer is up to date. */ > > + /* Pairs with smp_wmb in __ptr_ring_produce. */ > > + smp_read_barrier_depends(); > > return ptr; > > } > >
[PATCH] ptr_ring: Add barriers to fix NULL-pointer exception
While running a multiple VM testscase with each VM running iperf traffic between others the following kernel NULL pointer exception was seen. Race appears when the tun driver instance of one VM calls skb_array_produce (from tun_net_xmit) and the the destined VM's skb_array_consume (from tun_ring_recv), which could run concurrently on another core. Due to which the sock_wfree gets called again from the tun_ring_recv context. The fix is to add write/read barrier calls to be sure that we get proper values in the tun_ring_recv context. Crash log [35321.580227] Unable to handle kernel NULL pointer dereference at virtual address 0060 [35321.596720] pgd = 809ee552f000 [35321.603723] [0060] *pgd=009f514ac003, *pud=009f54f7c003, *pmd= [35321.620588] Internal error: Oops: 9606 1 SMP [35321.630461] Modules linked in: xt_CHECKSUM iptable_mangle ipt_MASQUERADE nf_nat_masquerade_ipv4 iptable_nat nf_nat_ipv4 nf_nat nf_conntrack_ipv4 nf_defrag_ipv4 xt_4 [35321.728501] CPU: 114 PID: 5560 Comm: qemu-system-aar Not tainted 4.11.8-4k-vhe-lse+ #3 [35321.744510] Hardware name: Cavium Inc. Unknown/Unknown, BIOS 1.0 07/24/2017 [35321.758602] task: 80bed7fca880 task.stack: 80beb5128000 [35321.770600] PC is at sock_wfree+0x24/0x80 [35321.778746] LR is at skb_release_head_state+0x68/0xf8 [35321.788979] pc : [] lr : [] pstate: 40400149 [35321.803930] sp : 80beb512bc30 [35321.810648] x29: 80beb512bc30 x28: 80bed7fca880 [35321.821400] x27: 004e x26: [35321.832156] x25: 000c x24: [35321.842947] x23: 809ece3e4900 x22: 80beb512be00 [35321.853729] x21: 09138000 x20: 0144 [35321.864507] x19: x18: 0014 [35321.875276] x17: 9d9689a0 x16: 0828b3f8 [35321.886048] x15: 504d7b00 x14: e90ab50c48680a08 [35321.896824] x13: 010117773f52 x12: 1080d422c00e5db6 [35321.907595] x11: 68c322bd3930cf7a x10: a8c0d07aa8c0ad16 [35321.918352] x9 : 1da4ed90 x8 : b50c48680a080101 [35321.929099] x7 : 17770c521080 x6 : 1d6c13f2 [35321.939865] x5 : 1d6c13f2 x4 : 000e [35321.950619] x3 : 00085ff97d82 x2 : [35321.961376] x1 : 08a772d8 x0 : 0500 [35321.975193] Process qemu-system-aar (pid: 5560, stack limit = 0x80beb5128000) [35321.990347] Stack: (0x80beb512bc30 to 0x80beb512c000) [35322.001982] bc20: 80beb512bc50 08a79238 [35322.017817] bc40: 809e8fd7be00 004e 80beb512bc70 08a79488 [35322.033651] bc60: 809e8fd7be00 0881307c 80beb512bc90 08a79678 [35322.049489] bc80: 809e8fd7be00 80beb512be00 80beb512bcb0 08812f24 [35322.065321] bca0: 809e8fd7be00 004e 80beb512bd50 088133f0 [35322.081165] bcc0: 809ece3e4900 00011000 80beb512bdd8 80beb512be00 [35322.097001] bce0: 1d6c13a4 0015 0124 003f [35322.112866] bd00: 08bc2000 0847b5ac 0002 80be0008 [35322.128701] bd20: 00220001 80bed7fc0010 08100c38 [35322.144539] bd40: 00040b08 80beb512bd80 08288f80 [35322.160395] bd60: 09138000 809ee7cd3500 00011000 80beb512beb0 [35322.176255] bd80: 80beb512be30 0828a224 00011000 809ee7cd3500 [35322.192109] bda0: 1d6c13a4 80beb512beb0 00011000 [35322.207974] bdc0: 1d6c13a4 00011000 809ee7cd3500 [35322.223822] bde0: 004e [35322.239661] be00: 80be 004e 00010fb2 80beb512bdc8 [35322.255519] be20: 0001 00040b08 80beb512be70 0828b464 [35322.271392] be40: 09138000 809ee7cd3501 809ee7cd3500 1d6c13a4 [35322.287255] be60: 00011000 0015 080833f0 [35322.303090] be80: 80bef0071000 9d9689cc [35322.318951] bea0: 8000 80bef0071000 004e 00040b08 [35322.334771] bec0: 000e 1d6c13a4 00011000 9cc89108 [35322.350640] bee0: 0002 9cc89000 9cc896f0 [35322.366500] bf00: 003f 1da4ed90 a8c0d07aa8c0ad16 68c322bd3930cf7a [35322.382358] bf20: 1080d422c00e5db6 010117773f52 e90ab50c48680a08 504d7b00 [35322.398209] bf40: 9d9689a0 0014 0030 [35322.414063] bf60: 1d6c13a4 1d6c0db0 1d6d1db0 006fbbc8 [35322.429901] bf80: 00011000 1d6ab3e8 1d6ab3a4 007ee4c8 [35322.445751] bfa0: f363ab70
Re: [PATCH] ptr_ring: add barriers
Hi Michael, On 12/06/2017 12:59 AM, Michael S. Tsirkin wrote: Users of ptr_ring expect that it's safe to give the data structure a pointer and have it be available to consumers, but that actually requires an smb_wmb or a stronger barrier. This is not the exact situation we are seeing. Let me try to explain the situation Affected on ARM64 platform. 1) tun_net_xmit calls skb_array_produce, which pushes the skb to the ptr_ring, this push is protected by a producer_lock. 2)Prior to this call the tun_net_xmit calls skb_orphan which calls the skb->destructor and sets skb->destructor and skb->sk as NULL. 2.a) These 2 writes are getting reordered 3) At the same time in the receive side (tun_ring_recv), which gets executed in another core calls skb_array_consume which pulls the skb from ptr ring, this pull is protected by a consumer lock. 4) eventually calling the skb->destructor (sock_wfree) with stale values. Also note that this issue is reproducible in a long run and doesn't happen immediately after the launch of multiple VM's (infact the particular test cases launches 56 VM's which does iperf back and forth) In absence of such barriers and on architectures that reorder writes, consumer might read an un=initialized value from an skb pointer stored in the skb array. This was observed causing crashes. To fix, add memory barriers. The barrier we use is a wmb, the assumption being that producers do not need to read the value so we do not need to order these reads. It is not the case that producer is reading the value, but the consumer reading stale value. So we need to have a strict rmb in place . Reported-by: George Cherian Suggested-by: Jason Wang Signed-off-by: Michael S. Tsirkin --- George, could you pls report whether this patch fixes the issue for you? This seems to be needed in stable as well. include/linux/ptr_ring.h | 9 + 1 file changed, 9 insertions(+) diff --git a/include/linux/ptr_ring.h b/include/linux/ptr_ring.h index 37b4bb2..6866df4 100644 --- a/include/linux/ptr_ring.h +++ b/include/linux/ptr_ring.h @@ -101,12 +101,18 @@ static inline bool ptr_ring_full_bh(struct ptr_ring *r) /* Note: callers invoking this in a loop must use a compiler barrier, * for example cpu_relax(). Callers must hold producer_lock. + * Callers are responsible for making sure pointer that is being queued + * points to a valid data. */ static inline int __ptr_ring_produce(struct ptr_ring *r, void *ptr) { if (unlikely(!r->size) || r->queue[r->producer]) return -ENOSPC; + /* Make sure the pointer we are storing points to a valid data. */ + /* Pairs with smp_read_barrier_depends in __ptr_ring_consume. */ + smp_wmb(); + r->queue[r->producer++] = ptr; if (unlikely(r->producer >= r->size)) r->producer = 0; @@ -275,6 +281,9 @@ static inline void *__ptr_ring_consume(struct ptr_ring *r) if (ptr) __ptr_ring_discard_one(r); + /* Make sure anyone accessing data through the pointer is up to date. */ + /* Pairs with smp_wmb in __ptr_ring_produce. */ + smp_read_barrier_depends(); return ptr; }
Re: [PATCH] ptr_ring: add barriers
On 2017年12月06日 10:53, Michael S. Tsirkin wrote: On Wed, Dec 06, 2017 at 10:31:39AM +0800, Jason Wang wrote: On 2017年12月06日 03:29, Michael S. Tsirkin wrote: Users of ptr_ring expect that it's safe to give the data structure a pointer and have it be available to consumers, but that actually requires an smb_wmb or a stronger barrier. In absence of such barriers and on architectures that reorder writes, consumer might read an un=initialized value from an skb pointer stored in the skb array. This was observed causing crashes. To fix, add memory barriers. The barrier we use is a wmb, the assumption being that producers do not need to read the value so we do not need to order these reads. Reported-by: George Cherian Suggested-by: Jason Wang Signed-off-by: Michael S. Tsirkin --- George, could you pls report whether this patch fixes the issue for you? This seems to be needed in stable as well. include/linux/ptr_ring.h | 9 + 1 file changed, 9 insertions(+) diff --git a/include/linux/ptr_ring.h b/include/linux/ptr_ring.h index 37b4bb2..6866df4 100644 --- a/include/linux/ptr_ring.h +++ b/include/linux/ptr_ring.h @@ -101,12 +101,18 @@ static inline bool ptr_ring_full_bh(struct ptr_ring *r) /* Note: callers invoking this in a loop must use a compiler barrier, * for example cpu_relax(). Callers must hold producer_lock. + * Callers are responsible for making sure pointer that is being queued + * points to a valid data. */ static inline int __ptr_ring_produce(struct ptr_ring *r, void *ptr) { if (unlikely(!r->size) || r->queue[r->producer]) return -ENOSPC; + /* Make sure the pointer we are storing points to a valid data. */ + /* Pairs with smp_read_barrier_depends in __ptr_ring_consume. */ + smp_wmb(); + r->queue[r->producer++] = ptr; if (unlikely(r->producer >= r->size)) r->producer = 0; @@ -275,6 +281,9 @@ static inline void *__ptr_ring_consume(struct ptr_ring *r) if (ptr) __ptr_ring_discard_one(r); + /* Make sure anyone accessing data through the pointer is up to date. */ + /* Pairs with smp_wmb in __ptr_ring_produce. */ + smp_read_barrier_depends(); return ptr; } I was thinking whether or not it's better to move those to the callers. Then we can save lots of barriers in e.g batch consuming. Thanks Batch consumers only do smp_read_barrier_depends which is free on non-alpha. I suggest we do the simple thing for stable and reserve optimizations for later. Right. Acked-by: Jason Wang
Re: [PATCH] ptr_ring: add barriers
On Wed, Dec 06, 2017 at 10:31:39AM +0800, Jason Wang wrote: > > > On 2017年12月06日 03:29, Michael S. Tsirkin wrote: > > Users of ptr_ring expect that it's safe to give the > > data structure a pointer and have it be available > > to consumers, but that actually requires an smb_wmb > > or a stronger barrier. > > > > In absence of such barriers and on architectures that reorder writes, > > consumer might read an un=initialized value from an skb pointer stored > > in the skb array. This was observed causing crashes. > > > > To fix, add memory barriers. The barrier we use is a wmb, the > > assumption being that producers do not need to read the value so we do > > not need to order these reads. > > > > Reported-by: George Cherian > > Suggested-by: Jason Wang > > Signed-off-by: Michael S. Tsirkin > > --- > > > > George, could you pls report whether this patch fixes > > the issue for you? > > > > This seems to be needed in stable as well. > > > > > > > > > > include/linux/ptr_ring.h | 9 + > > 1 file changed, 9 insertions(+) > > > > diff --git a/include/linux/ptr_ring.h b/include/linux/ptr_ring.h > > index 37b4bb2..6866df4 100644 > > --- a/include/linux/ptr_ring.h > > +++ b/include/linux/ptr_ring.h > > @@ -101,12 +101,18 @@ static inline bool ptr_ring_full_bh(struct ptr_ring > > *r) > > /* Note: callers invoking this in a loop must use a compiler barrier, > >* for example cpu_relax(). Callers must hold producer_lock. > > + * Callers are responsible for making sure pointer that is being queued > > + * points to a valid data. > >*/ > > static inline int __ptr_ring_produce(struct ptr_ring *r, void *ptr) > > { > > if (unlikely(!r->size) || r->queue[r->producer]) > > return -ENOSPC; > > + /* Make sure the pointer we are storing points to a valid data. */ > > + /* Pairs with smp_read_barrier_depends in __ptr_ring_consume. */ > > + smp_wmb(); > > + > > r->queue[r->producer++] = ptr; > > if (unlikely(r->producer >= r->size)) > > r->producer = 0; > > @@ -275,6 +281,9 @@ static inline void *__ptr_ring_consume(struct ptr_ring > > *r) > > if (ptr) > > __ptr_ring_discard_one(r); > > + /* Make sure anyone accessing data through the pointer is up to date. */ > > + /* Pairs with smp_wmb in __ptr_ring_produce. */ > > + smp_read_barrier_depends(); > > return ptr; > > } > > I was thinking whether or not it's better to move those to the callers. Then > we can save lots of barriers in e.g batch consuming. > > Thanks Batch consumers only do smp_read_barrier_depends which is free on non-alpha. I suggest we do the simple thing for stable and reserve optimizations for later. -- MST
Re: [PATCH] ptr_ring: add barriers
On 2017年12月06日 03:29, Michael S. Tsirkin wrote: Users of ptr_ring expect that it's safe to give the data structure a pointer and have it be available to consumers, but that actually requires an smb_wmb or a stronger barrier. In absence of such barriers and on architectures that reorder writes, consumer might read an un=initialized value from an skb pointer stored in the skb array. This was observed causing crashes. To fix, add memory barriers. The barrier we use is a wmb, the assumption being that producers do not need to read the value so we do not need to order these reads. Reported-by: George Cherian Suggested-by: Jason Wang Signed-off-by: Michael S. Tsirkin --- George, could you pls report whether this patch fixes the issue for you? This seems to be needed in stable as well. include/linux/ptr_ring.h | 9 + 1 file changed, 9 insertions(+) diff --git a/include/linux/ptr_ring.h b/include/linux/ptr_ring.h index 37b4bb2..6866df4 100644 --- a/include/linux/ptr_ring.h +++ b/include/linux/ptr_ring.h @@ -101,12 +101,18 @@ static inline bool ptr_ring_full_bh(struct ptr_ring *r) /* Note: callers invoking this in a loop must use a compiler barrier, * for example cpu_relax(). Callers must hold producer_lock. + * Callers are responsible for making sure pointer that is being queued + * points to a valid data. */ static inline int __ptr_ring_produce(struct ptr_ring *r, void *ptr) { if (unlikely(!r->size) || r->queue[r->producer]) return -ENOSPC; + /* Make sure the pointer we are storing points to a valid data. */ + /* Pairs with smp_read_barrier_depends in __ptr_ring_consume. */ + smp_wmb(); + r->queue[r->producer++] = ptr; if (unlikely(r->producer >= r->size)) r->producer = 0; @@ -275,6 +281,9 @@ static inline void *__ptr_ring_consume(struct ptr_ring *r) if (ptr) __ptr_ring_discard_one(r); + /* Make sure anyone accessing data through the pointer is up to date. */ + /* Pairs with smp_wmb in __ptr_ring_produce. */ + smp_read_barrier_depends(); return ptr; } I was thinking whether or not it's better to move those to the callers. Then we can save lots of barriers in e.g batch consuming. Thanks
[PATCH] ptr_ring: add barriers
Users of ptr_ring expect that it's safe to give the data structure a pointer and have it be available to consumers, but that actually requires an smb_wmb or a stronger barrier. In absence of such barriers and on architectures that reorder writes, consumer might read an un=initialized value from an skb pointer stored in the skb array. This was observed causing crashes. To fix, add memory barriers. The barrier we use is a wmb, the assumption being that producers do not need to read the value so we do not need to order these reads. Reported-by: George Cherian Suggested-by: Jason Wang Signed-off-by: Michael S. Tsirkin --- George, could you pls report whether this patch fixes the issue for you? This seems to be needed in stable as well. include/linux/ptr_ring.h | 9 + 1 file changed, 9 insertions(+) diff --git a/include/linux/ptr_ring.h b/include/linux/ptr_ring.h index 37b4bb2..6866df4 100644 --- a/include/linux/ptr_ring.h +++ b/include/linux/ptr_ring.h @@ -101,12 +101,18 @@ static inline bool ptr_ring_full_bh(struct ptr_ring *r) /* Note: callers invoking this in a loop must use a compiler barrier, * for example cpu_relax(). Callers must hold producer_lock. + * Callers are responsible for making sure pointer that is being queued + * points to a valid data. */ static inline int __ptr_ring_produce(struct ptr_ring *r, void *ptr) { if (unlikely(!r->size) || r->queue[r->producer]) return -ENOSPC; + /* Make sure the pointer we are storing points to a valid data. */ + /* Pairs with smp_read_barrier_depends in __ptr_ring_consume. */ + smp_wmb(); + r->queue[r->producer++] = ptr; if (unlikely(r->producer >= r->size)) r->producer = 0; @@ -275,6 +281,9 @@ static inline void *__ptr_ring_consume(struct ptr_ring *r) if (ptr) __ptr_ring_discard_one(r); + /* Make sure anyone accessing data through the pointer is up to date. */ + /* Pairs with smp_wmb in __ptr_ring_produce. */ + smp_read_barrier_depends(); return ptr; } -- MST