| 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: <88a67cc10907926204a478c58e361cb6706a939a.camel@codeconstruct.com.au>
Date: Fri, 22 Aug 2025 16:21:45 +0800
From: Jeremy Kerr <jk@...econstruct.com.au>
To: admiyo@...amperecomputing.com, Matt Johnston
<matt@...econstruct.com.au>, 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>
Cc: netdev@...r.kernel.org, linux-kernel@...r.kernel.org, Sudeep Holla
<sudeep.holla@....com>, Jonathan Cameron <Jonathan.Cameron@...wei.com>,
Huisong Li <lihuisong@...wei.com>
Subject: Re: [PATCH net-next v25 1/1] mctp pcc: Implement MCTP over PCC
Transport
Hi Adam,
A few comments inline, mainly on the locking & list traversal.
> +struct mctp_pcc_mailbox {
> + u32 index;
> + struct pcc_mbox_chan *chan;
> + struct mbox_client client;
> + struct sk_buff_head packets;
> +};
> +
> +/* The netdev structure. One of these per PCC adapter. */
> +struct mctp_pcc_ndev {
> + /* spinlock to serialize access to queue that holds a copy of the
> + * sk_buffs that are also in the ring buffers of the mailbox.
> + */
> + spinlock_t lock;
> + struct net_device *ndev;
> + struct acpi_device *acpi_device;
> + struct mctp_pcc_mailbox inbox;
> + struct mctp_pcc_mailbox outbox;
> +};
> +
> +static void *mctp_pcc_rx_alloc(struct mbox_client *c, int size)
> +{
> + struct mctp_pcc_ndev *mctp_pcc_ndev;
> + struct mctp_pcc_mailbox *box;
> + struct sk_buff *skb;
> +
> + mctp_pcc_ndev = container_of(c, struct mctp_pcc_ndev, inbox.client);
> + box = &mctp_pcc_ndev->inbox;
> +
> + if (size > mctp_pcc_ndev->ndev->mtu)
> + return NULL;
> + skb = netdev_alloc_skb(mctp_pcc_ndev->ndev, size);
> + if (!skb)
> + return NULL;
> + skb_put(skb, size);
> + skb->protocol = htons(ETH_P_MCTP);
> +
> + spin_lock(&mctp_pcc_ndev->lock);
> + skb_queue_head(&box->packets, skb);
> + spin_unlock(&mctp_pcc_ndev->lock);
Given you're using your own locking here (which seems sensible, since
you're iterating elsewhere), you're not relying on the list's lock. In
which case you can use __skb_queue_head() (and __skb_unlink below) as
lockless variants.
> +
> + return skb->data;
> +}
> +
> +static void mctp_pcc_client_rx_callback(struct mbox_client *c, void *buffer)
> +{
> + struct mctp_pcc_ndev *mctp_pcc_ndev;
> + struct pcc_header pcc_header;
> + struct sk_buff *skb = NULL;
> + struct mctp_skb_cb *cb;
> +
> + mctp_pcc_ndev = container_of(c, struct mctp_pcc_ndev, inbox.client);
> + if (!buffer) {
> + dev_dstats_rx_dropped(mctp_pcc_ndev->ndev);
> + return;
> + }
> +
> + spin_lock(&mctp_pcc_ndev->lock);
> + skb_queue_walk(&mctp_pcc_ndev->inbox.packets, skb) {
> + if (skb->data != buffer)
> + continue;
> + skb_unlink(skb, &mctp_pcc_ndev->inbox.packets);
> + break;
> + }
> + spin_unlock(&mctp_pcc_ndev->lock);
> +
> + if (skb) {
The termination case of skb_queue_walk() will not leave skb as null.
Instead, you will probably want to use a temporary variable for the
iterator, and set skb when you find a match.
> + dev_dstats_rx_add(mctp_pcc_ndev->ndev, skb->len);
> + skb_reset_mac_header(skb);
> + skb_pull(skb, sizeof(pcc_header));
> + skb_reset_network_header(skb);
> + cb = __mctp_cb(skb);
> + cb->halen = 0;
> + netif_rx(skb);
> + }
> +}
> +
> +static void mctp_pcc_tx_done(struct mbox_client *c, void *mssg, int r)
> +{
> + struct mctp_pcc_ndev *mctp_pcc_ndev;
> + struct mctp_pcc_mailbox *box;
> + struct sk_buff *skb = NULL;
> +
> + mctp_pcc_ndev = container_of(c, struct mctp_pcc_ndev, outbox.client);
> + box = container_of(c, struct mctp_pcc_mailbox, client);
> + spin_lock(&mctp_pcc_ndev->lock);
> + skb_queue_walk(&box->packets, skb) {
> + if (skb->data == mssg) {
> + skb_unlink(skb, &box->packets);
> + break;
> + }
> + }
> + spin_unlock(&mctp_pcc_ndev->lock);
> +
> + if (skb)
Same as above; this will not be null in the not-found case, but will
point to the list head.
> + dev_consume_skb_any(skb);
... but even if so, you could pass skb as NULL here and avoid the if.
> +}
> +
> +static netdev_tx_t mctp_pcc_tx(struct sk_buff *skb, struct net_device *ndev)
> +{
> + struct mctp_pcc_ndev *mpnd = netdev_priv(ndev);
> + struct pcc_header *pcc_header;
> + int len = skb->len;
> + int rc;
> +
> + rc = skb_cow_head(skb, sizeof(*pcc_header));
> + if (rc) {
> + dev_dstats_tx_dropped(ndev);
> + kfree_skb(skb);
> + return NETDEV_TX_OK;
> + }
> +
> + pcc_header = skb_push(skb, sizeof(*pcc_header));
> + pcc_header->signature = PCC_SIGNATURE | mpnd->outbox.index;
> + pcc_header->flags = PCC_CMD_COMPLETION_NOTIFY;
> + memcpy(&pcc_header->command, MCTP_SIGNATURE, MCTP_SIGNATURE_LENGTH);
> + pcc_header->length = len + MCTP_SIGNATURE_LENGTH;
> +
> + spin_lock(&mpnd->lock);
> + skb_queue_head(&mpnd->outbox.packets, skb);
> + spin_unlock(&mpnd->lock);
> +
> + rc = mbox_send_message(mpnd->outbox.chan->mchan, skb->data);
> +
> + if (rc < 0) {
> + skb_unlink(skb, &mpnd->outbox.packets);
This does not hold the lock you are using for the list traversal. If you
want to rely on the list-internal locking, that's fine, but you need to
be consistent in your approach.
> + return NETDEV_TX_BUSY;
> + }
> +
> + dev_dstats_tx_add(ndev, len);
> + return NETDEV_TX_OK;
> +}
> +
> +static void drain_packets(struct sk_buff_head *list)
> +{
> + struct sk_buff *skb;
> +
> + while (!skb_queue_empty(list)) {
> + skb = skb_dequeue(list);
> + dev_consume_skb_any(skb);
> + }
> +}
> +
> +static int mctp_pcc_ndo_open(struct net_device *ndev)
> +{
> + struct mctp_pcc_ndev *mctp_pcc_ndev =
> + netdev_priv(ndev);
> + struct mctp_pcc_mailbox *outbox =
> + &mctp_pcc_ndev->outbox;
> + struct mctp_pcc_mailbox *inbox =
> + &mctp_pcc_ndev->inbox;
Minor: I don't think these need wrapping?
> + int mctp_pcc_mtu;
> +
> + outbox->chan = pcc_mbox_request_channel(&outbox->client, outbox->index);
> + if (IS_ERR(outbox->chan))
> + return PTR_ERR(outbox->chan);
> +
> + inbox->chan = pcc_mbox_request_channel(&inbox->client, inbox->index);
> + if (IS_ERR(inbox->chan)) {
> + pcc_mbox_free_channel(outbox->chan);
> + return PTR_ERR(inbox->chan);
> + }
> +
> + mctp_pcc_ndev->inbox.chan->rx_alloc = mctp_pcc_rx_alloc;
> + mctp_pcc_ndev->outbox.chan->manage_writes = true;
> +
> + /* There is no clean way to pass the MTU to the callback function
> + * used for registration, so set the values ahead of time.
> + */
For my own clarity, what's "the callback function used for
registration"?
> + mctp_pcc_mtu = mctp_pcc_ndev->outbox.chan->shmem_size -
> + sizeof(struct pcc_header);
> + ndev->mtu = MCTP_MIN_MTU;
> + ndev->max_mtu = mctp_pcc_mtu;
> + ndev->min_mtu = MCTP_MIN_MTU;
> +
> + return 0;
> +}
> +
> +static int mctp_pcc_ndo_stop(struct net_device *ndev)
> +{
> + struct mctp_pcc_ndev *mctp_pcc_ndev =
> + netdev_priv(ndev);
> + struct mctp_pcc_mailbox *outbox =
> + &mctp_pcc_ndev->outbox;
> + struct mctp_pcc_mailbox *inbox =
> + &mctp_pcc_ndev->inbox;
No wrapping needed here either.
Cheers,
Jeremy
Powered by blists - more mailing lists