[PATCH bpf] xsk: Initialise xskb free_list_node

[PATCH bpf] xsk: Initialise xskb free_list_node

[Ciara Loftus]

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.

Fixes: 2b43470add8c ("xsk: Introduce AF_XDP buffer allocation API")

Signed-off-by: Ciara Loftus <ciara.loftus@intel.com>
---
 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

Re: [PATCH bpf] xsk: Initialise xskb free_list_node

[Magnus Karlsson]

On Mon, Dec 20, 2021 at 9:10 PM Ciara Loftus <ciara.loftus@intel.com> 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?

Acked-by: Magnus Karlsson <magnus.karlsson@intel.com>

> Fixes: 2b43470add8c ("xsk: Introduce AF_XDP buffer allocation API")
>
> Signed-off-by: Ciara Loftus <ciara.loftus@intel.com>
> ---
>  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
>

RE: [PATCH bpf] xsk: Initialise xskb free_list_node

[Loftus, Ciara]

> On Mon, Dec 20, 2021 at 9:10 PM Ciara Loftus <ciara.loftus@intel.com>
> 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")

If I need to submit a v2 with the change please let me know.

Thanks,
Ciara

> 
> Acked-by: Magnus Karlsson <magnus.karlsson@intel.com>
> 
> > Fixes: 2b43470add8c ("xsk: Introduce AF_XDP buffer allocation API")
> >
> > Signed-off-by: Ciara Loftus <ciara.loftus@intel.com>
> > ---
> >  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
> >

Re: [PATCH bpf] xsk: Initialise xskb free_list_node

[Magnus Karlsson]

On Tue, Dec 21, 2021 at 9:32 AM Loftus, Ciara <ciara.loftus@intel.com> wrote:
>
> > On Mon, Dec 20, 2021 at 9:10 PM Ciara Loftus <ciara.loftus@intel.com>
> > 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@intel.com>
> >
> > > Fixes: 2b43470add8c ("xsk: Introduce AF_XDP buffer allocation API")
> > >
> > > Signed-off-by: Ciara Loftus <ciara.loftus@intel.com>
> > > ---
> > >  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
> > >

Re: [PATCH bpf] xsk: Initialise xskb free_list_node

[Jakub Kicinski]

On Tue, 21 Dec 2021 10:00:13 +0100 Magnus Karlsson wrote:
> On Tue, Dec 21, 2021 at 9:32 AM Loftus, Ciara <ciara.loftus@intel.com> wrote:
> > > 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.

My $0.02 would be that if all relevant commits form a chain of fixes
it doesn't matter much which one you put in the tag. To me your
suggestion of going with 199d983bc015 makes most sense since from a
cursory look the direct issue doesn't really exist without that commit.

Plus we probably don't want 199d983bc015 to be backported until we
apply this fix, so it'd be good if "Fixes: 199d983bc015" appeared in
linux-next.

You can always put multiple Fixes tags on the commit, if you're unsure.

Re: [PATCH bpf] xsk: Initialise xskb free_list_node

[Alexei Starovoitov]

On Tue, Dec 21, 2021 at 12:21 PM Jakub Kicinski <kuba@kernel.org> wrote:
>
> On Tue, 21 Dec 2021 10:00:13 +0100 Magnus Karlsson wrote:
> > On Tue, Dec 21, 2021 at 9:32 AM Loftus, Ciara <ciara.loftus@intel.com> wrote:
> > > > 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.
>
> My $0.02 would be that if all relevant commits form a chain of fixes
> it doesn't matter much which one you put in the tag. To me your
> suggestion of going with 199d983bc015 makes most sense since from a
> cursory look the direct issue doesn't really exist without that commit.
>
> Plus we probably don't want 199d983bc015 to be backported until we
> apply this fix, so it'd be good if "Fixes: 199d983bc015" appeared in
> linux-next.
>
> You can always put multiple Fixes tags on the commit, if you're unsure.

It sounds that the fix should get into net and linus tree asap?
In such a case mabe take it into net directly?

Re: [PATCH bpf] xsk: Initialise xskb free_list_node

[Jakub Kicinski]

On Tue, 28 Dec 2021 19:04:10 -0800 Alexei Starovoitov wrote:
> > My $0.02 would be that if all relevant commits form a chain of fixes
> > it doesn't matter much which one you put in the tag. To me your
> > suggestion of going with 199d983bc015 makes most sense since from a
> > cursory look the direct issue doesn't really exist without that commit.
> >
> > Plus we probably don't want 199d983bc015 to be backported until we
> > apply this fix, so it'd be good if "Fixes: 199d983bc015" appeared in
> > linux-next.
> >
> > You can always put multiple Fixes tags on the commit, if you're unsure.  
> 
> It sounds that the fix should get into net and linus tree asap?
> In such a case mabe take it into net directly?

Ack

Re: [PATCH bpf] xsk: Initialise xskb free_list_node

[patchwork-bot+netdevbpf]

Hello:

This patch was applied to netdev/net.git (master)
by Jakub Kicinski <kuba@kernel.org>:

On Mon, 20 Dec 2021 15:52:50 +0000 you 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.
> 
> [...]

Here is the summary with links:
  - [bpf] xsk: Initialise xskb free_list_node
    https://git.kernel.org/netdev/net/c/5bec7ca2be69

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html