From mboxrd@z Thu Jan 1 00:00:00 1970 From: Marcelo Ricardo Leitner Subject: Re: qdisc_pkt_len_init: SCTP/GSO_BY_FRAGS and robustness questions Date: Wed, 7 Feb 2018 15:54:13 -0200 Message-ID: <20180207175413.GD7402@localhost.localdomain> References: <871shxy4zv.fsf@linkitivity.dja.id.au> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Cc: netdev@vger.kernel.org, Eric Dumazet To: Daniel Axtens Return-path: Received: from mail-qt0-f173.google.com ([209.85.216.173]:38689 "EHLO mail-qt0-f173.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753896AbeBGRyT (ORCPT ); Wed, 7 Feb 2018 12:54:19 -0500 Received: by mail-qt0-f173.google.com with SMTP id z10so2773289qti.5 for ; Wed, 07 Feb 2018 09:54:19 -0800 (PST) Content-Disposition: inline In-Reply-To: <871shxy4zv.fsf@linkitivity.dja.id.au> Sender: netdev-owner@vger.kernel.org List-ID: 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 > >