Re: [PATCH net-next v5 00/12] net: mctp: usb: Add support for MCTP-over-USB v1.1

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

Hi all,

Some review feedback from sashiko, along with responses:

> --- Patch [2]: [PATCH net-next v5 02/12] net: mctp: usb: Use packet-length max for maximum packet-size check ---
> commit df3ce01edadf3abe9a5e04b1500317e960abd98f
> Author: Jeremy Kerr <jk@codeconstruct.com.au>
> 
> net: mctp: usb: Use packet-length max for maximum packet-size check
> 
> This commit updates the maximum packet size check in mctp-usb to use a new
> define based on the u8 transport header length field. It also applies this
> new maximum to the MTU calculation.
> 
> > diff --git a/drivers/net/mctp/mctp-usb.c b/drivers/net/mctp/mctp-usb.c
> > index 545eff06322ce..c6e36b63e87a0 100644
> > --- a/drivers/net/mctp/mctp-usb.c
> > +++ b/drivers/net/mctp/mctp-usb.c
> > @@ -76,7 +76,7 @@ static netdev_tx_t mctp_usb_start_xmit(struct sk_buff *skb,
> >  
> >  	plen = skb->len;
> >  
> > -	if (plen + sizeof(*hdr) > MCTP_USB_1_0_XFER_SIZE)
> > +	if (plen + sizeof(*hdr) > MCTP_USB_1_0_PKTLEN_MAX)
> >  		goto err_drop;
> >  
> >  	rc = skb_cow_head(skb, sizeof(*hdr));
> 
> This isn't a bug introduced by this patch, but could a transient USB error
> during TX permanently stall the network device's transmit queue?

This is a shortcoming in the existing tx path, and is addressed by patch
05/12.

[...]

> This is also a pre-existing issue, but does the RX polling loop stop
> permanently on transient USB errors?

... and this by patch 04/12.


> --- Patch [5]: [PATCH net-next v5 05/12] net: mctp: usblib: Move TX transfer processing to mctp-usblib ---
> commit ee53d6679c1c34404c4b886475dd086d680df184
> Author: Jeremy Kerr <jk@codeconstruct.com.au>
> 
> net: mctp: usblib: Move TX transfer processing to mctp-usblib
> 
> This commit moves the TX processing alongside RX processing in mctp-usblib
> to accommodate packed transfers where a transfer may contain multiple MCTP
> packets. It splits the transmit API into push, send, and complete stages
> and fixes an inconsistency between tx and rx stats.
> 
> > diff --git a/drivers/net/mctp/mctp-usblib.c b/drivers/net/mctp/mctp-usblib.c
> > --- a/drivers/net/mctp/mctp-usblib.c
> > +++ b/drivers/net/mctp/mctp-usblib.c
> [ ... ]
> > +static int mctp_usblib_tx_skb_prepare(struct sk_buff *skb,
> > +				      enum skb_drop_reason *reason)
> > +{
> > +	struct mctp_usb_hdr *hdr;
> > +	unsigned long plen;
> > +	int rc;
> [ ... ]
> > +	hdr = skb_push(skb, sizeof(*hdr));
> > +	if (!hdr) {
> > +		*reason = SKB_DROP_REASON_NOMEM;
> > +		return -ENOMEM;
> > +	}
> > +
> > +	hdr->id = cpu_to_be16(MCTP_USB_DMTF_ID);
> > +	hdr->rsvd = 0;
> > +	hdr->len = plen + sizeof(*hdr);
> 
> This isn't a bug introduced by this patch, but does this assignment miss a
> cpu_to_be16() conversion?
> 
> The len field in struct mctp_usb_hdr is defined as __be16, but it appears to
> be assigned a native integer value here.

At this point in the series, hdr->len is a u8, not __be16.

We change this to a __be16 two patches later in 07/12, and at the same
time add the necessary endian conversion to all uses of hdr->len.

> --- Patch [10]: [PATCH net-next v5 10/12] net: mctp: usblib: Add initial kunit tests ---
> commit 6e78640ea8bf2b13bb32c513ae1e30a04bebf125
> Author: Jeremy Kerr <jk@codeconstruct.com.au>
> 
> net: mctp: usblib: Add initial kunit tests
> 
> This commit adds initial KUnit tests for the MCTP USB library receive
> path to verify extraction of MCTP packets from incoming USB transfer data.
> 
> > diff --git a/drivers/net/mctp/mctp-usblib-test.c b/drivers/net/mctp/mctp-usblib-test.c
> > new file mode 100644
> > --- /dev/null
> > +++ b/drivers/net/mctp/mctp-usblib-test.c
> 
> [ ... ]
> 
> > +static int mctp_usblib_test_dst_output(struct mctp_dst *dst,
> > +				       struct sk_buff *skb)
> > +{
> > +	struct mctp_usblib_test_dev *dev = netdev_priv(skb->dev);
> > +
> > +	skb_queue_tail(&dev->rx_pkts, skb);
> > +
> > +	return 0;
> > +}
> 
> Could this lead to memory corruption if an external packet is routed here?

This is testcase code, so intentionally simplified, and typically
running within the kunit harness. We should not be receiving or routing
external packets at this point.

Cheers,


Jeremy