| 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: <20220317115900.5vfomgespvzvgzdt@skbuf>
Date: Thu, 17 Mar 2022 13:59:00 +0200
From: Vladimir Oltean <olteanv@...il.com>
To: Mattias Forsblad <mattias.forsblad@...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 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.
> > > > return br_pass_frame_up(skb);
> > > >
> > > > if (now != dst->used)
> > > > @@ -190,6 +195,16 @@ int br_handle_frame_finish(struct net *net, struct sock *sk, struct sk_buff *skb
> > > > }
> > > > EXPORT_SYMBOL_GPL(br_handle_frame_finish);
> > > >
> > > > +bool br_flood_enabled(const struct net_device *dev)
> > > > +{
> > > > + struct net_bridge *br = netdev_priv(dev);
> > > > +
> > > > + return !!(br_opt_get(br, BROPT_FLOOD) ||
> > > > + br_opt_get(br, BROPT_MCAST_FLOOD) ||
> > > > + br_opt_get(br, BROPT_BCAST_FLOOD));
> > > > +}
> > > > +EXPORT_SYMBOL_GPL(br_flood_enabled);
> > > > +
> > > > static void __br_handle_local_finish(struct sk_buff *skb)
> > > > {
> > > > struct net_bridge_port *p = br_port_get_rcu(skb->dev);
> > > > diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h
> > > > index 48bc61ebc211..cf88dce0b92b 100644
> > > > --- a/net/bridge/br_private.h
> > > > +++ b/net/bridge/br_private.h
> > > > @@ -445,6 +445,9 @@ enum net_bridge_opts {
> > > > BROPT_NO_LL_LEARN,
> > > > BROPT_VLAN_BRIDGE_BINDING,
> > > > BROPT_MCAST_VLAN_SNOOPING_ENABLED,
> > > > + BROPT_FLOOD,
> > > > + BROPT_MCAST_FLOOD,
> > > > + BROPT_BCAST_FLOOD,
> > > > };
> > > >
> > > > struct net_bridge {
> > > > @@ -720,6 +723,7 @@ int br_boolopt_multi_toggle(struct net_bridge *br,
> > > > void br_boolopt_multi_get(const struct net_bridge *br,
> > > > struct br_boolopt_multi *bm);
> > > > void br_opt_toggle(struct net_bridge *br, enum net_bridge_opts opt, bool on);
> > > > +int br_flood_toggle(struct net_bridge *br, enum br_boolopt_id opt, bool on);
> > > >
> > > > /* br_device.c */
> > > > void br_dev_setup(struct net_device *dev);
> > >
Powered by blists - more mailing lists