On Thu, Feb 06, 2025 at 02:48:24PM +0800, Jeremy Kerr wrote:
> Add an implementation for DMTF DSP0283, which defines a MCTP-over-USB
> transport. As per that spec, we're restricted to full speed mode,
> requiring 512-byte transfers.
>
> Each MCTP-over-USB interface is a peer-to-peer link to a single MCTP
> endpoint, so no physical addressing is required (of course, that MCTP
> endpoint may then bridge to further MCTP endpoints). Consequently,
> interfaces will report with no lladdr data:
>
> # mctp link
> dev lo index 1 address 00:00:00:00:00:00 net 1 mtu 65536 up
> dev mctpusb0 index 6 address none net 1 mtu 68 up
>
> This is a simple initial implementation, with single rx & tx urbs, and
> no multi-packet tx transfers - although we do accept multi-packet rx
> from the device.
>
> Includes suggested fixes from Santosh Puranik <spuranik@xxxxxxxxxx>.
>
> Signed-off-by: Jeremy Kerr <jk@xxxxxxxxxxxxxxxxxxxx>
> Cc: Santosh Puranik <spuranik@xxxxxxxxxx>
...
> diff --git a/drivers/net/mctp/mctp-usb.c b/drivers/net/mctp/mctp-usb.c
...
> +static void mctp_usb_in_complete(struct urb *urb)
> +{
> + struct sk_buff *skb = urb->context;
> + struct net_device *netdev = skb->dev;
> + struct pcpu_dstats *dstats = this_cpu_ptr(netdev->dstats);
> + struct mctp_usb *mctp_usb = netdev_priv(netdev);
> + struct mctp_skb_cb *cb;
> + unsigned int len;
> + int status;
> +
> + status = urb->status;
> +
> + switch (status) {
> + case -ENOENT:
> + case -ECONNRESET:
> + case -ESHUTDOWN:
> + case -EPROTO:
> + kfree_skb(skb);
> + return;
> + case 0:
> + break;
> + default:
> + dev_err(&mctp_usb->usbdev->dev, "%s: urb status: %d\n",
> + __func__, status);
> + kfree_skb(skb);
> + return;
> + }
> +
> + len = urb->actual_length;
> + __skb_put(skb, len);
> +
> + while (skb) {
> + struct sk_buff *skb2 = NULL;
> + struct mctp_usb_hdr *hdr;
> + u8 pkt_len; /* length of MCTP packet, no USB header */
> +
> + hdr = skb_pull_data(skb, sizeof(*hdr));
> + if (!hdr)
> + break;
> +
> + if (be16_to_cpu(hdr->id) != MCTP_USB_DMTF_ID) {
> + dev_dbg(&mctp_usb->usbdev->dev, "%s: invalid id %04x\n",
> + __func__, be16_to_cpu(hdr->id));
> + break;
> + }
> +
> + if (hdr->len <
> + sizeof(struct mctp_hdr) + sizeof(struct mctp_usb_hdr)) {
> + dev_dbg(&mctp_usb->usbdev->dev,
> + "%s: short packet (hdr) %d\n",
> + __func__, hdr->len);
> + break;
> + }
> +
> + /* we know we have at least sizeof(struct mctp_usb_hdr) here */
> + pkt_len = hdr->len - sizeof(struct mctp_usb_hdr);
> + if (pkt_len > skb->len) {
> + dev_dbg(&mctp_usb->usbdev->dev,
> + "%s: short packet (xfer) %d, actual %d\n",
> + __func__, hdr->len, skb->len);
> + break;
> + }
> +
> + if (pkt_len < skb->len) {
> + /* more packets may follow - clone to a new
> + * skb to use on the next iteration
> + */
> + skb2 = skb_clone(skb, GFP_ATOMIC);
> + if (skb2) {
> + if (!skb_pull(skb2, pkt_len)) {
> + kfree_skb(skb2);
> + skb2 = NULL;
> + }
> + }
> + skb_trim(skb, pkt_len);
> + }
> +
> + skb->protocol = htons(ETH_P_MCTP);
> + skb_reset_network_header(skb);
> + cb = __mctp_cb(skb);
> + cb->halen = 0;
> + netif_rx(skb);
Hi Jeremy,
skb is dereferenced a few lines further down,
but I don't think it is is safe to do so after calling netif_rx().
> +
> + u64_stats_update_begin(&dstats->syncp);
> + u64_stats_inc(&dstats->rx_packets);
> + u64_stats_add(&dstats->rx_bytes, skb->len);
> + u64_stats_update_end(&dstats->syncp);
> +
> + skb = skb2;
> + }
> +
> + if (skb)
> + kfree_skb(skb);
> +
> + mctp_usb_rx_queue(mctp_usb);
> +}
...
