[PATCH net] ptr_ring: use kmalloc_array()

[PATCH net] ptr_ring: use kmalloc_array()

[Eric Dumazet]

From: Eric Dumazet <edumazet@google.com>

As found by syzkaller, malicious users can set whatever tx_queue_len
on a tun device and eventually crash the kernel.

Lets remove the ALIGN(XXX, SMP_CACHE_BYTES) thing since a small
ring buffer is not fast anyway.

Fixes: 2e0ab8ca83c1 ("ptr_ring: array based FIFO for pointers")
Signed-off-by: Eric Dumazet <edumazet@google.com>
Reported-by: Dmitry Vyukov <dvyukov@google.com>
Cc: Michael S. Tsirkin <mst@redhat.com>
Cc: Jason Wang <jasowang@redhat.com>
---
 include/linux/ptr_ring.h  |    9 +++++----
 include/linux/skb_array.h |    3 ++-
 2 files changed, 7 insertions(+), 5 deletions(-)

diff --git a/include/linux/ptr_ring.h b/include/linux/ptr_ring.h
index d8c97ec8a8e6..37b4bb2545b3 100644
--- a/include/linux/ptr_ring.h
+++ b/include/linux/ptr_ring.h
@@ -436,9 +436,9 @@ static inline int ptr_ring_consume_batched_bh(struct ptr_ring *r,
 	__PTR_RING_PEEK_CALL_v; \
 })
 
-static inline void **__ptr_ring_init_queue_alloc(int size, gfp_t gfp)
+static inline void **__ptr_ring_init_queue_alloc(unsigned int size, gfp_t gfp)
 {
-	return kzalloc(ALIGN(size * sizeof(void *), SMP_CACHE_BYTES), gfp);
+	return kcalloc(size, sizeof(void *), gfp);
 }
 
 static inline void __ptr_ring_set_size(struct ptr_ring *r, int size)
@@ -582,7 +582,8 @@ static inline int ptr_ring_resize(struct ptr_ring *r, int size, gfp_t gfp,
  * In particular if you consume ring in interrupt or BH context, you must
  * disable interrupts/BH when doing so.
  */
-static inline int ptr_ring_resize_multiple(struct ptr_ring **rings, int nrings,
+static inline int ptr_ring_resize_multiple(struct ptr_ring **rings,
+					   unsigned int nrings,
 					   int size,
 					   gfp_t gfp, void (*destroy)(void *))
 {
@@ -590,7 +591,7 @@ static inline int ptr_ring_resize_multiple(struct ptr_ring **rings, int nrings,
 	void ***queues;
 	int i;
 
-	queues = kmalloc(nrings * sizeof *queues, gfp);
+	queues = kmalloc_array(nrings, sizeof(*queues), gfp);
 	if (!queues)
 		goto noqueues;
 
diff --git a/include/linux/skb_array.h b/include/linux/skb_array.h
index 35226cd4efb0..8621ffdeecbf 100644
--- a/include/linux/skb_array.h
+++ b/include/linux/skb_array.h
@@ -193,7 +193,8 @@ static inline int skb_array_resize(struct skb_array *a, int size, gfp_t gfp)
 }
 
 static inline int skb_array_resize_multiple(struct skb_array **rings,
-					    int nrings, int size, gfp_t gfp)
+					    int nrings, unsigned int size,
+					    gfp_t gfp)
 {
 	BUILD_BUG_ON(offsetof(struct skb_array, ring));
 	return ptr_ring_resize_multiple((struct ptr_ring **)rings,

Re: [PATCH net] ptr_ring: use kmalloc_array()

[David Miller]

From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Wed, 16 Aug 2017 10:36:47 -0700

> From: Eric Dumazet <edumazet@google.com>
> 
> As found by syzkaller, malicious users can set whatever tx_queue_len
> on a tun device and eventually crash the kernel.
> 
> Lets remove the ALIGN(XXX, SMP_CACHE_BYTES) thing since a small
> ring buffer is not fast anyway.
> 
> Fixes: 2e0ab8ca83c1 ("ptr_ring: array based FIFO for pointers")
> Signed-off-by: Eric Dumazet <edumazet@google.com>
> Reported-by: Dmitry Vyukov <dvyukov@google.com>

Applied and queued up for -stable.

Re: [PATCH net] ptr_ring: use kmalloc_array()

[Michael S. Tsirkin]

On Wed, Aug 16, 2017 at 10:36:47AM -0700, Eric Dumazet wrote:
> From: Eric Dumazet <edumazet@google.com>
> 
> As found by syzkaller, malicious users can set whatever tx_queue_len
> on a tun device and eventually crash the kernel.
> 
> Lets remove the ALIGN(XXX, SMP_CACHE_BYTES) thing since a small
> ring buffer is not fast anyway.

I'm not sure it's worth changing for small rings.

Does kmalloc_array guarantee cache line alignment for big buffers
then? If the ring is misaligned it will likely cause false sharing
as it's designed to be accessed from two CPUs.

> Fixes: 2e0ab8ca83c1 ("ptr_ring: array based FIFO for pointers")
> Signed-off-by: Eric Dumazet <edumazet@google.com>
> Reported-by: Dmitry Vyukov <dvyukov@google.com>
> Cc: Michael S. Tsirkin <mst@redhat.com>
> Cc: Jason Wang <jasowang@redhat.com>
> ---
>  include/linux/ptr_ring.h  |    9 +++++----
>  include/linux/skb_array.h |    3 ++-
>  2 files changed, 7 insertions(+), 5 deletions(-)
> 
> diff --git a/include/linux/ptr_ring.h b/include/linux/ptr_ring.h
> index d8c97ec8a8e6..37b4bb2545b3 100644
> --- a/include/linux/ptr_ring.h
> +++ b/include/linux/ptr_ring.h
> @@ -436,9 +436,9 @@ static inline int ptr_ring_consume_batched_bh(struct ptr_ring *r,
>  	__PTR_RING_PEEK_CALL_v; \
>  })
>  
> -static inline void **__ptr_ring_init_queue_alloc(int size, gfp_t gfp)
> +static inline void **__ptr_ring_init_queue_alloc(unsigned int size, gfp_t gfp)
>  {
> -	return kzalloc(ALIGN(size * sizeof(void *), SMP_CACHE_BYTES), gfp);
> +	return kcalloc(size, sizeof(void *), gfp);
>  }
>  
>  static inline void __ptr_ring_set_size(struct ptr_ring *r, int size)
> @@ -582,7 +582,8 @@ static inline int ptr_ring_resize(struct ptr_ring *r, int size, gfp_t gfp,
>   * In particular if you consume ring in interrupt or BH context, you must
>   * disable interrupts/BH when doing so.
>   */
> -static inline int ptr_ring_resize_multiple(struct ptr_ring **rings, int nrings,
> +static inline int ptr_ring_resize_multiple(struct ptr_ring **rings,
> +					   unsigned int nrings,
>  					   int size,
>  					   gfp_t gfp, void (*destroy)(void *))
>  {
> @@ -590,7 +591,7 @@ static inline int ptr_ring_resize_multiple(struct ptr_ring **rings, int nrings,
>  	void ***queues;
>  	int i;
>  
> -	queues = kmalloc(nrings * sizeof *queues, gfp);
> +	queues = kmalloc_array(nrings, sizeof(*queues), gfp);
>  	if (!queues)
>  		goto noqueues;
>  
> diff --git a/include/linux/skb_array.h b/include/linux/skb_array.h
> index 35226cd4efb0..8621ffdeecbf 100644
> --- a/include/linux/skb_array.h
> +++ b/include/linux/skb_array.h
> @@ -193,7 +193,8 @@ static inline int skb_array_resize(struct skb_array *a, int size, gfp_t gfp)
>  }
>  
>  static inline int skb_array_resize_multiple(struct skb_array **rings,
> -					    int nrings, int size, gfp_t gfp)
> +					    int nrings, unsigned int size,
> +					    gfp_t gfp)
>  {
>  	BUILD_BUG_ON(offsetof(struct skb_array, ring));
>  	return ptr_ring_resize_multiple((struct ptr_ring **)rings,
> 

Re: [PATCH net] ptr_ring: use kmalloc_array()

[Eric Dumazet]

On Fri, 2017-08-25 at 21:03 +0300, Michael S. Tsirkin wrote:
> On Wed, Aug 16, 2017 at 10:36:47AM -0700, Eric Dumazet wrote:
> > From: Eric Dumazet <edumazet@google.com>
> > 
> > As found by syzkaller, malicious users can set whatever tx_queue_len
> > on a tun device and eventually crash the kernel.
> > 
> > Lets remove the ALIGN(XXX, SMP_CACHE_BYTES) thing since a small
> > ring buffer is not fast anyway.
> 
> I'm not sure it's worth changing for small rings.
> 
> Does kmalloc_array guarantee cache line alignment for big buffers
> then? If the ring is misaligned it will likely cause false sharing
> as it's designed to be accessed from two CPUs.

I specifically said that in the changelog :

"since a small ring buffer is not fast anyway."

If one user sets up a pathological small ring buffer, kernel should not
try to fix it.

In this case, you would have to setup a ring of 2 or 4 slots to
eventually hit false sharing.

Re: [PATCH net] ptr_ring: use kmalloc_array()

[Michael S. Tsirkin]

On Fri, Aug 25, 2017 at 11:57:19AM -0700, Eric Dumazet wrote:
> On Fri, 2017-08-25 at 21:03 +0300, Michael S. Tsirkin wrote:
> > On Wed, Aug 16, 2017 at 10:36:47AM -0700, Eric Dumazet wrote:
> > > From: Eric Dumazet <edumazet@google.com>
> > > 
> > > As found by syzkaller, malicious users can set whatever tx_queue_len
> > > on a tun device and eventually crash the kernel.
> > > 
> > > Lets remove the ALIGN(XXX, SMP_CACHE_BYTES) thing since a small
> > > ring buffer is not fast anyway.
> > 
> > I'm not sure it's worth changing for small rings.
> > 
> > Does kmalloc_array guarantee cache line alignment for big buffers
> > then? If the ring is misaligned it will likely cause false sharing
> > as it's designed to be accessed from two CPUs.
> 
> I specifically said that in the changelog :
> 
> "since a small ring buffer is not fast anyway."
> 
> If one user sets up a pathological small ring buffer, kernel should not
> try to fix it.

Yes, I got that point. My question is about big buffers.
Does kmalloc_array give us an aligned array in that case?

E.g. imagine a 100 slot array. Will 800 bytes be allocated?
In that case it uses up 12.5 cache lines. It looks like the
last cache line can become false shared with something else,
causing cache line bounces on each wrap around.


> In this case, you would have to setup a ring of 2 or 4 slots to
> eventually hit false sharing.
> 

I don't think many people set up such tiny rings so I do not really
think we care what happens in that case. But you need 8 slots to avoid
false sharing I think.

-- 
MST

Re: [PATCH net] ptr_ring: use kmalloc_array()

[Eric Dumazet]

On Fri, 2017-08-25 at 22:25 +0300, Michael S. Tsirkin wrote:
> On Fri, Aug 25, 2017 at 11:57:19AM -0700, Eric Dumazet wrote:
> > On Fri, 2017-08-25 at 21:03 +0300, Michael S. Tsirkin wrote:
> > > On Wed, Aug 16, 2017 at 10:36:47AM -0700, Eric Dumazet wrote:
> > > > From: Eric Dumazet <edumazet@google.com>
> > > > 
> > > > As found by syzkaller, malicious users can set whatever tx_queue_len
> > > > on a tun device and eventually crash the kernel.
> > > > 
> > > > Lets remove the ALIGN(XXX, SMP_CACHE_BYTES) thing since a small
> > > > ring buffer is not fast anyway.
> > > 
> > > I'm not sure it's worth changing for small rings.
> > > 
> > > Does kmalloc_array guarantee cache line alignment for big buffers
> > > then? If the ring is misaligned it will likely cause false sharing
> > > as it's designed to be accessed from two CPUs.
> > 
> > I specifically said that in the changelog :
> > 
> > "since a small ring buffer is not fast anyway."
> > 
> > If one user sets up a pathological small ring buffer, kernel should not
> > try to fix it.
> 
> Yes, I got that point. My question is about big buffers.
> Does kmalloc_array give us an aligned array in that case?
> 

The answer is yes, kmalloc() uses aligned slabs for allocations larger
than the L1 cache sizes.

> E.g. imagine a 100 slot array. Will 800 bytes be allocated?
> In that case it uses up 12.5 cache lines. It looks like the
> last cache line can become false shared with something else,
> causing cache line bounces on each wrap around.
> 

800 bytes are rounded to 1024 by slab allocators.

> 
> > In this case, you would have to setup a ring of 2 or 4 slots to
> > eventually hit false sharing.
> > 
> 
> I don't think many people set up such tiny rings so I do not really
> think we care what happens in that case. But you need 8 slots to avoid
> false sharing I think.
>