[PATCH bpf-next] xdp: move the rxq_info.mem clearing to unreg_mem_model()

[PATCH bpf-next] xdp: move the rxq_info.mem clearing to unreg_mem_model()

[Jakub Kicinski]

xdp_rxq_info_unreg() implicitly calls xdp_rxq_info_unreg_mem_model().
This may well be confusing to the driver authors, and lead to double
free if they call xdp_rxq_info_unreg_mem_model() before
xdp_rxq_info_unreg() (when mem model type == MEM_TYPE_PAGE_POOL).

In fact error path of mvpp2_rxq_init() seems to currently do
exactly that.

The double free will result in refcount underflow
in page_pool_destroy(). Make the interface a little more
programmer friendly by clearing type and id so that
xdp_rxq_info_unreg_mem_model() can be called multiple
times.

Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
Targeting bpf-next, the mvpp2 bug (if it's real) can be addressed
separately. IMHO it may be a little late in the cycle for
"API convenience" changes but I tested against bpf as well.

 net/core/xdp.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/net/core/xdp.c b/net/core/xdp.c
index 858276e72c68..6ea3302c42eb 100644
--- a/net/core/xdp.c
+++ b/net/core/xdp.c
@@ -113,8 +113,13 @@ static void mem_allocator_disconnect(void *allocator)
 void xdp_rxq_info_unreg_mem_model(struct xdp_rxq_info *xdp_rxq)
 {
 	struct xdp_mem_allocator *xa;
+	int type = xdp_rxq->mem.type;
 	int id = xdp_rxq->mem.id;
 
+	/* Reset mem info to defaults */
+	xdp_rxq->mem.id = 0;
+	xdp_rxq->mem.type = 0;
+
 	if (xdp_rxq->reg_state != REG_STATE_REGISTERED) {
 		WARN(1, "Missing register, driver bug");
 		return;
@@ -123,7 +128,7 @@ void xdp_rxq_info_unreg_mem_model(struct xdp_rxq_info *xdp_rxq)
 	if (id == 0)
 		return;
 
-	if (xdp_rxq->mem.type == MEM_TYPE_PAGE_POOL) {
+	if (type == MEM_TYPE_PAGE_POOL) {
 		rcu_read_lock();
 		xa = rhashtable_lookup(mem_id_ht, &id, mem_id_rht_params);
 		page_pool_destroy(xa->page_pool);
@@ -144,10 +149,6 @@ void xdp_rxq_info_unreg(struct xdp_rxq_info *xdp_rxq)
 
 	xdp_rxq->reg_state = REG_STATE_UNREGISTERED;
 	xdp_rxq->dev = NULL;
-
-	/* Reset mem info to defaults */
-	xdp_rxq->mem.id = 0;
-	xdp_rxq->mem.type = 0;
 }
 EXPORT_SYMBOL_GPL(xdp_rxq_info_unreg);
 
-- 
2.31.1

Re: [PATCH bpf-next] xdp: move the rxq_info.mem clearing to unreg_mem_model()

[patchwork-bot+netdevbpf]

Hello:

This patch was applied to bpf/bpf-next.git (refs/heads/master):

On Fri, 25 Jun 2021 15:16:12 -0700 you wrote:
> xdp_rxq_info_unreg() implicitly calls xdp_rxq_info_unreg_mem_model().
> This may well be confusing to the driver authors, and lead to double
> free if they call xdp_rxq_info_unreg_mem_model() before
> xdp_rxq_info_unreg() (when mem model type == MEM_TYPE_PAGE_POOL).
> 
> In fact error path of mvpp2_rxq_init() seems to currently do
> exactly that.
> 
> [...]

Here is the summary with links:
  - [bpf-next] xdp: move the rxq_info.mem clearing to unreg_mem_model()
    https://git.kernel.org/bpf/bpf-next/c/a78cae247681

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