[PATCH bpf] xsk: Initialise xskb free_list_node

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

On Mon, Dec 20, 2021 at 9:10 PM Ciara Loftus [off-list ref] 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>

On Mon, Dec 20, 2021 at 9:10 PM Ciara Loftus [off-list ref]
wrote:

quoted

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>

quoted

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

Signed-off-by: Ciara Loftus <redacted>
---
 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,

quoted

                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

On Tue, Dec 21, 2021 at 9:32 AM Loftus, Ciara [off-list ref] wrote:

quoted

On Mon, Dec 20, 2021 at 9:10 PM Ciara Loftus [off-list ref]
wrote:

quoted

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

quoted

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

quoted

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

Signed-off-by: Ciara Loftus <redacted>
---
 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,

quoted

                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

On Tue, 21 Dec 2021 10:00:13 +0100 Magnus Karlsson wrote:

On Tue, Dec 21, 2021 at 9:32 AM Loftus, Ciara [off-list ref] wrote:

quoted

quoted

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.

On Tue, Dec 21, 2021 at 12:21 PM Jakub Kicinski [off-list ref] wrote:

On Tue, 21 Dec 2021 10:00:13 +0100 Magnus Karlsson wrote:

quoted

On Tue, Dec 21, 2021 at 9:32 AM Loftus, Ciara [off-list ref] wrote:

quoted

quoted

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?

On Tue, 28 Dec 2021 19:04:10 -0800 Alexei Starovoitov wrote:

quoted

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

Hello:

This patch was applied to netdev/net.git (master)
by Jakub Kicinski [off-list ref]:

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