Re: [PATCH net-next v5 11/15] net: macb: change function signatures to take contexts

[Théo Lebrun]

Replies to Sashiko reviews (only those that aren't about pre-existing
issues).

On this patch, only from sashiko.dev:
https://sashiko.dev/#/patchset/20260724-macb-context-v5-0-569b1852bc7f%40bootlin.com

> > @@ -186,40 +188,42 @@ static struct macb_tx_skb *macb_tx_skb(struct macb_queue *queue,
> >  {
> >   struct macb_txq *txq = macb_txq(queue);
> >
> > - return &txq->skb[macb_tx_ring_wrap(queue->bp, index)];
> > + return &txq->skb[macb_tx_ring_wrap(queue->bp->ctx, index)];
>
> [Severity: Medium]
> Will this continue to dynamically evaluate queue->bp->ctx instead of
> accepting the context parameter? It looks like this leaves a mixed reference
> which might defeat the refactoring goal of avoiding bugs from using bp->ctx
> instead of ctx.

The main goal of refactoring to take ctx (+ queue index) is that, on
transition states, we cannot mess up the buffers we access.

The macb_tx_skb() function is by definition a function that is only
called in places where the interface must be up and running. Changing
its signature would require passing a queue index and that means a tiny
bit more code in the 11 calling locations without any safety benefit.

> > @@ -1620,6 +1631,8 @@ static int gem_rx(struct macb_queue *queue, struct napi_struct *napi,
> >        int budget)
> >  {
> > + unsigned int q = queue - queue->bp->queues;
> > + struct macb_context *ctx = queue->bp->ctx;
> >   struct macb_rxq *rxq = macb_rxq(queue);
> >   struct macb *bp = queue->bp;
>
> [Severity: Medium]
> Can this cause a mismatch in size when processing descriptors later in
> the function? Since dma_unmap_single() uses bp->ctx->rx_buffer_size
> instead of ctx->rx_buffer_size:
>
> gem_rx() {
>     ...
>     dma_unmap_single(&bp->pdev->dev, addr,
>                      bp->ctx->rx_buffer_size, DMA_FROM_DEVICE);
>
> Could this lead to wrong DMA unmap sizes if a concurrent swap modifies
> bp->ctx?

I like this review: our function signature bp to ctx rework is aimed at
avoiding those exact bugs. But here, no possible issue. We don't allow
concurrent swaps.

gem_rx() can only be called when a single context exists. Only functions
which might be called at a timeframe where two contexts might be alive
need to be careful about the context they use.

> > @@ -2358,14 +2381,14 @@ static unsigned int macb_tx_map(struct macb *bp,
> >   do {
> >      i--;
> >      tx_skb = macb_tx_skb(queue, i);
> > -    desc = macb_tx_desc(queue, i);
> > +    desc = macb_tx_desc(ctx, q, i);
> >
> >      ctrl = (u32)tx_skb->size;
> >      if (eof) {
> >         ctrl |= MACB_BIT(TX_LAST);
> >         eof = 0;
> >      }
> > -    if (unlikely(macb_tx_ring_wrap(bp, i) ==
> > +    if (unlikely(macb_tx_ring_wrap(ctx, i) ==
> >            bp->ctx->tx_ring_size - 1))
> >         ctrl |= MACB_BIT(TX_WRAP);
>
> [Severity: Medium]
> Does mixing the explicit ctx and bp->ctx in the same bounds calculation
> create a time-of-check to time-of-use race? If the context sizes ever differ
> due to a concurrent context swap, could this lead to buffer overflows by
> failing to set TX_WRAP?

Again, same reasoning. macb_tx_map() cannot be called in a timeframe
where two contexts are alive.

I'll clarify the commit message to avoid the confusion.

Thanks,

--
Théo Lebrun, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com