| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <c85e031b-b4ed-a638-6448-205ef9803aa8@gmail.com>
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