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