diff mbox series

[v8,net-next,02/12] net: bridge: add blackhole fdb entry flag

Message ID 20221018165619.134535-3-netdev@kapio-technology.com
State New
Headers
Series Extend locked port feature with FDB locked flag (MAC-Auth/MAB) |

Commit Message

Hans Schultz Oct. 18, 2022, 4:56 p.m. UTC 
  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.

When the blackhole fdb flag is set, the 'local' flag will also be enabled
as blackhole entries are not associated with any port.

Thus the command will be alike to:
bridge fdb add MAC dev br0 local blackhole

Signed-off-by: Hans J. Schultz <netdev@kapio-technology.com>
---
 include/uapi/linux/neighbour.h |  4 ++++
 net/bridge/br_fdb.c            | 37 ++++++++++++++++++++++++++++------
 net/bridge/br_input.c          |  5 ++++-
 net/bridge/br_private.h        |  1 +
 4 files changed, 40 insertions(+), 7 deletions(-)

Comments

Ido Schimmel Oct. 20, 2022, 1:06 p.m. UTC | #1 
On Tue, Oct 18, 2022 at 06:56:09PM +0200, Hans J. Schultz wrote:
> 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.
> 
> When the blackhole fdb flag is set, the 'local' flag will also be enabled
> as blackhole entries are not associated with any port.

It reads as if the kernel will enable the 'local' flag automatically,
which is not true anymore. The bridge driver enforces that
'NUD_PERMANENT' is set if 'NTF_EXT_BLACKHOLE' is specified.

> 
> Thus the command will be alike to:
> bridge fdb add MAC dev br0 local blackhole
> 
> Signed-off-by: Hans J. Schultz <netdev@kapio-technology.com>

Looks OK to me. See one comment below.

Reviewed-by: Ido Schimmel <idosch@nvidia.com>

[...]

> @@ -1140,7 +1148,7 @@ static int __br_fdb_add(struct ndmsg *ndm, struct net_bridge *br,
>  		err = br_fdb_external_learn_add(br, p, addr, vid, true);
>  	} else {
>  		spin_lock_bh(&br->hash_lock);
> -		err = fdb_add_entry(br, p, addr, ndm, nlh_flags, vid, nfea_tb);
> +		err = fdb_add_entry(br, p, addr, ndm, nlh_flags, vid, ext_flags, nfea_tb);

I believe the preference is to wrap to 80 columns when possible.

>  		spin_unlock_bh(&br->hash_lock);
>  	}
Hans Schultz Oct. 20, 2022, 7:34 p.m. UTC | #2 
On 2022-10-20 15:06, Ido Schimmel wrote:
> On Tue, Oct 18, 2022 at 06:56:09PM +0200, Hans J. Schultz wrote:
>> 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.
>> 
>> When the blackhole fdb flag is set, the 'local' flag will also be 
>> enabled
>> as blackhole entries are not associated with any port.
> 
> It reads as if the kernel will enable the 'local' flag automatically,
> which is not true anymore. The bridge driver enforces that
> 'NUD_PERMANENT' is set if 'NTF_EXT_BLACKHOLE' is specified.
> 
>> 
>> Thus the command will be alike to:
>> bridge fdb add MAC dev br0 local blackhole
>> 
>> Signed-off-by: Hans J. Schultz <netdev@kapio-technology.com>
> 
> Looks OK to me. See one comment below.
> 
> Reviewed-by: Ido Schimmel <idosch@nvidia.com>
> 
> [...]
> 
>> @@ -1140,7 +1148,7 @@ static int __br_fdb_add(struct ndmsg *ndm, 
>> struct net_bridge *br,
>>  		err = br_fdb_external_learn_add(br, p, addr, vid, true);
>>  	} else {
>>  		spin_lock_bh(&br->hash_lock);
>> -		err = fdb_add_entry(br, p, addr, ndm, nlh_flags, vid, nfea_tb);
>> +		err = fdb_add_entry(br, p, addr, ndm, nlh_flags, vid, ext_flags, 
>> nfea_tb);
> 
> I believe the preference is to wrap to 80 columns when possible.

Ok, I only have knowledge of 100 columns as a limit.
Hans Schultz Oct. 23, 2022, 5:32 a.m. UTC | #3 
On 2022-10-20 15:06, Ido Schimmel wrote:

> [...]
> 
>> @@ -1140,7 +1148,7 @@ static int __br_fdb_add(struct ndmsg *ndm, 
>> struct net_bridge *br,
>>  		err = br_fdb_external_learn_add(br, p, addr, vid, true);
>>  	} else {
>>  		spin_lock_bh(&br->hash_lock);
>> -		err = fdb_add_entry(br, p, addr, ndm, nlh_flags, vid, nfea_tb);
>> +		err = fdb_add_entry(br, p, addr, ndm, nlh_flags, vid, ext_flags, 
>> nfea_tb);
> 
> I believe the preference is to wrap to 80 columns when possible.

Very strange... since I ran checkpatch.pl from the net-next kernel 
itself and it did not
give me any warnings about 80 columns, but rather said 'patch is ready 
for submission'.

As this is silent, could it be some missing python plugins or something 
to do with perl?
Jakub Kicinski Oct. 24, 2022, 5:08 p.m. UTC | #4 
On Sun, 23 Oct 2022 07:32:02 +0200 netdev@kapio-technology.com wrote:
> >> @@ -1140,7 +1148,7 @@ static int __br_fdb_add(struct ndmsg *ndm, 
> >> struct net_bridge *br,
> >>  		err = br_fdb_external_learn_add(br, p, addr, vid, true);
> >>  	} else {
> >>  		spin_lock_bh(&br->hash_lock);
> >> -		err = fdb_add_entry(br, p, addr, ndm, nlh_flags, vid, nfea_tb);
> >> +		err = fdb_add_entry(br, p, addr, ndm, nlh_flags, vid, ext_flags, 
> >> nfea_tb);  
> > 
> > I believe the preference is to wrap to 80 columns when possible.  
> 
> Very strange... since I ran checkpatch.pl from the net-next kernel 
> itself and it did not
> give me any warnings about 80 columns, but rather said 'patch is ready 
> for submission'.
> 
> As this is silent, could it be some missing python plugins or something 
> to do with perl?

I run:

./scripts/checkpatch.pl --strict --max-line-length=80
diff mbox series

Patch

diff --git a/include/uapi/linux/neighbour.h b/include/uapi/linux/neighbour.h
index 4dda051b0ba8..cc7d540eb734 100644
--- a/include/uapi/linux/neighbour.h
+++ b/include/uapi/linux/neighbour.h
@@ -54,6 +54,7 @@  enum {
 /* Extended flags under NDA_FLAGS_EXT: */
 #define NTF_EXT_MANAGED		(1 << 0)
 #define NTF_EXT_LOCKED		(1 << 1)
+#define NTF_EXT_BLACKHOLE	(1 << 2)
 
 /*
  *	Neighbor Cache Entry States.
@@ -91,6 +92,9 @@  enum {
  * NTF_EXT_LOCKED flagged FDB entries are placeholder entries used with the
  * locked port feature, that ensures that an entry exists while at the same
  * time dropping packets on ingress with src MAC and VID matching the entry.
+ *
+ * NTF_EXT_BLACKHOLE flagged FDB entries ensure that no forwarding is allowed
+ * from any port to the destination MAC, VID pair associated with it.
  */
 
 struct nda_cacheinfo {
diff --git a/net/bridge/br_fdb.c b/net/bridge/br_fdb.c
index 2cf695ee61c5..15ead4dc6190 100644
--- a/net/bridge/br_fdb.c
+++ b/net/bridge/br_fdb.c
@@ -128,6 +128,8 @@  static int fdb_fill_info(struct sk_buff *skb, const struct net_bridge *br,
 		ndm->ndm_flags |= NTF_STICKY;
 	if (test_bit(BR_FDB_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;
@@ -1018,8 +1020,9 @@  static bool fdb_handle_notify(struct net_bridge_fdb_entry *fdb, u8 notify)
 /* Update (create or replace) forwarding database entry */
 static int fdb_add_entry(struct net_bridge *br, struct net_bridge_port *source,
 			 const u8 *addr, struct ndmsg *ndm, u16 flags, u16 vid,
-			 struct nlattr *nfea_tb[])
+			 u32 ext_flags, struct nlattr *nfea_tb[])
 {
+	bool blackhole = !!(ext_flags & NTF_EXT_BLACKHOLE);
 	bool is_sticky = !!(ndm->ndm_flags & NTF_STICKY);
 	bool refresh = !nfea_tb[NFEA_DONT_REFRESH];
 	struct net_bridge_fdb_entry *fdb;
@@ -1092,6 +1095,11 @@  static int fdb_add_entry(struct net_bridge *br, struct net_bridge_port *source,
 		modified = true;
 	}
 
+	if (blackhole != test_bit(BR_FDB_BLACKHOLE, &fdb->flags)) {
+		change_bit(BR_FDB_BLACKHOLE, &fdb->flags);
+		modified = true;
+	}
+
 	if (test_and_clear_bit(BR_FDB_LOCKED, &fdb->flags))
 		modified = true;
 
@@ -1113,7 +1121,7 @@  static int fdb_add_entry(struct net_bridge *br, struct net_bridge_port *source,
 static int __br_fdb_add(struct ndmsg *ndm, struct net_bridge *br,
 			struct net_bridge_port *p, const unsigned char *addr,
 			u16 nlh_flags, u16 vid, struct nlattr *nfea_tb[],
-			struct netlink_ext_ack *extack)
+			u32 ext_flags, struct netlink_ext_ack *extack)
 {
 	int err = 0;
 
@@ -1140,7 +1148,7 @@  static int __br_fdb_add(struct ndmsg *ndm, struct net_bridge *br,
 		err = br_fdb_external_learn_add(br, p, addr, vid, true);
 	} else {
 		spin_lock_bh(&br->hash_lock);
-		err = fdb_add_entry(br, p, addr, ndm, nlh_flags, vid, nfea_tb);
+		err = fdb_add_entry(br, p, addr, ndm, nlh_flags, vid, ext_flags, nfea_tb);
 		spin_unlock_bh(&br->hash_lock);
 	}
 
@@ -1200,6 +1208,23 @@  int br_fdb_add(struct ndmsg *ndm, struct nlattr *tb[],
 		return -EINVAL;
 	}
 
+	if (ext_flags & NTF_EXT_BLACKHOLE) {
+		if (!(ndm->ndm_state & NUD_PERMANENT)) {
+			NL_SET_ERR_MSG_MOD(extack, "Blackhole FDB entry must be permanent");
+			return -EINVAL;
+		}
+		if (p) {
+			NL_SET_ERR_MSG_MOD(extack,
+					   "Blackhole FDB entry cannot be applied on a port");
+			return -EINVAL;
+		}
+		if (ndm->ndm_flags & NTF_EXT_LEARNED) {
+			NL_SET_ERR_MSG_MOD(extack,
+					   "Blackhole FDB entry cannot be added as ext. learned");
+			return -EINVAL;
+		}
+	}
+
 	if (tb[NDA_FDB_EXT_ATTRS]) {
 		attr = tb[NDA_FDB_EXT_ATTRS];
 		err = nla_parse_nested(nfea_tb, NFEA_MAX, attr,
@@ -1219,10 +1244,10 @@  int br_fdb_add(struct ndmsg *ndm, struct nlattr *tb[],
 
 		/* VID was specified, so use it. */
 		err = __br_fdb_add(ndm, br, p, addr, nlh_flags, vid, nfea_tb,
-				   extack);
+				   ext_flags, extack);
 	} else {
 		err = __br_fdb_add(ndm, br, p, addr, nlh_flags, 0, nfea_tb,
-				   extack);
+				   ext_flags, extack);
 		if (err || !vg || !vg->num_vlans)
 			goto out;
 
@@ -1234,7 +1259,7 @@  int br_fdb_add(struct ndmsg *ndm, struct nlattr *tb[],
 			if (!br_vlan_should_use(v))
 				continue;
 			err = __br_fdb_add(ndm, br, p, addr, nlh_flags, v->vid,
-					   nfea_tb, extack);
+					   nfea_tb, ext_flags, extack);
 			if (err)
 				goto out;
 		}
diff --git a/net/bridge/br_input.c b/net/bridge/br_input.c
index 068fced7693c..665d1d6bdc75 100644
--- a/net/bridge/br_input.c
+++ b/net/bridge/br_input.c
@@ -193,8 +193,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;
diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h
index 4ce8b8e5ae0b..e7a08657c7ed 100644
--- a/net/bridge/br_private.h
+++ b/net/bridge/br_private.h
@@ -253,6 +253,7 @@  enum {
 	BR_FDB_NOTIFY,
 	BR_FDB_NOTIFY_INACTIVE,
 	BR_FDB_LOCKED,
+	BR_FDB_BLACKHOLE,
 };
 
 struct net_bridge_fdb_key {