Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:1 as permitted sender) client-ip=2620:137:e000::3:1;
Message-ID: <651fb2a8.c20a0220.8d6c3.0fd9@mx.google.com>
Date:   Fri, 6 Oct 2023 10:09:20 +0300
From:   Andrew Kanner <andrew.kanner@gmail.com>
To:     Martin KaFai Lau <martin.lau@linux.dev>
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()
References: <20231005193548.515-1-andrew.kanner@gmail.com>
 <7aa47549-5a95-22d7-1d03-ffdd251cec6d@linux.dev>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <7aa47549-5a95-22d7-1d03-ffdd251cec6d@linux.dev>
Precedence: bulk

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

-- 
Andrew Kanner