Add an intermediate state for clients behind a locked port to allow for
possible opening of the port for said clients. The clients mac address
will be added with the locked flag set, denying access through the port
for the mac address, but also creating a new FDB add event giving
userspace daemons the ability to unlock the mac address. This feature
corresponds to the Mac-Auth and MAC Authentication Bypass (MAB) named
features. The latter defined by Cisco.
As locked FDB entries are a security measure to deny access for
unauthorized hosts on specific ports, they will deny forwarding from
any port to the (MAC, vlan) pair involved and locked entries will not
be able by learning or otherwise to change the associated port.
Only the kernel can set this FDB entry flag, while userspace can read
the flag and remove it by replacing or deleting the FDB entry.
Locked entries will age out with the set bridge ageing time.
Also add a 'blackhole' fdb flag, ensuring that no forwarding from any
port to a destination MAC that has a FDB entry with this flag on will
occur. The packets will thus be dropped.
Signed-off-by: Hans Schultz <[email protected]>
---
include/linux/if_bridge.h | 1 +
include/uapi/linux/if_link.h | 1 +
include/uapi/linux/neighbour.h | 4 +++-
net/bridge/br_fdb.c | 29 +++++++++++++++++++++++++++++
net/bridge/br_input.c | 16 +++++++++++++++-
net/bridge/br_netlink.c | 9 ++++++++-
net/bridge/br_private.h | 4 +++-
7 files changed, 60 insertions(+), 4 deletions(-)
diff --git a/include/linux/if_bridge.h b/include/linux/if_bridge.h
index d62ef428e3aa..1668ac4d7adc 100644
--- a/include/linux/if_bridge.h
+++ b/include/linux/if_bridge.h
@@ -59,6 +59,7 @@ struct br_ip_list {
#define BR_MRP_LOST_IN_CONT BIT(19)
#define BR_TX_FWD_OFFLOAD BIT(20)
#define BR_PORT_LOCKED BIT(21)
+#define BR_PORT_MAB BIT(22)
#define BR_DEFAULT_AGEING_TIME (300 * HZ)
diff --git a/include/uapi/linux/if_link.h b/include/uapi/linux/if_link.h
index e36d9d2c65a7..fcbd0b85ad53 100644
--- a/include/uapi/linux/if_link.h
+++ b/include/uapi/linux/if_link.h
@@ -560,6 +560,7 @@ enum {
IFLA_BRPORT_MCAST_EHT_HOSTS_LIMIT,
IFLA_BRPORT_MCAST_EHT_HOSTS_CNT,
IFLA_BRPORT_LOCKED,
+ IFLA_BRPORT_MAB,
__IFLA_BRPORT_MAX
};
#define IFLA_BRPORT_MAX (__IFLA_BRPORT_MAX - 1)
diff --git a/include/uapi/linux/neighbour.h b/include/uapi/linux/neighbour.h
index a998bf761635..bc1440a56b70 100644
--- a/include/uapi/linux/neighbour.h
+++ b/include/uapi/linux/neighbour.h
@@ -52,7 +52,9 @@ enum {
#define NTF_STICKY (1 << 6)
#define NTF_ROUTER (1 << 7)
/* Extended flags under NDA_FLAGS_EXT: */
-#define NTF_EXT_MANAGED (1 << 0)
+#define NTF_EXT_MANAGED (1 << 0)
+#define NTF_EXT_LOCKED (1 << 1)
+#define NTF_EXT_BLACKHOLE (1 << 2)
/*
* Neighbor Cache Entry States.
diff --git a/net/bridge/br_fdb.c b/net/bridge/br_fdb.c
index e7f4fccb6adb..1962d9594a48 100644
--- a/net/bridge/br_fdb.c
+++ b/net/bridge/br_fdb.c
@@ -105,6 +105,7 @@ static int fdb_fill_info(struct sk_buff *skb, const struct net_bridge *br,
struct nda_cacheinfo ci;
struct nlmsghdr *nlh;
struct ndmsg *ndm;
+ u32 ext_flags = 0;
nlh = nlmsg_put(skb, portid, seq, type, sizeof(*ndm), flags);
if (nlh == NULL)
@@ -125,11 +126,18 @@ static int fdb_fill_info(struct sk_buff *skb, const struct net_bridge *br,
ndm->ndm_flags |= NTF_EXT_LEARNED;
if (test_bit(BR_FDB_STICKY, &fdb->flags))
ndm->ndm_flags |= NTF_STICKY;
+ if (test_bit(BR_FDB_ENTRY_LOCKED, &fdb->flags))
+ ext_flags |= NTF_EXT_LOCKED;
+ if (test_bit(BR_FDB_BLACKHOLE, &fdb->flags))
+ ext_flags |= NTF_EXT_BLACKHOLE;
if (nla_put(skb, NDA_LLADDR, ETH_ALEN, &fdb->key.addr))
goto nla_put_failure;
if (nla_put_u32(skb, NDA_MASTER, br->dev->ifindex))
goto nla_put_failure;
+ if (nla_put_u32(skb, NDA_FLAGS_EXT, ext_flags))
+ goto nla_put_failure;
+
ci.ndm_used = jiffies_to_clock_t(now - fdb->used);
ci.ndm_confirmed = 0;
ci.ndm_updated = jiffies_to_clock_t(now - fdb->updated);
@@ -171,6 +179,7 @@ static inline size_t fdb_nlmsg_size(void)
return NLMSG_ALIGN(sizeof(struct ndmsg))
+ nla_total_size(ETH_ALEN) /* NDA_LLADDR */
+ nla_total_size(sizeof(u32)) /* NDA_MASTER */
+ + nla_total_size(sizeof(u32)) /* NDA_FLAGS_EXT */
+ nla_total_size(sizeof(u16)) /* NDA_VLAN */
+ nla_total_size(sizeof(struct nda_cacheinfo))
+ nla_total_size(0) /* NDA_FDB_EXT_ATTRS */
@@ -879,6 +888,10 @@ void br_fdb_update(struct net_bridge *br, struct net_bridge_port *source,
&fdb->flags)))
clear_bit(BR_FDB_ADDED_BY_EXT_LEARN,
&fdb->flags);
+ if (source->flags & BR_PORT_MAB)
+ set_bit(BR_FDB_ENTRY_LOCKED, &fdb->flags);
+ else
+ clear_bit(BR_FDB_ENTRY_LOCKED, &fdb->flags);
}
if (unlikely(test_bit(BR_FDB_ADDED_BY_USER, &flags)))
@@ -1082,6 +1095,16 @@ static int fdb_add_entry(struct net_bridge *br, struct net_bridge_port *source,
modified = true;
}
+ if (test_bit(BR_FDB_ENTRY_LOCKED, &fdb->flags)) {
+ clear_bit(BR_FDB_ENTRY_LOCKED, &fdb->flags);
+ modified = true;
+ }
+
+ if (test_bit(BR_FDB_BLACKHOLE, &fdb->flags)) {
+ clear_bit(BR_FDB_BLACKHOLE, &fdb->flags);
+ modified = true;
+ }
+
if (fdb_handle_notify(fdb, notify))
modified = true;
@@ -1178,6 +1201,12 @@ int br_fdb_add(struct ndmsg *ndm, struct nlattr *tb[],
vg = nbp_vlan_group(p);
}
+ if (tb[NDA_FLAGS_EXT] &&
+ (nla_get_u32(tb[NDA_FLAGS_EXT]) & (NTF_EXT_LOCKED | NTF_EXT_BLACKHOLE))) {
+ pr_info("bridge: RTM_NEWNEIGH has invalid extended flags\n");
+ return -EINVAL;
+ }
+
if (tb[NDA_FDB_EXT_ATTRS]) {
attr = tb[NDA_FDB_EXT_ATTRS];
err = nla_parse_nested(nfea_tb, NFEA_MAX, attr,
diff --git a/net/bridge/br_input.c b/net/bridge/br_input.c
index 68b3e850bcb9..3d48aa7fa778 100644
--- a/net/bridge/br_input.c
+++ b/net/bridge/br_input.c
@@ -110,8 +110,19 @@ int br_handle_frame_finish(struct net *net, struct sock *sk, struct sk_buff *skb
br_fdb_find_rcu(br, eth_hdr(skb)->h_source, vid);
if (!fdb_src || READ_ONCE(fdb_src->dst) != p ||
- test_bit(BR_FDB_LOCAL, &fdb_src->flags))
+ test_bit(BR_FDB_LOCAL, &fdb_src->flags) ||
+ test_bit(BR_FDB_ENTRY_LOCKED, &fdb_src->flags)) {
+ if (!fdb_src || (READ_ONCE(fdb_src->dst) != p &&
+ (p->flags & BR_LEARNING))) {
+ unsigned long flags = 0;
+
+ if (p->flags & BR_PORT_MAB) {
+ __set_bit(BR_FDB_ENTRY_LOCKED, &flags);
+ br_fdb_update(br, p, eth_hdr(skb)->h_source, vid, flags);
+ }
+ }
goto drop;
+ }
}
nbp_switchdev_frame_mark(p, skb);
@@ -185,6 +196,9 @@ int br_handle_frame_finish(struct net *net, struct sock *sk, struct sk_buff *skb
if (test_bit(BR_FDB_LOCAL, &dst->flags))
return br_pass_frame_up(skb);
+ if (test_bit(BR_FDB_BLACKHOLE, &dst->flags))
+ goto drop;
+
if (now != dst->used)
dst->used = now;
br_forward(dst->dst, skb, local_rcv, false);
diff --git a/net/bridge/br_netlink.c b/net/bridge/br_netlink.c
index 5aeb3646e74c..34483420c9e4 100644
--- a/net/bridge/br_netlink.c
+++ b/net/bridge/br_netlink.c
@@ -188,6 +188,7 @@ static inline size_t br_port_info_size(void)
+ nla_total_size(1) /* IFLA_BRPORT_NEIGH_SUPPRESS */
+ nla_total_size(1) /* IFLA_BRPORT_ISOLATED */
+ nla_total_size(1) /* IFLA_BRPORT_LOCKED */
+ + nla_total_size(1) /* IFLA_BRPORT_MAB */
+ nla_total_size(sizeof(struct ifla_bridge_id)) /* IFLA_BRPORT_ROOT_ID */
+ nla_total_size(sizeof(struct ifla_bridge_id)) /* IFLA_BRPORT_BRIDGE_ID */
+ nla_total_size(sizeof(u16)) /* IFLA_BRPORT_DESIGNATED_PORT */
@@ -274,7 +275,8 @@ static int br_port_fill_attrs(struct sk_buff *skb,
nla_put_u8(skb, IFLA_BRPORT_MRP_IN_OPEN,
!!(p->flags & BR_MRP_LOST_IN_CONT)) ||
nla_put_u8(skb, IFLA_BRPORT_ISOLATED, !!(p->flags & BR_ISOLATED)) ||
- nla_put_u8(skb, IFLA_BRPORT_LOCKED, !!(p->flags & BR_PORT_LOCKED)))
+ nla_put_u8(skb, IFLA_BRPORT_LOCKED, !!(p->flags & BR_PORT_LOCKED)) ||
+ nla_put_u8(skb, IFLA_BRPORT_MAB, !!(p->flags & BR_PORT_MAB)))
return -EMSGSIZE;
timerval = br_timer_value(&p->message_age_timer);
@@ -876,6 +878,7 @@ static const struct nla_policy br_port_policy[IFLA_BRPORT_MAX + 1] = {
[IFLA_BRPORT_NEIGH_SUPPRESS] = { .type = NLA_U8 },
[IFLA_BRPORT_ISOLATED] = { .type = NLA_U8 },
[IFLA_BRPORT_LOCKED] = { .type = NLA_U8 },
+ [IFLA_BRPORT_MAB] = { .type = NLA_U8 },
[IFLA_BRPORT_BACKUP_PORT] = { .type = NLA_U32 },
[IFLA_BRPORT_MCAST_EHT_HOSTS_LIMIT] = { .type = NLA_U32 },
};
@@ -943,6 +946,10 @@ static int br_setport(struct net_bridge_port *p, struct nlattr *tb[],
br_set_port_flag(p, tb, IFLA_BRPORT_NEIGH_SUPPRESS, BR_NEIGH_SUPPRESS);
br_set_port_flag(p, tb, IFLA_BRPORT_ISOLATED, BR_ISOLATED);
br_set_port_flag(p, tb, IFLA_BRPORT_LOCKED, BR_PORT_LOCKED);
+ br_set_port_flag(p, tb, IFLA_BRPORT_MAB, BR_PORT_MAB);
+
+ if (!(p->flags & BR_PORT_LOCKED))
+ p->flags &= ~BR_PORT_MAB;
changed_mask = old_flags ^ p->flags;
diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h
index 06e5f6faa431..048e4afbc5a0 100644
--- a/net/bridge/br_private.h
+++ b/net/bridge/br_private.h
@@ -251,7 +251,9 @@ enum {
BR_FDB_ADDED_BY_EXT_LEARN,
BR_FDB_OFFLOADED,
BR_FDB_NOTIFY,
- BR_FDB_NOTIFY_INACTIVE
+ BR_FDB_NOTIFY_INACTIVE,
+ BR_FDB_ENTRY_LOCKED,
+ BR_FDB_BLACKHOLE,
};
struct net_bridge_fdb_key {
--
2.30.2
On 26/08/2022 14:45, Hans Schultz wrote:
> Add an intermediate state for clients behind a locked port to allow for
> possible opening of the port for said clients. The clients mac address
> will be added with the locked flag set, denying access through the port
> for the mac address, but also creating a new FDB add event giving
> userspace daemons the ability to unlock the mac address. This feature
> corresponds to the Mac-Auth and MAC Authentication Bypass (MAB) named
> features. The latter defined by Cisco.
>
> As locked FDB entries are a security measure to deny access for
> unauthorized hosts on specific ports, they will deny forwarding from
> any port to the (MAC, vlan) pair involved and locked entries will not
> be able by learning or otherwise to change the associated port.
>
> Only the kernel can set this FDB entry flag, while userspace can read
> the flag and remove it by replacing or deleting the FDB entry.
>
> Locked entries will age out with the set bridge ageing time.
>
> Also add a 'blackhole' fdb flag, ensuring that no forwarding from any
> port to a destination MAC that has a FDB entry with this flag on will
> occur. The packets will thus be dropped.
>
> Signed-off-by: Hans Schultz <[email protected]>
> ---
> include/linux/if_bridge.h | 1 +
> include/uapi/linux/if_link.h | 1 +
> include/uapi/linux/neighbour.h | 4 +++-
> net/bridge/br_fdb.c | 29 +++++++++++++++++++++++++++++
> net/bridge/br_input.c | 16 +++++++++++++++-
> net/bridge/br_netlink.c | 9 ++++++++-
> net/bridge/br_private.h | 4 +++-
> 7 files changed, 60 insertions(+), 4 deletions(-)
>
Hi,
Please add the blackhole flag in a separate patch.
A few more comments and questions below..
> diff --git a/include/linux/if_bridge.h b/include/linux/if_bridge.h
> index d62ef428e3aa..1668ac4d7adc 100644
> --- a/include/linux/if_bridge.h
> +++ b/include/linux/if_bridge.h
> @@ -59,6 +59,7 @@ struct br_ip_list {
> #define BR_MRP_LOST_IN_CONT BIT(19)
> #define BR_TX_FWD_OFFLOAD BIT(20)
> #define BR_PORT_LOCKED BIT(21)
> +#define BR_PORT_MAB BIT(22)
>
> #define BR_DEFAULT_AGEING_TIME (300 * HZ)
>
> diff --git a/include/uapi/linux/if_link.h b/include/uapi/linux/if_link.h
> index e36d9d2c65a7..fcbd0b85ad53 100644
> --- a/include/uapi/linux/if_link.h
> +++ b/include/uapi/linux/if_link.h
> @@ -560,6 +560,7 @@ enum {
> IFLA_BRPORT_MCAST_EHT_HOSTS_LIMIT,
> IFLA_BRPORT_MCAST_EHT_HOSTS_CNT,
> IFLA_BRPORT_LOCKED,
> + IFLA_BRPORT_MAB,
> __IFLA_BRPORT_MAX
> };
> #define IFLA_BRPORT_MAX (__IFLA_BRPORT_MAX - 1)
> diff --git a/include/uapi/linux/neighbour.h b/include/uapi/linux/neighbour.h
> index a998bf761635..bc1440a56b70 100644
> --- a/include/uapi/linux/neighbour.h
> +++ b/include/uapi/linux/neighbour.h
> @@ -52,7 +52,9 @@ enum {
> #define NTF_STICKY (1 << 6)
> #define NTF_ROUTER (1 << 7)
> /* Extended flags under NDA_FLAGS_EXT: */
> -#define NTF_EXT_MANAGED (1 << 0)
> +#define NTF_EXT_MANAGED (1 << 0)
> +#define NTF_EXT_LOCKED (1 << 1)
> +#define NTF_EXT_BLACKHOLE (1 << 2)
>
> /*
> * Neighbor Cache Entry States.
> diff --git a/net/bridge/br_fdb.c b/net/bridge/br_fdb.c
> index e7f4fccb6adb..1962d9594a48 100644
> --- a/net/bridge/br_fdb.c
> +++ b/net/bridge/br_fdb.c
> @@ -105,6 +105,7 @@ static int fdb_fill_info(struct sk_buff *skb, const struct net_bridge *br,
> struct nda_cacheinfo ci;
> struct nlmsghdr *nlh;
> struct ndmsg *ndm;
> + u32 ext_flags = 0;
>
> nlh = nlmsg_put(skb, portid, seq, type, sizeof(*ndm), flags);
> if (nlh == NULL)
> @@ -125,11 +126,18 @@ static int fdb_fill_info(struct sk_buff *skb, const struct net_bridge *br,
> ndm->ndm_flags |= NTF_EXT_LEARNED;
> if (test_bit(BR_FDB_STICKY, &fdb->flags))
> ndm->ndm_flags |= NTF_STICKY;
> + if (test_bit(BR_FDB_ENTRY_LOCKED, &fdb->flags))
> + ext_flags |= NTF_EXT_LOCKED;
> + if (test_bit(BR_FDB_BLACKHOLE, &fdb->flags))
> + ext_flags |= NTF_EXT_BLACKHOLE;
>
> if (nla_put(skb, NDA_LLADDR, ETH_ALEN, &fdb->key.addr))
> goto nla_put_failure;
> if (nla_put_u32(skb, NDA_MASTER, br->dev->ifindex))
> goto nla_put_failure;
> + if (nla_put_u32(skb, NDA_FLAGS_EXT, ext_flags))
> + goto nla_put_failure;
> +
> ci.ndm_used = jiffies_to_clock_t(now - fdb->used);
> ci.ndm_confirmed = 0;
> ci.ndm_updated = jiffies_to_clock_t(now - fdb->updated);
> @@ -171,6 +179,7 @@ static inline size_t fdb_nlmsg_size(void)
> return NLMSG_ALIGN(sizeof(struct ndmsg))
> + nla_total_size(ETH_ALEN) /* NDA_LLADDR */
> + nla_total_size(sizeof(u32)) /* NDA_MASTER */
> + + nla_total_size(sizeof(u32)) /* NDA_FLAGS_EXT */
> + nla_total_size(sizeof(u16)) /* NDA_VLAN */
> + nla_total_size(sizeof(struct nda_cacheinfo))
> + nla_total_size(0) /* NDA_FDB_EXT_ATTRS */
> @@ -879,6 +888,10 @@ void br_fdb_update(struct net_bridge *br, struct net_bridge_port *source,
> &fdb->flags)))
> clear_bit(BR_FDB_ADDED_BY_EXT_LEARN,
> &fdb->flags);
> + if (source->flags & BR_PORT_MAB)
> + set_bit(BR_FDB_ENTRY_LOCKED, &fdb->flags);
> + else
> + clear_bit(BR_FDB_ENTRY_LOCKED, &fdb->flags);
Please add a test for that bit and only then change it.
> }
>
> if (unlikely(test_bit(BR_FDB_ADDED_BY_USER, &flags)))
> @@ -1082,6 +1095,16 @@ static int fdb_add_entry(struct net_bridge *br, struct net_bridge_port *source,
> modified = true;
> }
>
> + if (test_bit(BR_FDB_ENTRY_LOCKED, &fdb->flags)) {
> + clear_bit(BR_FDB_ENTRY_LOCKED, &fdb->flags);
> + modified = true;
> + }
> +
> + if (test_bit(BR_FDB_BLACKHOLE, &fdb->flags)) {
> + clear_bit(BR_FDB_BLACKHOLE, &fdb->flags);
> + modified = true;
> + }
> +
> if (fdb_handle_notify(fdb, notify))
> modified = true;
>
> @@ -1178,6 +1201,12 @@ int br_fdb_add(struct ndmsg *ndm, struct nlattr *tb[],
> vg = nbp_vlan_group(p);
> }
>
> + if (tb[NDA_FLAGS_EXT] &&
> + (nla_get_u32(tb[NDA_FLAGS_EXT]) & (NTF_EXT_LOCKED | NTF_EXT_BLACKHOLE))) {
> + pr_info("bridge: RTM_NEWNEIGH has invalid extended flags\n");
> + return -EINVAL;
> + }
> +
> if (tb[NDA_FDB_EXT_ATTRS]) {
> attr = tb[NDA_FDB_EXT_ATTRS];
> err = nla_parse_nested(nfea_tb, NFEA_MAX, attr,
> diff --git a/net/bridge/br_input.c b/net/bridge/br_input.c
> index 68b3e850bcb9..3d48aa7fa778 100644
> --- a/net/bridge/br_input.c
> +++ b/net/bridge/br_input.c
> @@ -110,8 +110,19 @@ int br_handle_frame_finish(struct net *net, struct sock *sk, struct sk_buff *skb
> br_fdb_find_rcu(br, eth_hdr(skb)->h_source, vid);
>
> if (!fdb_src || READ_ONCE(fdb_src->dst) != p ||
> - test_bit(BR_FDB_LOCAL, &fdb_src->flags))
> + test_bit(BR_FDB_LOCAL, &fdb_src->flags) ||
> + test_bit(BR_FDB_ENTRY_LOCKED, &fdb_src->flags)) {
> + if (!fdb_src || (READ_ONCE(fdb_src->dst) != p &&
> + (p->flags & BR_LEARNING))) {
please break the second part of the check on a new line instead
> + unsigned long flags = 0;
> +
> + if (p->flags & BR_PORT_MAB) {
combine this and the above as one "if" block, no need to have a new one here
which preferrably starts with the MAB check
> + __set_bit(BR_FDB_ENTRY_LOCKED, &flags);
> + br_fdb_update(br, p, eth_hdr(skb)->h_source, vid, flags);> + }
> + }
> goto drop;
> + }
> }
>
> nbp_switchdev_frame_mark(p, skb);
> @@ -185,6 +196,9 @@ int br_handle_frame_finish(struct net *net, struct sock *sk, struct sk_buff *skb
> if (test_bit(BR_FDB_LOCAL, &dst->flags))
> return br_pass_frame_up(skb);
>
> + if (test_bit(BR_FDB_BLACKHOLE, &dst->flags))
> + goto drop;
> +
Not happy about adding a new test in arguably the most used fast-path, but I don't see
a better way to do blackhole right now. Could you please make it an unlikely() ?
I guess the blackhole flag will be allowed for user-space to set at some point, why
not do it from the start?
Actually adding a BR_FDB_LOCAL and BR_FDB_BLACKHOLE would be a conflict above -
the packet will be received. So you should move the blackhole check above the
BR_FDB_LOCAL one if user-space is allowed to set it to any entry.
> if (now != dst->used)
> dst->used = now;
> br_forward(dst->dst, skb, local_rcv, false);
> diff --git a/net/bridge/br_netlink.c b/net/bridge/br_netlink.c
> index 5aeb3646e74c..34483420c9e4 100644
> --- a/net/bridge/br_netlink.c
> +++ b/net/bridge/br_netlink.c
> @@ -188,6 +188,7 @@ static inline size_t br_port_info_size(void)
> + nla_total_size(1) /* IFLA_BRPORT_NEIGH_SUPPRESS */
> + nla_total_size(1) /* IFLA_BRPORT_ISOLATED */
> + nla_total_size(1) /* IFLA_BRPORT_LOCKED */
> + + nla_total_size(1) /* IFLA_BRPORT_MAB */
> + nla_total_size(sizeof(struct ifla_bridge_id)) /* IFLA_BRPORT_ROOT_ID */
> + nla_total_size(sizeof(struct ifla_bridge_id)) /* IFLA_BRPORT_BRIDGE_ID */
> + nla_total_size(sizeof(u16)) /* IFLA_BRPORT_DESIGNATED_PORT */
> @@ -274,7 +275,8 @@ static int br_port_fill_attrs(struct sk_buff *skb,
> nla_put_u8(skb, IFLA_BRPORT_MRP_IN_OPEN,
> !!(p->flags & BR_MRP_LOST_IN_CONT)) ||
> nla_put_u8(skb, IFLA_BRPORT_ISOLATED, !!(p->flags & BR_ISOLATED)) ||
> - nla_put_u8(skb, IFLA_BRPORT_LOCKED, !!(p->flags & BR_PORT_LOCKED)))
> + nla_put_u8(skb, IFLA_BRPORT_LOCKED, !!(p->flags & BR_PORT_LOCKED)) ||
> + nla_put_u8(skb, IFLA_BRPORT_MAB, !!(p->flags & BR_PORT_MAB)))
> return -EMSGSIZE;
>
> timerval = br_timer_value(&p->message_age_timer);
> @@ -876,6 +878,7 @@ static const struct nla_policy br_port_policy[IFLA_BRPORT_MAX + 1] = {
> [IFLA_BRPORT_NEIGH_SUPPRESS] = { .type = NLA_U8 },
> [IFLA_BRPORT_ISOLATED] = { .type = NLA_U8 },
> [IFLA_BRPORT_LOCKED] = { .type = NLA_U8 },
> + [IFLA_BRPORT_MAB] = { .type = NLA_U8 },
> [IFLA_BRPORT_BACKUP_PORT] = { .type = NLA_U32 },
> [IFLA_BRPORT_MCAST_EHT_HOSTS_LIMIT] = { .type = NLA_U32 },
> };
> @@ -943,6 +946,10 @@ static int br_setport(struct net_bridge_port *p, struct nlattr *tb[],
> br_set_port_flag(p, tb, IFLA_BRPORT_NEIGH_SUPPRESS, BR_NEIGH_SUPPRESS);
> br_set_port_flag(p, tb, IFLA_BRPORT_ISOLATED, BR_ISOLATED);
> br_set_port_flag(p, tb, IFLA_BRPORT_LOCKED, BR_PORT_LOCKED);
> + br_set_port_flag(p, tb, IFLA_BRPORT_MAB, BR_PORT_MAB);
> +
> + if (!(p->flags & BR_PORT_LOCKED))
> + p->flags &= ~BR_PORT_MAB;
>
> changed_mask = old_flags ^ p->flags;
>
> diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h
> index 06e5f6faa431..048e4afbc5a0 100644
> --- a/net/bridge/br_private.h
> +++ b/net/bridge/br_private.h
> @@ -251,7 +251,9 @@ enum {
> BR_FDB_ADDED_BY_EXT_LEARN,
> BR_FDB_OFFLOADED,
> BR_FDB_NOTIFY,
> - BR_FDB_NOTIFY_INACTIVE
> + BR_FDB_NOTIFY_INACTIVE,
> + BR_FDB_ENTRY_LOCKED,
> + BR_FDB_BLACKHOLE,
> };
>
> struct net_bridge_fdb_key {
Cheers,
Nik
On Sat, Aug 27, 2022 at 02:30:25PM +0300, Nikolay Aleksandrov wrote:
> On 26/08/2022 14:45, Hans Schultz wrote:
> Please add the blackhole flag in a separate patch.
+1
[...]
> > @@ -185,6 +196,9 @@ int br_handle_frame_finish(struct net *net, struct sock *sk, struct sk_buff *skb
> > if (test_bit(BR_FDB_LOCAL, &dst->flags))
> > return br_pass_frame_up(skb);
> >
> > + if (test_bit(BR_FDB_BLACKHOLE, &dst->flags))
> > + goto drop;
> > +
> Not happy about adding a new test in arguably the most used fast-path, but I don't see
> a better way to do blackhole right now. Could you please make it an unlikely() ?
>
> I guess the blackhole flag will be allowed for user-space to set at some point, why
> not do it from the start?
>
> Actually adding a BR_FDB_LOCAL and BR_FDB_BLACKHOLE would be a conflict above -
> the packet will be received. So you should move the blackhole check above the
> BR_FDB_LOCAL one if user-space is allowed to set it to any entry.
Agree about unlikely() and making it writeable from user space from the
start. This flag is different from the "locked" flag that should only be
ever set by the kernel.
Regarding BR_FDB_LOCAL, I think BR_FDB_BLACKHOLE should only be allowed
with BR_FDB_LOCAL as these entries are similar in the following ways:
1. It doesn't make sense to associate a blackhole entry with a specific
port. The packet will never be forwarded to this port, but dropped by
the bridge. This means user space will add them on the bridge itself:
# bridge fdb add 00:11:22:33:44:55 dev br0 self local blackhole
2. If you agree that these entries should not be associated with a
specific port, then it also does not make sense to subject them to
ageing and roaming, just like existing local/permanent entries.
The above allows us to push the new check under the BR_FDB_LOCAL check:
diff --git a/net/bridge/br_input.c b/net/bridge/br_input.c
index 68b3e850bcb9..4357445529a5 100644
--- a/net/bridge/br_input.c
+++ b/net/bridge/br_input.c
@@ -182,8 +182,11 @@ 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)) {
+ if (unlikely(test_bit(BR_FDB_BLACKHOLE, &dst->flags)))
+ goto drop;
return br_pass_frame_up(skb);
+ }
if (now != dst->used)
dst->used = now;
On 27/08/2022 16:17, Ido Schimmel wrote:
> On Sat, Aug 27, 2022 at 02:30:25PM +0300, Nikolay Aleksandrov wrote:
>> On 26/08/2022 14:45, Hans Schultz wrote:
>> Please add the blackhole flag in a separate patch.
>
> +1
>
> [...]
>
>>> @@ -185,6 +196,9 @@ int br_handle_frame_finish(struct net *net, struct sock *sk, struct sk_buff *skb
>>> if (test_bit(BR_FDB_LOCAL, &dst->flags))
>>> return br_pass_frame_up(skb);
>>>
>>> + if (test_bit(BR_FDB_BLACKHOLE, &dst->flags))
>>> + goto drop;
>>> +
>> Not happy about adding a new test in arguably the most used fast-path, but I don't see
>> a better way to do blackhole right now. Could you please make it an unlikely() ?
>>
>> I guess the blackhole flag will be allowed for user-space to set at some point, why
>> not do it from the start?
>>
>> Actually adding a BR_FDB_LOCAL and BR_FDB_BLACKHOLE would be a conflict above -
>> the packet will be received. So you should move the blackhole check above the
>> BR_FDB_LOCAL one if user-space is allowed to set it to any entry.
>
> Agree about unlikely() and making it writeable from user space from the
> start. This flag is different from the "locked" flag that should only be
> ever set by the kernel.
>
> Regarding BR_FDB_LOCAL, I think BR_FDB_BLACKHOLE should only be allowed
> with BR_FDB_LOCAL as these entries are similar in the following ways:
>
> 1. It doesn't make sense to associate a blackhole entry with a specific
> port. The packet will never be forwarded to this port, but dropped by
> the bridge. This means user space will add them on the bridge itself:
>
Right, good point.
> # bridge fdb add 00:11:22:33:44:55 dev br0 self local blackhole
>
> 2. If you agree that these entries should not be associated with a
> specific port, then it also does not make sense to subject them to
> ageing and roaming, just like existing local/permanent entries.
>
> The above allows us to push the new check under the BR_FDB_LOCAL check:
>
hmm.. so only the driver will be allowed to add non-BR_FDB_LOCAL blackhole
entries with locked flag set as well, that sounds ok as they will be extern_learn
and enforced by it. It is a little discrepancy as we cannot add similar entries in SW
but it really doesn't make any sense to have blackhole fdbs pointing to a port.
SW won't be able to have a locked entry w/ blackhole set, but I like that it is hidden
in the fdb local case when fwding and that's good enough for me.
> diff --git a/net/bridge/br_input.c b/net/bridge/br_input.c
> index 68b3e850bcb9..4357445529a5 100644
> --- a/net/bridge/br_input.c
> +++ b/net/bridge/br_input.c
> @@ -182,8 +182,11 @@ 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)) {
> + if (unlikely(test_bit(BR_FDB_BLACKHOLE, &dst->flags)))
> + goto drop;
> return br_pass_frame_up(skb);
> + }
>
> if (now != dst->used)
> dst->used = now;
On Fri, Aug 26, 2022 at 01:45:33PM +0200, Hans Schultz wrote:
> diff --git a/include/uapi/linux/neighbour.h b/include/uapi/linux/neighbour.h
> index a998bf761635..bc1440a56b70 100644
> --- a/include/uapi/linux/neighbour.h
> +++ b/include/uapi/linux/neighbour.h
> @@ -52,7 +52,9 @@ enum {
> #define NTF_STICKY (1 << 6)
> #define NTF_ROUTER (1 << 7)
> /* Extended flags under NDA_FLAGS_EXT: */
> -#define NTF_EXT_MANAGED (1 << 0)
> +#define NTF_EXT_MANAGED (1 << 0)
> +#define NTF_EXT_LOCKED (1 << 1)
> +#define NTF_EXT_BLACKHOLE (1 << 2)
A few lines below in the file there is a comment explaining
NTF_EXT_MANAGED. Please document NTF_EXT_LOCKED and NTF_EXT_BLACKHOLE as
well.
>
> /*
> * Neighbor Cache Entry States.
[...]
> @@ -1082,6 +1095,16 @@ static int fdb_add_entry(struct net_bridge *br, struct net_bridge_port *source,
> modified = true;
> }
>
> + if (test_bit(BR_FDB_ENTRY_LOCKED, &fdb->flags)) {
> + clear_bit(BR_FDB_ENTRY_LOCKED, &fdb->flags);
> + modified = true;
> + }
Should be able to use test_and_clear_bit():
diff --git a/net/bridge/br_fdb.c b/net/bridge/br_fdb.c
index e7f4fccb6adb..e5561ee2bfac 100644
--- a/net/bridge/br_fdb.c
+++ b/net/bridge/br_fdb.c
@@ -1082,6 +1082,9 @@ static int fdb_add_entry(struct net_bridge *br, struct net_bridge_port *source,
modified = true;
}
+ if (test_and_clear_bit(BR_FDB_ENTRY_LOCKED, &fdb->flags))
+ modified = true;
+
if (fdb_handle_notify(fdb, notify))
modified = true;
> +
> + if (test_bit(BR_FDB_BLACKHOLE, &fdb->flags)) {
> + clear_bit(BR_FDB_BLACKHOLE, &fdb->flags);
> + modified = true;
> + }
This will need to change to allow user space to set the flag.
> +
> if (fdb_handle_notify(fdb, notify))
> modified = true;
>
> @@ -1178,6 +1201,12 @@ int br_fdb_add(struct ndmsg *ndm, struct nlattr *tb[],
> vg = nbp_vlan_group(p);
> }
>
> + if (tb[NDA_FLAGS_EXT] &&
> + (nla_get_u32(tb[NDA_FLAGS_EXT]) & (NTF_EXT_LOCKED | NTF_EXT_BLACKHOLE))) {
> + pr_info("bridge: RTM_NEWNEIGH has invalid extended flags\n");
> + return -EINVAL;
> + }
> +
> if (tb[NDA_FDB_EXT_ATTRS]) {
> attr = tb[NDA_FDB_EXT_ATTRS];
> err = nla_parse_nested(nfea_tb, NFEA_MAX, attr,
> diff --git a/net/bridge/br_input.c b/net/bridge/br_input.c
> index 68b3e850bcb9..3d48aa7fa778 100644
> --- a/net/bridge/br_input.c
> +++ b/net/bridge/br_input.c
> @@ -110,8 +110,19 @@ int br_handle_frame_finish(struct net *net, struct sock *sk, struct sk_buff *skb
> br_fdb_find_rcu(br, eth_hdr(skb)->h_source, vid);
>
> if (!fdb_src || READ_ONCE(fdb_src->dst) != p ||
> - test_bit(BR_FDB_LOCAL, &fdb_src->flags))
> + test_bit(BR_FDB_LOCAL, &fdb_src->flags) ||
> + test_bit(BR_FDB_ENTRY_LOCKED, &fdb_src->flags)) {
> + if (!fdb_src || (READ_ONCE(fdb_src->dst) != p &&
> + (p->flags & BR_LEARNING))) {
It looks like you are allowing a locked port to:
1. Overtake a local entry. Actually, it will be rejected by
br_fdb_update() with a rate limited error message, but best to avoid it.
2. Overtake an entry pointing to an unlocked port. There is no reason
for an authorized port to lose communication because an unauthorized
port decided to spoof its MAC.
> + unsigned long flags = 0;
> +
> + if (p->flags & BR_PORT_MAB) {
> + __set_bit(BR_FDB_ENTRY_LOCKED, &flags);
> + br_fdb_update(br, p, eth_hdr(skb)->h_source, vid, flags);
> + }
> + }
> goto drop;
> + }
> }
How about the below (untested):
diff --git a/net/bridge/br_input.c b/net/bridge/br_input.c
index 68b3e850bcb9..9143a94a1c57 100644
--- a/net/bridge/br_input.c
+++ b/net/bridge/br_input.c
@@ -109,9 +109,18 @@ int br_handle_frame_finish(struct net *net, struct sock *sk, struct sk_buff *skb
struct net_bridge_fdb_entry *fdb_src =
br_fdb_find_rcu(br, eth_hdr(skb)->h_source, vid);
- if (!fdb_src || READ_ONCE(fdb_src->dst) != p ||
- test_bit(BR_FDB_LOCAL, &fdb_src->flags))
+ if (!fdb_src) {
+ if (p->flags & BR_PORT_MAB) {
+ __set_bit(BR_FDB_ENTRY_LOCKED, &flags);
+ br_fdb_update(br, p, eth_hdr(skb)->h_source,
+ vid, flags);
+ }
+ goto drop;
+ } else if (READ_ONCE(fdb_src->dst) != p ||
+ test_bit(BR_FDB_LOCAL, &fdb_src->flags) ||
+ test_bit(BR_FDB_LOCKED, &fdb_src->flags)) {
goto drop;
+ }
}
The semantics are very clear, IMO. On FDB miss, add a locked FDB entry
and drop the packet. On FDB mismatch, drop the packet.
Entry can roam from an unauthorized port to an authorized port, but not
the other way around. Not sure what is the use case for allowing roaming
between unauthorized ports.
Note that with the above, locked entries are not refreshed and will
therefore age out unless replaced by user space.
>
> nbp_switchdev_frame_mark(p, skb);
> @@ -943,6 +946,10 @@ static int br_setport(struct net_bridge_port *p, struct nlattr *tb[],
> br_set_port_flag(p, tb, IFLA_BRPORT_NEIGH_SUPPRESS, BR_NEIGH_SUPPRESS);
> br_set_port_flag(p, tb, IFLA_BRPORT_ISOLATED, BR_ISOLATED);
> br_set_port_flag(p, tb, IFLA_BRPORT_LOCKED, BR_PORT_LOCKED);
> + br_set_port_flag(p, tb, IFLA_BRPORT_MAB, BR_PORT_MAB);
> +
> + if (!(p->flags & BR_PORT_LOCKED))
> + p->flags &= ~BR_PORT_MAB;
Any reason not to emit an error if MAB is enabled while the port is
unlocked? Something like this (untested):
diff --git a/net/bridge/br_netlink.c b/net/bridge/br_netlink.c
index 5aeb3646e74c..18353a4c29e1 100644
--- a/net/bridge/br_netlink.c
+++ b/net/bridge/br_netlink.c
@@ -944,6 +944,12 @@ static int br_setport(struct net_bridge_port *p, struct nlattr *tb[],
br_set_port_flag(p, tb, IFLA_BRPORT_ISOLATED, BR_ISOLATED);
br_set_port_flag(p, tb, IFLA_BRPORT_LOCKED, BR_PORT_LOCKED);
+ if (!(p->flags & BR_PORT_LOCKED) && (p->flags & BR_PORT_MAB)) {
+ NL_SET_ERR_MSG(extack, "MAB cannot be enabled when port is unlocked");
+ p->flags = old_flags;
+ return -EINVAL;
+ }
+
changed_mask = old_flags ^ p->flags;
err = br_switchdev_set_port_flag(p, p->flags, changed_mask, extack);
>
> changed_mask = old_flags ^ p->flags;
>
> diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h
> index 06e5f6faa431..048e4afbc5a0 100644
> --- a/net/bridge/br_private.h
> +++ b/net/bridge/br_private.h
> @@ -251,7 +251,9 @@ enum {
> BR_FDB_ADDED_BY_EXT_LEARN,
> BR_FDB_OFFLOADED,
> BR_FDB_NOTIFY,
> - BR_FDB_NOTIFY_INACTIVE
> + BR_FDB_NOTIFY_INACTIVE,
> + BR_FDB_ENTRY_LOCKED,
> + BR_FDB_BLACKHOLE,
> };
>
> struct net_bridge_fdb_key {
> --
> 2.30.2
>
On 2022-08-27 17:19, Ido Schimmel wrote:
> On Fri, Aug 26, 2022 at 01:45:33PM +0200, Hans Schultz wrote:
>>
>> nbp_switchdev_frame_mark(p, skb);
>> @@ -943,6 +946,10 @@ static int br_setport(struct net_bridge_port *p,
>> struct nlattr *tb[],
>> br_set_port_flag(p, tb, IFLA_BRPORT_NEIGH_SUPPRESS,
>> BR_NEIGH_SUPPRESS);
>> br_set_port_flag(p, tb, IFLA_BRPORT_ISOLATED, BR_ISOLATED);
>> br_set_port_flag(p, tb, IFLA_BRPORT_LOCKED, BR_PORT_LOCKED);
>> + br_set_port_flag(p, tb, IFLA_BRPORT_MAB, BR_PORT_MAB);
>> +
>> + if (!(p->flags & BR_PORT_LOCKED))
>> + p->flags &= ~BR_PORT_MAB;
The reason for this is that I wanted it to be so that if you have MAB
enabled (and locked of course) and unlock the port, it will
automatically clear both flags instead of having to first disable MAB
and then unlock the port.
>
> Any reason not to emit an error if MAB is enabled while the port is
> unlocked? Something like this (untested):
>
> diff --git a/net/bridge/br_netlink.c b/net/bridge/br_netlink.c
> index 5aeb3646e74c..18353a4c29e1 100644
> --- a/net/bridge/br_netlink.c
> +++ b/net/bridge/br_netlink.c
> @@ -944,6 +944,12 @@ static int br_setport(struct net_bridge_port *p,
> struct nlattr *tb[],
> br_set_port_flag(p, tb, IFLA_BRPORT_ISOLATED, BR_ISOLATED);
> br_set_port_flag(p, tb, IFLA_BRPORT_LOCKED, BR_PORT_LOCKED);
>
> + if (!(p->flags & BR_PORT_LOCKED) && (p->flags & BR_PORT_MAB)) {
> + NL_SET_ERR_MSG(extack, "MAB cannot be enabled when
> port is unlocked");
> + p->flags = old_flags;
> + return -EINVAL;
> + }
> +
> changed_mask = old_flags ^ p->flags;
>
> err = br_switchdev_set_port_flag(p, p->flags, changed_mask,
> extack);
>
On 2022-08-27 13:30, Nikolay Aleksandrov wrote:
> On 26/08/2022 14:45, Hans Schultz wrote:
>> Add an intermediate state for clients behind a locked port to allow
>> for
>> possible opening of the port for said clients. The clients mac address
>> will be added with the locked flag set, denying access through the
>> port
>> for the mac address, but also creating a new FDB add event giving
>> userspace daemons the ability to unlock the mac address. This feature
>> corresponds to the Mac-Auth and MAC Authentication Bypass (MAB) named
>> features. The latter defined by Cisco.
>>
>> As locked FDB entries are a security measure to deny access for
>> unauthorized hosts on specific ports, they will deny forwarding from
>> any port to the (MAC, vlan) pair involved and locked entries will not
>> be able by learning or otherwise to change the associated port.
>>
>> Only the kernel can set this FDB entry flag, while userspace can read
>> the flag and remove it by replacing or deleting the FDB entry.
>>
>> Locked entries will age out with the set bridge ageing time.
>>
>> Also add a 'blackhole' fdb flag, ensuring that no forwarding from any
>> port to a destination MAC that has a FDB entry with this flag on will
>> occur. The packets will thus be dropped.
>>
>> Signed-off-by: Hans Schultz <[email protected]>
>> ---
>> include/linux/if_bridge.h | 1 +
>> include/uapi/linux/if_link.h | 1 +
>> include/uapi/linux/neighbour.h | 4 +++-
>> net/bridge/br_fdb.c | 29 +++++++++++++++++++++++++++++
>> net/bridge/br_input.c | 16 +++++++++++++++-
>> net/bridge/br_netlink.c | 9 ++++++++-
>> net/bridge/br_private.h | 4 +++-
>> 7 files changed, 60 insertions(+), 4 deletions(-)
>>
>
> Hi,
> Please add the blackhole flag in a separate patch.
> A few more comments and questions below..
>
Okay, shall do. :-)
>> diff --git a/include/linux/if_bridge.h b/include/linux/if_bridge.h
>> index d62ef428e3aa..1668ac4d7adc 100644
>> --- a/include/linux/if_bridge.h
>> +++ b/include/linux/if_bridge.h
>> @@ -59,6 +59,7 @@ struct br_ip_list {
>> #define BR_MRP_LOST_IN_CONT BIT(19)
>> #define BR_TX_FWD_OFFLOAD BIT(20)
>> #define BR_PORT_LOCKED BIT(21)
>> +#define BR_PORT_MAB BIT(22)
>>
>> #define BR_DEFAULT_AGEING_TIME (300 * HZ)
>>
>> diff --git a/include/uapi/linux/if_link.h
>> b/include/uapi/linux/if_link.h
>> index e36d9d2c65a7..fcbd0b85ad53 100644
>> --- a/include/uapi/linux/if_link.h
>> +++ b/include/uapi/linux/if_link.h
>> @@ -560,6 +560,7 @@ enum {
>> IFLA_BRPORT_MCAST_EHT_HOSTS_LIMIT,
>> IFLA_BRPORT_MCAST_EHT_HOSTS_CNT,
>> IFLA_BRPORT_LOCKED,
>> + IFLA_BRPORT_MAB,
>> __IFLA_BRPORT_MAX
>> };
>> #define IFLA_BRPORT_MAX (__IFLA_BRPORT_MAX - 1)
>> diff --git a/include/uapi/linux/neighbour.h
>> b/include/uapi/linux/neighbour.h
>> index a998bf761635..bc1440a56b70 100644
>> --- a/include/uapi/linux/neighbour.h
>> +++ b/include/uapi/linux/neighbour.h
>> @@ -52,7 +52,9 @@ enum {
>> #define NTF_STICKY (1 << 6)
>> #define NTF_ROUTER (1 << 7)
>> /* Extended flags under NDA_FLAGS_EXT: */
>> -#define NTF_EXT_MANAGED (1 << 0)
>> +#define NTF_EXT_MANAGED (1 << 0)
>> +#define NTF_EXT_LOCKED (1 << 1)
>> +#define NTF_EXT_BLACKHOLE (1 << 2)
>>
>> /*
>> * Neighbor Cache Entry States.
>> diff --git a/net/bridge/br_fdb.c b/net/bridge/br_fdb.c
>> index e7f4fccb6adb..1962d9594a48 100644
>> --- a/net/bridge/br_fdb.c
>> +++ b/net/bridge/br_fdb.c
>> @@ -105,6 +105,7 @@ static int fdb_fill_info(struct sk_buff *skb,
>> const struct net_bridge *br,
>> struct nda_cacheinfo ci;
>> struct nlmsghdr *nlh;
>> struct ndmsg *ndm;
>> + u32 ext_flags = 0;
>>
>> nlh = nlmsg_put(skb, portid, seq, type, sizeof(*ndm), flags);
>> if (nlh == NULL)
>> @@ -125,11 +126,18 @@ static int fdb_fill_info(struct sk_buff *skb,
>> const struct net_bridge *br,
>> ndm->ndm_flags |= NTF_EXT_LEARNED;
>> if (test_bit(BR_FDB_STICKY, &fdb->flags))
>> ndm->ndm_flags |= NTF_STICKY;
>> + if (test_bit(BR_FDB_ENTRY_LOCKED, &fdb->flags))
>> + ext_flags |= NTF_EXT_LOCKED;
>> + if (test_bit(BR_FDB_BLACKHOLE, &fdb->flags))
>> + ext_flags |= NTF_EXT_BLACKHOLE;
>>
>> if (nla_put(skb, NDA_LLADDR, ETH_ALEN, &fdb->key.addr))
>> goto nla_put_failure;
>> if (nla_put_u32(skb, NDA_MASTER, br->dev->ifindex))
>> goto nla_put_failure;
>> + if (nla_put_u32(skb, NDA_FLAGS_EXT, ext_flags))
>> + goto nla_put_failure;
>> +
>> ci.ndm_used = jiffies_to_clock_t(now - fdb->used);
>> ci.ndm_confirmed = 0;
>> ci.ndm_updated = jiffies_to_clock_t(now - fdb->updated);
>> @@ -171,6 +179,7 @@ static inline size_t fdb_nlmsg_size(void)
>> return NLMSG_ALIGN(sizeof(struct ndmsg))
>> + nla_total_size(ETH_ALEN) /* NDA_LLADDR */
>> + nla_total_size(sizeof(u32)) /* NDA_MASTER */
>> + + nla_total_size(sizeof(u32)) /* NDA_FLAGS_EXT */
>> + nla_total_size(sizeof(u16)) /* NDA_VLAN */
>> + nla_total_size(sizeof(struct nda_cacheinfo))
>> + nla_total_size(0) /* NDA_FDB_EXT_ATTRS */
>> @@ -879,6 +888,10 @@ void br_fdb_update(struct net_bridge *br, struct
>> net_bridge_port *source,
>> &fdb->flags)))
>> clear_bit(BR_FDB_ADDED_BY_EXT_LEARN,
>> &fdb->flags);
>> + if (source->flags & BR_PORT_MAB)
>> + set_bit(BR_FDB_ENTRY_LOCKED, &fdb->flags);
>> + else
>> + clear_bit(BR_FDB_ENTRY_LOCKED, &fdb->flags);
> Please add a test for that bit and only then change it.
>
Okay.
>> }
>>
>> if (unlikely(test_bit(BR_FDB_ADDED_BY_USER, &flags)))
>> @@ -1082,6 +1095,16 @@ static int fdb_add_entry(struct net_bridge *br,
>> struct net_bridge_port *source,
>> modified = true;
>> }
>>
>> + if (test_bit(BR_FDB_ENTRY_LOCKED, &fdb->flags)) {
>> + clear_bit(BR_FDB_ENTRY_LOCKED, &fdb->flags);
>> + modified = true;
>> + }
>> +
>> + if (test_bit(BR_FDB_BLACKHOLE, &fdb->flags)) {
>> + clear_bit(BR_FDB_BLACKHOLE, &fdb->flags);
>> + modified = true;
>> + }
>> +
>> if (fdb_handle_notify(fdb, notify))
>> modified = true;
>>
>> @@ -1178,6 +1201,12 @@ int br_fdb_add(struct ndmsg *ndm, struct nlattr
>> *tb[],
>> vg = nbp_vlan_group(p);
>> }
>>
>> + if (tb[NDA_FLAGS_EXT] &&
>> + (nla_get_u32(tb[NDA_FLAGS_EXT]) & (NTF_EXT_LOCKED |
>> NTF_EXT_BLACKHOLE))) {
>> + pr_info("bridge: RTM_NEWNEIGH has invalid extended flags\n");
>> + return -EINVAL;
>> + }
>> +
>> if (tb[NDA_FDB_EXT_ATTRS]) {
>> attr = tb[NDA_FDB_EXT_ATTRS];
>> err = nla_parse_nested(nfea_tb, NFEA_MAX, attr,
>> diff --git a/net/bridge/br_input.c b/net/bridge/br_input.c
>> index 68b3e850bcb9..3d48aa7fa778 100644
>> --- a/net/bridge/br_input.c
>> +++ b/net/bridge/br_input.c
>> @@ -110,8 +110,19 @@ int br_handle_frame_finish(struct net *net,
>> struct sock *sk, struct sk_buff *skb
>> br_fdb_find_rcu(br, eth_hdr(skb)->h_source, vid);
>>
>> if (!fdb_src || READ_ONCE(fdb_src->dst) != p ||
>> - test_bit(BR_FDB_LOCAL, &fdb_src->flags))
>> + test_bit(BR_FDB_LOCAL, &fdb_src->flags) ||
>> + test_bit(BR_FDB_ENTRY_LOCKED, &fdb_src->flags)) {
>> + if (!fdb_src || (READ_ONCE(fdb_src->dst) != p &&
>> + (p->flags & BR_LEARNING))) {
>
> please break the second part of the check on a new line instead
>
>> + unsigned long flags = 0;
>> +
>> + if (p->flags & BR_PORT_MAB) {
>
> combine this and the above as one "if" block, no need to have a new one
> here
> which preferrably starts with the MAB check
>
As the above if is an "or", and the MAB check works as an "and", it will
have to be replicated for all "or" checks, thus twice in the above if.
The first check in the above "or" is for new entries, while the second
part of the "or" is to allow roaming to a locked port.
>> + __set_bit(BR_FDB_ENTRY_LOCKED, &flags);
>> + br_fdb_update(br, p, eth_hdr(skb)->h_source, vid, flags);>
>> + }
>> + }
>> goto drop;
>> + }
>> }
>>
>> nbp_switchdev_frame_mark(p, skb);
>> @@ -185,6 +196,9 @@ int br_handle_frame_finish(struct net *net, struct
>> sock *sk, struct sk_buff *skb
>> if (test_bit(BR_FDB_LOCAL, &dst->flags))
>> return br_pass_frame_up(skb);
>>
>> + if (test_bit(BR_FDB_BLACKHOLE, &dst->flags))
>> + goto drop;
>> +
> Not happy about adding a new test in arguably the most used fast-path,
> but I don't see
> a better way to do blackhole right now. Could you please make it an
> unlikely() ?
Surely.
>
> I guess the blackhole flag will be allowed for user-space to set at
> some point, why
> not do it from the start?
I guess it should be so. :-)
>
> Actually adding a BR_FDB_LOCAL and BR_FDB_BLACKHOLE would be a conflict
> above -
> the packet will be received. So you should move the blackhole check
> above the
> BR_FDB_LOCAL one if user-space is allowed to set it to any entry.
>
>> if (now != dst->used)
>> dst->used = now;
>> br_forward(dst->dst, skb, local_rcv, false);
>> diff --git a/net/bridge/br_netlink.c b/net/bridge/br_netlink.c
>> index 5aeb3646e74c..34483420c9e4 100644
>> --- a/net/bridge/br_netlink.c
>> +++ b/net/bridge/br_netlink.c
>> @@ -188,6 +188,7 @@ static inline size_t br_port_info_size(void)
>> + nla_total_size(1) /* IFLA_BRPORT_NEIGH_SUPPRESS */
>> + nla_total_size(1) /* IFLA_BRPORT_ISOLATED */
>> + nla_total_size(1) /* IFLA_BRPORT_LOCKED */
>> + + nla_total_size(1) /* IFLA_BRPORT_MAB */
>> + nla_total_size(sizeof(struct ifla_bridge_id)) /*
>> IFLA_BRPORT_ROOT_ID */
>> + nla_total_size(sizeof(struct ifla_bridge_id)) /*
>> IFLA_BRPORT_BRIDGE_ID */
>> + nla_total_size(sizeof(u16)) /* IFLA_BRPORT_DESIGNATED_PORT */
>> @@ -274,7 +275,8 @@ static int br_port_fill_attrs(struct sk_buff *skb,
>> nla_put_u8(skb, IFLA_BRPORT_MRP_IN_OPEN,
>> !!(p->flags & BR_MRP_LOST_IN_CONT)) ||
>> nla_put_u8(skb, IFLA_BRPORT_ISOLATED, !!(p->flags &
>> BR_ISOLATED)) ||
>> - nla_put_u8(skb, IFLA_BRPORT_LOCKED, !!(p->flags &
>> BR_PORT_LOCKED)))
>> + nla_put_u8(skb, IFLA_BRPORT_LOCKED, !!(p->flags &
>> BR_PORT_LOCKED)) ||
>> + nla_put_u8(skb, IFLA_BRPORT_MAB, !!(p->flags & BR_PORT_MAB)))
>> return -EMSGSIZE;
>>
>> timerval = br_timer_value(&p->message_age_timer);
>> @@ -876,6 +878,7 @@ static const struct nla_policy
>> br_port_policy[IFLA_BRPORT_MAX + 1] = {
>> [IFLA_BRPORT_NEIGH_SUPPRESS] = { .type = NLA_U8 },
>> [IFLA_BRPORT_ISOLATED] = { .type = NLA_U8 },
>> [IFLA_BRPORT_LOCKED] = { .type = NLA_U8 },
>> + [IFLA_BRPORT_MAB] = { .type = NLA_U8 },
>> [IFLA_BRPORT_BACKUP_PORT] = { .type = NLA_U32 },
>> [IFLA_BRPORT_MCAST_EHT_HOSTS_LIMIT] = { .type = NLA_U32 },
>> };
>> @@ -943,6 +946,10 @@ static int br_setport(struct net_bridge_port *p,
>> struct nlattr *tb[],
>> br_set_port_flag(p, tb, IFLA_BRPORT_NEIGH_SUPPRESS,
>> BR_NEIGH_SUPPRESS);
>> br_set_port_flag(p, tb, IFLA_BRPORT_ISOLATED, BR_ISOLATED);
>> br_set_port_flag(p, tb, IFLA_BRPORT_LOCKED, BR_PORT_LOCKED);
>> + br_set_port_flag(p, tb, IFLA_BRPORT_MAB, BR_PORT_MAB);
>> +
>> + if (!(p->flags & BR_PORT_LOCKED))
>> + p->flags &= ~BR_PORT_MAB;
>>
>> changed_mask = old_flags ^ p->flags;
>>
>> diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h
>> index 06e5f6faa431..048e4afbc5a0 100644
>> --- a/net/bridge/br_private.h
>> +++ b/net/bridge/br_private.h
>> @@ -251,7 +251,9 @@ enum {
>> BR_FDB_ADDED_BY_EXT_LEARN,
>> BR_FDB_OFFLOADED,
>> BR_FDB_NOTIFY,
>> - BR_FDB_NOTIFY_INACTIVE
>> + BR_FDB_NOTIFY_INACTIVE,
>> + BR_FDB_ENTRY_LOCKED,
>> + BR_FDB_BLACKHOLE,
>> };
>>
>> struct net_bridge_fdb_key {
>
> Cheers,
> Nik
On 2022-08-27 15:17, Ido Schimmel wrote:
> On Sat, Aug 27, 2022 at 02:30:25PM +0300, Nikolay Aleksandrov wrote:
>> On 26/08/2022 14:45, Hans Schultz wrote:
>> Please add the blackhole flag in a separate patch.
>
> +1
>
> [...]
>
>> > @@ -185,6 +196,9 @@ int br_handle_frame_finish(struct net *net, struct sock *sk, struct sk_buff *skb
>> > if (test_bit(BR_FDB_LOCAL, &dst->flags))
>> > return br_pass_frame_up(skb);
>> >
>> > + if (test_bit(BR_FDB_BLACKHOLE, &dst->flags))
>> > + goto drop;
>> > +
>> Not happy about adding a new test in arguably the most used fast-path,
>> but I don't see
>> a better way to do blackhole right now. Could you please make it an
>> unlikely() ?
>>
>> I guess the blackhole flag will be allowed for user-space to set at
>> some point, why
>> not do it from the start?
>>
>> Actually adding a BR_FDB_LOCAL and BR_FDB_BLACKHOLE would be a
>> conflict above -
>> the packet will be received. So you should move the blackhole check
>> above the
>> BR_FDB_LOCAL one if user-space is allowed to set it to any entry.
>
> Agree about unlikely() and making it writeable from user space from the
> start. This flag is different from the "locked" flag that should only
> be
> ever set by the kernel.
>
> Regarding BR_FDB_LOCAL, I think BR_FDB_BLACKHOLE should only be allowed
> with BR_FDB_LOCAL as these entries are similar in the following ways:
>
> 1. It doesn't make sense to associate a blackhole entry with a specific
> port. The packet will never be forwarded to this port, but dropped by
> the bridge. This means user space will add them on the bridge itself:
>
> # bridge fdb add 00:11:22:33:44:55 dev br0 self local blackhole
>
> 2. If you agree that these entries should not be associated with a
> specific port, then it also does not make sense to subject them to
> ageing and roaming, just like existing local/permanent entries.
>
> The above allows us to push the new check under the BR_FDB_LOCAL check:
>
> diff --git a/net/bridge/br_input.c b/net/bridge/br_input.c
> index 68b3e850bcb9..4357445529a5 100644
> --- a/net/bridge/br_input.c
> +++ b/net/bridge/br_input.c
> @@ -182,8 +182,11 @@ 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)) {
> + if (unlikely(test_bit(BR_FDB_BLACKHOLE,
> &dst->flags)))
> + goto drop;
> return br_pass_frame_up(skb);
> + }
>
> if (now != dst->used)
> dst->used = now;
It shall be so as suggested. :-)
On Sun, Aug 28, 2022 at 12:23:30PM +0200, [email protected] wrote:
> On 2022-08-27 17:19, Ido Schimmel wrote:
> > On Fri, Aug 26, 2022 at 01:45:33PM +0200, Hans Schultz wrote:
> > >
> > > nbp_switchdev_frame_mark(p, skb);
> > > @@ -943,6 +946,10 @@ static int br_setport(struct net_bridge_port
> > > *p, struct nlattr *tb[],
> > > br_set_port_flag(p, tb, IFLA_BRPORT_NEIGH_SUPPRESS,
> > > BR_NEIGH_SUPPRESS);
> > > br_set_port_flag(p, tb, IFLA_BRPORT_ISOLATED, BR_ISOLATED);
> > > br_set_port_flag(p, tb, IFLA_BRPORT_LOCKED, BR_PORT_LOCKED);
> > > + br_set_port_flag(p, tb, IFLA_BRPORT_MAB, BR_PORT_MAB);
> > > +
> > > + if (!(p->flags & BR_PORT_LOCKED))
> > > + p->flags &= ~BR_PORT_MAB;
>
> The reason for this is that I wanted it to be so that if you have MAB
> enabled (and locked of course) and unlock the port, it will automatically
> clear both flags instead of having to first disable MAB and then unlock the
> port.
User space can just do:
# bridge link set dev swp1 locked off mab off
I prefer not to push such logic into the kernel and instead fail
explicitly. I won't argue if more people are in favor.
On 2022-08-29 09:52, Ido Schimmel wrote:
> On Sun, Aug 28, 2022 at 12:23:30PM +0200, [email protected]
> wrote:
>> On 2022-08-27 17:19, Ido Schimmel wrote:
>> > On Fri, Aug 26, 2022 at 01:45:33PM +0200, Hans Schultz wrote:
>> > >
>> > > nbp_switchdev_frame_mark(p, skb);
>> > > @@ -943,6 +946,10 @@ static int br_setport(struct net_bridge_port
>> > > *p, struct nlattr *tb[],
>> > > br_set_port_flag(p, tb, IFLA_BRPORT_NEIGH_SUPPRESS,
>> > > BR_NEIGH_SUPPRESS);
>> > > br_set_port_flag(p, tb, IFLA_BRPORT_ISOLATED, BR_ISOLATED);
>> > > br_set_port_flag(p, tb, IFLA_BRPORT_LOCKED, BR_PORT_LOCKED);
>> > > + br_set_port_flag(p, tb, IFLA_BRPORT_MAB, BR_PORT_MAB);
>> > > +
>> > > + if (!(p->flags & BR_PORT_LOCKED))
>> > > + p->flags &= ~BR_PORT_MAB;
>>
>> The reason for this is that I wanted it to be so that if you have MAB
>> enabled (and locked of course) and unlock the port, it will
>> automatically
>> clear both flags instead of having to first disable MAB and then
>> unlock the
>> port.
>
> User space can just do:
>
> # bridge link set dev swp1 locked off mab off
>
> I prefer not to push such logic into the kernel and instead fail
> explicitly. I won't argue if more people are in favor.
I shall do it as you suggest. It sounds fair. :-)
On 2022-08-27 17:19, Ido Schimmel wrote:
> On Fri, Aug 26, 2022 at 01:45:33PM +0200, Hans Schultz wrote:
> How about the below (untested):
>
> diff --git a/net/bridge/br_input.c b/net/bridge/br_input.c
> index 68b3e850bcb9..9143a94a1c57 100644
> --- a/net/bridge/br_input.c
> +++ b/net/bridge/br_input.c
> @@ -109,9 +109,18 @@ int br_handle_frame_finish(struct net *net,
> struct sock *sk, struct sk_buff *skb
> struct net_bridge_fdb_entry *fdb_src =
> br_fdb_find_rcu(br, eth_hdr(skb)->h_source,
> vid);
>
> - if (!fdb_src || READ_ONCE(fdb_src->dst) != p ||
> - test_bit(BR_FDB_LOCAL, &fdb_src->flags))
> + if (!fdb_src) {
> + if (p->flags & BR_PORT_MAB) {
> + __set_bit(BR_FDB_ENTRY_LOCKED, &flags);
> + br_fdb_update(br, p,
> eth_hdr(skb)->h_source,
> + vid, flags);
> + }
> + goto drop;
> + } else if (READ_ONCE(fdb_src->dst) != p ||
> + test_bit(BR_FDB_LOCAL, &fdb_src->flags) ||
> + test_bit(BR_FDB_LOCKED, &fdb_src->flags)) {
> goto drop;
> + }
> }
>
> The semantics are very clear, IMO. On FDB miss, add a locked FDB entry
> and drop the packet. On FDB mismatch, drop the packet.
>
> Entry can roam from an unauthorized port to an authorized port, but not
> the other way around. Not sure what is the use case for allowing
> roaming
> between unauthorized ports.
>
> Note that with the above, locked entries are not refreshed and will
> therefore age out unless replaced by user space.
>
Okay I was under the impression that entries should be able to roam
freely between authorized and unauthorized ports in the bridge as long
as the locked flag is on when roaming to the MAB enabled port. As you
know roaming is not a big issue with mv88e6xxx.
As I see this code, an entry cannot roam to an authorized port as there
is no update after the port mismatch check and the packet is dropped as
it should in this case in the locked section.
On 29/08/2022 10:52, Ido Schimmel wrote:
> On Sun, Aug 28, 2022 at 12:23:30PM +0200, [email protected] wrote:
>> On 2022-08-27 17:19, Ido Schimmel wrote:
>>> On Fri, Aug 26, 2022 at 01:45:33PM +0200, Hans Schultz wrote:
>>>>
>>>> nbp_switchdev_frame_mark(p, skb);
>>>> @@ -943,6 +946,10 @@ static int br_setport(struct net_bridge_port
>>>> *p, struct nlattr *tb[],
>>>> br_set_port_flag(p, tb, IFLA_BRPORT_NEIGH_SUPPRESS,
>>>> BR_NEIGH_SUPPRESS);
>>>> br_set_port_flag(p, tb, IFLA_BRPORT_ISOLATED, BR_ISOLATED);
>>>> br_set_port_flag(p, tb, IFLA_BRPORT_LOCKED, BR_PORT_LOCKED);
>>>> + br_set_port_flag(p, tb, IFLA_BRPORT_MAB, BR_PORT_MAB);
>>>> +
>>>> + if (!(p->flags & BR_PORT_LOCKED))
>>>> + p->flags &= ~BR_PORT_MAB;
>>
>> The reason for this is that I wanted it to be so that if you have MAB
>> enabled (and locked of course) and unlock the port, it will automatically
>> clear both flags instead of having to first disable MAB and then unlock the
>> port.
>
> User space can just do:
>
> # bridge link set dev swp1 locked off mab off
>
> I prefer not to push such logic into the kernel and instead fail
> explicitly. I won't argue if more people are in favor.
+1
I prefer to fail explicitly too, actually I also had a comment about this but
somehow have managed to delete it before sending my review. :)
On 2022-08-27 17:19, Ido Schimmel wrote:
> On Fri, Aug 26, 2022 at 01:45:33PM +0200, Hans Schultz wrote:
How about this?
diff --git a/net/bridge/br_input.c b/net/bridge/br_input.c
index 1064a5b2d478..82bb50851716 100644
--- a/net/bridge/br_input.c
+++ b/net/bridge/br_input.c
@@ -103,8 +103,19 @@ int br_handle_frame_finish(struct net *net, struct
sock *sk, struct sk_buff *skb
br_fdb_find_rcu(br, eth_hdr(skb)->h_source,
vid);
if (!fdb_src || READ_ONCE(fdb_src->dst) != p ||
- test_bit(BR_FDB_LOCAL, &fdb_src->flags))
+ test_bit(BR_FDB_LOCAL, &fdb_src->flags) ||
+ test_bit(BR_FDB_ENTRY_LOCKED, &fdb_src->flags)) {
+ if (!fdb_src || ((READ_ONCE(fdb_src->dst) != p)
&&
+
(!unlikely(test_bit(BR_FDB_LOCAL, &fdb_src->flags))))) {
+ unsigned long flags = 0;
+
+ if (p->flags & BR_PORT_MAB) {
+ __set_bit(BR_FDB_ENTRY_LOCKED,
&flags);
+ br_fdb_update(br, p,
eth_hdr(skb)->h_source, vid, flags);
+ }
+ }
goto drop;
+ }
}
nbp_switchdev_frame_mark(p, skb);
It will allow roaming to a MAB enabled port (no roaming to a simply
locked port should be allowed of course), and it will not change a local
entry and not rely on 'learning on' on the locked port of course.
Roaming to an unlocked port will also be allowed, and the locked flag
will be removed in this case according to code in br_fdb_update().
On 2022-08-27 17:19, Ido Schimmel wrote:
>
> How about the below (untested):
>
> diff --git a/net/bridge/br_input.c b/net/bridge/br_input.c
> index 68b3e850bcb9..9143a94a1c57 100644
> --- a/net/bridge/br_input.c
> +++ b/net/bridge/br_input.c
> @@ -109,9 +109,18 @@ int br_handle_frame_finish(struct net *net,
> struct sock *sk, struct sk_buff *skb
> struct net_bridge_fdb_entry *fdb_src =
> br_fdb_find_rcu(br, eth_hdr(skb)->h_source,
> vid);
>
> - if (!fdb_src || READ_ONCE(fdb_src->dst) != p ||
> - test_bit(BR_FDB_LOCAL, &fdb_src->flags))
> + if (!fdb_src) {
> + if (p->flags & BR_PORT_MAB) {
> + __set_bit(BR_FDB_ENTRY_LOCKED, &flags);
> + br_fdb_update(br, p,
> eth_hdr(skb)->h_source,
> + vid, flags);
> + }
> + goto drop;
> + } else if (READ_ONCE(fdb_src->dst) != p ||
> + test_bit(BR_FDB_LOCAL, &fdb_src->flags) ||
> + test_bit(BR_FDB_LOCKED, &fdb_src->flags)) {
> goto drop;
> + }
> }
>
> The semantics are very clear, IMO. On FDB miss, add a locked FDB entry
> and drop the packet. On FDB mismatch, drop the packet.
>
> Entry can roam from an unauthorized port to an authorized port, but not
> the other way around. Not sure what is the use case for allowing
> roaming
> between unauthorized ports.
>
> Note that with the above, locked entries are not refreshed and will
> therefore age out unless replaced by user space.
>
Okay, I got the semantics (locked/unlocked vs unauthorized/authorized)
reversed, so I will go with your suggestion.
> On 2022-08-27 13:30, Nikolay Aleksandrov wrote:
>> @@ -879,6 +888,10 @@ void br_fdb_update(struct net_bridge *br, struct
>> net_bridge_port *source,
>> &fdb->flags)))
>> clear_bit(BR_FDB_ADDED_BY_EXT_LEARN,
>> &fdb->flags);
>> + if (source->flags & BR_PORT_MAB)
>> + set_bit(BR_FDB_ENTRY_LOCKED, &fdb->flags);
>> + else
>> + clear_bit(BR_FDB_ENTRY_LOCKED, &fdb->flags);
> Please add a test for that bit and only then change it.
>
Okay, I have revised this part now. I hope that it is suitable?
@@ -749,6 +756,10 @@ void br_fdb_update(struct net_bridge *br, struct
net_bridge_port *source,
&fdb->flags)))
clear_bit(BR_FDB_ADDED_BY_EXT_LEARN,
&fdb->flags);
+ /* Allow roaming from an unauthorized
port to an
+ * authorized port */
+ if
(unlikely(test_bit(BR_FDB_ENTRY_LOCKED, &fdb->flags)))
+ clear_bit(BR_FDB_ENTRY_LOCKED,
&fdb->flags);
}
if (unlikely(test_bit(BR_FDB_ADDED_BY_USER,
&flags)))
> On 2022-08-27 13:30, Nikolay Aleksandrov wrote:
>> @@ -879,6 +888,10 @@ void br_fdb_update(struct net_bridge *br, struct
>> net_bridge_port *source,
>> &fdb->flags)))
>> clear_bit(BR_FDB_ADDED_BY_EXT_LEARN,
>> &fdb->flags);
>> + if (source->flags & BR_PORT_MAB)
>> + set_bit(BR_FDB_ENTRY_LOCKED, &fdb->flags);
>> + else
>> + clear_bit(BR_FDB_ENTRY_LOCKED, &fdb->flags);
> Please add a test for that bit and only then change it.
>
Something like this?
@@ -749,6 +756,12 @@ void br_fdb_update(struct net_bridge *br, struct
net_bridge_port *source,
&fdb->flags)))
clear_bit(BR_FDB_ADDED_BY_EXT_LEARN,
&fdb->flags);
+ if
(unlikely(test_bit(BR_FDB_ENTRY_LOCKED, &fdb->flags))) {
+ if (!(source->flags &
BR_PORT_MAB))
+
clear_bit(BR_FDB_ENTRY_LOCKED, &fdb->flags);
+ } else
+ if (source->flags & BR_PORT_MAB)
+
set_bit(BR_FDB_ENTRY_LOCKED, &fdb->flags);
}
if (unlikely(test_bit(BR_FDB_ADDED_BY_USER,
&flags)))
On 2022-08-27 13:30, Nikolay Aleksandrov wrote:
> On 26/08/2022 14:45, Hans Schultz wrote:
>
> Hi,
> Please add the blackhole flag in a separate patch.
> A few more comments and questions below..
>
Hi,
if userspace is to set this flag I think I need to change stuff in
rtnetlink.c, as I will need to extent struct ndmsg with a new u32 entry
as the old u8 flags is full.
Maybe this is straight forward, but I am not so sure as I don't know
that code too well. Maybe someone can give me a hint...?
On Mon, Aug 29, 2022 at 04:02:46PM +0200, [email protected] wrote:
> On 2022-08-27 13:30, Nikolay Aleksandrov wrote:
> > On 26/08/2022 14:45, Hans Schultz wrote:
> >
> > Hi,
> > Please add the blackhole flag in a separate patch.
> > A few more comments and questions below..
> >
>
> Hi,
> if userspace is to set this flag I think I need to change stuff in
> rtnetlink.c, as I will need to extent struct ndmsg with a new u32 entry as
> the old u8 flags is full.
You cannot extend 'struct ndmsg'. That's why 'NDA_FLAGS_EXT' was
introduced. See:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=2c611ad97a82b51221bb0920cc6cac0b1d4c0e52
'NTF_EXT_BLACKHOLE' belongs in 'NDA_FLAGS_EXT' like you have it now, but
the kernel should not reject it in br_fdb_add().
> Maybe this is straight forward, but I am not so sure as I don't know that
> code too well. Maybe someone can give me a hint...?
On 2022-08-29 18:12, Ido Schimmel wrote:
> On Mon, Aug 29, 2022 at 04:02:46PM +0200, [email protected]
> wrote:
>> On 2022-08-27 13:30, Nikolay Aleksandrov wrote:
>> > On 26/08/2022 14:45, Hans Schultz wrote:
>> >
>> > Hi,
>> > Please add the blackhole flag in a separate patch.
>> > A few more comments and questions below..
>> >
>>
>> Hi,
>> if userspace is to set this flag I think I need to change stuff in
>> rtnetlink.c, as I will need to extent struct ndmsg with a new u32
>> entry as
>> the old u8 flags is full.
>
> You cannot extend 'struct ndmsg'. That's why 'NDA_FLAGS_EXT' was
> introduced. See:
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=2c611ad97a82b51221bb0920cc6cac0b1d4c0e52
>
> 'NTF_EXT_BLACKHOLE' belongs in 'NDA_FLAGS_EXT' like you have it now,
> but
> the kernel should not reject it in br_fdb_add().
>
>> Maybe this is straight forward, but I am not so sure as I don't know
>> that
>> code too well. Maybe someone can give me a hint...?
Thanks! I see that I was in trouble there... :D
On 2022-08-29 18:12, Ido Schimmel wrote:
> On Mon, Aug 29, 2022 at 04:02:46PM +0200, [email protected]
> wrote:
>> On 2022-08-27 13:30, Nikolay Aleksandrov wrote:
>> > On 26/08/2022 14:45, Hans Schultz wrote:
>> >
>> > Hi,
>> > Please add the blackhole flag in a separate patch.
>> > A few more comments and questions below..
>> >
>>
>> Hi,
>> if userspace is to set this flag I think I need to change stuff in
>> rtnetlink.c, as I will need to extent struct ndmsg with a new u32
>> entry as
>> the old u8 flags is full.
>
> You cannot extend 'struct ndmsg'. That's why 'NDA_FLAGS_EXT' was
> introduced. See:
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=2c611ad97a82b51221bb0920cc6cac0b1d4c0e52
>
> 'NTF_EXT_BLACKHOLE' belongs in 'NDA_FLAGS_EXT' like you have it now,
> but
> the kernel should not reject it in br_fdb_add().
>
>> Maybe this is straight forward, but I am not so sure as I don't know
>> that
>> code too well. Maybe someone can give me a hint...?
The question I have is if I can use nlh_flags to send the extended flags
further on to fdb_add_entry(), where I expect to need it?
(the extended flags are u32, while nlh_flags are u16)
On Tue, Aug 30, 2022 at 04:19:16PM +0200, [email protected] wrote:
> On 2022-08-29 18:12, Ido Schimmel wrote:
> > On Mon, Aug 29, 2022 at 04:02:46PM +0200, [email protected]
> > wrote:
> > > On 2022-08-27 13:30, Nikolay Aleksandrov wrote:
> > > > On 26/08/2022 14:45, Hans Schultz wrote:
> > > >
> > > > Hi,
> > > > Please add the blackhole flag in a separate patch.
> > > > A few more comments and questions below..
> > > >
> > >
> > > Hi,
> > > if userspace is to set this flag I think I need to change stuff in
> > > rtnetlink.c, as I will need to extent struct ndmsg with a new u32
> > > entry as
> > > the old u8 flags is full.
> >
> > You cannot extend 'struct ndmsg'. That's why 'NDA_FLAGS_EXT' was
> > introduced. See:
> > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=2c611ad97a82b51221bb0920cc6cac0b1d4c0e52
> >
> > 'NTF_EXT_BLACKHOLE' belongs in 'NDA_FLAGS_EXT' like you have it now, but
> > the kernel should not reject it in br_fdb_add().
> >
> > > Maybe this is straight forward, but I am not so sure as I don't know
> > > that
> > > code too well. Maybe someone can give me a hint...?
>
> The question I have is if I can use nlh_flags to send the extended flags
> further on to fdb_add_entry(), where I expect to need it?
A separate argument looks cleaner to me.
> (the extended flags are u32, while nlh_flags are u16)