netdev - Re: [PATCH net-next v28 1/1] mctp pcc: Implement MCTP over PCC Transport

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
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <b4891bd683d4802d6ab3c542b446c14081ecf8d6.camel@codeconstruct.com.au>
Date: Thu, 04 Sep 2025 14:16:27 +0800
From: Jeremy Kerr <jk@...econstruct.com.au>
To: 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

Hi Adam,

You seem to have missed any fixes from Alok's review here.

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?

> +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.

> +
> +       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.

> +
> +       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.

Cheers,


Jeremy