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

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