[PATCH net V3 1/2] ptr_ring: try vmalloc() when kmalloc() fails

[PATCH net V3 1/2] ptr_ring: try vmalloc() when kmalloc() fails

[Jason Wang]

This patch switch to use kvmalloc_array() for using a vmalloc()
fallback to help in case kmalloc() fails.

Reported-by: syzbot+e4d4f9ddd4295539735d@syzkaller.appspotmail.com
Fixes: 2e0ab8ca83c12 ("ptr_ring: array based FIFO for pointers")
Signed-off-by: Jason Wang <jasowang@redhat.com>
---
 include/linux/ptr_ring.h | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/include/linux/ptr_ring.h b/include/linux/ptr_ring.h
index 1883d61..2af71a7 100644
--- a/include/linux/ptr_ring.h
+++ b/include/linux/ptr_ring.h
@@ -466,7 +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)
 {
-	return kcalloc(size, sizeof(void *), gfp);
+	return kvmalloc_array(size, sizeof(void *), gfp | __GFP_ZERO);
 }
 
 static inline void __ptr_ring_set_size(struct ptr_ring *r, int size)
@@ -601,7 +601,7 @@ static inline int ptr_ring_resize(struct ptr_ring *r, int size, gfp_t gfp,
 	spin_unlock(&(r)->producer_lock);
 	spin_unlock_irqrestore(&(r)->consumer_lock, flags);
 
-	kfree(old);
+	kvfree(old);
 
 	return 0;
 }
@@ -641,7 +641,7 @@ static inline int ptr_ring_resize_multiple(struct ptr_ring **rings,
 	}
 
 	for (i = 0; i < nrings; ++i)
-		kfree(queues[i]);
+		kvfree(queues[i]);
 
 	kfree(queues);
 
@@ -649,7 +649,7 @@ static inline int ptr_ring_resize_multiple(struct ptr_ring **rings,
 
 nomem:
 	while (--i >= 0)
-		kfree(queues[i]);
+		kvfree(queues[i]);
 
 	kfree(queues);
 
@@ -664,7 +664,7 @@ static inline void ptr_ring_cleanup(struct ptr_ring *r, void (*destroy)(void *))
 	if (destroy)
 		while ((ptr = ptr_ring_consume(r)))
 			destroy(ptr);
-	kfree(r->queue);
+	kvfree(r->queue);
 }
 
 #endif /* _LINUX_PTR_RING_H  */
-- 
2.7.4

[PATCH net V3 2/2] ptr_ring: fail on large queue size (>64K)

[Jason Wang]

We need limit the maximum size of queue, otherwise it may cause
several side effects e.g slab will warn when the size exceeds
KMALLOC_MAX_SIZE. Using KMALLOC_MAX_SIZE still looks too so this patch
tries to limit it to 64K. This value could be revisited if we found a
real case that needs more.

Reported-by: syzbot+e4d4f9ddd4295539735d@syzkaller.appspotmail.com
Fixes: 2e0ab8ca83c12 ("ptr_ring: array based FIFO for pointers")
Signed-off-by: Jason Wang <jasowang@redhat.com>
---
 include/linux/ptr_ring.h | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/include/linux/ptr_ring.h b/include/linux/ptr_ring.h
index 2af71a7..5858d48 100644
--- a/include/linux/ptr_ring.h
+++ b/include/linux/ptr_ring.h
@@ -44,6 +44,8 @@ struct ptr_ring {
 	void **queue;
 };
 
+#define PTR_RING_MAX_ALLOC 65536
+
 /* Note: callers invoking this in a loop must use a compiler barrier,
  * for example cpu_relax().
  *
@@ -466,6 +468,8 @@ 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)
 {
+	if (size > PTR_RING_MAX_ALLOC)
+		return NULL;
 	return kvmalloc_array(size, sizeof(void *), gfp | __GFP_ZERO);
 }
 
-- 
2.7.4

Re: [PATCH net V3 1/2] ptr_ring: try vmalloc() when kmalloc() fails

[Michael S. Tsirkin]

On Thu, Feb 08, 2018 at 11:59:24AM +0800, Jason Wang wrote:
> This patch switch to use kvmalloc_array() for using a vmalloc()
> fallback to help in case kmalloc() fails.
> 
> Reported-by: syzbot+e4d4f9ddd4295539735d@syzkaller.appspotmail.com
> Fixes: 2e0ab8ca83c12 ("ptr_ring: array based FIFO for pointers")

I guess the actual patch is the one that switches tun to ptr_ring.

In fact, I think the actual bugfix is patch 2/2. This specific one
just makes kmalloc less likely to fail but that's
not what syzbot reported.

Then I would add this patch on top to make kmalloc less likely to fail.

> Signed-off-by: Jason Wang <jasowang@redhat.com>



> ---
>  include/linux/ptr_ring.h | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/include/linux/ptr_ring.h b/include/linux/ptr_ring.h
> index 1883d61..2af71a7 100644
> --- a/include/linux/ptr_ring.h
> +++ b/include/linux/ptr_ring.h
> @@ -466,7 +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)
>  {
> -	return kcalloc(size, sizeof(void *), gfp);
> +	return kvmalloc_array(size, sizeof(void *), gfp | __GFP_ZERO);
>  }
>  
>  static inline void __ptr_ring_set_size(struct ptr_ring *r, int size)

This implies a bunch of limitations on the flags. From kvmalloc_node
docs:

 * Reclaim modifiers - __GFP_NORETRY and __GFP_NOFAIL are not supported.
 * __GFP_RETRY_MAYFAIL is supported, and it should be used only if kmalloc is
 * preferable to the vmalloc fallback, due to visible performance drawbacks.

Fine with all the current users, but if we go this way, please add
documentation so future users don't misuse this API.
Alternatively, test flags and call kvmalloc or kcalloc?


> @@ -601,7 +601,7 @@ static inline int ptr_ring_resize(struct ptr_ring *r, int size, gfp_t gfp,
>  	spin_unlock(&(r)->producer_lock);
>  	spin_unlock_irqrestore(&(r)->consumer_lock, flags);
>  
> -	kfree(old);
> +	kvfree(old);
>  
>  	return 0;
>  }
> @@ -641,7 +641,7 @@ static inline int ptr_ring_resize_multiple(struct ptr_ring **rings,
>  	}
>  
>  	for (i = 0; i < nrings; ++i)
> -		kfree(queues[i]);
> +		kvfree(queues[i]);
>  
>  	kfree(queues);
>  
> @@ -649,7 +649,7 @@ static inline int ptr_ring_resize_multiple(struct ptr_ring **rings,
>  
>  nomem:
>  	while (--i >= 0)
> -		kfree(queues[i]);
> +		kvfree(queues[i]);
>  
>  	kfree(queues);
>  
> @@ -664,7 +664,7 @@ static inline void ptr_ring_cleanup(struct ptr_ring *r, void (*destroy)(void *))
>  	if (destroy)
>  		while ((ptr = ptr_ring_consume(r)))
>  			destroy(ptr);
> -	kfree(r->queue);
> +	kvfree(r->queue);
>  }
>  
>  #endif /* _LINUX_PTR_RING_H  */
> -- 
> 2.7.4

Re: [PATCH net V3 2/2] ptr_ring: fail on large queue size (>64K)

[Michael S. Tsirkin]

On Thu, Feb 08, 2018 at 11:59:25AM +0800, Jason Wang wrote:
> We need limit the maximum size of queue, otherwise it may cause
> several side effects e.g slab will warn when the size exceeds
> KMALLOC_MAX_SIZE. Using KMALLOC_MAX_SIZE still looks too so this patch
> tries to limit it to 64K. This value could be revisited if we found a
> real case that needs more.
> 
> Reported-by: syzbot+e4d4f9ddd4295539735d@syzkaller.appspotmail.com
> Fixes: 2e0ab8ca83c12 ("ptr_ring: array based FIFO for pointers")
> Signed-off-by: Jason Wang <jasowang@redhat.com>
> ---
>  include/linux/ptr_ring.h | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/include/linux/ptr_ring.h b/include/linux/ptr_ring.h
> index 2af71a7..5858d48 100644
> --- a/include/linux/ptr_ring.h
> +++ b/include/linux/ptr_ring.h
> @@ -44,6 +44,8 @@ struct ptr_ring {
>  	void **queue;
>  };
>  

Seems like a weird location for a define. Either put defines on
top of the file, or near where they are used. I prefer the
second option.

> +#define PTR_RING_MAX_ALLOC 65536
> +

I guess it's an arbitrary number. Seems like a sufficiently large one,
but pls add a comment so readers don't wonder. And please explain what
it does:

/* Callers can create ptr_ring structures with userspace-supplied
 * parameters. This sets a limit on the size to make that usecase
 * safe. If you ever change this, make sure to audit all callers.
 */

Also I think we should generally use either hex 0x10000 or (1 << 16).

>  /* Note: callers invoking this in a loop must use a compiler barrier,
>   * for example cpu_relax().
>   *
> @@ -466,6 +468,8 @@ 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)
>  {
> +	if (size > PTR_RING_MAX_ALLOC)
> +		return NULL;
>  	return kvmalloc_array(size, sizeof(void *), gfp | __GFP_ZERO);
>  }
>  
> -- 
> 2.7.4

Re: [PATCH net V3 1/2] ptr_ring: try vmalloc() when kmalloc() fails

[Jason Wang]

On 2018年02月08日 12:45, Michael S. Tsirkin wrote:
> On Thu, Feb 08, 2018 at 11:59:24AM +0800, Jason Wang wrote:
>> This patch switch to use kvmalloc_array() for using a vmalloc()
>> fallback to help in case kmalloc() fails.
>>
>> Reported-by: syzbot+e4d4f9ddd4295539735d@syzkaller.appspotmail.com
>> Fixes: 2e0ab8ca83c12 ("ptr_ring: array based FIFO for pointers")
> I guess the actual patch is the one that switches tun to ptr_ring.

I think not, since the issue was large allocation.

>
> In fact, I think the actual bugfix is patch 2/2. This specific one
> just makes kmalloc less likely to fail but that's
> not what syzbot reported.

Agree.

>
> Then I would add this patch on top to make kmalloc less likely to fail.

Ok.

>> Signed-off-by: Jason Wang <jasowang@redhat.com>
>
>
>> ---
>>   include/linux/ptr_ring.h | 10 +++++-----
>>   1 file changed, 5 insertions(+), 5 deletions(-)
>>
>> diff --git a/include/linux/ptr_ring.h b/include/linux/ptr_ring.h
>> index 1883d61..2af71a7 100644
>> --- a/include/linux/ptr_ring.h
>> +++ b/include/linux/ptr_ring.h
>> @@ -466,7 +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)
>>   {
>> -	return kcalloc(size, sizeof(void *), gfp);
>> +	return kvmalloc_array(size, sizeof(void *), gfp | __GFP_ZERO);
>>   }
>>   
>>   static inline void __ptr_ring_set_size(struct ptr_ring *r, int size)
> This implies a bunch of limitations on the flags. From kvmalloc_node
> docs:
>
>   * Reclaim modifiers - __GFP_NORETRY and __GFP_NOFAIL are not supported.
>   * __GFP_RETRY_MAYFAIL is supported, and it should be used only if kmalloc is
>   * preferable to the vmalloc fallback, due to visible performance drawbacks.
>
> Fine with all the current users, but if we go this way, please add
> documentation so future users don't misuse this API.

I suspect this is somehow a overkill since this means we need sync with 
mm/vmalloc changes in the future to keep it synced.

> Alternatively, test flags and call kvmalloc or kcalloc?

Similar to the above issue, I would rather leave it as is.

Thanks

>
>
>> @@ -601,7 +601,7 @@ static inline int ptr_ring_resize(struct ptr_ring *r, int size, gfp_t gfp,
>>   	spin_unlock(&(r)->producer_lock);
>>   	spin_unlock_irqrestore(&(r)->consumer_lock, flags);
>>   
>> -	kfree(old);
>> +	kvfree(old);
>>   
>>   	return 0;
>>   }
>> @@ -641,7 +641,7 @@ static inline int ptr_ring_resize_multiple(struct ptr_ring **rings,
>>   	}
>>   
>>   	for (i = 0; i < nrings; ++i)
>> -		kfree(queues[i]);
>> +		kvfree(queues[i]);
>>   
>>   	kfree(queues);
>>   
>> @@ -649,7 +649,7 @@ static inline int ptr_ring_resize_multiple(struct ptr_ring **rings,
>>   
>>   nomem:
>>   	while (--i >= 0)
>> -		kfree(queues[i]);
>> +		kvfree(queues[i]);
>>   
>>   	kfree(queues);
>>   
>> @@ -664,7 +664,7 @@ static inline void ptr_ring_cleanup(struct ptr_ring *r, void (*destroy)(void *))
>>   	if (destroy)
>>   		while ((ptr = ptr_ring_consume(r)))
>>   			destroy(ptr);
>> -	kfree(r->queue);
>> +	kvfree(r->queue);
>>   }
>>   
>>   #endif /* _LINUX_PTR_RING_H  */
>> -- 
>> 2.7.4

Re: [PATCH net V3 2/2] ptr_ring: fail on large queue size (>64K)

[Jason Wang]

On 2018年02月08日 12:52, Michael S. Tsirkin wrote:
> On Thu, Feb 08, 2018 at 11:59:25AM +0800, Jason Wang wrote:
>> We need limit the maximum size of queue, otherwise it may cause
>> several side effects e.g slab will warn when the size exceeds
>> KMALLOC_MAX_SIZE. Using KMALLOC_MAX_SIZE still looks too so this patch
>> tries to limit it to 64K. This value could be revisited if we found a
>> real case that needs more.
>>
>> Reported-by: syzbot+e4d4f9ddd4295539735d@syzkaller.appspotmail.com
>> Fixes: 2e0ab8ca83c12 ("ptr_ring: array based FIFO for pointers")
>> Signed-off-by: Jason Wang <jasowang@redhat.com>
>> ---
>>   include/linux/ptr_ring.h | 4 ++++
>>   1 file changed, 4 insertions(+)
>>
>> diff --git a/include/linux/ptr_ring.h b/include/linux/ptr_ring.h
>> index 2af71a7..5858d48 100644
>> --- a/include/linux/ptr_ring.h
>> +++ b/include/linux/ptr_ring.h
>> @@ -44,6 +44,8 @@ struct ptr_ring {
>>   	void **queue;
>>   };
>>   
> Seems like a weird location for a define. Either put defines on
> top of the file, or near where they are used. I prefer the
> second option.

Ok.

>
>> +#define PTR_RING_MAX_ALLOC 65536
>> +
> I guess it's an arbitrary number. Seems like a sufficiently large one,
> but pls add a comment so readers don't wonder. And please explain what
> it does:
>
> /* Callers can create ptr_ring structures with userspace-supplied
>   * parameters. This sets a limit on the size to make that usecase
>   * safe. If you ever change this, make sure to audit all callers.
>   */
>
> Also I think we should generally use either hex 0x10000 or (1 << 16).

I agree the number is arbitrary, so I still prefer the KMALLOC_MAX_SIZE 
especially consider it was used by pfifo_fast now. Try to limit it to an 
arbitrary may break lots of exist setups. E.g just google "txqueuelen 
100000" can give me a lots of search results.

We can do any kind of optimization on top but not for -net now.

Thanks

>
>>   /* Note: callers invoking this in a loop must use a compiler barrier,
>>    * for example cpu_relax().
>>    *
>> @@ -466,6 +468,8 @@ 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)
>>   {
>> +	if (size > PTR_RING_MAX_ALLOC)
>> +		return NULL;
>>   	return kvmalloc_array(size, sizeof(void *), gfp | __GFP_ZERO);
>>   }
>>   
>> -- 
>> 2.7.4

Re: [PATCH net V3 2/2] ptr_ring: fail on large queue size (>64K)

[Michael S. Tsirkin]

On Thu, Feb 08, 2018 at 03:11:22PM +0800, Jason Wang wrote:
> 
> 
> On 2018年02月08日 12:52, Michael S. Tsirkin wrote:
> > On Thu, Feb 08, 2018 at 11:59:25AM +0800, Jason Wang wrote:
> > > We need limit the maximum size of queue, otherwise it may cause
> > > several side effects e.g slab will warn when the size exceeds
> > > KMALLOC_MAX_SIZE. Using KMALLOC_MAX_SIZE still looks too so this patch
> > > tries to limit it to 64K. This value could be revisited if we found a
> > > real case that needs more.
> > > 
> > > Reported-by: syzbot+e4d4f9ddd4295539735d@syzkaller.appspotmail.com
> > > Fixes: 2e0ab8ca83c12 ("ptr_ring: array based FIFO for pointers")
> > > Signed-off-by: Jason Wang <jasowang@redhat.com>
> > > ---
> > >   include/linux/ptr_ring.h | 4 ++++
> > >   1 file changed, 4 insertions(+)
> > > 
> > > diff --git a/include/linux/ptr_ring.h b/include/linux/ptr_ring.h
> > > index 2af71a7..5858d48 100644
> > > --- a/include/linux/ptr_ring.h
> > > +++ b/include/linux/ptr_ring.h
> > > @@ -44,6 +44,8 @@ struct ptr_ring {
> > >   	void **queue;
> > >   };
> > Seems like a weird location for a define. Either put defines on
> > top of the file, or near where they are used. I prefer the
> > second option.
> 
> Ok.
> 
> > 
> > > +#define PTR_RING_MAX_ALLOC 65536
> > > +
> > I guess it's an arbitrary number. Seems like a sufficiently large one,
> > but pls add a comment so readers don't wonder. And please explain what
> > it does:
> > 
> > /* Callers can create ptr_ring structures with userspace-supplied
> >   * parameters. This sets a limit on the size to make that usecase
> >   * safe. If you ever change this, make sure to audit all callers.
> >   */
> > 
> > Also I think we should generally use either hex 0x10000 or (1 << 16).
> 
> I agree the number is arbitrary, so I still prefer the KMALLOC_MAX_SIZE
> especially consider it was used by pfifo_fast now. Try to limit it to an
> arbitrary may break lots of exist setups. E.g just google "txqueuelen
> 100000" can give me a lots of search results.
> 
> We can do any kind of optimization on top but not for -net now.
> 
> Thanks

Interesting. I have an idea for fixing this, but maybe
for now KMALLOC_MAX_SIZE does make sense. It's unfortunate that
this value is architecture dependent.

The patch still needs code comments though, and fix the math to
use the proper size.


> > 
> > >   /* Note: callers invoking this in a loop must use a compiler barrier,
> > >    * for example cpu_relax().
> > >    *
> > > @@ -466,6 +468,8 @@ 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)
> > >   {
> > > +	if (size > PTR_RING_MAX_ALLOC)
> > > +		return NULL;
> > >   	return kvmalloc_array(size, sizeof(void *), gfp | __GFP_ZERO);
> > >   }
> > > -- 
> > > 2.7.4

Re: [PATCH net V3 2/2] ptr_ring: fail on large queue size (>64K)

[David Miller]

From: Jason Wang <jasowang@redhat.com>
Date: Thu,  8 Feb 2018 11:59:25 +0800

> We need limit the maximum size of queue, otherwise it may cause
> several side effects e.g slab will warn when the size exceeds
> KMALLOC_MAX_SIZE. Using KMALLOC_MAX_SIZE still looks too so this patch
> tries to limit it to 64K. This value could be revisited if we found a
> real case that needs more.
> 
> Reported-by: syzbot+e4d4f9ddd4295539735d@syzkaller.appspotmail.com
> Fixes: 2e0ab8ca83c12 ("ptr_ring: array based FIFO for pointers")
> Signed-off-by: Jason Wang <jasowang@redhat.com>
 ...
> @@ -466,6 +468,8 @@ 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)
>  {
> +	if (size > PTR_RING_MAX_ALLOC)
> +		return NULL;
>  	return kvmalloc_array(size, sizeof(void *), gfp | __GFP_ZERO);
>  }

This doesn't limit the allocation to 64K.  It limits it to 64K * sizeof(void *).

Re: [PATCH net V3 1/2] ptr_ring: try vmalloc() when kmalloc() fails

[Michael S. Tsirkin]

On Thu, Feb 08, 2018 at 02:58:40PM +0800, Jason Wang wrote:
> 
> 
> On 2018年02月08日 12:45, Michael S. Tsirkin wrote:
> > On Thu, Feb 08, 2018 at 11:59:24AM +0800, Jason Wang wrote:
> > > This patch switch to use kvmalloc_array() for using a vmalloc()
> > > fallback to help in case kmalloc() fails.
> > > 
> > > Reported-by: syzbot+e4d4f9ddd4295539735d@syzkaller.appspotmail.com
> > > Fixes: 2e0ab8ca83c12 ("ptr_ring: array based FIFO for pointers")
> > I guess the actual patch is the one that switches tun to ptr_ring.
> 
> I think not, since the issue was large allocation.
> 
> > 
> > In fact, I think the actual bugfix is patch 2/2. This specific one
> > just makes kmalloc less likely to fail but that's
> > not what syzbot reported.
> 
> Agree.
> 
> > 
> > Then I would add this patch on top to make kmalloc less likely to fail.
> 
> Ok.
> 
> > > Signed-off-by: Jason Wang <jasowang@redhat.com>
> > 
> > 
> > > ---
> > >   include/linux/ptr_ring.h | 10 +++++-----
> > >   1 file changed, 5 insertions(+), 5 deletions(-)
> > > 
> > > diff --git a/include/linux/ptr_ring.h b/include/linux/ptr_ring.h
> > > index 1883d61..2af71a7 100644
> > > --- a/include/linux/ptr_ring.h
> > > +++ b/include/linux/ptr_ring.h
> > > @@ -466,7 +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)
> > >   {
> > > -	return kcalloc(size, sizeof(void *), gfp);
> > > +	return kvmalloc_array(size, sizeof(void *), gfp | __GFP_ZERO);
> > >   }
> > >   static inline void __ptr_ring_set_size(struct ptr_ring *r, int size)
> > This implies a bunch of limitations on the flags. From kvmalloc_node
> > docs:
> > 
> >   * Reclaim modifiers - __GFP_NORETRY and __GFP_NOFAIL are not supported.
> >   * __GFP_RETRY_MAYFAIL is supported, and it should be used only if kmalloc is
> >   * preferable to the vmalloc fallback, due to visible performance drawbacks.
> > 
> > Fine with all the current users, but if we go this way, please add
> > documentation so future users don't misuse this API.
> 
> I suspect this is somehow a overkill since this means we need sync with
> mm/vmalloc changes in the future to keep it synced.
> 
> > Alternatively, test flags and call kvmalloc or kcalloc?
> 
> Similar to the above issue, I would rather leave it as is.
> 
> Thanks

How do we prevent someone from inevitably trying to use this with
GFP_ATOMIC?

> > 
> > 
> > > @@ -601,7 +601,7 @@ static inline int ptr_ring_resize(struct ptr_ring *r, int size, gfp_t gfp,
> > >   	spin_unlock(&(r)->producer_lock);
> > >   	spin_unlock_irqrestore(&(r)->consumer_lock, flags);
> > > -	kfree(old);
> > > +	kvfree(old);
> > >   	return 0;
> > >   }
> > > @@ -641,7 +641,7 @@ static inline int ptr_ring_resize_multiple(struct ptr_ring **rings,
> > >   	}
> > >   	for (i = 0; i < nrings; ++i)
> > > -		kfree(queues[i]);
> > > +		kvfree(queues[i]);
> > >   	kfree(queues);
> > > @@ -649,7 +649,7 @@ static inline int ptr_ring_resize_multiple(struct ptr_ring **rings,
> > >   nomem:
> > >   	while (--i >= 0)
> > > -		kfree(queues[i]);
> > > +		kvfree(queues[i]);
> > >   	kfree(queues);
> > > @@ -664,7 +664,7 @@ static inline void ptr_ring_cleanup(struct ptr_ring *r, void (*destroy)(void *))
> > >   	if (destroy)
> > >   		while ((ptr = ptr_ring_consume(r)))
> > >   			destroy(ptr);
> > > -	kfree(r->queue);
> > > +	kvfree(r->queue);
> > >   }
> > >   #endif /* _LINUX_PTR_RING_H  */
> > > -- 
> > > 2.7.4

Re: [PATCH net V3 1/2] ptr_ring: try vmalloc() when kmalloc() fails

[Jason Wang]

On 2018年02月09日 03:17, Michael S. Tsirkin wrote:
> On Thu, Feb 08, 2018 at 02:58:40PM +0800, Jason Wang wrote:
>> On 2018年02月08日 12:45, Michael S. Tsirkin wrote:
>>> On Thu, Feb 08, 2018 at 11:59:24AM +0800, Jason Wang wrote:
>>>> This patch switch to use kvmalloc_array() for using a vmalloc()
>>>> fallback to help in case kmalloc() fails.
>>>>
>>>> Reported-by:syzbot+e4d4f9ddd4295539735d@syzkaller.appspotmail.com
>>>> Fixes: 2e0ab8ca83c12 ("ptr_ring: array based FIFO for pointers")
>>> I guess the actual patch is the one that switches tun to ptr_ring.
>> I think not, since the issue was large allocation.
>>
>>> In fact, I think the actual bugfix is patch 2/2. This specific one
>>> just makes kmalloc less likely to fail but that's
>>> not what syzbot reported.
>> Agree.
>>
>>> Then I would add this patch on top to make kmalloc less likely to fail.
>> Ok.
>>
>>>> Signed-off-by: Jason Wang<jasowang@redhat.com>
>>>> ---
>>>>    include/linux/ptr_ring.h | 10 +++++-----
>>>>    1 file changed, 5 insertions(+), 5 deletions(-)
>>>>
>>>> diff --git a/include/linux/ptr_ring.h b/include/linux/ptr_ring.h
>>>> index 1883d61..2af71a7 100644
>>>> --- a/include/linux/ptr_ring.h
>>>> +++ b/include/linux/ptr_ring.h
>>>> @@ -466,7 +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)
>>>>    {
>>>> -	return kcalloc(size, sizeof(void *), gfp);
>>>> +	return kvmalloc_array(size, sizeof(void *), gfp | __GFP_ZERO);
>>>>    }
>>>>    static inline void __ptr_ring_set_size(struct ptr_ring *r, int size)
>>> This implies a bunch of limitations on the flags. From kvmalloc_node
>>> docs:
>>>
>>>    * Reclaim modifiers - __GFP_NORETRY and __GFP_NOFAIL are not supported.
>>>    * __GFP_RETRY_MAYFAIL is supported, and it should be used only if kmalloc is
>>>    * preferable to the vmalloc fallback, due to visible performance drawbacks.
>>>
>>> Fine with all the current users, but if we go this way, please add
>>> documentation so future users don't misuse this API.
>> I suspect this is somehow a overkill since this means we need sync with
>> mm/vmalloc changes in the future to keep it synced.
>>
>>> Alternatively, test flags and call kvmalloc or kcalloc?
>> Similar to the above issue, I would rather leave it as is.
>>
>> Thanks
> How do we prevent someone from inevitably trying to use this with
> GFP_ATOMIC?
>

Well, we somehow can't prevent this even if there's a documentation, 
that's why there's a BUG() in vmalloc code I think. And kvmalloc also 
requires GFP_KERNEL otherewise another WARN().

So looks like the WARN()/BUG() should be sufficient?

Thanks

Another thing is kvm

Re: [PATCH net V3 2/2] ptr_ring: fail on large queue size (>64K)

[Jason Wang]

On 2018年02月08日 23:50, Michael S. Tsirkin wrote:
> On Thu, Feb 08, 2018 at 03:11:22PM +0800, Jason Wang wrote:
>> On 2018年02月08日 12:52, Michael S. Tsirkin wrote:
>>> On Thu, Feb 08, 2018 at 11:59:25AM +0800, Jason Wang wrote:
>>>> We need limit the maximum size of queue, otherwise it may cause
>>>> several side effects e.g slab will warn when the size exceeds
>>>> KMALLOC_MAX_SIZE. Using KMALLOC_MAX_SIZE still looks too so this patch
>>>> tries to limit it to 64K. This value could be revisited if we found a
>>>> real case that needs more.
>>>>
>>>> Reported-by:syzbot+e4d4f9ddd4295539735d@syzkaller.appspotmail.com
>>>> Fixes: 2e0ab8ca83c12 ("ptr_ring: array based FIFO for pointers")
>>>> Signed-off-by: Jason Wang<jasowang@redhat.com>
>>>> ---
>>>>    include/linux/ptr_ring.h | 4 ++++
>>>>    1 file changed, 4 insertions(+)
>>>>
>>>> diff --git a/include/linux/ptr_ring.h b/include/linux/ptr_ring.h
>>>> index 2af71a7..5858d48 100644
>>>> --- a/include/linux/ptr_ring.h
>>>> +++ b/include/linux/ptr_ring.h
>>>> @@ -44,6 +44,8 @@ struct ptr_ring {
>>>>    	void **queue;
>>>>    };
>>> Seems like a weird location for a define. Either put defines on
>>> top of the file, or near where they are used. I prefer the
>>> second option.
>> Ok.
>>
>>>> +#define PTR_RING_MAX_ALLOC 65536
>>>> +
>>> I guess it's an arbitrary number. Seems like a sufficiently large one,
>>> but pls add a comment so readers don't wonder. And please explain what
>>> it does:
>>>
>>> /* Callers can create ptr_ring structures with userspace-supplied
>>>    * parameters. This sets a limit on the size to make that usecase
>>>    * safe. If you ever change this, make sure to audit all callers.
>>>    */
>>>
>>> Also I think we should generally use either hex 0x10000 or (1 << 16).
>> I agree the number is arbitrary, so I still prefer the KMALLOC_MAX_SIZE
>> especially consider it was used by pfifo_fast now. Try to limit it to an
>> arbitrary may break lots of exist setups. E.g just google "txqueuelen
>> 100000" can give me a lots of search results.
>>
>> We can do any kind of optimization on top but not for -net now.
>>
>> Thanks
> Interesting. I have an idea for fixing this, but maybe
> for now KMALLOC_MAX_SIZE does make sense. It's unfortunate that
> this value is architecture dependent.
>
> The patch still needs code comments though, and fix the math to
> use the proper size.
>

Yes.

Thanks

Re: [PATCH net V3 2/2] ptr_ring: fail on large queue size (>64K)

[Jason Wang]

On 2018年02月09日 03:09, David Miller wrote:
> From: Jason Wang <jasowang@redhat.com>
> Date: Thu,  8 Feb 2018 11:59:25 +0800
>
>> We need limit the maximum size of queue, otherwise it may cause
>> several side effects e.g slab will warn when the size exceeds
>> KMALLOC_MAX_SIZE. Using KMALLOC_MAX_SIZE still looks too so this patch
>> tries to limit it to 64K. This value could be revisited if we found a
>> real case that needs more.
>>
>> Reported-by: syzbot+e4d4f9ddd4295539735d@syzkaller.appspotmail.com
>> Fixes: 2e0ab8ca83c12 ("ptr_ring: array based FIFO for pointers")
>> Signed-off-by: Jason Wang <jasowang@redhat.com>
>   ...
>> @@ -466,6 +468,8 @@ 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)
>>   {
>> +	if (size > PTR_RING_MAX_ALLOC)
>> +		return NULL;
>>   	return kvmalloc_array(size, sizeof(void *), gfp | __GFP_ZERO);
>>   }
> This doesn't limit the allocation to 64K.  It limits it to 64K * sizeof(void *).

Right, will fix this.

Thanks

Re: [PATCH net V3 1/2] ptr_ring: try vmalloc() when kmalloc() fails

[Michael S. Tsirkin]

On Fri, Feb 09, 2018 at 11:49:12AM +0800, Jason Wang wrote:
> 
> 
> On 2018年02月09日 03:17, Michael S. Tsirkin wrote:
> > On Thu, Feb 08, 2018 at 02:58:40PM +0800, Jason Wang wrote:
> > > On 2018年02月08日 12:45, Michael S. Tsirkin wrote:
> > > > On Thu, Feb 08, 2018 at 11:59:24AM +0800, Jason Wang wrote:
> > > > > This patch switch to use kvmalloc_array() for using a vmalloc()
> > > > > fallback to help in case kmalloc() fails.
> > > > > 
> > > > > Reported-by:syzbot+e4d4f9ddd4295539735d@syzkaller.appspotmail.com
> > > > > Fixes: 2e0ab8ca83c12 ("ptr_ring: array based FIFO for pointers")
> > > > I guess the actual patch is the one that switches tun to ptr_ring.
> > > I think not, since the issue was large allocation.
> > > 
> > > > In fact, I think the actual bugfix is patch 2/2. This specific one
> > > > just makes kmalloc less likely to fail but that's
> > > > not what syzbot reported.
> > > Agree.
> > > 
> > > > Then I would add this patch on top to make kmalloc less likely to fail.
> > > Ok.
> > > 
> > > > > Signed-off-by: Jason Wang<jasowang@redhat.com>
> > > > > ---
> > > > >    include/linux/ptr_ring.h | 10 +++++-----
> > > > >    1 file changed, 5 insertions(+), 5 deletions(-)
> > > > > 
> > > > > diff --git a/include/linux/ptr_ring.h b/include/linux/ptr_ring.h
> > > > > index 1883d61..2af71a7 100644
> > > > > --- a/include/linux/ptr_ring.h
> > > > > +++ b/include/linux/ptr_ring.h
> > > > > @@ -466,7 +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)
> > > > >    {
> > > > > -	return kcalloc(size, sizeof(void *), gfp);
> > > > > +	return kvmalloc_array(size, sizeof(void *), gfp | __GFP_ZERO);
> > > > >    }
> > > > >    static inline void __ptr_ring_set_size(struct ptr_ring *r, int size)
> > > > This implies a bunch of limitations on the flags. From kvmalloc_node
> > > > docs:
> > > > 
> > > >    * Reclaim modifiers - __GFP_NORETRY and __GFP_NOFAIL are not supported.
> > > >    * __GFP_RETRY_MAYFAIL is supported, and it should be used only if kmalloc is
> > > >    * preferable to the vmalloc fallback, due to visible performance drawbacks.
> > > > 
> > > > Fine with all the current users, but if we go this way, please add
> > > > documentation so future users don't misuse this API.
> > > I suspect this is somehow a overkill since this means we need sync with
> > > mm/vmalloc changes in the future to keep it synced.
> > > 
> > > > Alternatively, test flags and call kvmalloc or kcalloc?
> > > Similar to the above issue, I would rather leave it as is.
> > > 
> > > Thanks
> > How do we prevent someone from inevitably trying to use this with
> > GFP_ATOMIC?
> > 
> 
> Well, we somehow can't prevent this even if there's a documentation, that's
> why there's a BUG() in vmalloc code I think. And kvmalloc also requires
> GFP_KERNEL otherewise another WARN().
> 
> So looks like the WARN()/BUG() should be sufficient?

Well vmalloc only triggers when you pass in a huge size.
Let's settle for

/* Not all gfp_t flags (besides GFP_KERNEL) are allowed. See
 * documentation for vmalloc for which of them are legal.
 */

> Thanks
> 
> Another thing is kvm

?

Re: [PATCH net V3 1/2] ptr_ring: try vmalloc() when kmalloc() fails

[Jason Wang]

On 2018年02月09日 11:56, Michael S. Tsirkin wrote:
> On Fri, Feb 09, 2018 at 11:49:12AM +0800, Jason Wang wrote:
>>
>> On 2018年02月09日 03:17, Michael S. Tsirkin wrote:
>>> On Thu, Feb 08, 2018 at 02:58:40PM +0800, Jason Wang wrote:
>>>> On 2018年02月08日 12:45, Michael S. Tsirkin wrote:
>>>>> On Thu, Feb 08, 2018 at 11:59:24AM +0800, Jason Wang wrote:
>>>>>> This patch switch to use kvmalloc_array() for using a vmalloc()
>>>>>> fallback to help in case kmalloc() fails.
>>>>>>
>>>>>> Reported-by:syzbot+e4d4f9ddd4295539735d@syzkaller.appspotmail.com
>>>>>> Fixes: 2e0ab8ca83c12 ("ptr_ring: array based FIFO for pointers")
>>>>> I guess the actual patch is the one that switches tun to ptr_ring.
>>>> I think not, since the issue was large allocation.
>>>>
>>>>> In fact, I think the actual bugfix is patch 2/2. This specific one
>>>>> just makes kmalloc less likely to fail but that's
>>>>> not what syzbot reported.
>>>> Agree.
>>>>
>>>>> Then I would add this patch on top to make kmalloc less likely to fail.
>>>> Ok.
>>>>
>>>>>> Signed-off-by: Jason Wang<jasowang@redhat.com>
>>>>>> ---
>>>>>>     include/linux/ptr_ring.h | 10 +++++-----
>>>>>>     1 file changed, 5 insertions(+), 5 deletions(-)
>>>>>>
>>>>>> diff --git a/include/linux/ptr_ring.h b/include/linux/ptr_ring.h
>>>>>> index 1883d61..2af71a7 100644
>>>>>> --- a/include/linux/ptr_ring.h
>>>>>> +++ b/include/linux/ptr_ring.h
>>>>>> @@ -466,7 +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)
>>>>>>     {
>>>>>> -	return kcalloc(size, sizeof(void *), gfp);
>>>>>> +	return kvmalloc_array(size, sizeof(void *), gfp | __GFP_ZERO);
>>>>>>     }
>>>>>>     static inline void __ptr_ring_set_size(struct ptr_ring *r, int size)
>>>>> This implies a bunch of limitations on the flags. From kvmalloc_node
>>>>> docs:
>>>>>
>>>>>     * Reclaim modifiers - __GFP_NORETRY and __GFP_NOFAIL are not supported.
>>>>>     * __GFP_RETRY_MAYFAIL is supported, and it should be used only if kmalloc is
>>>>>     * preferable to the vmalloc fallback, due to visible performance drawbacks.
>>>>>
>>>>> Fine with all the current users, but if we go this way, please add
>>>>> documentation so future users don't misuse this API.
>>>> I suspect this is somehow a overkill since this means we need sync with
>>>> mm/vmalloc changes in the future to keep it synced.
>>>>
>>>>> Alternatively, test flags and call kvmalloc or kcalloc?
>>>> Similar to the above issue, I would rather leave it as is.
>>>>
>>>> Thanks
>>> How do we prevent someone from inevitably trying to use this with
>>> GFP_ATOMIC?
>>>
>> Well, we somehow can't prevent this even if there's a documentation, that's
>> why there's a BUG() in vmalloc code I think. And kvmalloc also requires
>> GFP_KERNEL otherewise another WARN().
>>
>> So looks like the WARN()/BUG() should be sufficient?
> Well vmalloc only triggers when you pass in a huge size.
> Let's settle for

There's a:

     BUG_ON(in_interrupt());

in __get_vm_area_node().

>
> /* Not all gfp_t flags (besides GFP_KERNEL) are allowed. See
>   * documentation for vmalloc for which of them are legal.
>   */

Fine with me.

>> Thanks
>>
>> Another thing is kvm
> ?

Sorry typo.

Thanks