From patchwork Thu Jul 11 07:08:26 2024 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Boris Sukholitko X-Patchwork-Id: 13730075 Received: from mail-qv1-f54.google.com (mail-qv1-f54.google.com [209.85.219.54]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 4B35012F386 for ; Thu, 11 Jul 2024 07:08:46 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=209.85.219.54 ARC-Seal: i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1720681728; cv=none; b=ZcOQga6r5Svmcy+jPNXoYUDcAth5QpwzZRfrip0APz+7qgSrvAwG2nms+xHbKIUUyKT0OdZgkGvyQ/YV93oJeQNX7/tHN6xMzdxC0l2e70IM8gjPtadoIHy9Qd96AKeu3usflOlH15AQi4v++4p85sgobh7LhSzqs5oURVUe1yg= ARC-Message-Signature: i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1720681728; c=relaxed/simple; bh=KGUumUGC7w9rFo3N9lC4m1NP7MQTpbG3Lin9+IoUoKs=; h=From:To:Cc:Subject:Date:Message-Id:MIME-Version:Content-Type; b=DQvvgG06+tGoLcdStCuD4UWz1/hQ4DMiZV0D/QRoDMZH8FixBorqoLx3+C37Mv0aNdajP6/MlTb5xk6Kl5LMJi1phedRXQDq5jAU0UZx/OWaSugnIW/5PNvt8xNBP1phkP58+nM0+2NK5gdaIJ5wFkRiNY0u1lS/8+5JqXVu3s4= ARC-Authentication-Results: i=1; smtp.subspace.kernel.org; dmarc=pass (p=quarantine dis=none) header.from=broadcom.com; spf=fail smtp.mailfrom=broadcom.com; dkim=pass (1024-bit key) header.d=broadcom.com header.i=@broadcom.com header.b=YkqMpXKV; arc=none smtp.client-ip=209.85.219.54 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=quarantine dis=none) header.from=broadcom.com Authentication-Results: smtp.subspace.kernel.org; spf=fail smtp.mailfrom=broadcom.com Authentication-Results: smtp.subspace.kernel.org; dkim=pass (1024-bit key) header.d=broadcom.com header.i=@broadcom.com header.b="YkqMpXKV" Received: by mail-qv1-f54.google.com with SMTP id 6a1803df08f44-6b6199ef089so3341206d6.3 for ; Thu, 11 Jul 2024 00:08:46 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=broadcom.com; s=google; t=1720681724; x=1721286524; darn=vger.kernel.org; h=mime-version:message-id:date:subject:cc:to:from:from:to:cc:subject :date:message-id:reply-to; bh=SmUjbyVVSDNMYkoqKB6mASyDtjHtnWE952N7/kTZtfk=; b=YkqMpXKVj9W7qmNu2Yld1WWE618RhoWkIUYdgyzx2dnSWMgygC3dxyYysqd1nNjGxN 5vlLLfi7RW16aTEHQeJCsIFYIYRQv20wuo/DkuM/MS01/FiGJy65mnEx5OyxLA9AYVpR 8TWA0R3j9/t8xXpRY95AYY5BobVyAp67Bwh5I= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1720681724; x=1721286524; h=mime-version:message-id:date:subject:cc:to:from:x-gm-message-state :from:to:cc:subject:date:message-id:reply-to; bh=SmUjbyVVSDNMYkoqKB6mASyDtjHtnWE952N7/kTZtfk=; b=rbt4vYqQAEjhZ4bTbYNeGgRq8RgXlsWRz1diqgr3X+Au00DdCY7Cslhy+VqXZ9rpM/ Od51QsOAkok2RRqeMzz407kRUnPPGhmgxpti2TFakVnMcs/SwZdnjwsP2U4W9JPTj0EV 6qcLb4mCDz8Fg4dgAv251/khtihMDCrTvJyEjSPw2+uVQs/QQft6SSAMdyFLsWW2h3tV OH/4c0LpDZGK1VvqxQ7rAs1Gfc/7MGTzTDVg51X3bA1xd/aDHtufqEjQiUOTfAxOhyIh gXkxhbf7u20gxEV3cn8bYNDwQ9nPsUt1nDvzbePQpuQnZgsivB55CAjNiohXiC5E361X 2RXg== X-Gm-Message-State: AOJu0YxWT4GEOwafkukLnOUKhd7t7iU9Z71lu98fyPWZ3le864yVxrvv +Fe0nM4q8laSkKz4jgseFfECb1AFRkhC/B5HHIrMKGwNY3EefOSxZWzVgletz2+fuJV7e5xt890 X3APZyi9vMAjOBv7rLTh0Qw01Nm+fdGjQG13i8YaGGKWTXrIZodmuuqOb5KJTTqenmjXpQbm0eC KEIpJRa4IsyMscmC5CcXV1BjnCbWzzx8mosW2xDTHCi75ufPM3 X-Google-Smtp-Source: AGHT+IH3CV6pC/HQcbwAS8scj4Yn5T8eRm0VLsLPUlx9bqk+RNlzzrmenJGvIMcdueIPys0qgJp5Tg== X-Received: by 2002:a05:6214:c43:b0:6b7:42bd:99e9 with SMTP id 6a1803df08f44-6b742bd9e21mr38228856d6.35.1720681724499; Thu, 11 Jul 2024 00:08:44 -0700 (PDT) Received: from localhost.localdomain ([192.19.250.250]) by smtp.gmail.com with ESMTPSA id 6a1803df08f44-6b61ba8d86asm23090006d6.125.2024.07.11.00.08.38 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Thu, 11 Jul 2024 00:08:43 -0700 (PDT) From: Boris Sukholitko To: netdev@vger.kernel.org, Alexei Starovoitov , Daniel Borkmann , Andrii Nakryiko , Martin KaFai Lau , Eduard Zingerman , Song Liu , Yonghong Song , John Fastabend , KP Singh , Stanislav Fomichev , Hao Luo , Jiri Olsa , "David S . Miller" , Eric Dumazet , Jakub Kicinski , Paolo Abeni , Pravin B Shelar , Jamal Hadi Salim , Cong Wang , Jiri Pirko , Willem de Bruijn , Simon Horman , Florian Westphal , Mina Almasry , Abhishek Chauhan , David Howells , Alexander Lobakin , Pavel Begunkov , Lorenzo Bianconi , =?utf-8?q?Thomas_Wei=C3=9Fschuh?= Cc: Ilya Lifshits Subject: [PATCH net-next 0/2] tc: adjust network header after second vlan push Date: Thu, 11 Jul 2024 10:08:26 +0300 Message-Id: <20240711070828.2741351-1-boris.sukholitko@broadcom.com> X-Mailer: git-send-email 2.38.1 Precedence: bulk X-Mailing-List: netdev@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 X-Patchwork-Delegate: kuba@kernel.org skb network header of the single-tagged vlan packet continues to point the vlan payload (e.g. IP) after second vlan tag is pushed by tc act_vlan. This causes problem at the dissector which expects double-tagged packet network header to point to the inner vlan. The fix is to adjust network header in tcf_act_vlan.c but requires minor changes to the generic skb_vlan_push function. Consider the following shell script snippet configuring TC rules on the veth interface: ip link add veth0 type veth peer veth1 ip link set veth0 up ip link set veth1 up tc qdisc add dev veth0 clsact tc filter add dev veth0 ingress pref 10 chain 0 flower \ num_of_vlans 2 cvlan_ethtype 0x800 action goto chain 5 tc filter add dev veth0 ingress pref 20 chain 0 flower \ num_of_vlans 1 action vlan push id 100 \ protocol 0x8100 action goto chain 5 tc filter add dev veth0 ingress pref 30 chain 5 flower \ num_of_vlans 2 cvlan_ethtype 0x800 action simple sdata "success" Sending double-tagged vlan packet with the IP payload inside: cat <protocol; } else { vlan = __skb_header_pointer(skb, nhoff, sizeof(_vlan), data, hlen, &_vlan); if (!vlan) { fdret = FLOW_DISSECT_RET_OUT_BAD; break; } proto = vlan->h_vlan_encapsulated_proto; nhoff += sizeof(*vlan); } The "else" clause above gets the protocol of the encapsulated packet from the skb data at the network header location. printk debugging has showed that in the good double-tagged packet case proto is htons(0x800 == ETH_P_IP) as expected. However in the single-tagged packet case proto is garbage leading to the failure to match tc filter 30. proto is being set from the skb header pointed by nhoff parameter which is defined at the beginning of __skb_flow_dissect (net/core/flow_dissector.c:1055 in the current version): nhoff = skb_network_offset(skb); Therefore the culprit seems to be that the skb network offset is different between double-tagged packet received from the interface and single-tagged packet having its vlan tag pushed by TC. Lets look at the interesting points of the lifetime of the single/double tagged packets as they traverse our packet flow. Both of them will start at __netif_receive_skb_core where the first vlan tag will be stripped: if (eth_type_vlan(skb->protocol)) { skb = skb_vlan_untag(skb); if (unlikely(!skb)) goto out; } At this stage in double-tagged case skb->data points to the second vlan tag while in single-tagged case skb->data points to the network (eg. IP) header. Looking at TC vlan push action (net/sched/act_vlan.c) we have the following code at tcf_vlan_act (interesting points are in square brackets): if (skb_at_tc_ingress(skb)) [1] skb_push_rcsum(skb, skb->mac_len); .... case TCA_VLAN_ACT_PUSH: err = skb_vlan_push(skb, p->tcfv_push_proto, p->tcfv_push_vid | (p->tcfv_push_prio << VLAN_PRIO_SHIFT), 0); if (err) goto drop; break; .... out: if (skb_at_tc_ingress(skb)) [3] skb_pull_rcsum(skb, skb->mac_len); And skb_vlan_push (net/core/skbuff.c:6204) function does: err = __vlan_insert_tag(skb, skb->vlan_proto, skb_vlan_tag_get(skb)); if (err) return err; skb->protocol = skb->vlan_proto; [2] skb->mac_len += VLAN_HLEN; in the case of pushing the second tag. Lets look at what happens with skb->data of the single-tagged packet at each of the above points: 1. As a result of the skb_push_rcsum, skb->data is moved back to the start of the packet. 2. First VLAN tag is moved from the skb into packet buffer, skb->mac_len is incremented, skb->data still points to the start of the packet. 3. As a result of the skb_pull_rcsum, skb->data is moved forward by the modified skb->mac_len, thus pointing to the network header again. Then __skb_flow_dissect will get confused by having double-tagged vlan packet with the skb->data at the network header. Our solution for the bug is to preserve "skb->data at second vlan header" semantics in the act_vlan.c. We do this by passing skb->mac_len advancement as an argument to skb_vlan_push (first patch in the series). Second patch modifies act_vlan.c to disable skb->mac_len increment and do network header adjustment by itself. Thanks, Boris. Boris Sukholitko (2): skb: skb_vlan_push gets VLAN_HLEN as an argument tc vlan: adjust network header in tcf_vlan_act include/linux/skbuff.h | 2 +- net/core/filter.c | 2 +- net/core/skbuff.c | 4 ++-- net/openvswitch/actions.c | 3 ++- net/sched/act_vlan.c | 8 ++++++-- 5 files changed, 12 insertions(+), 7 deletions(-)