From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-5.5 required=3.0 tests=HEADER_FROM_DIFFERENT_DOMAINS, MAILING_LIST_MULTI,SIGNED_OFF_BY,SPF_PASS,USER_AGENT_MUTT autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id BB105C282C3 for ; Thu, 24 Jan 2019 16:01:15 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id 8E776217D7 for ; Thu, 24 Jan 2019 16:01:15 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1728726AbfAXQBO (ORCPT ); Thu, 24 Jan 2019 11:01:14 -0500 Received: from mail-wm1-f66.google.com ([209.85.128.66]:37593 "EHLO mail-wm1-f66.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1728465AbfAXQBO (ORCPT ); Thu, 24 Jan 2019 11:01:14 -0500 Received: by mail-wm1-f66.google.com with SMTP id g67so3611038wmd.2 for ; Thu, 24 Jan 2019 08:01:13 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:from:date:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to:user-agent; bh=UkbHARNRrmR+eeRcaTJwDe6Z2M8gYamQTB5sandah3s=; b=K5HgeCsLM/jC2S8/NoPKc2VqdHkZvhrHqwiJDw7WGaiHIV7cB81u6Kir+/LEmDY7/a FpxsrS8tKOQp4LOGYa5PwJabkGyc0T1208KWoQfae3wBMyIAo6Qg2v1Kuj1ZNYTjAFsI QHItmqr4LyAUtKc2bwV8LU9zSbvwK3i0eiFSvBXRaJvVJAmh+OTd5iaU0iCuCuZSjlSy vqt63jXd0IsgVvn/kp8z9JK4u5TY2V8CcaHBi+sk8ofukw/3neCIfE9uucUb0jkfUG8O 5g75/CDv00q8UL7BG7Q8VXrmNz5Beq8mA5e5+JJZEPw92drS9JJ2AizmmvTXceX/LAIM hugg== X-Gm-Message-State: AJcUukfw0LQOk5ItwO5STX1qKzj2M85mqfTSahq2WkaIlTmDrXgK3XVG jAgy4Ve5Lt+BzQEbQoRnyeYK0Q== X-Google-Smtp-Source: ALg8bN5sIqcNKhtapt4q3R+DBcvf3PRiGjsMYx1fG6iGSouD66qLgPpDwNDBS6cFAbjiVbky9g/7Dw== X-Received: by 2002:a1c:be11:: with SMTP id o17mr3160599wmf.111.1548345671982; Thu, 24 Jan 2019 08:01:11 -0800 (PST) Received: from localhost.localdomain (ovpn-brq.redhat.com. [213.175.37.11]) by smtp.gmail.com with ESMTPSA id 127sm113756262wmm.45.2019.01.24.08.01.10 (version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256); Thu, 24 Jan 2019 08:01:11 -0800 (PST) From: Guillaume Nault X-Google-Original-From: Guillaume Nault Date: Thu, 24 Jan 2019 17:01:09 +0100 To: Jacob Wen Cc: netdev@vger.kernel.org, eric.dumazet@gmail.com, g.nault@alphalink.fr Subject: Re: [PATCH net-next v2] net: l2tp: fix reading optional fields of L2TPv3 Message-ID: <20190124160107.GA6470@localhost.localdomain> References: <20190124074917.31173-1-jian.w.wen@oracle.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20190124074917.31173-1-jian.w.wen@oracle.com> User-Agent: Mutt/1.10.1 (2018-07-13) Sender: netdev-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: netdev@vger.kernel.org On Thu, Jan 24, 2019 at 03:49:17PM +0800, Jacob Wen wrote: > Use pskb_may_pull() to make sure the optional fields are in skb linear > parts, so we can safely read them later. > > It's easy to reproduce the issue with a net driver that supports paged > skb data. Just create a L2TPv3 over IP tunnel and then generates some > network traffic. > Once reproduced, rx err in /sys/kernel/debug/l2tp/tunnels will increase. > > Signed-off-by: Jacob Wen > --- > Changes in v2: > 1. Only fix L2TPv3 to make code simple. > To fix both L2TPv3 and L2TPv2, we'd better refactor l2tp_recv_common. > It's complicated to do so. > Yes, the L2TP data path definitely needs some care. But for a one-off patch like this, it'd probably make more sense to respect the current code structure instead of adding yet more special cases. I mean, l2tp_recv_common() assumes that it can safely access the L2TP header: pskb_may_pull() is done in l2tp_udp_recv_core() (which probably should pull more bytes in case the length field is present BTW). It's up to l2tp_ip (and l2tp_ip6) to respect this requirement, so that's where pskb_may_pull() should be done. Yes it'd be better to linearise data close to the place we access them, but that'd be long term refactoring. If we don't have the resources to do that, let's just, at least keep some consistency.