| Message ID | 20241216-no-vlan-0-v1-1-62586f97fd88@narfation.org (mailing list archive) |
|---|---|
| State | Accepted, archived |
| Delegated to: | Antonio Quartulli |
| Headers |
Return-Path: <b.a.t.m.a.n-bounces+patchwork=open-mesh.org@lists.open-mesh.org> X-Original-To: patchwork@open-mesh.org Delivered-To: patchwork@open-mesh.org Received: from diktynna.open-mesh.org (localhost [IPv6:::1]) by diktynna.open-mesh.org (Postfix) with ESMTP id 585A183FAA for <patchwork@open-mesh.org>; Mon, 16 Dec 2024 19:38:24 +0100 (CET) ARC-Seal: i=2; cv=pass; a=rsa-sha256; d=open-mesh.org; s=20121; t=1734374304; b=irhfAdR5YluLwEzLcnuoSoG7N90xJXpn8cHL0K9EsA4W5mpS3zk8Jss6iSnsnQ6fl/fz8 DOFIi4Eb72DkI+6kOyg9mZM4wl0HWcTUABNC94uwNxRhMtibUYe1KAK3F8XSJ/3u/rDlBP5 L54C/1/imxMSCvgHxbXQhQNNQmID+qU= ARC-Message-Signature: i=2; a=rsa-sha256; c=relaxed/relaxed; d=open-mesh.org; s=20121; t=1734374304; h=from : sender : reply-to : subject : date : message-id : to : cc : mime-version : content-type : content-transfer-encoding : content-id : content-description : resent-date : resent-from : resent-sender : resent-to : resent-cc : resent-message-id : in-reply-to : references : list-id : list-help : list-unsubscribe : list-subscribe : list-post : list-owner : list-archive; bh=WV7YsBo72+haMjufm3ReCWM30T9c3oBk4qv40HoeU0I=; b=l7wQWn661KJw7cMatQRnH2FB4JketcNySP33pmmtFOwzVtQ2Lei5SS/TQxLoh/NUQwNEY VW+00xYrnmthv4EPYQB0YuMC238nfQfFsvDwJc1a9vcoBNnYRKo04gr3/SOrXiat9tom3T9 Hpzrqa5fendko8m/onqhY6qcxyC4bbI= ARC-Authentication-Results: i=2; open-mesh.org; dkim=pass header.d=narfation.org; arc=pass; dmarc=pass (Used From Domain Record) header.from=narfation.org policy.dmarc=none Authentication-Results: open-mesh.org; dkim=pass header.d=narfation.org; arc=pass; dmarc=pass (Used From Domain Record) header.from=narfation.org policy.dmarc=none Received: from dvalin.narfation.org (dvalin.narfation.org [IPv6:2a00:17d8:100::8b1]) by diktynna.open-mesh.org (Postfix) with ESMTPS id 4932C80F42 for <b.a.t.m.a.n@lists.open-mesh.org>; Mon, 16 Dec 2024 19:37:38 +0100 (CET) ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=open-mesh.org; s=20121; t=1734374258; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding:dkim-signature; bh=WV7YsBo72+haMjufm3ReCWM30T9c3oBk4qv40HoeU0I=; b=qIk+X1FFMGKXjlR5ZdbrZGSXC+LAfqTFbPFdtE92OtCp+XwGkr0IG1TgcJy8bXyRS7DYxN vxS3jeKrQVeOJi/KoA27RPQ7K424o+xusNB3TLFMiWhIGRPlBpMkE0BAg9s7uvgzXYRBC5 agVeU2HTGKMgVSSroysCtFn873eZywc= ARC-Seal: i=1; s=20121; d=open-mesh.org; t=1734374258; a=rsa-sha256; cv=none; b=pO8VnNAFx0dXpMfPqQf4Bpw0xDB54F8AqzsiAv9B3Vm9KgbtbaAnbpXZ6b/WvohLbKnqP5 CCPkT+4Rvy1sRa9LI3Zrcb5Z/wPhZQ0FwiaBKvriKJuVu65W6DWafNBsPkn0SIfcCiXBHw 97WutWevZVGep7fZlGnPYgvx+FM4HYI= ARC-Authentication-Results: i=1; diktynna.open-mesh.org; dkim=pass header.d=narfation.org header.s=20121 header.b=ZvtzBLAQ; spf=pass (diktynna.open-mesh.org: domain of sven@narfation.org designates 2a00:17d8:100::8b1 as permitted sender) smtp.mailfrom=sven@narfation.org; dmarc=pass (policy=none) header.from=narfation.org DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=narfation.org; s=20121; t=1734374257; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding; bh=WV7YsBo72+haMjufm3ReCWM30T9c3oBk4qv40HoeU0I=; b=ZvtzBLAQzg7/Ek5lXlWBz0eWeaf21uqS+ta1aexypKZuHuvbwbnafZAPXZf2wRnjh/eL97 uGF5i46kIAIYUIUvk4rUZP/CPMzf11evWoRSdNX8+JQzTObX3gljh0ZNeUdgwdMOV8m9bR yjCwFVNr8mzRvgJhJ739aNzwlCzjYeg= From: Sven Eckelmann <sven@narfation.org> Date: Mon, 16 Dec 2024 19:37:12 +0100 Subject: [PATCH] batman-adv: Map VID 0 to untagged TT VLAN MIME-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: 8bit Message-Id: <20241216-no-vlan-0-v1-1-62586f97fd88@narfation.org> X-B4-Tracking: v=1; b=H4sIAFdzYGcC/6tWKk4tykwtVrJSqFYqSi3LLM7MzwNyDHUUlJIzE vPSU3UzU4B8JSMDIxNDI0Mz3bx83bKcxDxdA11TC1NTEwPzJMtks2QloPqCotS0zAqwWdGxtbU AjMKUiFsAAAA= X-Change-ID: 20241216-no-vlan-0-5855407b9c6c To: b.a.t.m.a.n@lists.open-mesh.org Cc: Sven Eckelmann <sven@narfation.org> X-Mailer: b4 0.14.2 X-Developer-Signature: v=1; a=openpgp-sha256; l=2967; i=sven@narfation.org; h=from:subject:message-id; bh=PNPaqyRtXFgSeCUQm7ygxT1TjfKbWiZCk0mYC4BYyA8=; b=owGbwMvMwCXmy1+ufVnk62nG02pJDOkJxek2x9o/f02zUrwiZmZzYu3zzqItctsm+nNreKqsO 3Z3zsJvHaUsDGJcDLJiiix7ruSf38z+Vv7ztI9HYeawMoEMYeDiFICJBC5jZHh+5kqf55MCAVnn 3qOm59112TP58uVbhXZv9Nyv4eDFIsrwh//ec+c7XG/vLmDJNRfL3Xd4l+DKY2aev12LvGXuWWn tYgEA X-Developer-Key: i=sven@narfation.org; a=openpgp; fpr=522D7163831C73A635D12FE5EC371482956781AF Message-ID-Hash: NRHO5V6UPI6DF2HT2PNCBS5Z2Y4MEGL3 X-Message-ID-Hash: NRHO5V6UPI6DF2HT2PNCBS5Z2Y4MEGL3 X-MailFrom: sven@narfation.org X-Mailman-Rule-Misses: dmarc-mitigation; no-senders; approved; emergency; loop; banned-address; member-moderation; header-match-b.a.t.m.a.n.lists.open-mesh.org-0; header-match-b.a.t.m.a.n.lists.open-mesh.org-1; header-match-b.a.t.m.a.n.lists.open-mesh.org-2; nonmember-moderation; administrivia; implicit-dest; max-recipients; max-size; news-moderation; no-subject; digests; suspicious-header X-Mailman-Version: 3.3.8 Precedence: list List-Id: The list for a Better Approach To Mobile Ad-hoc Networking <b.a.t.m.a.n.lists.open-mesh.org> Archived-At: <https://lists.open-mesh.org/mailman3/hyperkitty/list/b.a.t.m.a.n@lists.open-mesh.org/message/NRHO5V6UPI6DF2HT2PNCBS5Z2Y4MEGL3/> List-Archive: <https://lists.open-mesh.org/mailman3/hyperkitty/list/b.a.t.m.a.n@lists.open-mesh.org/> List-Help: <mailto:b.a.t.m.a.n-request@lists.open-mesh.org?subject=help> List-Owner: <mailto:b.a.t.m.a.n-owner@lists.open-mesh.org> List-Post: <mailto:b.a.t.m.a.n@lists.open-mesh.org> List-Subscribe: <mailto:b.a.t.m.a.n-join@lists.open-mesh.org> List-Unsubscribe: <mailto:b.a.t.m.a.n-leave@lists.open-mesh.org> |
| Series |
batman-adv: Map VID 0 to untagged TT VLAN
|
|
Commit Message
Sven Eckelmann
Dec. 16, 2024, 6:37 p.m. UTC
VID 0 is not a valid VLAN according to "802.1Q-2011" "Table 9-2—Reserved
VID values". It is only used to indicate "priority tag" frames which only
contain priority information and no VID.
The 8021q is also redirecting the priority tagged frames to the underlying
interface since commit ad1afb003939 ("vlan_dev: VLAN 0 should be treated as
"no vlan tag" (802.1p packet)"). But at the same time, it automatically
adds the VID 0 to all devices to ensure that VID 0 is in the allowed list
of the HW filter. This resulted in a VLAN 0 which was always announced in
OGM messages.
batman-adv should therefore not create a new batadv_softif_vlan for VID 0
and handle all VID 0 related frames using the "untagged" global/local
translation tables.
Signed-off-by: Sven Eckelmann <sven@narfation.org>
---
net/batman-adv/main.c | 7 +++++++
net/batman-adv/soft-interface.c | 14 ++++++++++++++
2 files changed, 21 insertions(+)
---
base-commit: 4e395d4d6908da373f00752c363c82ccf99a427e
change-id: 20241216-no-vlan-0-5855407b9c6c
Best regards,
Comments
On 16/12/2024 19:37, Sven Eckelmann wrote: > VID 0 is not a valid VLAN according to "802.1Q-2011" "Table 9-2—Reserved > VID values". It is only used to indicate "priority tag" frames which only > contain priority information and no VID. > > The 8021q is also redirecting the priority tagged frames to the underlying > interface since commit ad1afb003939 ("vlan_dev: VLAN 0 should be treated as > "no vlan tag" (802.1p packet)"). But at the same time, it automatically > adds the VID 0 to all devices to ensure that VID 0 is in the allowed list > of the HW filter. This resulted in a VLAN 0 which was always announced in > OGM messages. > > batman-adv should therefore not create a new batadv_softif_vlan for VID 0 > and handle all VID 0 related frames using the "untagged" global/local > translation tables. > > Signed-off-by: Sven Eckelmann <sven@narfation.org> Acked-by: Antonio Quartulli <antonio@mandelbit.com>
On Mon, Dec 16, 2024 at 07:37:12PM +0100, Sven Eckelmann wrote: > diff --git a/net/batman-adv/main.c b/net/batman-adv/main.c > index 8e0f44c71696f642d80304ec2724e8b5e56a5d93..333e947afcce7ca4128be8406f23295df723515c 100644 > --- a/net/batman-adv/main.c > +++ b/net/batman-adv/main.c > @@ -637,6 +637,13 @@ unsigned short batadv_get_vid(struct sk_buff *skb, size_t header_len) > > vhdr = (struct vlan_ethhdr *)(skb->data + header_len); > vid = ntohs(vhdr->h_vlan_TCI) & VLAN_VID_MASK; > + > + /* VID 0 is only used to indicate "priority tag" frames which only > + * contain priority information and no VID. > + */ > + if (vid == 0) > + return BATADV_NO_FLAGS; > + > vid |= BATADV_VLAN_HAS_TAG; > > return vid; I guess with this patch all TT entries previously in TT VLAN 0 would be moved to untagged/NO_FLAGS TT entries, right? Wouldn't that technically break compatibility? Let's say someone uses VLAN headers with VID 0 to be able to use priorities / QoS. What if some old nodes still announced+used VLAN 0 in batman-adv while others used it after this patch, with the mapping to NO_FLAGS?
On Tuesday, 17 December 2024 14:53:23 CET Linus Lüssing wrote: > On Mon, Dec 16, 2024 at 07:37:12PM +0100, Sven Eckelmann wrote: > > diff --git a/net/batman-adv/main.c b/net/batman-adv/main.c > > index 8e0f44c71696f642d80304ec2724e8b5e56a5d93..333e947afcce7ca4128be8406f23295df723515c 100644 > > --- a/net/batman-adv/main.c > > +++ b/net/batman-adv/main.c > > @@ -637,6 +637,13 @@ unsigned short batadv_get_vid(struct sk_buff *skb, size_t header_len) > > > > vhdr = (struct vlan_ethhdr *)(skb->data + header_len); > > vid = ntohs(vhdr->h_vlan_TCI) & VLAN_VID_MASK; > > + > > + /* VID 0 is only used to indicate "priority tag" frames which only > > + * contain priority information and no VID. > > + */ > > + if (vid == 0) > > + return BATADV_NO_FLAGS; > > + > > vid |= BATADV_VLAN_HAS_TAG; > > > > return vid; > > I guess with this patch all TT entries previously in > TT VLAN 0 would be moved to untagged/NO_FLAGS TT entries, right? Yes, as specified by 802.1Q-2011, it is meant to transport only priority information and not a VID. For a switch, the PVID would be used but because batman-adv is here used as the lower device (for either a VLAN aware bridge or 8021q device), we don't have to add the PVID - the VID is simply missing (because it is !BATADV_VLAN_HAS_TAG) and therefore has to check for the "untagged" TT global entries (or add entries to the "untagged" TT local part) > Wouldn't that technically break compatibility? Let's say someone > uses VLAN headers with VID 0 to be able to use priorities / QoS. Then this person should have noticed that it broken at the moment and doesn't work as expected (to reach the "untagged remotes" with the priority tagged packets) > What if some old nodes still announced+used VLAN 0 in batman-adv > while others used it after this patch, with the mapping to > NO_FLAGS? Then the misbehaving old node would still misbehave. Because you should actually be able to talk with VID 0 to the untagged global TT entries - which the old node fails to do. So I could also add Fixes: 0ffa9e8d86d6 ("batman-adv: use vid when computing local and global TT CRC") Fixes: 5d2c05b21337 ("batman-adv: add per VLAN interface attribute framework") if you prefer and transmit it via the batadv/net queue. But I considered VID 0 somewhat esoteric for in-Linux usage because most tools just use DSCP. I am only away of tools like isochron-send which just inject raw packets with the VLAN headers directly. And using another 8021q device with VID on one side is a good way to create a unidirectional communication (when you want a bidirectional one) because the other end will just reply with a vanilla, untagged packet. And because of that, things like ARP will not be able to "finish" because the answers are received on the non-VID0 interface. But maybe I am wrong about that. Kind regards, Sven
On 17/12/2024 17:38, Sven Eckelmann wrote: > On Tuesday, 17 December 2024 14:53:23 CET Linus Lüssing wrote: >> On Mon, Dec 16, 2024 at 07:37:12PM +0100, Sven Eckelmann wrote: >>> diff --git a/net/batman-adv/main.c b/net/batman-adv/main.c >>> index 8e0f44c71696f642d80304ec2724e8b5e56a5d93..333e947afcce7ca4128be8406f23295df723515c 100644 >>> --- a/net/batman-adv/main.c >>> +++ b/net/batman-adv/main.c >>> @@ -637,6 +637,13 @@ unsigned short batadv_get_vid(struct sk_buff *skb, size_t header_len) >>> >>> vhdr = (struct vlan_ethhdr *)(skb->data + header_len); >>> vid = ntohs(vhdr->h_vlan_TCI) & VLAN_VID_MASK; >>> + >>> + /* VID 0 is only used to indicate "priority tag" frames which only >>> + * contain priority information and no VID. >>> + */ >>> + if (vid == 0) >>> + return BATADV_NO_FLAGS; >>> + >>> vid |= BATADV_VLAN_HAS_TAG; >>> >>> return vid; >> >> I guess with this patch all TT entries previously in >> TT VLAN 0 would be moved to untagged/NO_FLAGS TT entries, right? > > Yes, as specified by 802.1Q-2011, it is meant to transport only priority > information and not a VID. For a switch, the PVID would be used but because > batman-adv is here used as the lower device (for either a VLAN aware bridge or > 8021q device), we don't have to add the PVID - the VID is simply missing > (because it is !BATADV_VLAN_HAS_TAG) and therefore has to check for the > "untagged" TT global entries (or add entries to the "untagged" TT local part) > >> Wouldn't that technically break compatibility? Let's say someone >> uses VLAN headers with VID 0 to be able to use priorities / QoS. > > Then this person should have noticed that it broken at the moment and doesn't > work as expected (to reach the "untagged remotes" with the priority tagged > packets) > >> What if some old nodes still announced+used VLAN 0 in batman-adv >> while others used it after this patch, with the mapping to >> NO_FLAGS? > > Then the misbehaving old node would still misbehave. Because you should > actually be able to talk with VID 0 to the untagged global TT entries - which > the old node fails to do. So I could also add > > Fixes: 0ffa9e8d86d6 ("batman-adv: use vid when computing local and global TT CRC") > Fixes: 5d2c05b21337 ("batman-adv: add per VLAN interface attribute framework") > > if you prefer and transmit it via the batadv/net queue. > > But I considered VID 0 somewhat esoteric for in-Linux usage because most tools > just use DSCP. I am only away of tools like isochron-send which just inject > raw packets with the VLAN headers directly. And using another 8021q device > with VID on one side is a good way to create a unidirectional communication > (when you want a bidirectional one) because the other end will just reply > with a vanilla, untagged packet. And because of that, things like ARP will > not be able to "finish" because the answers are received on the non-VID0 > interface. I am not aware of 0 being used as a sane VID. I have seen it mostly used internally, but never truly used with something like "eth0.0". Therefore I agree with Sven that this should not cause any real compat issue. If we truly break something, then we can probably assume that the scenario was already ill formed. Regards,
On Wed, Dec 18, 2024 at 09:02:27AM +0100, Antonio Quartulli wrote: > I am not aware of 0 being used as a sane VID. > I have seen it mostly used internally, but never truly used with something > like "eth0.0". > > Therefore I agree with Sven that this should not cause any real compat > issue. If we truly break something, then we can probably assume that the > scenario was already ill formed. Ok, thanks you two for elaborating on this. Ok, these are good points. If no one uses VLAN 0 (in some partially broken/worked around/ out-of-spec way) and no one screams right now then I also like this change. And would probably make sense to make this change now, before adding (some) VLAN learning to batman-adv. Where otherwise potentially less informed, random user (compared to node administrators?) would otherwise more likely use VLAN 0 in an unintended/out-of-spec way.
diff --git a/net/batman-adv/main.c b/net/batman-adv/main.c index 8e0f44c71696f642d80304ec2724e8b5e56a5d93..333e947afcce7ca4128be8406f23295df723515c 100644 --- a/net/batman-adv/main.c +++ b/net/batman-adv/main.c @@ -637,6 +637,13 @@ unsigned short batadv_get_vid(struct sk_buff *skb, size_t header_len) vhdr = (struct vlan_ethhdr *)(skb->data + header_len); vid = ntohs(vhdr->h_vlan_TCI) & VLAN_VID_MASK; + + /* VID 0 is only used to indicate "priority tag" frames which only + * contain priority information and no VID. + */ + if (vid == 0) + return BATADV_NO_FLAGS; + vid |= BATADV_VLAN_HAS_TAG; return vid; diff --git a/net/batman-adv/soft-interface.c b/net/batman-adv/soft-interface.c index 1f06861bc86a7d23c48d91e61298f48f6ec6b3f9..282a8f9b144471b12f62a547b3e57666cbb22c6d 100644 --- a/net/batman-adv/soft-interface.c +++ b/net/batman-adv/soft-interface.c @@ -637,6 +637,14 @@ static int batadv_interface_add_vid(struct net_device *dev, __be16 proto, if (proto != htons(ETH_P_8021Q)) return -EINVAL; + /* VID 0 is only used to indicate "priority tag" frames which only + * contain priority information and no VID. No management structures + * should be created for this VID and it should be handled like an + * untagged frame. + */ + if (vid == 0) + return 0; + vid |= BATADV_VLAN_HAS_TAG; /* if a new vlan is getting created and it already exists, it means that @@ -684,6 +692,12 @@ static int batadv_interface_kill_vid(struct net_device *dev, __be16 proto, if (proto != htons(ETH_P_8021Q)) return -EINVAL; + /* "priority tag" frames are handled like "untagged" frames + * and no softif_vlan needs to be destroyed + */ + if (vid == 0) + return 0; + vlan = batadv_softif_vlan_get(bat_priv, vid | BATADV_VLAN_HAS_TAG); if (!vlan) return -ENOENT;