Re: [PATCH net-next v1 10/11] eth: fbnic: Support RX buffers it cannot split

Thank you for your contribution! Sashiko AI review found 3 potential
issue(s) to consider.

Critical: 0 · High: 0 · Medium: 2 · Low: 1

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] fbnic_config_drop_mode() in
  drivers/net/ethernet/meta/fbnic/fbnic_txrx.c changes the drop mode in…
- [Medium] eth: fbnic: incomplete split-buffer checks in
  fbnic_bd_one_desc() callers
- [Low] (Baseline: rejected.) At this commit none of the whole-buffer
  code can run.

--

eth: fbnic: Support RX buffers it cannot split

The patch lets fbnic RX queues use a page pool provider without
MP_CAP_FRAG. Each such buffer gets a single reference, the queue is
programmed so each header and payload takes its own buffer, and packets
that break this are dropped. Header split is now set per queue, and link
up only modifies the drop mode in RDE_CTL0.

[ ... ]

[Severity: Low]
This isn't a bug, but can this branch be reached at this commit?

Both fbnic page pools are still created without PP_FLAG_MEM_PROVIDER.
That means no provider without MP_CAP_FRAG can be attached (xsk_pp only
advertises MP_CAP_READABLE), so page_pool_supports_frag() is always true
here.

As a result, qt->hdr_max stays 0. The whole-buffer paths in
fbnic_bd_one_desc(), fbnic_page_pool_init() and fbnic_enable_rcq() are
not used until the later patch "eth: fbnic: Support AF_XDP page pool zero
copy" sets PP_FLAG_MEM_PROVIDER.

Could the commit message say that a later patch in the series enables
this?

 	if (netif_rxq_has_unreadable_mp(fbn->netdev, rxq_idx)) {
 		pp_params.order = get_order(rx_page_size);
 		pp_params.max_len = rx_page_size;

[ ... ]

[Severity: Medium]
Does anything serialize this read-modify-write of FBNIC_QUEUE_RDE_CTL0
against a queue restart?

fbnic_config_drop_mode() is called from fbnic_phylink_mac_link_up():

fbnic_phylink.c:fbnic_phylink_mac_link_up() {
    ...
	fbn->tx_pause = tx_pause;
	fbnic_config_drop_mode(fbn, tx_pause);
    ...
}

That runs from the phylink_resolve() work item. The work item holds
pl->state_mutex and the phy locks, but not rtnl or the netdev instance
lock.

The queue start path runs under the netdev instance lock and writes the
whole register from per-queue state:

fbnic_queue_start()
  __fbnic_nv_restart()
    fbnic_enable_rcq()
      fbnic_config_drop_mode_rcq()
        fbnic_ring_wr32(rcq, FBNIC_QUEUE_RDE_CTL0, rcq_ctl);

Suppose the link up worker reads CTL0. A queue start then writes a new
CTL0 with a different EN_HDR_SPLIT, derived from qt->hdr_max. The worker
then writes its saved copy back.

Wouldn't the restarted queue keep the old EN_HDR_SPLIT, which no longer
matches the MAX_HDR, PAYLD_OFF and PAYLD_PG_CL values in CTL1? Nothing
corrects it until the next restart of that queue.

At this commit qt->hdr_max is always 0, so the written-back value still
matches. Later in the series, "eth: fbnic: Support AF_XDP page pool zero
copy" makes hdr_max and MIN_HROOM (from qt->rx_hroom) truly per queue.

After that, an AF_XDP bind or unbind that overlaps a link up event could
leave the queue misconfigured. For example, a stale EN_HDR_SPLIT on a
non-SG AF_XDP queue would make every packet multi-buffer, and
fbnic_run_xdp() would drop it.

 		}
 	}
 }

[ ... ]

[Severity: Medium]
Do the two callers of fbnic_bd_one_desc() check enough to catch a
whole buffer that the device splits across more than one completion?

The commit message says packets that break the one descriptor per
buffer rule are dropped. Each caller only checks half of that
condition, though.

In fbnic_add_rx_frag() the ok argument is only !pg_off, and
FBNIC_RCD_AL_PAGE_FIN is not checked. Suppose a payload buffer ends up
holding more than one payload. The first completion then has
pg_off == 0 and PAGE_FIN == 0, so it passes. fbnic_page_pool_get_data()
drops pagecnt_bias from 1 to 0, and the packet takes the only reference
to the netmem.

That reference can then go back to the pool through
page_pool_put_full_netmem(), or be handed to the AF_XDP socket. The
device still has the buffer open at that point and may DMA the next
payload into it. The next completion for that buffer is rejected
because pagecnt_bias is now 0, but the buffer has already been
released by then.

fbnic_pkt_prepare() has the opposite gap. It checks only PAGE_FIN, not
where the header sits in the buffer. Suppose two headers land in the
same header buffer. The first completion has PAGE_FIN == 0 and is
rejected before fbnic_page_pool_get_head() runs, so pagecnt_bias stays
at 1.

The second completion has PAGE_FIN == 1 at a non-zero offset. It still
sees pagecnt_bias == 1 and is accepted. An xdp_buff is then built from
the middle of a buffer that was meant to hold a single header.

A conforming device should not do either of these with the MAX_HDR,
PAYLD_OFF and PAYLD_PG_CL values programmed in fbnic_enable_rcq().
However, these checks exist to catch the case where it does.

Should both callers require that the completion starts the buffer and
also closes it, i.e. a zero offset and PAGE_FIN set? Passing the rcd
into fbnic_bd_one_desc() and doing both checks there might keep the
header and payload paths from drifting apart again.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261110102039.1002605-1-bjorn%40kernel.org