Re: [PATCH 1/3] net: bridge: Implement bridge flag local_receive

Nikolay Aleksandrov
2022-03-01T22:43:51Z

On 1 March 2022 13:31:02 CET, Mattias Forsblad <mattias.forsblad@gmail.com> wrote:
>This patch implements the bridge flag local_receive. When this
>flag is cleared packets received on bridge ports will not be forwarded up.
>This makes is possible to only forward traffic between the port members
>of the bridge.
>
>Signed-off-by: Mattias Forsblad <mattias.forsblad+netdev@gmail.com>
>---
> include/linux/if_bridge.h | 6 ++++++
> include/net/switchdev.h | 2 ++
> include/uapi/linux/if_bridge.h | 1 +
> include/uapi/linux/if_link.h | 1 +
> net/bridge/br.c | 18 ++++++++++++++++++
> net/bridge/br_device.c | 1 +
> net/bridge/br_input.c | 3 +++
> net/bridge/br_ioctl.c | 1 +
> net/bridge/br_netlink.c | 14 +++++++++++++-
> net/bridge/br_private.h | 2 ++
> net/bridge/br_sysfs_br.c | 23 +++++++++++++++++++++++
> net/bridge/br_vlan.c | 8 ++++++++
> 12 files changed, 79 insertions(+), 1 deletion(-)
>
>diff --git a/include/linux/if_bridge.h b/include/linux/if_bridge.h
>index 3aae023a9353..e6b77d18c1d2 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_local_receive_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_local_receive_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/net/switchdev.h b/include/net/switchdev.h
>index 3e424d40fae3..aec5c1f9b5c7 100644
>--- a/include/net/switchdev.h
>+++ b/include/net/switchdev.h
>@@ -25,6 +25,7 @@ enum switchdev_attr_id {
> SWITCHDEV_ATTR_ID_BRIDGE_AGEING_TIME,
> SWITCHDEV_ATTR_ID_BRIDGE_VLAN_FILTERING,
> SWITCHDEV_ATTR_ID_BRIDGE_VLAN_PROTOCOL,
>+ SWITCHDEV_ATTR_ID_BRIDGE_LOCAL_RECEIVE,
> SWITCHDEV_ATTR_ID_BRIDGE_MC_DISABLED,
> SWITCHDEV_ATTR_ID_BRIDGE_MROUTER,
> SWITCHDEV_ATTR_ID_MRP_PORT_ROLE,
>@@ -50,6 +51,7 @@ struct switchdev_attr {
> u16 vlan_protocol; /* BRIDGE_VLAN_PROTOCOL */
> bool mc_disabled; /* MC_DISABLED */
> u8 mrp_port_role; /* MRP_PORT_ROLE */
>+ bool local_receive; /* BRIDGE_LOCAL_RECEIVE */
> } u;
> };
>
>diff --git a/include/uapi/linux/if_bridge.h b/include/uapi/linux/if_bridge.h
>index 2711c3522010..fc889b5ccd69 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 local_receive;
> };
>
> struct __port_info {
>diff --git a/include/uapi/linux/if_link.h b/include/uapi/linux/if_link.h
>index e315e53125f4..bb7c25e1c89c 100644
>--- a/include/uapi/linux/if_link.h
>+++ b/include/uapi/linux/if_link.h
>@@ -482,6 +482,7 @@ enum {
> IFLA_BR_VLAN_STATS_PER_PORT,
> IFLA_BR_MULTI_BOOLOPT,
> IFLA_BR_MCAST_QUERIER_STATE,
>+ IFLA_BR_LOCAL_RECEIVE,

Please use the boolopt api for new boolean options
We're trying to limit the nl options expansion as the bridge is the
largest user.

> __IFLA_BR_MAX,
> };
>
>diff --git a/net/bridge/br.c b/net/bridge/br.c
>index b1dea3febeea..ff7eb4f269ec 100644
>--- a/net/bridge/br.c
>+++ b/net/bridge/br.c
>@@ -325,6 +325,24 @@ void br_boolopt_multi_get(const struct net_bridge *br,
> bm->optmask = GENMASK((BR_BOOLOPT_MAX - 1), 0);
> }
>
>+int br_local_receive_change(struct net_bridge *p,
>+ bool local_receive)
>+{
>+ struct switchdev_attr attr = {
>+ .orig_dev = p->dev,
>+ .id = SWITCHDEV_ATTR_ID_BRIDGE_LOCAL_RECEIVE,
>+ .flags = SWITCHDEV_F_DEFER,
>+ .u.local_receive = local_receive,
>+ };
>+ int ret;
>+
>+ ret = switchdev_port_attr_set(p->dev, &attr, NULL);
>+ if (!ret)
>+ br_opt_toggle(p, BROPT_LOCAL_RECEIVE, local_receive);
>+
>+ 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..7cd9c5880d18 100644
>--- a/net/bridge/br_device.c
>+++ b/net/bridge/br_device.c
>@@ -524,6 +524,7 @@ 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_LOCAL_RECEIVE, 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..5864b61157d3 100644
>--- a/net/bridge/br_input.c
>+++ b/net/bridge/br_input.c
>@@ -163,6 +163,9 @@ int br_handle_frame_finish(struct net *net, struct sock *sk, struct sk_buff *skb
> break;
> }
>
>+ if (local_rcv && !br_opt_get(br, BROPT_LOCAL_RECEIVE))
>+ local_rcv = false;
>+

this affects the whole fast path, it can be better localized to make sure
it will not affect all use cases

> if (dst) {
> unsigned long now = jiffies;
>
>diff --git a/net/bridge/br_ioctl.c b/net/bridge/br_ioctl.c
>index f213ed108361..abe538129290 100644
>--- a/net/bridge/br_ioctl.c
>+++ b/net/bridge/br_ioctl.c
>@@ -177,6 +177,7 @@ int br_dev_siocdevprivate(struct net_device *dev, struct ifreq *rq,
> b.topology_change = br->topology_change;
> b.topology_change_detected = br->topology_change_detected;
> b.root_port = br->root_port;
>+ b.local_receive = br_opt_get(br, BROPT_LOCAL_RECEIVE) ? 1 : 0;

ioctl is not being extended anymore, please drop it

>
> b.stp_enabled = (br->stp_enabled != BR_NO_STP);
> b.ageing_time = jiffies_to_clock_t(br->ageing_time);
>diff --git a/net/bridge/br_netlink.c b/net/bridge/br_netlink.c
>index 7d4432ca9a20..5e7f99950195 100644
>--- a/net/bridge/br_netlink.c
>+++ b/net/bridge/br_netlink.c
>@@ -1163,6 +1163,7 @@ static const struct nla_policy br_policy[IFLA_BR_MAX + 1] = {
> [IFLA_BR_MCAST_IGMP_VERSION] = { .type = NLA_U8 },
> [IFLA_BR_MCAST_MLD_VERSION] = { .type = NLA_U8 },
> [IFLA_BR_VLAN_STATS_PER_PORT] = { .type = NLA_U8 },
>+ [IFLA_BR_LOCAL_RECEIVE] = { .type = NLA_U8 },
> [IFLA_BR_MULTI_BOOLOPT] =
> NLA_POLICY_EXACT_LEN(sizeof(struct br_boolopt_multi)),
> };
>@@ -1434,6 +1435,14 @@ static int br_changelink(struct net_device *brdev, struct nlattr *tb[],
> return err;
> }
>
>+ if (data[IFLA_BR_LOCAL_RECEIVE]) {
>+ u8 val = nla_get_u8(data[IFLA_BR_LOCAL_RECEIVE]);
>+
>+ err = br_local_receive_change(br, !!val);
>+ if (err)
>+ return err;
>+ }
>+
> return 0;
> }
>
>@@ -1514,6 +1523,7 @@ static size_t br_get_size(const struct net_device *brdev)
> nla_total_size(sizeof(u8)) + /* IFLA_BR_NF_CALL_ARPTABLES */
> #endif
> nla_total_size(sizeof(struct br_boolopt_multi)) + /* IFLA_BR_MULTI_BOOLOPT */
>+ nla_total_size(sizeof(u8)) + /* IFLA_BR_LOCAL_RECEIVE */
> 0;
> }
>
>@@ -1527,6 +1537,7 @@ static int br_fill_info(struct sk_buff *skb, const struct net_device *brdev)
> u32 stp_enabled = br->stp_enabled;
> u16 priority = (br->bridge_id.prio[0] << 8) | br->bridge_id.prio[1];
> u8 vlan_enabled = br_vlan_enabled(br->dev);
>+ u8 local_receive = br_opt_get(br, BROPT_LOCAL_RECEIVE) ? 1 : 0;
> struct br_boolopt_multi bm;
> u64 clockval;
>
>@@ -1563,7 +1574,8 @@ static int br_fill_info(struct sk_buff *skb, const struct net_device *brdev)
> nla_put_u8(skb, IFLA_BR_TOPOLOGY_CHANGE_DETECTED,
> br->topology_change_detected) ||
> nla_put(skb, IFLA_BR_GROUP_ADDR, ETH_ALEN, br->group_addr) ||
>- nla_put(skb, IFLA_BR_MULTI_BOOLOPT, sizeof(bm), &bm))
>+ nla_put(skb, IFLA_BR_MULTI_BOOLOPT, sizeof(bm), &bm) ||
>+ nla_put_u8(skb, IFLA_BR_LOCAL_RECEIVE, local_receive))
> return -EMSGSIZE;
>
> #ifdef CONFIG_BRIDGE_VLAN_FILTERING
>diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h
>index 48bc61ebc211..01fa5426094b 100644
>--- a/net/bridge/br_private.h
>+++ b/net/bridge/br_private.h
>@@ -445,6 +445,7 @@ enum net_bridge_opts {
> BROPT_NO_LL_LEARN,
> BROPT_VLAN_BRIDGE_BINDING,
> BROPT_MCAST_VLAN_SNOOPING_ENABLED,
>+ BROPT_LOCAL_RECEIVE,
> };
>
> struct net_bridge {
>@@ -720,6 +721,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_local_receive_change(struct net_bridge *p, bool local_receive);
>
> /* br_device.c */
> void br_dev_setup(struct net_device *dev);
>diff --git a/net/bridge/br_sysfs_br.c b/net/bridge/br_sysfs_br.c
>index 3f7ca88c2aa3..9af0c2ba929c 100644
>--- a/net/bridge/br_sysfs_br.c
>+++ b/net/bridge/br_sysfs_br.c
>@@ -84,6 +84,28 @@ static ssize_t forward_delay_store(struct device *d,
> }
> static DEVICE_ATTR_RW(forward_delay);
>
>+static ssize_t local_receive_show(struct device *d,
>+ struct device_attribute *attr, char *buf)
>+{
>+ struct net_bridge *br = to_bridge(d);
>+
>+ return sprintf(buf, "%u\n", br_opt_get(br, BROPT_LOCAL_RECEIVE));
>+}
>+
>+static int set_local_receive(struct net_bridge *br, unsigned long val,
>+ struct netlink_ext_ack *extack)
>+{
>+ return br_local_receive_change(br, !!val);
>+}
>+
>+static ssize_t local_receive_store(struct device *d,
>+ struct device_attribute *attr,
>+ const char *buf, size_t len)
>+{
>+ return store_bridge_parm(d, buf, len, set_local_receive);
>+}
>+static DEVICE_ATTR_RW(local_receive);
>+

Drop sysfs too, netlink only

> static ssize_t hello_time_show(struct device *d, struct device_attribute *attr,
> char *buf)
> {
>@@ -950,6 +972,7 @@ static struct attribute *bridge_attrs[] = {
> &dev_attr_group_addr.attr,
> &dev_attr_flush.attr,
> &dev_attr_no_linklocal_learn.attr,
>+ &dev_attr_local_receive.attr,
> #ifdef CONFIG_BRIDGE_IGMP_SNOOPING
> &dev_attr_multicast_router.attr,
> &dev_attr_multicast_snooping.attr,
>diff --git a/net/bridge/br_vlan.c b/net/bridge/br_vlan.c
>index 7557e90b60e1..57dd14d5e360 100644
>--- a/net/bridge/br_vlan.c
>+++ b/net/bridge/br_vlan.c
>@@ -905,6 +905,14 @@ bool br_vlan_enabled(const struct net_device *dev)
> }
> EXPORT_SYMBOL_GPL(br_vlan_enabled);
>
>+bool br_local_receive_enabled(const struct net_device *dev)
>+{
>+ struct net_bridge *br = netdev_priv(dev);
>+
>+ return br_opt_get(br, BROPT_LOCAL_RECEIVE);
>+}
>+EXPORT_SYMBOL_GPL(br_local_receive_enabled);
>+

What the hell is this doing in br_vlan.c???

> int br_vlan_get_proto(const struct net_device *dev, u16 *p_proto)
> {
> struct net_bridge *br = netdev_priv(dev);