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

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 <redacted>
---
 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

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 <redacted>
---
 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

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 <redacted>

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?

On Thu, Feb 08, 2018 at 11:59:25AM +0800, Jason Wang wrote:

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).

On 2018年02月08日 12:45, Michael S. Tsirkin wrote:

On Thu, Feb 08, 2018 at 11:59:24AM +0800, Jason Wang wrote:

quoted

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.

quoted

Signed-off-by: Jason Wang <redacted>

quoted

---
  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

quoted

@@ -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

On 2018年02月08日 12:52, Michael S. Tsirkin wrote:

On Thu, Feb 08, 2018 at 11:59:25AM +0800, Jason Wang wrote:

quoted

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 <redacted>
---
  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.

quoted

+#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

quoted

  /* 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

On Thu, Feb 08, 2018 at 03:11:22PM +0800, Jason Wang wrote:

On 2018年02月08日 12:52, Michael S. Tsirkin wrote:

quoted

On Thu, Feb 08, 2018 at 11:59:25AM +0800, Jason Wang wrote:

quoted

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 <redacted>
---
  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.

quoted

quoted

+#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.

quoted

quoted

  /* 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

From: Jason Wang <redacted>
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 <redacted>

 ...

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

On Thu, Feb 08, 2018 at 02:58:40PM +0800, Jason Wang wrote:

On 2018年02月08日 12:45, Michael S. Tsirkin wrote:

quoted

On Thu, Feb 08, 2018 at 11:59:24AM +0800, Jason Wang wrote:

quoted

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.

quoted

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.

quoted

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

Ok.

quoted

quoted

Signed-off-by: Jason Wang <redacted>

quoted

---
  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.

quoted

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?

quoted

quoted

@@ -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

On 2018年02月09日 03:17, Michael S. Tsirkin wrote:

On Thu, Feb 08, 2018 at 02:58:40PM +0800, Jason Wang wrote:

quoted

On 2018年02月08日 12:45, Michael S. Tsirkin wrote:

quoted

On Thu, Feb 08, 2018 at 11:59:24AM +0800, Jason Wang wrote:

quoted

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.

quoted

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.

quoted

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

Ok.

quoted

quoted

Signed-off-by: Jason Wang<redacted>
---
   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.

quoted

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

On 2018年02月08日 23:50, Michael S. Tsirkin wrote:

On Thu, Feb 08, 2018 at 03:11:22PM +0800, Jason Wang wrote:

quoted

On 2018年02月08日 12:52, Michael S. Tsirkin wrote:

quoted

On Thu, Feb 08, 2018 at 11:59:25AM +0800, Jason Wang wrote:

quoted

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<redacted>
---
   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.

quoted

quoted

+#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

On 2018年02月09日 03:09, David Miller wrote:

From: Jason Wang <redacted>
Date: Thu,  8 Feb 2018 11:59:25 +0800

quoted

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 <redacted>

  ...

quoted

@@ -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

On Fri, Feb 09, 2018 at 11:49:12AM +0800, Jason Wang wrote:

On 2018年02月09日 03:17, Michael S. Tsirkin wrote:

quoted

On Thu, Feb 08, 2018 at 02:58:40PM +0800, Jason Wang wrote:

quoted

On 2018年02月08日 12:45, Michael S. Tsirkin wrote:

quoted

On Thu, Feb 08, 2018 at 11:59:24AM +0800, Jason Wang wrote:

quoted

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.

quoted

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.

quoted

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

Ok.

quoted

quoted

Signed-off-by: Jason Wang<redacted>
---
   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.

quoted

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

?

On 2018年02月09日 11:56, Michael S. Tsirkin wrote:

On Fri, Feb 09, 2018 at 11:49:12AM +0800, Jason Wang wrote:

quoted

On 2018年02月09日 03:17, Michael S. Tsirkin wrote:

quoted

On Thu, Feb 08, 2018 at 02:58:40PM +0800, Jason Wang wrote:

quoted

On 2018年02月08日 12:45, Michael S. Tsirkin wrote:

quoted

On Thu, Feb 08, 2018 at 11:59:24AM +0800, Jason Wang wrote:

quoted

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.

quoted

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.

quoted

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

Ok.

quoted

quoted

Signed-off-by: Jason Wang<redacted>
---
    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.

quoted

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.

quoted

Thanks

Another thing is kvm

?

Sorry typo.

Thanks