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=-8.3 required=3.0 tests=DKIM_INVALID,DKIM_SIGNED, HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_PATCH,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 F3875C37120 for ; Mon, 21 Jan 2019 20:38:26 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id B168720879 for ; Mon, 21 Jan 2019 20:38:26 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=fail reason="signature verification failed" (2048-bit key) header.d=googlemail.com header.i=@googlemail.com header.b="YrUxn+f2" Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1726694AbfAUUiZ (ORCPT ); Mon, 21 Jan 2019 15:38:25 -0500 Received: from mail-ed1-f65.google.com ([209.85.208.65]:34226 "EHLO mail-ed1-f65.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1725993AbfAUUiZ (ORCPT ); Mon, 21 Jan 2019 15:38:25 -0500 Received: by mail-ed1-f65.google.com with SMTP id b3so17679055ede.1 for ; Mon, 21 Jan 2019 12:38:23 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=googlemail.com; s=20161025; h=sender:date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to:user-agent; bh=vhNpk5OlnHcgSSnbI5y7VUQSl1K3mSA+8ZcjpFzkH6s=; b=YrUxn+f28DotevxA17x5fijtUV2In+vA2YMT3D0lsIDG0viU8qw/xvP+YfCSN7Gxs5 lFX+LYZi/HhVPDVOL0cKl5HSfeSSiqz6kWGUJ/yYFDfP45XFGPu1CN/9UXyLGrfq5N1v gCTGJccPlg8+UxwummGs4mQj3UM7wT2E2SMx6Jev+IPT8Uf4PcgRmYl4QNItm1rxBpHi sQNZoLMIcrGSrxvfUlV5p+LnV8btIN5PJsAdsGpn2sGS5wIAzRGwkrxqklghrLMrmYHy 8j1YLn8kDoDK2TXtaPU2O+HvfHoAjZjW1ubGXiuutS+wc2I/5jp8C5m6L7wSq/ZKa7LG 1Nxg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:sender:date:from:to:cc:subject:message-id :references:mime-version:content-disposition:in-reply-to:user-agent; bh=vhNpk5OlnHcgSSnbI5y7VUQSl1K3mSA+8ZcjpFzkH6s=; b=cLJY7mOQXZsnp6WtGECAPYV0tbJjE6kP5NH7wqI2iVjW2EmOj8MJbcobaQMi/dTNHb WTMw22ioYdxzrqnfKfPQJrL9Tk6nhCQ+NnI3i1n8gGeQIqAmW+uZhtrd3yM3EMEHBrDW ZyX3gAo5c3IDLIQDhKKkxfw2eyJMj9XqJUEtGsc3kbDARUyqv6lieOnG7A5AFTRFqTQE Quv8QsXxt9+6Kv8/J9407Pu6dScPz5Yv4BhJaL2VW9W8gJSDdl4kAs3zT+Y/npYk9H9J 6/XkYCAke78UHKteM9M65ztNVoETbXPjoPpOsXg91HKJLkLrlrOMdzufFgOl4E0Zy1WO +u/g== X-Gm-Message-State: AJcUukfh4HjiQpHkG22ZV0MUFmUP97gha82Ih/Sw7eoKyCXMiTuAnbCg 1xiZfHDQgUYQK64tqRxoxYA= X-Google-Smtp-Source: ALg8bN7tz48JlIAmgCOdIX79gRtwJr5qPoItTvPfHQtFLl+flLOFGXGEOdBbhm2EFeQjxJEOgY4rLQ== X-Received: by 2002:a50:f4c3:: with SMTP id v3mr28045873edm.196.1548103102357; Mon, 21 Jan 2019 12:38:22 -0800 (PST) Received: from riot ([2a02:810d:1500:e01:f02f:97ff:feb5:2c7d]) by smtp.gmail.com with ESMTPSA id j31sm9345926eda.46.2019.01.21.12.38.21 (version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256); Mon, 21 Jan 2019 12:38:21 -0800 (PST) Date: Mon, 21 Jan 2019 22:11:10 +0100 From: Zahari Doychev To: Toshiaki Makita Cc: Cong Wang , Linux Kernel Network Developers , bridge@lists.linux-foundation.org, Nikolay Aleksandrov , Roopa Prabhu , Johannes Berg , Jamal Hadi Salim , Jiri Pirko Subject: Re: [Bridge] [PATCH 1/2] net: bridge: fix tc added QinQ forwarding Message-ID: <20190121211110.GA8389@riot> References: <20190113135939.8970-1-zahari.doychev@linux.com> <20190113135939.8970-2-zahari.doychev@linux.com> <7dee9e67-bad5-84eb-9ff1-fa0d0aef8be0@lab.ntt.co.jp> <20190117081707.GA13853@riot> <28ee9bc4-ab7e-285f-50c4-685dd2468cf5@lab.ntt.co.jp> <1aae290d-2408-2bd0-cf8a-4dcc9ae6fbd8@lab.ntt.co.jp> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1aae290d-2408-2bd0-cf8a-4dcc9ae6fbd8@lab.ntt.co.jp> User-Agent: Mutt/1.11.1 (2018-12-01) Sender: netdev-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: netdev@vger.kernel.org On Fri, Jan 18, 2019 at 11:29:52AM +0900, Toshiaki Makita wrote: > On 2019/01/18 4:19, Cong Wang wrote: > > On Thu, Jan 17, 2019 at 12:59 AM Toshiaki Makita > > wrote: > >> On 2019/01/17 17:17, Zahari Doychev wrote: > >>> On Tue, Jan 15, 2019 at 03:11:28PM +0900, Toshiaki Makita wrote: > >>>> On 2019/01/13 22:59, Zahari Doychev wrote: > >>>>> Use the skb->mac_len instead of using the ETH_HLEN when pushing the skb > >>>>> data pointer. This fixes sending incorrect packets when more than one > >>>>> vlan tags are pushed by tc-vlan and the mac header length is bigger than > >>>>> ETH_HLEN. In this way the vlan tagged contained in the skb is inserted at > >>>>> right offset in the packet payload before sending the packet. > >>>>> > >>>>> Signed-off-by: Zahari Doychev > >>>>> --- > >>>>> net/bridge/br_forward.c | 2 +- > >>>>> 1 file changed, 1 insertion(+), 1 deletion(-) > >>>>> > >>>>> diff --git a/net/bridge/br_forward.c b/net/bridge/br_forward.c > >>>>> index 5372e2042adf..55f928043f77 100644 > >>>>> --- a/net/bridge/br_forward.c > >>>>> +++ b/net/bridge/br_forward.c > >>>>> @@ -39,7 +39,7 @@ int br_dev_queue_push_xmit(struct net *net, struct sock *sk, struct sk_buff *skb > >>>>> if (!is_skb_forwardable(skb->dev, skb)) > >>>>> goto drop; > >>>>> > >>>>> - skb_push(skb, ETH_HLEN); > >>>>> + skb_push(skb, skb->mac_len); > >>>>> br_drop_fake_rtable(skb); > >>>>> > >>>>> if (skb->ip_summed == CHECKSUM_PARTIAL && > >>>>> > >>>> > >>>> I guess you mean skb->data points to mac_header + ETH_HLEN + VLAN_HLEN > >>>> when bridge receives skbs in br_handle_frame()? > >>> > >>> yes, this is what I see. > >>> > >>>> If so, the behavior of act_vlan is odd. Normal double tagged skbs from > >>>> hardware devices should have skb->data pointing to mac_header + ETH_HLEN > >>>> because they just call eth_type_trans() before entering > >>>> netif_receive_skb()... > >>>> I think act_vlan needs some fix. > >>> > >>> The act_valn is using the skb_vlan_push(...) to add the vlan tags and in this > >>> way increasing the skb->data and mac_len. So I think I can add a fix there to > >>> set the skb->data to point to mac_header + ETH_HLEN when more tags are added. > >> > >> As skb->data always points to mac_header after calling skb_vlan_push(), > >> we probably need to remember mac_len before invocation of it? > >> > >> The problem should be this part in tcf_vlan_act(): > >> > >>> out: > >>> if (skb_at_tc_ingress(skb)) > >>> skb_pull_rcsum(skb, skb->mac_len); > >> > >> skb->mac_len should not be used here. > > > > I am confused. This code is to push skb->data back to network header. > > If skb_vlan_push() pushes skb->data to mac header, then this code > > is correct for pulling it back to network header, as skb->mac_len is > > updated accordingly inside skb_vlan_push() too. > > > > What goes wrong here? skb->mac_len isn't correct for double tagging? > > Bridge and VLAN code (skb_vlan_untag in __netif_receive_skb_core) > expects skb->data to point to the start of VLAN header, not the next > (network) header. After calling tcf_vlan_act() on ingress filter, > skb->data points to the next of VLAN header (network header), while > non-hwaccel VLAN packets (including double tagged ones) from NIC drivers > have skb->data pointing to the start of VLAN header as expected. > > I'm not sure if mac_len should or should not be updated in > skb_vlan_push(). Anyway IIRC skb_vlan_untag() and bridge code do not > rely on mac_len so mac_len should not cause problems there. Replacing the usage of the skb->maclen in act vlan with ETH_HLEN in the skb_push/pull calls fixes the problem. I have to do some more testing then I can send a new patch. I still see the problem in br_vlan __allowed_ingress when the skb->vlan_proto and br->vlan_proto does not match. If the network header is not reset then the flow dissector does not correctly detect the vlan tags. The the network header points to the second tag where as the skb->data points to the first one and the egress rule in my example cannot match. Is this the correct way to fix this and is __allowed_ingress the correct place? Thanks Zahari > > -- > Toshiaki Makita >