From mboxrd@z Thu Jan 1 00:00:00 1970 From: Eric Dumazet Subject: Re: qdisc_pkt_len_init: SCTP/GSO_BY_FRAGS and robustness questions Date: Tue, 06 Feb 2018 20:54:35 -0800 Message-ID: <1517979275.3715.155.camel@gmail.com> References: <871shxy4zv.fsf@linkitivity.dja.id.au> Mime-Version: 1.0 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: 7bit To: Daniel Axtens , Marcelo Ricardo Leitner , netdev@vger.kernel.org Return-path: Received: from mail-io0-f171.google.com ([209.85.223.171]:39449 "EHLO mail-io0-f171.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752524AbeBGEyi (ORCPT ); Tue, 6 Feb 2018 23:54:38 -0500 Received: by mail-io0-f171.google.com with SMTP id b198so585713iof.6 for ; Tue, 06 Feb 2018 20:54:38 -0800 (PST) In-Reply-To: <871shxy4zv.fsf@linkitivity.dja.id.au> Sender: netdev-owner@vger.kernel.org List-ID: 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 > >