Date:   Thu, 17 Mar 2022 13:08:58 +0100
From:   Mattias Forsblad <mattias.forsblad@...il.com>
To:     Vladimir Oltean <olteanv@...il.com>
Cc:     Nikolay Aleksandrov <razor@...ckwall.org>, netdev@...r.kernel.org,
        "David S . Miller" <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>, Andrew Lunn <andrew@...n.ch>,
        Florian Fainelli <f.fainelli@...il.com>,
        Vivien Didelot <vivien.didelot@...il.com>,
        Roopa Prabhu <roopa@...dia.com>,
        Tobias Waldekranz <tobias@...dekranz.com>,
        Joachim Wiberg <troglobit@...il.com>
Subject: Re: [PATCH 2/5] net: bridge: Implement bridge flood flag

On 2022-03-17 12:59, Vladimir Oltean wrote:
> On Thu, Mar 17, 2022 at 01:57:03PM +0200, Vladimir Oltean wrote:
>> On Thu, Mar 17, 2022 at 01:15:45PM +0200, Vladimir Oltean wrote:
>>> On Thu, Mar 17, 2022 at 12:11:40PM +0200, Nikolay Aleksandrov wrote:
>>>> On 17/03/2022 08:50, Mattias Forsblad wrote:
>>>>> This patch implements the bridge flood flags. There are three different
>>>>> flags matching unicast, multicast and broadcast. When the corresponding
>>>>> flag is cleared packets received on bridge ports will not be flooded
>>>>> towards the bridge.
>>>>> This makes is possible to only forward selected traffic between the
>>>>> port members of the bridge.
>>>>>
>>>>> Signed-off-by: Mattias Forsblad <mattias.forsblad@...il.com>
>>>>> ---
>>>>>  include/linux/if_bridge.h      |  6 +++++
>>>>>  include/uapi/linux/if_bridge.h |  9 ++++++-
>>>>>  net/bridge/br.c                | 45 ++++++++++++++++++++++++++++++++++
>>>>>  net/bridge/br_device.c         |  3 +++
>>>>>  net/bridge/br_input.c          | 23 ++++++++++++++---
>>>>>  net/bridge/br_private.h        |  4 +++
>>>>>  6 files changed, 85 insertions(+), 5 deletions(-)
>>>>>
>>>> Please always CC bridge maintainers for bridge patches.
>>>> I almost missed this one. I'll add my reply to Joachim's notes
>>>> which are pretty spot on.
>>>
>>> And DSA maintainers for DSA patches ;) I was aimlessly scrolling through
>>> patchwork when I happened to notice these, and the series is already at v3.
>>>
>>> As a matter of fact, I downloaded these patches from the mailing list
>>> with the intention of giving them a spin on mv88e6xxx to see what
>>> they're about, and to my surprise, this particular patch (I haven't even
>>> reached the offloading part) breaks DHCP on my bridge, so it can no
>>> longer get an IP address. I haven't toggled any bridge flag through
>>> netlink, just booted the board with systemd-networkd. The same thing
>>> happens with my LS1028A board. Further investigation to come, but this
>>> isn't off to a good start, I'm afraid...
>>>
>>>>
>>>>> diff --git a/include/linux/if_bridge.h b/include/linux/if_bridge.h
>>>>> index 3aae023a9353..fa8e000a6fb9 100644
>>>>> --- a/include/linux/if_bridge.h
>>>>> +++ b/include/linux/if_bridge.h
>>>>> @@ -157,6 +157,7 @@ static inline int br_vlan_get_info_rcu(const struct net_device *dev, u16 vid,
>>>>>  struct net_device *br_fdb_find_port(const struct net_device *br_dev,
>>>>>  				    const unsigned char *addr,
>>>>>  				    __u16 vid);
>>>>> +bool br_flood_enabled(const struct net_device *dev);
>>>>>  void br_fdb_clear_offload(const struct net_device *dev, u16 vid);
>>>>>  bool br_port_flag_is_set(const struct net_device *dev, unsigned long flag);
>>>>>  u8 br_port_get_stp_state(const struct net_device *dev);
>>>>> @@ -170,6 +171,11 @@ br_fdb_find_port(const struct net_device *br_dev,
>>>>>  	return NULL;
>>>>>  }
>>>>>  
>>>>> +static inline bool br_flood_enabled(const struct net_device *dev)
>>>>> +{
>>>>> +	return true;
>>>>> +}
>>>>> +
>>>>>  static inline void br_fdb_clear_offload(const struct net_device *dev, u16 vid)
>>>>>  {
>>>>>  }
>>>>> diff --git a/include/uapi/linux/if_bridge.h b/include/uapi/linux/if_bridge.h
>>>>> index 2711c3522010..765ed70c9b28 100644
>>>>> --- a/include/uapi/linux/if_bridge.h
>>>>> +++ b/include/uapi/linux/if_bridge.h
>>>>> @@ -72,6 +72,7 @@ struct __bridge_info {
>>>>>  	__u32 tcn_timer_value;
>>>>>  	__u32 topology_change_timer_value;
>>>>>  	__u32 gc_timer_value;
>>>>> +	__u8 flood;
>>>>>  };
>>>>>  
>>>>>  struct __port_info {
>>>>> @@ -752,13 +753,19 @@ struct br_mcast_stats {
>>>>>  /* bridge boolean options
>>>>>   * BR_BOOLOPT_NO_LL_LEARN - disable learning from link-local packets
>>>>>   * BR_BOOLOPT_MCAST_VLAN_SNOOPING - control vlan multicast snooping
>>>>> + * BR_BOOLOPT_FLOOD - control bridge flood flag
>>>>> + * BR_BOOLOPT_MCAST_FLOOD - control bridge multicast flood flag
>>>>> + * BR_BOOLOPT_BCAST_FLOOD - control bridge broadcast flood flag
>>>>>   *
>>>>>   * IMPORTANT: if adding a new option do not forget to handle
>>>>> - *            it in br_boolopt_toggle/get and bridge sysfs
>>>>> + *            it in br_boolopt_toggle/get
>>>>>   */
>>>>>  enum br_boolopt_id {
>>>>>  	BR_BOOLOPT_NO_LL_LEARN,
>>>>>  	BR_BOOLOPT_MCAST_VLAN_SNOOPING,
>>>>> +	BR_BOOLOPT_FLOOD,
>>>>> +	BR_BOOLOPT_MCAST_FLOOD,
>>>>> +	BR_BOOLOPT_BCAST_FLOOD,
>>>>>  	BR_BOOLOPT_MAX
>>>>>  };
>>>>>  
>>>>> diff --git a/net/bridge/br.c b/net/bridge/br.c
>>>>> index b1dea3febeea..63a17bed6c63 100644
>>>>> --- a/net/bridge/br.c
>>>>> +++ b/net/bridge/br.c
>>>>> @@ -265,6 +265,11 @@ int br_boolopt_toggle(struct net_bridge *br, enum br_boolopt_id opt, bool on,
>>>>>  	case BR_BOOLOPT_MCAST_VLAN_SNOOPING:
>>>>>  		err = br_multicast_toggle_vlan_snooping(br, on, extack);
>>>>>  		break;
>>>>> +	case BR_BOOLOPT_FLOOD:
>>>>> +	case BR_BOOLOPT_MCAST_FLOOD:
>>>>> +	case BR_BOOLOPT_BCAST_FLOOD:
>>>>> +		err = br_flood_toggle(br, opt, on);
>>>>> +		break;
>>>>>  	default:
>>>>>  		/* shouldn't be called with unsupported options */
>>>>>  		WARN_ON(1);
>>>>> @@ -281,6 +286,12 @@ int br_boolopt_get(const struct net_bridge *br, enum br_boolopt_id opt)
>>>>>  		return br_opt_get(br, BROPT_NO_LL_LEARN);
>>>>>  	case BR_BOOLOPT_MCAST_VLAN_SNOOPING:
>>>>>  		return br_opt_get(br, BROPT_MCAST_VLAN_SNOOPING_ENABLED);
>>>>> +	case BR_BOOLOPT_FLOOD:
>>>>> +		return br_opt_get(br, BROPT_FLOOD);
>>>>> +	case BR_BOOLOPT_MCAST_FLOOD:
>>>>> +		return br_opt_get(br, BROPT_MCAST_FLOOD);
>>>>> +	case BR_BOOLOPT_BCAST_FLOOD:
>>>>> +		return br_opt_get(br, BROPT_BCAST_FLOOD);
>>>>>  	default:
>>>>>  		/* shouldn't be called with unsupported options */
>>>>>  		WARN_ON(1);
>>>>> @@ -325,6 +336,40 @@ void br_boolopt_multi_get(const struct net_bridge *br,
>>>>>  	bm->optmask = GENMASK((BR_BOOLOPT_MAX - 1), 0);
>>>>>  }
>>>>>  
>>>>> +int br_flood_toggle(struct net_bridge *br, enum br_boolopt_id opt,
>>>>> +		    bool on)
>>>>> +{
>>>>> +	struct switchdev_attr attr = {
>>>>> +		.orig_dev = br->dev,
>>>>> +		.id = SWITCHDEV_ATTR_ID_BRIDGE_FLOOD,
>>>>> +		.flags = SWITCHDEV_F_DEFER,
>>>>> +	};
>>>>> +	enum net_bridge_opts bropt;
>>>>> +	int ret;
>>>>> +
>>>>> +	switch (opt) {
>>>>> +	case BR_BOOLOPT_FLOOD:
>>>>> +		bropt = BROPT_FLOOD;
>>>>> +		break;
>>>>> +	case BR_BOOLOPT_MCAST_FLOOD:
>>>>> +		bropt = BROPT_MCAST_FLOOD;
>>>>> +		break;
>>>>> +	case BR_BOOLOPT_BCAST_FLOOD:
>>>>> +		bropt = BROPT_BCAST_FLOOD;
>>>>> +		break;
>>>>> +	default:
>>>>> +		WARN_ON(1);
>>>>> +		return -EINVAL;
>>>>> +	}
>>>>> +	br_opt_toggle(br, bropt, on);
>>>>> +
>>>>> +	attr.u.brport_flags.mask = BIT(bropt);
>>>>> +	attr.u.brport_flags.val = on << bropt;
>>>>> +	ret = switchdev_port_attr_set(br->dev, &attr, NULL);
>>>>> +
>>>>> +	return ret;
>>>>> +}
>>>>> +
>>>>>  /* private bridge options, controlled by the kernel */
>>>>>  void br_opt_toggle(struct net_bridge *br, enum net_bridge_opts opt, bool on)
>>>>>  {
>>>>> diff --git a/net/bridge/br_device.c b/net/bridge/br_device.c
>>>>> index 8d6bab244c4a..fafaef9d4b3a 100644
>>>>> --- a/net/bridge/br_device.c
>>>>> +++ b/net/bridge/br_device.c
>>>>> @@ -524,6 +524,9 @@ void br_dev_setup(struct net_device *dev)
>>>>>  	br->bridge_hello_time = br->hello_time = 2 * HZ;
>>>>>  	br->bridge_forward_delay = br->forward_delay = 15 * HZ;
>>>>>  	br->bridge_ageing_time = br->ageing_time = BR_DEFAULT_AGEING_TIME;
>>>>> +	br_opt_toggle(br, BROPT_FLOOD, true);
>>>>> +	br_opt_toggle(br, BROPT_MCAST_FLOOD, true);
>>>>> +	br_opt_toggle(br, BROPT_BCAST_FLOOD, true);
>>>>>  	dev->max_mtu = ETH_MAX_MTU;
>>>>>  
>>>>>  	br_netfilter_rtable_init(br);
>>>>> diff --git a/net/bridge/br_input.c b/net/bridge/br_input.c
>>>>> index e0c13fcc50ed..fcb0757bfdcc 100644
>>>>> --- a/net/bridge/br_input.c
>>>>> +++ b/net/bridge/br_input.c
>>>>> @@ -109,11 +109,12 @@ int br_handle_frame_finish(struct net *net, struct sock *sk, struct sk_buff *skb
>>>>>  		/* by definition the broadcast is also a multicast address */
>>>>>  		if (is_broadcast_ether_addr(eth_hdr(skb)->h_dest)) {
>>>>>  			pkt_type = BR_PKT_BROADCAST;
>>>>> -			local_rcv = true;
>>>>> +			local_rcv = true && br_opt_get(br, BROPT_BCAST_FLOOD);
>>>>>  		} else {
>>>>>  			pkt_type = BR_PKT_MULTICAST;
>>>>> -			if (br_multicast_rcv(&brmctx, &pmctx, vlan, skb, vid))
>>>>> -				goto drop;
>>>>> +			if (br_opt_get(br, BROPT_MCAST_FLOOD))
>>>>> +				if (br_multicast_rcv(&brmctx, &pmctx, vlan, skb, vid))
>>>>> +					goto drop;
>>>>>  		}
>>>>>  	}
>>>>>  
>>>>> @@ -155,9 +156,13 @@ int br_handle_frame_finish(struct net *net, struct sock *sk, struct sk_buff *skb
>>>>>  			local_rcv = true;
>>>>>  			br->dev->stats.multicast++;
>>>>>  		}
>>>>> +		if (!br_opt_get(br, BROPT_MCAST_FLOOD))
>>>>> +			local_rcv = false;
>>>>>  		break;
>>>>>  	case BR_PKT_UNICAST:
>>>>>  		dst = br_fdb_find_rcu(br, eth_hdr(skb)->h_dest, vid);
>>>>> +		if (!br_opt_get(br, BROPT_FLOOD))
>>>>> +			local_rcv = false;
>>>>>  		break;
>>>>>  	default:
>>>>>  		break;
>>>>> @@ -166,7 +171,7 @@ int br_handle_frame_finish(struct net *net, struct sock *sk, struct sk_buff *skb
>>>>>  	if (dst) {
>>>>>  		unsigned long now = jiffies;
>>>>>  
>>>>> -		if (test_bit(BR_FDB_LOCAL, &dst->flags))
>>>>> +		if (test_bit(BR_FDB_LOCAL, &dst->flags) && local_rcv)
>>
>> So this is the line that breaks local termination. Could you explain
>> the reasoning for this change? For unicast packets matching a local FDB
>> entry, local_rcv used to be irrelevant (and wasn't even set to true,
>> unless the bridge device was promiscuous).
> 
> Sorry, it wasn't obvious from the To: field, but the question was
> targeted to Mattias and not to Nikolay.
> 

It might as simple be that I've misunderstood that flow.

/Mattias

Powered by blists - more mailing lists