On Tue, Dec 21, 2021 at 9:32 AM Loftus, Ciara <ciara.loftus@xxxxxxxxx> wrote:
>
> > On Mon, Dec 20, 2021 at 9:10 PM Ciara Loftus <ciara.loftus@xxxxxxxxx>
> > wrote:
> > >
> > > This commit initialises the xskb's free_list_node when the xskb is
> > > allocated. This prevents a potential false negative returned from a call
> > > to list_empty for that node, such as the one introduced in commit
> > > 199d983bc015 ("xsk: Fix crash on double free in buffer pool")
> > >
> > > In my environment this issue caused packets to not be received by
> > > the xdpsock application if the traffic was running prior to application
> > > launch. This happened when the first batch of packets failed the xskmap
> > > lookup and XDP_PASS was returned from the bpf program. This action is
> > > handled in the i40e zc driver (and others) by allocating an skbuff,
> > > freeing the xdp_buff and adding the associated xskb to the
> > > xsk_buff_pool's free_list if it hadn't been added already. Without this
> > > fix, the xskb is not added to the free_list because the check to determine
> > > if it was added already returns an invalid positive result. Later, this
> > > caused allocation errors in the driver and the failure to receive packets.
> >
> > Thank you for this fix Ciara! Though I do think the Fixes tag should
> > be the one above: 199d983bc015 ("xsk: Fix crash on double free in
> > buffer pool"). Before that commit, there was no test for an empty list
> > in the xp_free path. The entry was unconditionally put on the list and
> > "initialized" in that way, so that code will work without this patch.
> > What do you think?
>
> Agree - that makes sense.
> Can the fixes tag be updated when pulled into the tree with:
> Fixes: 199d983bc015 ("xsk: Fix crash on double free in buffer pool")
On the other hand, this was a fix for 2b43470add8c ("xsk: Introduce
AF_XDP buffer allocation API"), the original tag you have in your
patch. What should the Fixes tag point to in this case? Need some
advice please.
> If I need to submit a v2 with the change please let me know.
>
> Thanks,
> Ciara
>
> >
> > Acked-by: Magnus Karlsson <magnus.karlsson@xxxxxxxxx>
> >
> > > Fixes: 2b43470add8c ("xsk: Introduce AF_XDP buffer allocation API")
> > >
> > > Signed-off-by: Ciara Loftus <ciara.loftus@xxxxxxxxx>
> > > ---
> > > net/xdp/xsk_buff_pool.c | 1 +
> > > 1 file changed, 1 insertion(+)
> > >
> > > diff --git a/net/xdp/xsk_buff_pool.c b/net/xdp/xsk_buff_pool.c
> > > index bc4ad48ea4f0..fd39bb660ebc 100644
> > > --- a/net/xdp/xsk_buff_pool.c
> > > +++ b/net/xdp/xsk_buff_pool.c
> > > @@ -83,6 +83,7 @@ struct xsk_buff_pool
> > *xp_create_and_assign_umem(struct xdp_sock *xs,
> > > xskb = &pool->heads[i];
> > > xskb->pool = pool;
> > > xskb->xdp.frame_sz = umem->chunk_size - umem->headroom;
> > > + INIT_LIST_HEAD(&xskb->free_list_node);
> > > if (pool->unaligned)
> > > pool->free_heads[i] = xskb;
> > > else
> > > --
> > > 2.17.1
> > >
