Re: [PATCH] [RFC PATCH] bpf/xdp: propagate queue_index via xdp_frame for cpumap

[bot+bpf-ci@kernel.org]

[-- Attachment #1: Type: text/plain, Size: 4230 bytes --]

> diff --git a/include/net/xdp.h b/include/net/xdp.h
> index aa742f413c35..11aecf84a896 100644
> --- a/include/net/xdp.h
> +++ b/include/net/xdp.h

[ ... ]

> @@ -392,6 +394,7 @@ void xdp_convert_frame_to_buff(const struct xdp_frame *frame,
>  	xdp->data = frame->data;
>  	xdp->data_end = frame->data + frame->len;
>  	xdp->data_meta = frame->data - frame->metasize;
> +	xdp->rxq->queue_index = frame->queue_index;
>  	xdp->frame_sz = frame->frame_sz;
>  	xdp->flags = frame->flags;
>  }

This new line dereferences xdp->rxq, but several callers of
xdp_convert_frame_to_buff() do not set xdp->rxq before calling it.

In veth_xdp_rcv_one():

    struct veth_xdp_buff vxbuf;
    struct xdp_buff *xdp = &vxbuf.xdp;

    xdp_convert_frame_to_buff(frame, xdp);   /* rxq is garbage here */
    xdp->rxq = &rq->xdp_rxq;                /* set too late */

In dev_map_bpf_prog_run():

    struct xdp_buff xdp;

    xdp_convert_frame_to_buff(xdpf, &xdp);   /* rxq is garbage here */
    xdp.txq = &txq;
    xdp.rxq = &rxq;                          /* set too late */

In bond_xdp_xmit():

    struct xdp_buff xdp;

    xdp_convert_frame_to_buff(frame, &xdp);   /* rxq is garbage here */
    /* rxq is never set */

In all three cases, xdp_buff is stack-allocated without initialization,
so xdp->rxq contains whatever was on the stack.  Writing to
xdp->rxq->queue_index then corrupts an arbitrary kernel address.

Would it be better to move the queue_index restore out of
xdp_convert_frame_to_buff() and into the callers that actually need it
(i.e. cpu_map_bpf_prog_run_xdp), or alternatively update all callers
to set xdp->rxq before calling this function?

> @@ -419,6 +422,7 @@ int xdp_update_frame_from_buff(const struct xdp_buff *xdp,
>  	xdp_frame->len  = xdp->data_end - xdp->data;
>  	xdp_frame->headroom = headroom - sizeof(*xdp_frame);
>  	xdp_frame->metasize = metasize;
> +	xdp_frame->queue_index = xdp->rxq->queue_index;
>  	xdp_frame->frame_sz = xdp->frame_sz;
>  	xdp_frame->flags = xdp->flags;

Same concern here: xdp_update_frame_from_buff() now also dereferences
xdp->rxq.  While callers that reach this point after surviving the
xdp_convert_frame_to_buff() crash would have rxq set by then, this
creates a hidden precondition that xdp->rxq must be valid, which was
not previously required by either function.

> diff --git a/kernel/bpf/cpumap.c b/kernel/bpf/cpumap.c
> index 5e59ab896f05..448da572de9a 100644
> --- a/kernel/bpf/cpumap.c
> +++ b/kernel/bpf/cpumap.c
> @@ -197,7 +197,7 @@ static int cpu_map_bpf_prog_run_xdp(struct bpf_cpu_map_entry *rcpu,
>
>  		rxq.dev = xdpf->dev_rx;
>  		rxq.mem.type = xdpf->mem_type;
> -		/* TODO: report queue_index to xdp_rxq_info */
> +		rxq.queue_index = xdpf->queue_index;
>
>  		xdp_convert_frame_to_buff(xdpf, &xdp);

This is the one caller where xdp.rxq is set before calling
xdp_convert_frame_to_buff(), so it works correctly here.  However, the
explicit rxq.queue_index assignment above is now redundant with the
assignment inside xdp_convert_frame_to_buff().

> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index 84699a428077..83566d2f446e 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -19098,12 +19098,16 @@ static struct bpf_iarray *jt_from_subprog(struct bpf_verifier_env *env,
>  	int i;
>
>  	for (i = 0; i < env->insn_array_map_cnt; i++) {
> -		/*
> -		 * TODO (when needed): collect only jump tables, not static keys
> -		 * or maps for indirect calls
> -		 */
>  		map = env->insn_array_maps[i];
>
> +		/* Only consider instruction array maps with multiple entries.
> +		 * These correspond to jump tables. Skip others (e.g. static keys,
> +		 * indirect call maps).
> +		 */
> +		if (map->map_type != BPF_MAP_TYPE_INSN_ARRAY ||
> +		    map->max_entries <= 1)
> +			continue;
> +

This change to jt_from_subprog() appears unrelated to the xdp
queue_index propagation described in the commit message.  Was this
intended to be part of this patch, or should it be a separate commit?


---
AI reviewed your patch. Please fix the bug or email reply why it's not a bug.
See: https://github.com/kernel-patches/vmtest/blob/master/ci/claude/README.md

CI run summary: https://github.com/kernel-patches/bpf/actions/runs/24042390210