| 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: <b577d90e-e8e8-4117-a5a0-c6644f2bb151@amperemail.onmicrosoft.com>
Date: Fri, 5 Sep 2025 16:11:46 -0400
From: Adam Young <admiyo@...eremail.onmicrosoft.com>
To: Jeremy Kerr <jk@...econstruct.com.au>,
Adam Young <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>, ALOK TIWARI <alok.a.tiwari@...cle.com>
Subject: Re: [PATCH net-next v28 1/1] mctp pcc: Implement MCTP over PCC
Transport
On 9/4/25 02:16, Jeremy Kerr wrote:
> Hi Adam,
>
> You seem to have missed any fixes from Alok's review here.
Yes, I missed that message. I will make them in my next version.
However, something more significant in this one might delay that....
>
> Further comments inline:
>
>> +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;
>> +
>> + 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->inbox.client.rx_callback = mctp_pcc_client_rx_callback;
>> + mctp_pcc_ndev->outbox.chan->manage_writes = true;
> From v25:
>
>> Minor: you have the convenience vars for ->inbox and ->outbox, may as
>> well use them.
> Also: you're setting the client rx_callback *after* having set up the
> PCC channel. Won't this race with RX on the inbox?
I think this might be a deal breaker. I was trying to avoid making a
change to the mailbox API, but I think I need a new client scoped
callback to replace the one that I added here: there is no way to avoid
the race condition without an atomic request_channel. I added this to
the channel (there is no PCC specific client) to try and minimize churn
on this set, but I think I need to do it the right way.
And, since Sudeep is unhappy with the changes to the PCC mailbox anyway,
I would expect a rewrite of that layer to happen before I get to address
this.
>
>> +static int initialize_MTU(struct net_device *ndev)
>> +{
>> + struct mctp_pcc_ndev *mctp_pcc_ndev = netdev_priv(ndev);
>> + struct mctp_pcc_mailbox *outbox;
>> + int mctp_pcc_mtu;
>> +
>> + outbox = &mctp_pcc_ndev->outbox;
>> + outbox->chan = pcc_mbox_request_channel(&outbox->client, outbox->index);
>> + mctp_pcc_mtu = outbox->chan->shmem_size - sizeof(struct pcc_header);
>> + if (IS_ERR(outbox->chan))
>> + return PTR_ERR(outbox->chan);
> You have already dereferenced outbox->chan before this confitional. Move
> the usage to after this check.
Will do.
>
>> +
>> + pcc_mbox_free_channel(mctp_pcc_ndev->outbox.chan);
>> +
>> + mctp_pcc_ndev = netdev_priv(ndev);
>> + ndev->mtu = MCTP_MIN_MTU;
>> + ndev->max_mtu = mctp_pcc_mtu;
>> + ndev->min_mtu = MCTP_MIN_MTU;
>> +
>> + return 0;
>> +}
>> +
>> +static int mctp_pcc_driver_add(struct acpi_device *acpi_dev)
>> +{
>> + struct mctp_pcc_lookup_context context = {0};
>> + struct mctp_pcc_ndev *mctp_pcc_ndev;
>> + struct device *dev = &acpi_dev->dev;
>> + struct net_device *ndev;
>> + acpi_handle dev_handle;
>> + acpi_status status;
>> + char name[32];
>> + int rc;
>> +
>> + dev_dbg(dev, "Adding mctp_pcc device for HID %s\n",
>> + acpi_device_hid(acpi_dev));
>> + dev_handle = acpi_device_handle(acpi_dev);
>> + status = acpi_walk_resources(dev_handle, "_CRS", lookup_pcct_indices,
>> + &context);
>> + if (!ACPI_SUCCESS(status)) {
>> + dev_err(dev, "FAILURE to lookup PCC indexes from CRS\n");
>> + return -EINVAL;
>> + }
>> +
>> + snprintf(name, sizeof(name), "mctppcc%d", context.inbox_index);
>> + ndev = alloc_netdev(sizeof(*mctp_pcc_ndev), name, NET_NAME_PREDICTABLE,
>> + mctp_pcc_setup);
>> + if (!ndev)
>> + return -ENOMEM;
>> +
>> + mctp_pcc_ndev = netdev_priv(ndev);
>> +
>> + mctp_pcc_initialize_mailbox(dev, &mctp_pcc_ndev->inbox,
>> + context.inbox_index);
>> + mctp_pcc_initialize_mailbox(dev, &mctp_pcc_ndev->outbox,
>> + context.outbox_index);
>> + if (rc)
>> + goto free_netdev;
> 'rc' has never been set at this point.
Artifact of old code that has been moved. I will remove the RC.
mctp_pcc_initialize_mailbox is simpler now, and has no failure case.
>
>> +
>> + mctp_pcc_ndev->outbox.client.tx_done = mctp_pcc_tx_done;
>> + mctp_pcc_ndev->acpi_device = acpi_dev;
>> + mctp_pcc_ndev->ndev = ndev;
>> + acpi_dev->driver_data = mctp_pcc_ndev;
>> +
>> + initialize_MTU(ndev);
> initialize_MTU() has an int return value; either make that void, or
> handle the error here.
I will handle it here.
>
> Cheers,
>
>
> Jeremy
Powered by blists - more mailing lists