Re: qdisc_pkt_len_init: SCTP/GSO_BY_FRAGS and robustness questions

[Marcelo Ricardo Leitner <marcelo.leitner@gmail.com>]

On Wed, Feb 07, 2018 at 12:15:32PM +1100, Daniel Axtens wrote:
> Hi Marcelo and Eric,
> 
> I'm working on checking code that might be impacted by GSO_BY_FRAGS -
> after finding that the token bucket filter qdisc code doesn't handle it
> properly, DaveM said I should look for other places where this might be
> an issue [0].
> 
> I'm currently looking at qdisc_pkt_len_init in net/core/dev.c. This is
> called by __dev_queue_xmit, before validate_xmit_skb, so before an SCTP
> skb would be segmented if the hardware doesn't support SCTP offload.
> 
> There are two things I was hoping you two could offer some advice on:
> 
> 1) Eric, in 7c68d1a6b4db ("net: qdisc_pkt_len_init() should be more
>    robust") you replaced a chunk of code that is similar to the code
>    found in skb_gso_transport_seglen() and replaced it with more robust
>    code. Do we need to change skb_gso_transport_seglen() in a similar way?
> 
> 2) Marcelo, unlike skb_gso_transport_seglen(), where you added a case
>    for SCTP in 90017accff61 ("sctp: Add GSO support"), there doesn't
>    seem to be a GSO_BY_FRAGS or SCTP check in qdisc_pkt_len_init, so I
>    think the accounting is probably wrong for SCTP. I'm not 100% sure

Right, it's wrong, but by not much.

>    how to fix this as it's now quite different from the calcuations in
>    skb_gso_transport_seglen() - so I was hoping that you might have an
>    idea.

If we can trust gso_segs, we can do similar math. The only thing that
is missing in there is to account for sctphdr where it is currently
defaulting to udphdr. Like:

@@ -3169,6 +3169,8 @@ static void qdisc_pkt_len_init(struct sk_buff *skb)
                /* + transport layer */
                if (likely(shinfo->gso_type & (SKB_GSO_TCPV4 | SKB_GSO_TCPV6)))
                        hdr_len += tcp_hdrlen(skb);
+               else if (shinfo->gso_type & SKB_GSO_SCTP)
+                       hdr_len += sizeof(struct sctphdr);
                else
                        hdr_len += sizeof(struct udphdr);

The DODGY case is more complicated, because as we can't trust
gso_segs, we can't know how many packets/headers will be needed. If we
take care of the input places as Eric said, this case disappears,
otherwise I'm afraid we have to traverse the frags to find out how
many packets it represents.

> 
> Thanks in advance!
> 
> [0]: https://patchwork.ozlabs.org/patch/869145/#1852414
> 
> Regards,
> Daniel
> 
>