[PATCH net] net: br: Fix igmp snooping offload with CONFIG_BRIDGE_VLAN_FILTERING

[PATCH net] net: br: Fix igmp snooping offload with CONFIG_BRIDGE_VLAN_FILTERING

[Andrew Lunn]

With CONFIG_BRIDGE_VLAN_FILTERING enabled, but the feature not enabled
via /sys/class/net/brX/bridge/vlan_filtering, mdb offloaded to the
kernel have the wrong VID.

When an interface is added to the bridge, switchdev is first used to
notify the hardware that a port has joined a bridge. This is
immediately followed by the default_pvid, 1, being added to the
interface via another switchdev call.

The bridge will then perform IGMP snooping, and offload an mdb entries
to the switch as needed. With vlan filtering disabled, the vid is left
as 0. This causes the switch to put the static mdb into the wrong
vlan, and so frames are not forwarded by the mdb entry.

If vlan filtering is disable, use the default_pvid, not 0.

Fixes: f1fecb1d10ec ("bridge: Reflect MDB entries to hardware")
Signed-off-by: Andrew Lunn <andrew@lunn.ch>
---
 net/bridge/br_vlan.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/net/bridge/br_vlan.c b/net/bridge/br_vlan.c
index 233a30040c91..aa3589891797 100644
--- a/net/bridge/br_vlan.c
+++ b/net/bridge/br_vlan.c
@@ -492,6 +492,7 @@ bool br_allowed_ingress(const struct net_bridge *br,
 	 */
 	if (!br->vlan_enabled) {
 		BR_INPUT_SKB_CB(skb)->vlan_filtered = false;
+		*vid = br_get_pvid(vg);
 		return true;
 	}
 
-- 
2.14.1

Re: [PATCH net] net: br: Fix igmp snooping offload with CONFIG_BRIDGE_VLAN_FILTERING

[Toshiaki Makita]

On 2017/10/03 9:55, Andrew Lunn wrote:
> With CONFIG_BRIDGE_VLAN_FILTERING enabled, but the feature not enabled
> via /sys/class/net/brX/bridge/vlan_filtering, mdb offloaded to the
> kernel have the wrong VID.
> 
> When an interface is added to the bridge, switchdev is first used to
> notify the hardware that a port has joined a bridge. This is
> immediately followed by the default_pvid, 1, being added to the
> interface via another switchdev call.
> 
> The bridge will then perform IGMP snooping, and offload an mdb entries
> to the switch as needed. With vlan filtering disabled, the vid is left
> as 0. This causes the switch to put the static mdb into the wrong
> vlan, and so frames are not forwarded by the mdb entry.
> 
> If vlan filtering is disable, use the default_pvid, not 0.
> 
> Fixes: f1fecb1d10ec ("bridge: Reflect MDB entries to hardware")
> Signed-off-by: Andrew Lunn <andrew@lunn.ch>
> ---
>  net/bridge/br_vlan.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/net/bridge/br_vlan.c b/net/bridge/br_vlan.c
> index 233a30040c91..aa3589891797 100644
> --- a/net/bridge/br_vlan.c
> +++ b/net/bridge/br_vlan.c
> @@ -492,6 +492,7 @@ bool br_allowed_ingress(const struct net_bridge *br,
>  	 */
>  	if (!br->vlan_enabled) {
>  		BR_INPUT_SKB_CB(skb)->vlan_filtered = false;
> +		*vid = br_get_pvid(vg);
>  		return true;
>  	}
>  

This does not look correct.
This will update fdb with vid which is not 0.
Pvid can be different between each port even when vlan_filtering is
disabled so unicast forwarding (fdb learning) will break.
Also, fdb is visible to userspace so this can break userspace which
expects fdb entries with 0 as well.

Why does the switch driver use pvid while vlan_filtering is disabled?
The (software) bridge does not use pvid for forwarding and use fdb/mdb
entires with vid 0 when vlan_filtering is disabled even if pvid has been
configured.

-- 
Toshiaki Makita

Re: [PATCH net] net: br: Fix igmp snooping offload with CONFIG_BRIDGE_VLAN_FILTERING

[Andrew Lunn]

On Tue, Oct 03, 2017 at 12:29:56PM +0900, Toshiaki Makita wrote:
> On 2017/10/03 9:55, Andrew Lunn wrote:
> > With CONFIG_BRIDGE_VLAN_FILTERING enabled, but the feature not enabled
> > via /sys/class/net/brX/bridge/vlan_filtering, mdb offloaded to the
> > kernel have the wrong VID.
> > 
> > When an interface is added to the bridge, switchdev is first used to
> > notify the hardware that a port has joined a bridge. This is
> > immediately followed by the default_pvid, 1, being added to the
> > interface via another switchdev call.
> > 
> > The bridge will then perform IGMP snooping, and offload an mdb entries
> > to the switch as needed. With vlan filtering disabled, the vid is left
> > as 0. This causes the switch to put the static mdb into the wrong
> > vlan, and so frames are not forwarded by the mdb entry.
> > 
> > If vlan filtering is disable, use the default_pvid, not 0.
> > 
> > Fixes: f1fecb1d10ec ("bridge: Reflect MDB entries to hardware")
> > Signed-off-by: Andrew Lunn <andrew@lunn.ch>
> > ---
> >  net/bridge/br_vlan.c | 1 +
> >  1 file changed, 1 insertion(+)
> > 
> > diff --git a/net/bridge/br_vlan.c b/net/bridge/br_vlan.c
> > index 233a30040c91..aa3589891797 100644
> > --- a/net/bridge/br_vlan.c
> > +++ b/net/bridge/br_vlan.c
> > @@ -492,6 +492,7 @@ bool br_allowed_ingress(const struct net_bridge *br,
> >  	 */
> >  	if (!br->vlan_enabled) {
> >  		BR_INPUT_SKB_CB(skb)->vlan_filtered = false;
> > +		*vid = br_get_pvid(vg);
> >  		return true;
> >  	}
> >  
> 
> This does not look correct.
> This will update fdb with vid which is not 0.
> Pvid can be different between each port even when vlan_filtering is
> disabled so unicast forwarding (fdb learning) will break.
> Also, fdb is visible to userspace so this can break userspace which
> expects fdb entries with 0 as well.
> 
> Why does the switch driver use pvid while vlan_filtering is disabled?

Hi Toshiaki

We get a vlan added to the port. I think it comes from a combination
of:


int br_vlan_init(struct net_bridge *br)
{
        struct net_bridge_vlan_group *vg;
        int ret = -ENOMEM;

        vg = kzalloc(sizeof(*vg), GFP_KERNEL);
        if (!vg)
                goto out;
        ret = rhashtable_init(&vg->vlan_hash, &br_vlan_rht_params);
        if (ret)
                goto err_rhtbl;
        ret = vlan_tunnel_init(vg);
        if (ret)
                goto err_tunnel_init;
        INIT_LIST_HEAD(&vg->vlan_list);
        br->vlan_proto = htons(ETH_P_8021Q);
        br->default_pvid = 1;

and

int nbp_vlan_init(struct net_bridge_port *p)
{
        struct switchdev_attr attr = {
                .orig_dev = p->br->dev,
                .id = SWITCHDEV_ATTR_ID_BRIDGE_VLAN_FILTERING,
                .flags = SWITCHDEV_F_SKIP_EOPNOTSUPP,
                .u.vlan_filtering = p->br->vlan_enabled,
        };
        struct net_bridge_vlan_group *vg;
        int ret = -ENOMEM;

        vg = kzalloc(sizeof(struct net_bridge_vlan_group), GFP_KERNEL);
        if (!vg)
                goto out;

        ret = switchdev_port_attr_set(p->dev, &attr);
        if (ret && ret != -EOPNOTSUPP)
                goto err_vlan_enabled;

        ret = rhashtable_init(&vg->vlan_hash, &br_vlan_rht_params);
        if (ret)
                goto err_rhtbl;
        ret = vlan_tunnel_init(vg);
        if (ret)
                goto err_tunnel_init;
        INIT_LIST_HEAD(&vg->vlan_list);
        rcu_assign_pointer(p->vlgrp, vg);
        if (p->br->default_pvid) {
                ret = nbp_vlan_add(p, p->br->default_pvid,
                                   BRIDGE_VLAN_INFO_PVID |
                                   BRIDGE_VLAN_INFO_UNTAGGED);

Now, i just noticed the switchdev call above. I don't think the DSA
layer implements SWITCHDEV_ATTR_ID_BRIDGE_VLAN_FILTERING. It probably
should. So what is it supposed to do with this VLAN when filtering is
disabled?

	Andrew

Re: [PATCH net] net: br: Fix igmp snooping offload with CONFIG_BRIDGE_VLAN_FILTERING

[Vivien Didelot]

Andrew Lunn <andrew@lunn.ch> writes:

> Now, i just noticed the switchdev call above. I don't think the DSA
> layer implements SWITCHDEV_ATTR_ID_BRIDGE_VLAN_FILTERING. It probably
> should. So what is it supposed to do with this VLAN when filtering is
> disabled?

The DSA layer does implement SWITCHDEV_ATTR_ID_BRIDGE_VLAN_FILTERING.
Its interpretation is to enable 802.1Q mode on targeted switch ports.
(hoping this is the correct thing to do.)

Re: [PATCH net] net: br: Fix igmp snooping offload with CONFIG_BRIDGE_VLAN_FILTERING

[Toshiaki Makita]

On 17/10/03 (火) 21:16, Andrew Lunn wrote:
> On Tue, Oct 03, 2017 at 12:29:56PM +0900, Toshiaki Makita wrote:
>> On 2017/10/03 9:55, Andrew Lunn wrote:
>>> With CONFIG_BRIDGE_VLAN_FILTERING enabled, but the feature not enabled
>>> via /sys/class/net/brX/bridge/vlan_filtering, mdb offloaded to the
>>> kernel have the wrong VID.
>>>
>>> When an interface is added to the bridge, switchdev is first used to
>>> notify the hardware that a port has joined a bridge. This is
>>> immediately followed by the default_pvid, 1, being added to the
>>> interface via another switchdev call.
>>>
>>> The bridge will then perform IGMP snooping, and offload an mdb entries
>>> to the switch as needed. With vlan filtering disabled, the vid is left
>>> as 0. This causes the switch to put the static mdb into the wrong
>>> vlan, and so frames are not forwarded by the mdb entry.
>>>
>>> If vlan filtering is disable, use the default_pvid, not 0.
>>>
>>> Fixes: f1fecb1d10ec ("bridge: Reflect MDB entries to hardware")
>>> Signed-off-by: Andrew Lunn <andrew@lunn.ch>
>>> ---
>>>  net/bridge/br_vlan.c | 1 +
>>>  1 file changed, 1 insertion(+)
>>>
>>> diff --git a/net/bridge/br_vlan.c b/net/bridge/br_vlan.c
>>> index 233a30040c91..aa3589891797 100644
>>> --- a/net/bridge/br_vlan.c
>>> +++ b/net/bridge/br_vlan.c
>>> @@ -492,6 +492,7 @@ bool br_allowed_ingress(const struct net_bridge *br,
>>>  	 */
>>>  	if (!br->vlan_enabled) {
>>>  		BR_INPUT_SKB_CB(skb)->vlan_filtered = false;
>>> +		*vid = br_get_pvid(vg);
>>>  		return true;
>>>  	}
>>>
>>
>> This does not look correct.
>> This will update fdb with vid which is not 0.
>> Pvid can be different between each port even when vlan_filtering is
>> disabled so unicast forwarding (fdb learning) will break.
>> Also, fdb is visible to userspace so this can break userspace which
>> expects fdb entries with 0 as well.
>>
>> Why does the switch driver use pvid while vlan_filtering is disabled?
>
> Hi Toshiaki
>
> We get a vlan added to the port. I think it comes from a combination
> of:
>
>
> int br_vlan_init(struct net_bridge *br)
> {
>         struct net_bridge_vlan_group *vg;
>         int ret = -ENOMEM;
>
>         vg = kzalloc(sizeof(*vg), GFP_KERNEL);
>         if (!vg)
>                 goto out;
>         ret = rhashtable_init(&vg->vlan_hash, &br_vlan_rht_params);
>         if (ret)
>                 goto err_rhtbl;
>         ret = vlan_tunnel_init(vg);
>         if (ret)
>                 goto err_tunnel_init;
>         INIT_LIST_HEAD(&vg->vlan_list);
>         br->vlan_proto = htons(ETH_P_8021Q);
>         br->default_pvid = 1;
>
> and
>
> int nbp_vlan_init(struct net_bridge_port *p)
> {
>         struct switchdev_attr attr = {
>                 .orig_dev = p->br->dev,
>                 .id = SWITCHDEV_ATTR_ID_BRIDGE_VLAN_FILTERING,
>                 .flags = SWITCHDEV_F_SKIP_EOPNOTSUPP,
>                 .u.vlan_filtering = p->br->vlan_enabled,
>         };
>         struct net_bridge_vlan_group *vg;
>         int ret = -ENOMEM;
>
>         vg = kzalloc(sizeof(struct net_bridge_vlan_group), GFP_KERNEL);
>         if (!vg)
>                 goto out;
>
>         ret = switchdev_port_attr_set(p->dev, &attr);
>         if (ret && ret != -EOPNOTSUPP)
>                 goto err_vlan_enabled;
>
>         ret = rhashtable_init(&vg->vlan_hash, &br_vlan_rht_params);
>         if (ret)
>                 goto err_rhtbl;
>         ret = vlan_tunnel_init(vg);
>         if (ret)
>                 goto err_tunnel_init;
>         INIT_LIST_HEAD(&vg->vlan_list);
>         rcu_assign_pointer(p->vlgrp, vg);
>         if (p->br->default_pvid) {
>                 ret = nbp_vlan_add(p, p->br->default_pvid,
>                                    BRIDGE_VLAN_INFO_PVID |
>                                    BRIDGE_VLAN_INFO_UNTAGGED);
>
> Now, i just noticed the switchdev call above. I don't think the DSA
> layer implements SWITCHDEV_ATTR_ID_BRIDGE_VLAN_FILTERING. It probably
> should. So what is it supposed to do with this VLAN when filtering is
> disabled?

The vlan will be effective only when vlan_filtering is enabled.
When vlan_filtering is disabled, vlan information is still kept in the 
bridge and gets effective later when vlan_filtering becomes enable.

Toshiaki Makita

Re: [PATCH net] net: br: Fix igmp snooping offload with CONFIG_BRIDGE_VLAN_FILTERING

[Andrew Lunn]

> The vlan will be effective only when vlan_filtering is enabled.
> When vlan_filtering is disabled, vlan information is still kept in the
> bridge and gets effective later when vlan_filtering becomes enable.

O.K, so things are starting to get clearer.

So when vlan filtering is disabled, the hardware should just ignore
the requests to add the vlan to the hardware?

When vlan_filtering is enabled, are all the vlans in the software
bridge again offloaded? Or do we need to remember all the vlans which
we ignored while vlan filtering was disabled? The average switch has
nowhere to store these disabled vlans. It can only store active vlans.

      Andrew

Re: [PATCH net] net: br: Fix igmp snooping offload with CONFIG_BRIDGE_VLAN_FILTERING

[Vivien Didelot]

Andrew Lunn <andrew@lunn.ch> writes:

>> The vlan will be effective only when vlan_filtering is enabled.
>> When vlan_filtering is disabled, vlan information is still kept in the
>> bridge and gets effective later when vlan_filtering becomes enable.
>
> O.K, so things are starting to get clearer.
>
> So when vlan filtering is disabled, the hardware should just ignore
> the requests to add the vlan to the hardware?
>
> When vlan_filtering is enabled, are all the vlans in the software
> bridge again offloaded? Or do we need to remember all the vlans which
> we ignored while vlan filtering was disabled? The average switch has
> nowhere to store these disabled vlans. It can only store active vlans.

When vlan_filtering is enabled on the bridge, the bridge code does
propagates the default_pvid again if I recall correctly.

In my opinion the hardware mustn't ignore the VLAN requests, because we
seem to agree that vlan_filtering disabled means that the target ports
should not care yet about 802.1Q. So having some unused hardware VLAN
entries and some ports with disabled 802.1Q mode must work together.

That being said we still have the wrong hardware FDB populated when
CONFIG_BRIDGE_VLAN_FILTERING is enabled but not vlan_filtering...

Re: [PATCH net] net: br: Fix igmp snooping offload with CONFIG_BRIDGE_VLAN_FILTERING

[Ido Schimmel]

On Tue, Oct 03, 2017 at 12:25:08PM -0400, Vivien Didelot wrote:
> Andrew Lunn <andrew@lunn.ch> writes:
> 
> >> The vlan will be effective only when vlan_filtering is enabled.
> >> When vlan_filtering is disabled, vlan information is still kept in the
> >> bridge and gets effective later when vlan_filtering becomes enable.
> >
> > O.K, so things are starting to get clearer.
> >
> > So when vlan filtering is disabled, the hardware should just ignore
> > the requests to add the vlan to the hardware?
> >
> > When vlan_filtering is enabled, are all the vlans in the software
> > bridge again offloaded? Or do we need to remember all the vlans which
> > we ignored while vlan filtering was disabled? The average switch has
> > nowhere to store these disabled vlans. It can only store active vlans.
> 
> When vlan_filtering is enabled on the bridge, the bridge code does
> propagates the default_pvid again if I recall correctly.
> 
> In my opinion the hardware mustn't ignore the VLAN requests, because we
> seem to agree that vlan_filtering disabled means that the target ports
> should not care yet about 802.1Q. So having some unused hardware VLAN
> entries and some ports with disabled 802.1Q mode must work together.
> 
> That being said we still have the wrong hardware FDB populated when
> CONFIG_BRIDGE_VLAN_FILTERING is enabled but not vlan_filtering...

The driver can make sure it's able to handle the configured
`vlan_filtering` state during port enslavement to the bridge and also
forbid it from being toggled once it's enslaved.

Re: [PATCH net] net: br: Fix igmp snooping offload with CONFIG_BRIDGE_VLAN_FILTERING

[Toshiaki Makita]

On 2017/10/04 1:42, Ido Schimmel wrote:
> On Tue, Oct 03, 2017 at 12:25:08PM -0400, Vivien Didelot wrote:
>> Andrew Lunn <andrew@lunn.ch> writes:
>>
>>>> The vlan will be effective only when vlan_filtering is enabled.
>>>> When vlan_filtering is disabled, vlan information is still kept in the
>>>> bridge and gets effective later when vlan_filtering becomes enable.
>>>
>>> O.K, so things are starting to get clearer.
>>>
>>> So when vlan filtering is disabled, the hardware should just ignore
>>> the requests to add the vlan to the hardware?
>>>
>>> When vlan_filtering is enabled, are all the vlans in the software
>>> bridge again offloaded? Or do we need to remember all the vlans which
>>> we ignored while vlan filtering was disabled? The average switch has
>>> nowhere to store these disabled vlans. It can only store active vlans.

Seems that __br_vlan_filter_toggle() only propagates
SWITCHDEV_ATTR_ID_BRIDGE_VLAN_FILTERING.
IMHO vlan-related objects (vlan, fdb, and mdb) should be remembered if
vlan_filtering can be enabled later. But this sounds redundant as the
same information is maintained in the bridge so I'm not sure this is the
best way.

>>
>> When vlan_filtering is enabled on the bridge, the bridge code does
>> propagates the default_pvid again if I recall correctly.

I couldn't find it in the source...

>>
>> In my opinion the hardware mustn't ignore the VLAN requests, because we
>> seem to agree that vlan_filtering disabled means that the target ports
>> should not care yet about 802.1Q. So having some unused hardware VLAN
>> entries and some ports with disabled 802.1Q mode must work together.

Probably I don't fully understand you, but I think hardware can ignore
VLAN requests while vlan_filtering is disabled, as long as they are
properly populated to hardware on enabling vlan_filtering.

>>
>> That being said we still have the wrong hardware FDB populated when
>> CONFIG_BRIDGE_VLAN_FILTERING is enabled but not vlan_filtering...
> 
> The driver can make sure it's able to handle the configured
> `vlan_filtering` state during port enslavement to the bridge and also
> forbid it from being toggled once it's enslaved.

That is a simple solution.
One concern is backward compatibility. I wonder if we can prohibit
toggling for some driver which currently allows it.

-- 
Toshiaki Makita

Re: [PATCH net] net: br: Fix igmp snooping offload with CONFIG_BRIDGE_VLAN_FILTERING

[Ido Schimmel]

On Wed, Oct 04, 2017 at 01:52:20PM +0900, Toshiaki Makita wrote:
> On 2017/10/04 1:42, Ido Schimmel wrote:
> > The driver can make sure it's able to handle the configured
> > `vlan_filtering` state during port enslavement to the bridge and also
> > forbid it from being toggled once it's enslaved.
> 
> That is a simple solution.
> One concern is backward compatibility. I wonder if we can prohibit
> toggling for some driver which currently allows it.

IMO, it's invalid to continue to allow this when we know the requested
behavior isn't honored and packets are forwarded incorrectly.