netdev - Re: [PATCH net-next v2 2/2] net: mctp: Add MCTP USB transport driver

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: <20250214194531.5ddded19@kernel.org>
Date: Fri, 14 Feb 2025 19:45:31 -0800
From: Jakub Kicinski <kuba@...nel.org>
To: Jeremy Kerr <jk@...econstruct.com.au>
Cc: Matt Johnston <matt@...econstruct.com.au>, Greg Kroah-Hartman
 <gregkh@...uxfoundation.org>, Andrew Lunn <andrew+netdev@...n.ch>, "David
 S. Miller" <davem@...emloft.net>, Eric Dumazet <edumazet@...gle.com>, Paolo
 Abeni <pabeni@...hat.com>, netdev@...r.kernel.org,
 linux-usb@...r.kernel.org, Santosh Puranik <spuranik@...dia.com>
Subject: Re: [PATCH net-next v2 2/2] net: mctp: Add MCTP USB transport
 driver

On Wed, 12 Feb 2025 10:46:51 +0800 Jeremy Kerr wrote:
> +	__u8 ep_in;
> +	__u8 ep_out;

same nit about u8 as on the header

> +	struct urb *tx_urb;
> +	struct urb *rx_urb;
> +};
> +
> +static void mctp_usb_stat_tx_dropped(struct net_device *dev)
> +{
> +	struct pcpu_dstats *dstats = this_cpu_ptr(dev->dstats);
> +
> +	u64_stats_update_begin(&dstats->syncp);
> +	u64_stats_inc(&dstats->tx_drops);
> +	u64_stats_update_end(&dstats->syncp);
> +}

Letter for letter dev_dstats_tx_dropped() ?

> +static void mctp_usb_stat_tx_done(struct net_device *dev, unsigned int len)
> +{
> +	struct pcpu_dstats *dstats = this_cpu_ptr(dev->dstats);
> +
> +	u64_stats_update_begin(&dstats->syncp);
> +	u64_stats_inc(&dstats->tx_packets);
> +	u64_stats_add(&dstats->tx_bytes, len);
> +	u64_stats_update_end(&dstats->syncp);
> +}

And this dev_dstats_tx_add() ?

> +static netdev_tx_t mctp_usb_start_xmit(struct sk_buff *skb,
> +				       struct net_device *dev)
> +{
> +	struct mctp_usb *mctp_usb = netdev_priv(dev);
> +	struct mctp_usb_hdr *hdr;
> +	unsigned int plen;
> +	struct urb *urb;
> +	int rc;
> +
> +	plen = skb->len;
> +
> +	if (plen + sizeof(*hdr) > MCTP_USB_XFER_SIZE)
> +		goto err_drop;
> +
> +	hdr = skb_push(skb, sizeof(*hdr));

Hm, I guess MCTP may have its own rules but technically you should
call skb_cow_head() before you start writing to the header buffer.

> +	if (!hdr)
> +		goto err_drop;
> +
> +	hdr->id = cpu_to_be16(MCTP_USB_DMTF_ID);
> +	hdr->rsvd = 0;
> +	hdr->len = plen + sizeof(*hdr);

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

> +		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);

dev_dstats_rx_add()

> +		skb->protocol = htons(ETH_P_MCTP);
> +		skb_reset_network_header(skb);
> +		cb = __mctp_cb(skb);
> +		cb->halen = 0;
> +		netif_rx(skb);
> +
> +		skb = skb2;
> +	}
> +
> +	if (skb)
> +		kfree_skb(skb);
> +
> +	mctp_usb_rx_queue(mctp_usb, GFP_ATOMIC);

What if we fail to allocate an skb ?
Admittedly the buffers are relatively small but if the allocation
fails we'd get stuck, no more packets will ever be received, right?
May be safer to allocate the skb first, and if it fails reuse the
skb that just completed (effectively discarding the incoming packets
until a replacement buffer can be allocated).

> +}

> +	dev->hard_header_len = sizeof(struct mctp_usb_hdr);
> +	dev->tx_queue_len = DEFAULT_TX_QUEUE_LEN;
> +	dev->addr_len = 0;
> +	dev->flags = IFF_NOARP;
> +	dev->netdev_ops = &mctp_usb_netdev_ops;
> +	dev->needs_free_netdev = false;

Is there a reason to set this to false?
dev memory is guaranteed to be zero'ed out.

> +	dev->pcpu_stat_type = NETDEV_PCPU_STAT_DSTATS;
> +}
-- 
pw-bot: cr