| Message ID | 20240805105649.1944132-1-boris.sukholitko@broadcom.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | tc: adjust network header after second vlan push | expand |
On Mon, 5 Aug 2024 13:56:44 +0300 Boris Sukholitko wrote: > More about the patch series: > > * patches 1-3 refactor skb_vlan_push to make skb_vlan_flush helper > * patch 4 open codes skb_vlan_push in act_vlan.c > * patch 5 contains the actual fix The series is structured quite nicely for review, so kudos for that. But I'm not seeing the motivation for changing how TC pushes VLANs and not changing OvS (or BPF?), IOW the other callers of skb_vlan_push(). Why would pushing a tag from TC actions behave differently? Please also add your test case to tools/testing/selftests/net/forwarding/tc_actions.sh if you can.
On Mon, Aug 12, 2024 at 05:40:47PM -0700, Jakub Kicinski wrote: > On Mon, 5 Aug 2024 13:56:44 +0300 Boris Sukholitko wrote: > > More about the patch series: > > > > * patches 1-3 refactor skb_vlan_push to make skb_vlan_flush helper > > * patch 4 open codes skb_vlan_push in act_vlan.c > > * patch 5 contains the actual fix > > The series is structured quite nicely for review, so kudos for that. > But I'm not seeing the motivation for changing how TC pushes VLANs > and not changing OvS (or BPF?), IOW the other callers of > skb_vlan_push(). > > Why would pushing a tag from TC actions behave differently? IMHO, the difference between TC and OvS and BPF is that in the TC case the dissector is invoked on the wrong position in the packet (IP vs L2 header). We can regard reading garbage from there as a bug. I am not sure that this is the case in OvS or BPF. E.g. in the BPF case there may some script expecting the skb to point to an IP header after second vlan push. My change will break it. > > Please also add your test case to > tools/testing/selftests/net/forwarding/tc_actions.sh > if you can. Done in v2. Thanks, Boris.
On Wed, 14 Aug 2024 16:07:14 +0300 Boris Sukholitko wrote: > > The series is structured quite nicely for review, so kudos for that. > > But I'm not seeing the motivation for changing how TC pushes VLANs > > and not changing OvS (or BPF?), IOW the other callers of > > skb_vlan_push(). > > > > Why would pushing a tag from TC actions behave differently? > > IMHO, the difference between TC and OvS and BPF is that in the TC case > the dissector is invoked on the wrong position in the packet (IP vs L2 > header). We can regard reading garbage from there as a bug. > > I am not sure that this is the case in OvS or BPF. E.g. in the BPF > case there may some script expecting the skb to point to an IP header > after second vlan push. My change will break it. The packet either has correct format or it doesn't. You could easily construct a TC ruleset which pushes the VLAN using act_bpf, instead of act_vlan. Let's not be too conservative, worrying about very unlikely regressions, IMHO. Such divergence makes the code base much harder to maintainer. > > Please also add your test case to > > tools/testing/selftests/net/forwarding/tc_actions.sh > > if you can. > > Done in v2. Please do not respond to a discussion and immediate send the next version.
On Wed, Aug 14, 2024 at 07:39:50AM -0700, Jakub Kicinski wrote: > On Wed, 14 Aug 2024 16:07:14 +0300 Boris Sukholitko wrote: > > > The series is structured quite nicely for review, so kudos for that. > > > But I'm not seeing the motivation for changing how TC pushes VLANs > > > and not changing OvS (or BPF?), IOW the other callers of > > > skb_vlan_push(). > > > > > > Why would pushing a tag from TC actions behave differently? > > > > IMHO, the difference between TC and OvS and BPF is that in the TC case > > the dissector is invoked on the wrong position in the packet (IP vs L2 > > header). We can regard reading garbage from there as a bug. > > > > I am not sure that this is the case in OvS or BPF. E.g. in the BPF > > case there may some script expecting the skb to point to an IP header > > after second vlan push. My change will break it. > > The packet either has correct format or it doesn't. You could easily > construct a TC ruleset which pushes the VLAN using act_bpf, instead of > act_vlan. > > Let's not be too conservative, worrying about very unlikely > regressions, IMHO. Such divergence makes the code base much harder > to maintainer. > I agree. I'll prepare v3 version with the changes folded into skb_vlan_push, updating the callers. > > > Please also add your test case to > > > tools/testing/selftests/net/forwarding/tc_actions.sh > > > if you can. > > > > Done in v2. > > Please do not respond to a discussion and immediate send the next > version. OK. Thanks, Boris.
<tldr> 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 refactoring of skb_vlan_push function. </tldr> 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 <<ENDS | text2pcap - - | tcpreplay -i veth1 - 0000 00 00 00 00 00 11 00 00 00 00 00 22 81 00 00 64 ..........."...d 0010 81 00 00 14 08 00 45 04 00 26 04 d2 00 00 7f 11 ......E..&...... 0020 18 ef 0a 00 00 01 14 00 00 02 00 00 00 00 00 12 ................ 0030 e1 c7 00 00 00 00 00 00 00 00 00 00 ............ ENDS will match rule 10, goto rule 30 in chain 5 and correctly emit "success" to the dmesg. OTOH, sending single-tagged vlan packet: cat <<ENDS | text2pcap - - | tcpreplay -i veth1 - 0000 00 00 00 00 00 11 00 00 00 00 00 22 81 00 00 14 ...........".... 0010 08 00 45 04 00 2a 04 d2 00 00 7f 11 18 eb 0a 00 ..E..*.......... 0020 00 01 14 00 00 02 00 00 00 00 00 16 e1 bf 00 00 ................ 0030 00 00 00 00 00 00 00 00 00 00 00 00 ............ ENDS will match rule 20, will push the second vlan tag but will *not* match rule 30. IOW, the match at rule 30 fails if the second vlan was freshly pushed by the kernel. Lets look at __skb_flow_dissect working on the double-tagged vlan packet. Here is the relevant code from around net/core/flow_dissector.c:1277 copy-pasted here for convenience: if (dissector_vlan == FLOW_DISSECTOR_KEY_MAX && skb && skb_vlan_tag_present(skb)) { proto = skb->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 manipulating skb->network_header rather than skb->mac_len in act_vlan.c. skb_reset_mac_len is done at the end. More about the patch series: * patches 1-3 refactor skb_vlan_push to make skb_vlan_flush helper * patch 4 open codes skb_vlan_push in act_vlan.c * patch 5 contains the actual fix Thanks, Boris. Boris Sukholitko (5): skb: add skb_vlan_flush helper skb: move mac_len adjustment out of skb_vlan_flush skb: export skb_vlan_flush act_vlan: open code skb_vlan_push act_vlan: adjust network header include/linux/skbuff.h | 1 + net/core/skbuff.c | 38 ++++++++++++++++++++++++-------------- net/sched/act_vlan.c | 12 +++++++++--- 3 files changed, 34 insertions(+), 17 deletions(-)