Re: [net-next v41] mctp pcc: Implement MCTP over PCC Transport
From: Adam Young
Date: Wed May 13 2026 - 13:36:20 EST
On 5/12/26 00:07, Jeremy Kerr wrote:
Hi Adam,
Sorry, I should have been more explicit here. I am not certain what isNothing is going to happen with fragmentation. Your driver will only see
going to happen with fragmentation, so I want to be protected against
future changes.
linear skbs in the ndo_start_xmit callback.
Adding the skb_linearize() there is unnecessary, and creates ambiguity
about the structure of the skbs that we're dealing with in that path.
The check in validate_xmit_skb() is good, as it protects against theI'm not clear on what changes you're referring to here?
current set up. So my option was to put a comment in here and hope both
changes happened together, or to just try and get this portion of the
driver solid against the change.
And I thought that was what you were suggesting in the comment. TheIt's less of an optimisation, and more removing something that is
original comment sounded more like an "here is an optimization" instead
of "this is important enough to kick back"
unneeded, and potentially confusing ("why is the driver doing this?")?
As for spacing, I get that there is a style, but it really should beI'm not too fussed about the style at the level you have here, these are
encoded in checkstyle.sh or something and automated. My own tendency is
to put way too many spaces in to chunk things together, and I end up
going over-draconian on stripping them out to try and meet the expected
layout.
suggestions to clean up if you're already re-rolling. My confusion is
that you had applied my feedback to one part of the code, but not to the
area I had commented on (which has the same style structure).
You also had some sashiko feedback on v40. I'm not sure whether all
items are relevant (I *think* you're OK for the first, for example), but
worth confirming:
https://sashiko.dev/#/patchset/20260508032953.337036-1-admiyo%40os.amperecomputing.com
I'd like to address this part specifically:
> +static int mctp_pcc_stop(struct net_device *ndev)
> +{
> + struct mctp_pcc_ndev *mctp_pcc_ndev;
> + unsigned int count, idx;
> + struct mbox_chan *chan;
> + struct sk_buff *skb;
> +
> + mctp_pcc_ndev = netdev_priv(ndev);
> + chan = mctp_pcc_ndev->outbox.chan->mchan;
> + pcc_mbox_free_channel(mctp_pcc_ndev->inbox.chan);
> + scoped_guard(spinlock_irqsave, &chan->lock) {
> + skb = chan->active_req;
> + chan->active_req = NULL;
> + if (skb) {
> + dev_dstats_tx_dropped(ndev);
> + dev_consume_skb_any(skb);
> + }
> + while (chan->msg_count > 0) {
> + count = chan->msg_count;
> + idx = chan->msg_free;
> + if (idx >= count)
> + idx -= count;
> + else
> + idx += MBOX_TX_QUEUE_LEN - count;
> + skb = chan->msg_data[idx];
> + dev_dstats_tx_dropped(ndev);
> + dev_consume_skb_any(skb);
> + chan->msg_count--;
> + }
> + }
Is it safe to directly access and modify internal mailbox controller
structures here?
By manually clearing chan->active_req and freeing the active skb while the
hardware controller might be actively transmitting it, could this cause a
use-after-free or double-free? If the controller's interrupt handler runs
concurrently, it might dereference the now-freed skb.
Should the driver treat struct mbox_chan as an opaque handle and rely on
the standard APIs instead of manually clearing the queue?
End Sashiko Output
There is no way to clear the SKBs in the ringbuffer without using the internal mailbox controller structures. THere is no callback to flush old messages. Thus, this is done inline in the client code using the tools available.
The first step in the code above is to free the channel. That will both disable the interrupt and unmap the shared memory. The primary way we could get a use-after-free is by the code executing from the interrupt handler, which is why we need to disable it before this point.
MCTP stop should be called after the MCTP infrastructure has removed the device as a viable target for forwarding packets. Thus, we should not get another tx request after this function gets called.
Cheers,
Jeremy