[ofa-general] Re: [PATCH] IB/ipoib: fix thinko in packet length checks

[Michael S. Tsirkin]

> Quoting Roland Dreier <rdreier at cisco.com>:
> Subject: Re: [PATCH] IB/ipoib: fix thinko in packet length checks
> 
>  > > After this change, the code looks like:
>  > > 
>  > > 	if (unlikely(skb->len > tx->mtu)) {
>  > > 		ipoib_warn(priv, "packet len %d (> %d) too long to send, dropping\n",
>  > > 			   skb->len, tx->mtu);
>  > > 		++priv->stats.tx_dropped;
>  > > 		++priv->stats.tx_errors;
>  > > 		ipoib_cm_skb_too_long(dev, skb, tx->mtu - IPOIB_ENCAP_LEN);
>  > > 		return;
>  > > 	}
>  > > 
>  > > so why is the test against just tx->mtu, while ipoib_cm_skb_too_long()
>  > > is passed an mtu of tx->mtu - IPOIB_ENCAP_LEN?
>  > 
>  > I actually copied this from datagram code.
>  > 
>  > What ipoib_cm_skb_too_long does is set the dest MTU metric.
> 
> Right, but this code sets the dest MTU to tx->mtu - IPOIB_ENCAP_LEN if
> it sees a packet longer than tx->mtu.  Shouldn't the initial test be
> against tx->mtu - IPOIB_ENCAP_LEN too?

Packets that we get here include IPOIB_ENCAP_LEN, right?
But the MTU that we set is for length of datagram, so we
subtract IPOIB_ENCAP_LEN.

>  > So look at this logic in datagram mode:
>  > 	if (new_mtu > IPOIB_PACKET_SIZE - IPOIB_ENCAP_LEN) {
>  > 		return -EINVAL;
>  > 	}
>  > 
>  > Why is the max MTU set to IPOIB_PACKET_SIZE - IPOIB_ENCAP_LEN and
>  > not to IPOIB_PACKET_SIZE?
> 
> Because the largest MTU we can handle is the biggest message we can
> receive minus the 4 byte encapsulation overhead.  In other words, if
> we can handle 2048 byte UD messages, then we shouldn't allow a
> datagram MTU above 2044.  The same reasoning applies to ethernet, so
> an ethernet interface has an MTU of 1500, because ethernet can send
> 1514 byte packets but space has to be left for the 14 bytes of
> src/dest/ethertype fields.
> 
> Or am I missing something here?

So same here.

-- 
MST