Re: [PATCH bpf v3] net/xdp: fix zero-size allocation warning in xskq_create()

From: Martin KaFai Lau <martin.lau@linux.dev>
To: Andrew Kanner <andrew.kanner@gmail.com>
Cc: linux-kernel-mentees@lists.linuxfoundation.org,
	netdev@vger.kernel.org, bpf@vger.kernel.org,
	linux-kernel@vger.kernel.org,
	syzbot+fae676d3cf469331fc89@syzkaller.appspotmail.com,
	syzbot+b132693e925cbbd89e26@syzkaller.appspotmail.com,
	bjorn@kernel.org, magnus.karlsson@intel.com,
	maciej.fijalkowski@intel.com, jonathan.lemon@gmail.com,
	davem@davemloft.net, edumazet@google.com, kuba@kernel.org,
	pabeni@redhat.com, aleksander.lobakin@intel.com,
	xuanzhuo@linux.alibaba.com, ast@kernel.org, hawk@kernel.org,
	john.fastabend@gmail.com, daniel@iogearbox.net
Subject: Re: [PATCH bpf v3] net/xdp: fix zero-size allocation warning in xskq_create()
Date: Fri, 6 Oct 2023 10:37:44 -0700	[thread overview]
Message-ID: <57c35480-983d-2056-1d72-f6e555069b83@linux.dev> (raw)
In-Reply-To: <651fb2a8.c20a0220.8d6c3.0fd9@mx.google.com>

On 10/6/23 12:09 AM, Andrew Kanner wrote:
> On Thu, Oct 05, 2023 at 06:00:46PM -0700, Martin KaFai Lau wrote:
> [...]
>>> diff --git a/net/xdp/xsk_queue.c b/net/xdp/xsk_queue.c
>>> index f8905400ee07..c7e8bbb12752 100644
>>> --- a/net/xdp/xsk_queue.c
>>> +++ b/net/xdp/xsk_queue.c
>>> @@ -34,6 +34,11 @@ struct xsk_queue *xskq_create(u32 nentries, bool umem_queue)
>>>    	q->ring_mask = nentries - 1;
>>>    	size = xskq_get_ring_size(q, umem_queue);
>>> +	if (unlikely(size == SIZE_MAX)) {
>>
>> What if "size" is SIZE_MAX-1? Would it still overflow the PAGE_ALIGN below?
>>
>>> +		kfree(q);
>>> +		return NULL;
>>> +	}
>>> +
>>>    	size = PAGE_ALIGN(size);
>>>    	q->ring = vmalloc_user(size);
>>
> 
> I asked myself the same question before v1. E.g. thinking about the
> check: (size > SIZE_MAX - PAGE_SIZE + 1)
> 
> But xskq_create() is called after the check for
> !is_power_of_2(entries) in xsk_init_queue(). So I tried the same
> reproducer and divided the (nentries) value by 2 in a loop - it hits
> either SIZE_MAX case or the normal cases without overflow (sometimes
> throwing vmalloc error complaining about size which exceed total pages
> in my arm setup).
> 
> So I can't see a way size will be SIZE_MAX-1, etc. Correct me if I'm
> wrong, please.
> 
> PS: In the output below the first 2 values of (nentries) hit SIZE_MAX

Thanks for the explanation, so iiuc it means it will overflow the struct_size() 
first because of the is_power_of_2(nentries) requirement? Could you help adding 
some comment to explain? Thanks.

> case, the rest hit the normal case, vmalloc_user() is complaining
> about 1 allocation:
> 
> 0x20000000
> 0x10000000
> 0x8000000
> [   41.759195][ T2807] pre PAGE_ALIGN size = 2147483968 (0x80000140), PAGE_SIZE = 4096 (0x1000)
> [   41.759621][ T2807] repro-iter: vmalloc error: size 2147487744, exceeds total pages, mode:0xdc0(GFP_KERNEL|__GFP_ZERO), nodemask=(null),cpuset=/,mems_allowed=0
> [...]
> 0x4000000
> 0x2000000
> 0x1000000
> 0x800000
> 0x400000
> 0x200000
> 0x100000
> 0x80000
> 0x40000
> 0x20000
> 0x10000
> 0x8000
> 0x4000
> 0x2000
> 0x1000
> 0x800
> 0x400
> 0x200
> 0x100
> 0x80
> 0x40
> 0x20
> 0x10
> 0x8
> 0x4
> 0x2
> 