Re: [PATCH RFC v1 net-next 11/12] bridge: br_vlan_fill_forward_path_mode no _UNTAG_HW for dsa

[Eric Woudstra <ericwouds@gmail.com>]

On 10/21/24 3:47 PM, Vladimir Oltean wrote:
> On Sun, Oct 20, 2024 at 11:23:18AM +0200, Eric Woudstra wrote:
>> So after doing some more reading, at creation of the code using
>> BR_VLFLAG_ADDED_BY_SWITCHDEV would have been without problems.
>>
>> After the switchdev was altered so that objects from foreign devices can
>> be added, it is problematic in br_vlan_fill_forward_path_mode(). I have
>> tested and indeed any foreign device does have this problem.
>>
>> So we need a way to distinguish in br_vlan_fill_forward_path_mode()
>> whether or not we are dealing with a (dsa) foreign device on the switchdev.
>>
>> I have come up with something, but this is most likely to crude to be
>> accepted, but for the sake of 'rfc' discussing it may lead to a proper
>> solution. So what does work is the following patch, so that
>> netif_has_dsa_foreign_vlan() can be used inside
>> br_vlan_fill_forward_path_mode().
>>
>> Any suggestions on how this could be implemented properly would be
>> greatly appreciated.


> I don't know nearly enough about the netfilter flowtable to even
> understand exactly the problem you're describing and are trying to solve.
> I've started to read up on things, but plenty of concepts are new and

Another way of shortly describing it, considering the software fastpath,
only there it is a problem:

Same case #1:
When looking at the ingress hook of the fastpath for a bridged
switchdev-port (non-dsa) with PVID set, the existing code is written so
that the flowtuple needs to match the packet INcluding the PVID.

Same case #2:
When considering the ingress hook of the fastpath of a bridged device
that is not part of a switchdev at all and there is no dsa on the bridge
(so it is not a foreign device), the existing code is written so that
the flowtuple needs to match the packet EXcluding the PVID.

When looking at the diagram of this patch, wlan1 would stand for any
foreign device. Because of the use of BR_VLFLAG_ADDED_BY_SWITCHDEV, case
#1 is applied, instead of case #2.

> I'm mixing this with plenty of other activities. If you could share some
> commands to build a test setup so I could form my own independent
> opinion of what is going on, it would be great as it would speed up that
> process.

I've only setup the bridged part with systemd-networkd. When trying to
re-create, it will only happen if the total action of the bridge,
towards the foreign port, is:

ingress + egress = untagging

So in the forward-fastpath, tagging in the forward path is done by a
vlan-device and untagging is done in the forward path of the bridge.

> With respect to the patch you've posted, it doesn't look exactly great.

Agreed. I was experimenting now with having br_switchdev_port_vlan_add()
first to try it only for non-foreign ports. If not successful, then try
it with foreign ports. This way the calling function will know if the
port is a foreign port. Therefore, no need for the switchdev driver to
communicate back to upper layers.

> One would need to make a thorough analysis of the bridge's use of
> BR_VLFLAG_ADDED_BY_SWITCHDEV, of whether it still makes sense in today's
> world where br_switchdev_vlan_replay() is a thing (a VLAN that used to
> not be "added by switchdev" can become "added by switchdev" after a
> replay, but this flag will remain incorrectly unset), of whether VLANs on
> foreign DSA interfaces should even have this flag set, and on whether
> your flowtable forwarding path patches are conceptually using it correctly.
> There's a lot to think about, and if somebody doesn't have the big picture,
> I'm worried that a wrong decision will be taken.

The entire usage BR_VLFLAG_ADDED_BY_SWITCHDEV does need a careful review.

I also realize my patch-set needs to do more with the switchdev and vlan
combination, then it does now. So for now I will leave it as RFC, as it
will not work properly with switchdevs other the dsa.