| Message ID | 20211003155141.2241314-1-andrew@lunn.ch (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net] dsa: tag_dsa: Handle !CONFIG_BRIDGE_VLAN_FILTERING case | expand |
| Context | Check | Description |
|---|---|---|
| netdev/cover_letter | success | Link |
| netdev/fixes_present | success | Link |
| netdev/patch_count | success | Link |
| netdev/tree_selection | success | Clearly marked for net |
| netdev/subject_prefix | success | Link |
| netdev/cc_maintainers | fail | 2 blamed authors not CCed: olteanv@gmail.com davem@davemloft.net; 4 maintainers not CCed: olteanv@gmail.com davem@davemloft.net vivien.didelot@gmail.com kuba@kernel.org |
| netdev/source_inline | success | Was 0 now: 0 |
| netdev/verify_signedoff | success | Link |
| netdev/module_param | success | Was 0 now: 0 |
| netdev/build_32bit | success | Errors and warnings before: 0 this patch: 0 |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/verify_fixes | success | Link |
| netdev/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 9 lines checked |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 0 this patch: 0 |
| netdev/header_inline | success | Link |
On Sun, Oct 03, 2021 at 05:51:41PM +0200, Andrew Lunn wrote: > If CONFIG_BRIDGE_VLAN_FILTERING is disabled, br_vlan_enabled() is > replaced with a stub which returns -EINVAL. br_vlan_enabled() returns bool, so it cannot hold -EINVAL. The stub for that returns false. We negate that false, make it true, and then call br_vlan_get_pvid_rcu() which returns -EINVAL because of _its_ stub implementation. > As a result the tagger > drops the frame. Rather than drop the frame, use a pvid of 0. > > Fixes: d82f8ab0d874 ("net: dsa: tag_dsa: offload the bridge forwarding process") > Signed-off-by: Andrew Lunn <andrew@lunn.ch> > --- > net/dsa/tag_dsa.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/net/dsa/tag_dsa.c b/net/dsa/tag_dsa.c > index e5127b7d1c6a..8daa8b7787c0 100644 > --- a/net/dsa/tag_dsa.c > +++ b/net/dsa/tag_dsa.c > @@ -149,7 +149,8 @@ static struct sk_buff *dsa_xmit_ll(struct sk_buff *skb, struct net_device *dev, > * inject packets to hardware using the bridge's pvid, since > * that's where the packets ingressed from. > */ > - if (!br_vlan_enabled(br)) { > + if (IS_ENABLED(CONFIG_BRIDGE_VLAN_FILTERING) && > + !br_vlan_enabled(br)) { > /* Safe because __dev_queue_xmit() runs under > * rcu_read_lock_bh() > */ > -- > 2.33.0 > So this got me thinking. We shouldn't behave differently when VLAN filtering is disabled vs when it is disabled and compiled out. In fact it is actually wrong to inject into the switch using the bridge's pvid, if VLAN awareness is turned off. We should be able to send and receive packets in that mode regardless of whether a pvid exists for the bridge device or not. That is also what we document in Documentation/networking/switchdev.rst. So if VLAN 0 does that trick, perfect, we should just delete the entire "if (!br_vlan_enabled(br))" block.
On Sun, Oct 03, 2021 at 09:03:55PM +0000, Vladimir Oltean wrote: > On Sun, Oct 03, 2021 at 05:51:41PM +0200, Andrew Lunn wrote: > > If CONFIG_BRIDGE_VLAN_FILTERING is disabled, br_vlan_enabled() is > > replaced with a stub which returns -EINVAL. > > br_vlan_enabled() returns bool, so it cannot hold -EINVAL. The stub for > that returns false. We negate that false, make it true, and then call > br_vlan_get_pvid_rcu() which returns -EINVAL because of _its_ stub > implementation. Yeh, i got the names of the functions wrong. I will fix that. > In fact it is actually wrong to inject into the switch using the > bridge's pvid, if VLAN awareness is turned off. We should be able to > send and receive packets in that mode regardless of whether a pvid > exists for the bridge device or not. That is also what we document in > Documentation/networking/switchdev.rst. > > So if VLAN 0 does that trick, perfect, we should just delete the entire > "if (!br_vlan_enabled(br))" block. I will rework the patch and test it without the if. Thanks Andrew
On Mon, Oct 04, 2021 at 02:18:54PM +0200, Andrew Lunn wrote: > On Sun, Oct 03, 2021 at 09:03:55PM +0000, Vladimir Oltean wrote: > > On Sun, Oct 03, 2021 at 05:51:41PM +0200, Andrew Lunn wrote: > > > If CONFIG_BRIDGE_VLAN_FILTERING is disabled, br_vlan_enabled() is > > > replaced with a stub which returns -EINVAL. > > > > br_vlan_enabled() returns bool, so it cannot hold -EINVAL. The stub for > > that returns false. We negate that false, make it true, and then call > > br_vlan_get_pvid_rcu() which returns -EINVAL because of _its_ stub > > implementation. > > Yeh, i got the names of the functions wrong. I will fix that. > > > In fact it is actually wrong to inject into the switch using the > > bridge's pvid, if VLAN awareness is turned off. We should be able to > > send and receive packets in that mode regardless of whether a pvid > > exists for the bridge device or not. That is also what we document in > > Documentation/networking/switchdev.rst. > > > > So if VLAN 0 does that trick, perfect, we should just delete the entire > > "if (!br_vlan_enabled(br))" block. > > I will rework the patch and test it without the if. > > Thanks > Andrew I already resent the patch here: https://patchwork.kernel.org/project/netdevbpf/patch/20211003222312.284175-2-vladimir.oltean@nxp.com/
diff --git a/net/dsa/tag_dsa.c b/net/dsa/tag_dsa.c index e5127b7d1c6a..8daa8b7787c0 100644 --- a/net/dsa/tag_dsa.c +++ b/net/dsa/tag_dsa.c @@ -149,7 +149,8 @@ static struct sk_buff *dsa_xmit_ll(struct sk_buff *skb, struct net_device *dev, * inject packets to hardware using the bridge's pvid, since * that's where the packets ingressed from. */ - if (!br_vlan_enabled(br)) { + if (IS_ENABLED(CONFIG_BRIDGE_VLAN_FILTERING) && + !br_vlan_enabled(br)) { /* Safe because __dev_queue_xmit() runs under * rcu_read_lock_bh() */
If CONFIG_BRIDGE_VLAN_FILTERING is disabled, br_vlan_enabled() is replaced with a stub which returns -EINVAL. As a result the tagger drops the frame. Rather than drop the frame, use a pvid of 0. Fixes: d82f8ab0d874 ("net: dsa: tag_dsa: offload the bridge forwarding process") Signed-off-by: Andrew Lunn <andrew@lunn.ch> --- net/dsa/tag_dsa.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)