Re: [PATCH] batman-adv: Map VID 0 to untagged TT VLAN

[Antonio Quartulli <a@unstable.cc>]

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,


-- 
Antonio Quartulli