| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-Id: <DEJK1461002Y.TQON2T91OS6B@bootlin.com>
Date: Thu, 27 Nov 2025 15:41:56 +0100
From: Théo Lebrun <theo.lebrun@...tlin.com>
To: "Paolo Valerio" <pvalerio@...hat.com>, <netdev@...r.kernel.org>
Cc: "Nicolas Ferre" <nicolas.ferre@...rochip.com>, "Claudiu Beznea"
<claudiu.beznea@...on.dev>, "Andrew Lunn" <andrew+netdev@...n.ch>, "David
S. Miller" <davem@...emloft.net>, "Eric Dumazet" <edumazet@...gle.com>,
"Jakub Kicinski" <kuba@...nel.org>, "Paolo Abeni" <pabeni@...hat.com>,
"Lorenzo Bianconi" <lorenzo@...nel.org>, "Thomas Petazzoni"
<thomas.petazzoni@...tlin.com>, Grégory Clement
<gregory.clement@...tlin.com>, Benoît Monin
<benoit.monin@...tlin.com>
Subject: Re: [PATCH RFC net-next 4/6] cadence: macb/gem: add XDP support for
gem
On Wed Nov 19, 2025 at 2:53 PM CET, Paolo Valerio wrote:
> @@ -1273,6 +1275,7 @@ struct macb_queue {
> struct queue_stats stats;
> struct page_pool *page_pool;
> struct sk_buff *skb;
> + struct xdp_rxq_info xdp_q;
> };
Those are always named `xdp_rxq` inside the kernel, we should stick with
the calling convention no?
> diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
> index 5829c1f773dd..53ea1958b8e4 100644
> --- a/drivers/net/ethernet/cadence/macb_main.c
> +++ b/drivers/net/ethernet/cadence/macb_main.c
> @@ -1344,10 +1344,51 @@ static void discard_partial_frame(struct macb_queue *queue, unsigned int begin,
> */
> }
>
> +static u32 gem_xdp_run(struct macb_queue *queue, struct xdp_buff *xdp,
> + struct net_device *dev)
Why pass `struct net_device` explicitly? It is in queue->bp->dev.
> +{
> + struct bpf_prog *prog;
> + u32 act = XDP_PASS;
> +
> + rcu_read_lock();
> +
> + prog = rcu_dereference(queue->bp->prog);
> + if (!prog)
> + goto out;
> +
> + act = bpf_prog_run_xdp(prog, xdp);
> + switch (act) {
> + case XDP_PASS:
> + goto out;
> + case XDP_REDIRECT:
> + if (unlikely(xdp_do_redirect(dev, xdp, prog))) {
> + act = XDP_DROP;
> + break;
> + }
> + goto out;
Why the `unlikely()`?
> + default:
> + bpf_warn_invalid_xdp_action(dev, prog, act);
> + fallthrough;
> + case XDP_ABORTED:
> + trace_xdp_exception(dev, prog, act);
> + fallthrough;
> + case XDP_DROP:
> + break;
> + }
> +
> + page_pool_put_full_page(queue->page_pool,
> + virt_to_head_page(xdp->data), true);
Maybe move that to the XDP_DROP, it is the only `break` in the above
switch statement. It will be used by the default and XDP_ABORTED cases
through fallthrough. We can avoid the out label and its two gotos that
way.
> static int gem_rx(struct macb_queue *queue, struct napi_struct *napi,
> int budget)
> {
> struct macb *bp = queue->bp;
> + bool xdp_flush = false;
> unsigned int len;
> unsigned int entry;
> void *data;
> @@ -1356,9 +1397,11 @@ static int gem_rx(struct macb_queue *queue, struct napi_struct *napi,
> int count = 0;
>
> while (count < budget) {
> - u32 ctrl;
> - dma_addr_t addr;
> bool rxused, first_frame;
> + struct xdp_buff xdp;
> + dma_addr_t addr;
> + u32 ctrl;
> + u32 ret;
>
> entry = macb_rx_ring_wrap(bp, queue->rx_tail);
> desc = macb_rx_desc(queue, entry);
> @@ -1403,6 +1446,22 @@ static int gem_rx(struct macb_queue *queue, struct napi_struct *napi,
> data_len = SKB_WITH_OVERHEAD(bp->rx_buffer_size) - bp->rx_offset;
> }
>
> + if (!(ctrl & MACB_BIT(RX_SOF) && ctrl & MACB_BIT(RX_EOF)))
> + goto skip_xdp;
> +
> + xdp_init_buff(&xdp, bp->rx_buffer_size, &queue->xdp_q);
> + xdp_prepare_buff(&xdp, data, bp->rx_offset, len,
> + false);
> + xdp_buff_clear_frags_flag(&xdp);
You prepare the XDP buffer before checking an XDP program is attached.
Could we avoid this work? We'd move the xdp_buff preparation into
gem_xdp_run(), after the RCU pointer dereference.
> -static void gem_create_page_pool(struct macb_queue *queue)
> +static void gem_create_page_pool(struct macb_queue *queue, int qid)
> {
> struct page_pool_params pp_params = {
> .order = 0,
> .flags = PP_FLAG_DMA_MAP | PP_FLAG_DMA_SYNC_DEV,
> .pool_size = queue->bp->rx_ring_size,
> .nid = NUMA_NO_NODE,
> - .dma_dir = DMA_FROM_DEVICE,
> + .dma_dir = rcu_access_pointer(queue->bp->prog)
> + ? DMA_BIDIRECTIONAL
> + : DMA_FROM_DEVICE,
Ah, that is the reason for page_pool_get_dma_dir() calls!
> static int macb_change_mtu(struct net_device *dev, int new_mtu)
> {
> + int frame_size = new_mtu + ETH_HLEN + ETH_FCS_LEN + MACB_MAX_PAD;
> + struct macb *bp = netdev_priv(dev);
> + struct bpf_prog *prog = bp->prog;
No fancy RCU macro?
> +static int macb_xdp(struct net_device *dev, struct netdev_bpf *xdp)
> +{
> + struct macb *bp = netdev_priv(dev);
> +
> + if (!macb_is_gem(bp))
> + return 0;
Returning 0 sounds like a mistake, -EOPNOTSUPP sounds more appropriate.
> + switch (xdp->command) {
> + case XDP_SETUP_PROG:
> + return gem_xdp_setup(dev, xdp->prog, xdp->extack);
> + default:
> + return -EINVAL;
Same here: we want -EOPNOTSUPP. Otherwise caller cannot dissociate an
unsupported call from one that is supported but failed.
> + }
> +}
> +
> static void gem_update_stats(struct macb *bp)
> {
> struct macb_queue *queue;
> @@ -4390,6 +4529,7 @@ static const struct net_device_ops macb_netdev_ops = {
> .ndo_hwtstamp_set = macb_hwtstamp_set,
> .ndo_hwtstamp_get = macb_hwtstamp_get,
> .ndo_setup_tc = macb_setup_tc,
> + .ndo_bpf = macb_xdp,
We want it to be "gem_" prefixed as it does not support MACB.
Thanks,
--
Théo Lebrun, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
Powered by blists - more mailing lists