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

[Jeremy Kerr <jk@codeconstruct.com.au>]

Hi Adam,

Some comments inline.

> +static void mctp_pcc_client_rx_callback(struct mbox_client *cl, void *mssg)
> +{
> +       struct acpi_pcct_ext_pcc_shared_memory pcc_header;
> +       struct mctp_pcc_ndev *mctp_pcc_ndev;
> +       struct mctp_pcc_mailbox *inbox;
> +       struct mctp_skb_cb *cb;
> +       struct sk_buff *skb;
> +       int size;
> +
> +       mctp_pcc_ndev = container_of(cl, struct mctp_pcc_ndev, inbox.client);
> +       inbox = &mctp_pcc_ndev->inbox;
> +       memcpy_fromio(&pcc_header, inbox->chan->shmem, sizeof(pcc_header));
> +       size = pcc_header.length - sizeof(u32);

Since size is signed, this may be negative...

(also, why sizeof(u32) here? from the spec, it seems like you're
trimming the signature here, so MCTP_SIGNATURE_LENGTH would be more
appropriate? or sizeof(pcc_header.command)?)

> +
> +       if (size == 0)
> +               return;

... which won't get picked up here

> +
> +       if (strncmp((unsigned char *)&pcc_header.command, MCTP_SIGNATURE, 4) != 0) {
> +               dev_dstats_rx_dropped(mctp_pcc_ndev->ndev);
> +               return;
> +       }

We shouldn't really be treating the signature as a string, just memcmp()
instead of strncmp(). You could also consider using a u32 (appropriately
commented) instead of the string for MCTP_SIGNATURE, and doing a direct
comparison with pcc_header.command.

4 -> MCTP_SIGNATURE_LENGTH, or whatever you chose for the above.

> +
> +       if (size > mctp_pcc_ndev->ndev->mtu)
> +               dev_dbg(cl->dev, "MCTP_PCC bytes available exceeds MTU");

(I assume we're fine to receive larger-than-MTU packets, in which
case probably don't need the dev_dbg?)

> +
> +       skb = netdev_alloc_skb(mctp_pcc_ndev->ndev, size);

... with the negative size, this will end up as a huge attempted
allocation here.

Maybe change the above check for pcc_header.length > sizeof(whatever),
instead of subtracting the sizeof() first, so you cannot underflow.
Then, use an unsigned type for size.

> +       if (!skb) {
> +               dev_dstats_rx_dropped(mctp_pcc_ndev->ndev);
> +               return;
> +       }
> +       skb_put(skb, size);
> +       skb->protocol = htons(ETH_P_MCTP);
> +       memcpy_fromio(skb->data, inbox->chan->shmem + sizeof(pcc_header),
> +                     size);

Minor: This seems oddly-wrapped, as you have longer lines above.

> +       dev_dstats_rx_add(mctp_pcc_ndev->ndev, skb->len);

Your RX stats do not include the PCC header, but your TX stats do.

I would suggest making the SKB layout consistent (ie., presence of the
transport header) across TX and RX. Include the pcc header in
the RX skb data (which you do in the TX path), and use skb->len for both
TX and RX accounting.

Cheers,


Jeremy