qdisc_pkt_len_init: SCTP/GSO_BY_FRAGS and robustness questions

qdisc_pkt_len_init: SCTP/GSO_BY_FRAGS and robustness questions

[Daniel Axtens]

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
   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.

Thanks in advance!

[0]: https://patchwork.ozlabs.org/patch/869145/#1852414

Regards,
Daniel

Re: qdisc_pkt_len_init: SCTP/GSO_BY_FRAGS and robustness questions

[Eric Dumazet]

On Wed, 2018-02-07 at 12:15 +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?

I would prefer we get rid of DODGY ability to provide buggy packets.

It would be silly to 'fix' all the places in the kernel (like hundred
of drivers I guess), while we can simply validate packets at the time
they are provided by malicious sources (user space using
af_packet/tun/virtio_net ...)


> 
> 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
>    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.
> 
> Thanks in advance!
> 
> [0]: https://patchwork.ozlabs.org/patch/869145/#1852414
> 
> Regards,
> Daniel
> 
> 

Re: qdisc_pkt_len_init: SCTP/GSO_BY_FRAGS and robustness questions

[Marcelo Ricardo Leitner]

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