[PATCH net-next] net/bridge: Add skb drop reasons to the most common drop points

[PATCH net-next] net/bridge: Add skb drop reasons to the most common drop points

[Radu Rendec]

The bridge input code may drop frames for various reasons and at various
points in the ingress handling logic. Currently kfree_skb() is used
everywhere, and therefore no drop reason is specified. Add drop reasons
to the most common drop points.

The purpose of this patch is to address the most common drop points on
the bridge ingress path. It does not exhaustively add drop reasons to
the entire bridge code. The intention here is to incrementally add drop
reasons to the rest of the bridge code in follow up patches.

Most of the skb drop points that are addressed in this patch can be
easily tested by sending crafted packets. The diagram below shows a
simple test configuration, and some examples using `packit`(*) are
also included. The bridge is set up with STP disabled.
(*) https://github.com/resurrecting-open-source-projects/packit

The following changes were *not* tested:
* SKB_DROP_REASON_BRIDGE_NO_EGRESS_PORT in br_multicast_flood(). I could
  not find an easy way to make a crafted packet get there.
* SKB_DROP_REASON_BRIDGE_INGRESS_PORT_NFWD in br_handle_frame_finish()
  when the port state is BR_STATE_DISABLED, because in that case the
  frame is already dropped in the switch/case block at the end of
  br_handle_frame().

    +---+---+
    |  br0  |
    +---+---+
        |
    +---+---+  veth pair  +-------+
    | veth0 +-------------+ xeth0 |
    +-------+             +-------+

SKB_DROP_REASON_MAC_INVALID_SOURCE - br_handle_frame()
packit -t UDP -s 192.168.0.1 -d 192.168.0.2 -S 8000 -D 8000 \
  -e 01:22:33:44:55:66 -E aa:bb:cc:dd:ee:ff -c 1 \
  -p '0x de ad be ef' -i xeth0

SKB_DROP_REASON_MAC_IEEE_MAC_CONTROL - br_handle_frame()
packit -t UDP -s 192.168.0.1 -d 192.168.0.2 -S 8000 -D 8000 \
  -e 02:22:33:44:55:66 -E 01:80:c2:00:00:01 -c 1 \
  -p '0x de ad be ef' -i xeth0

SKB_DROP_REASON_BRIDGE_INGRESS_PORT_NFWD - br_handle_frame()
bridge link set dev veth0 state 0 # disabled
packit -t UDP -s 192.168.0.1 -d 192.168.0.2 -S 8000 -D 8000 \
  -e 02:22:33:44:55:66 -E aa:bb:cc:dd:ee:ff -c 1 \
  -p '0x de ad be ef' -i xeth0

SKB_DROP_REASON_BRIDGE_INGRESS_PORT_NFWD - br_handle_frame_finish()
bridge link set dev veth0 state 2 # learning
packit -t UDP -s 192.168.0.1 -d 192.168.0.2 -S 8000 -D 8000 \
  -e 02:22:33:44:55:66 -E aa:bb:cc:dd:ee:ff -c 1 \
  -p '0x de ad be ef' -i xeth0

SKB_DROP_REASON_BRIDGE_NO_EGRESS_PORT - br_flood()
packit -t UDP -s 192.168.0.1 -d 192.168.0.2 -S 8000 -D 8000 \
  -e 02:22:33:44:55:66 -E aa:bb:cc:dd:ee:ff -c 1 \
  -p '0x de ad be ef' -i xeth0

Signed-off-by: Radu Rendec <rrendec@redhat.com>
---
 include/net/dropreason-core.h | 18 ++++++++++++++++++
 net/bridge/br_forward.c       |  4 ++--
 net/bridge/br_input.c         | 24 +++++++++++++++---------
 3 files changed, 35 insertions(+), 11 deletions(-)

diff --git a/include/net/dropreason-core.h b/include/net/dropreason-core.h
index c29282fabae6..1f2ae5b387c1 100644
--- a/include/net/dropreason-core.h
+++ b/include/net/dropreason-core.h
@@ -108,6 +108,9 @@
 	FN(TUNNEL_TXINFO)		\
 	FN(LOCAL_MAC)			\
 	FN(ARP_PVLAN_DISABLE)		\
+	FN(MAC_IEEE_MAC_CONTROL)	\
+	FN(BRIDGE_INGRESS_PORT_NFWD)	\
+	FN(BRIDGE_NO_EGRESS_PORT)	\
 	FNe(MAX)
 
 /**
@@ -502,6 +505,21 @@ enum skb_drop_reason {
 	 * enabled.
 	 */
 	SKB_DROP_REASON_ARP_PVLAN_DISABLE,
+	/**
+	 * @SKB_DROP_REASON_MAC_IEEE_MAC_CONTROL: the destination MAC address
+	 * is an IEEE MAC Control address.
+	 */
+	SKB_DROP_REASON_MAC_IEEE_MAC_CONTROL,
+	/**
+	 * @SKB_DROP_REASON_BRIDGE_INGRESS_PORT_NFWD: the STP state of the
+	 * ingress bridge port does not allow frames to be forwarded.
+	 */
+	SKB_DROP_REASON_BRIDGE_INGRESS_PORT_NFWD,
+	/**
+	 * SKB_DROP_REASON_BRIDGE_NO_EGRESS_PORT: no eligible egress port was
+	 * found while attempting to flood the frame.
+	 */
+	SKB_DROP_REASON_BRIDGE_NO_EGRESS_PORT,
 	/**
 	 * @SKB_DROP_REASON_MAX: the maximum of core drop reasons, which
 	 * shouldn't be used as a real 'reason' - only for tracing code gen
diff --git a/net/bridge/br_forward.c b/net/bridge/br_forward.c
index e19b583ff2c6..e33e2f4fc3d9 100644
--- a/net/bridge/br_forward.c
+++ b/net/bridge/br_forward.c
@@ -249,7 +249,7 @@ void br_flood(struct net_bridge *br, struct sk_buff *skb,
 
 out:
 	if (!local_rcv)
-		kfree_skb(skb);
+		kfree_skb_reason(skb, SKB_DROP_REASON_BRIDGE_NO_EGRESS_PORT);
 }
 
 #ifdef CONFIG_BRIDGE_IGMP_SNOOPING
@@ -349,6 +349,6 @@ void br_multicast_flood(struct net_bridge_mdb_entry *mdst,
 
 out:
 	if (!local_rcv)
-		kfree_skb(skb);
+		kfree_skb_reason(skb, SKB_DROP_REASON_BRIDGE_NO_EGRESS_PORT);
 }
 #endif
diff --git a/net/bridge/br_input.c b/net/bridge/br_input.c
index ceaa5a89b947..fc00e172e1e1 100644
--- a/net/bridge/br_input.c
+++ b/net/bridge/br_input.c
@@ -96,8 +96,10 @@ int br_handle_frame_finish(struct net *net, struct sock *sk, struct sk_buff *skb
 	if (br_mst_is_enabled(br)) {
 		state = BR_STATE_FORWARDING;
 	} else {
-		if (p->state == BR_STATE_DISABLED)
-			goto drop;
+		if (p->state == BR_STATE_DISABLED) {
+			kfree_skb_reason(skb, SKB_DROP_REASON_BRIDGE_INGRESS_PORT_NFWD);
+			return 0;
+		}
 
 		state = p->state;
 	}
@@ -155,8 +157,10 @@ int br_handle_frame_finish(struct net *net, struct sock *sk, struct sk_buff *skb
 		}
 	}
 
-	if (state == BR_STATE_LEARNING)
-		goto drop;
+	if (state == BR_STATE_LEARNING) {
+		kfree_skb_reason(skb, SKB_DROP_REASON_BRIDGE_INGRESS_PORT_NFWD);
+		return 0;
+	}
 
 	BR_INPUT_SKB_CB(skb)->brdev = br->dev;
 	BR_INPUT_SKB_CB(skb)->src_port_isolated = !!(p->flags & BR_ISOLATED);
@@ -331,8 +335,10 @@ static rx_handler_result_t br_handle_frame(struct sk_buff **pskb)
 	if (unlikely(skb->pkt_type == PACKET_LOOPBACK))
 		return RX_HANDLER_PASS;
 
-	if (!is_valid_ether_addr(eth_hdr(skb)->h_source))
-		goto drop;
+	if (!is_valid_ether_addr(eth_hdr(skb)->h_source)) {
+		kfree_skb_reason(skb, SKB_DROP_REASON_MAC_INVALID_SOURCE);
+		return RX_HANDLER_CONSUMED;
+	}
 
 	skb = skb_share_check(skb, GFP_ATOMIC);
 	if (!skb)
@@ -374,7 +380,8 @@ static rx_handler_result_t br_handle_frame(struct sk_buff **pskb)
 			return RX_HANDLER_PASS;
 
 		case 0x01:	/* IEEE MAC (Pause) */
-			goto drop;
+			kfree_skb_reason(skb, SKB_DROP_REASON_MAC_IEEE_MAC_CONTROL);
+			return RX_HANDLER_CONSUMED;
 
 		case 0x0E:	/* 802.1AB LLDP */
 			fwd_mask |= p->br->group_fwd_mask;
@@ -423,8 +430,7 @@ static rx_handler_result_t br_handle_frame(struct sk_buff **pskb)
 
 		return nf_hook_bridge_pre(skb, pskb);
 	default:
-drop:
-		kfree_skb(skb);
+		kfree_skb_reason(skb, SKB_DROP_REASON_BRIDGE_INGRESS_PORT_NFWD);
 	}
 	return RX_HANDLER_CONSUMED;
 }
-- 
2.47.1

Re: [PATCH net-next] net/bridge: Add skb drop reasons to the most common drop points

[Nikolay Aleksandrov]

On 12/9/24 00:18, Radu Rendec wrote:
> The bridge input code may drop frames for various reasons and at various
> points in the ingress handling logic. Currently kfree_skb() is used
> everywhere, and therefore no drop reason is specified. Add drop reasons
> to the most common drop points.
> 
> The purpose of this patch is to address the most common drop points on
> the bridge ingress path. It does not exhaustively add drop reasons to
> the entire bridge code. The intention here is to incrementally add drop
> reasons to the rest of the bridge code in follow up patches.
> 
> Most of the skb drop points that are addressed in this patch can be
> easily tested by sending crafted packets. The diagram below shows a
> simple test configuration, and some examples using `packit`(*) are
> also included. The bridge is set up with STP disabled.
> (*) https://github.com/resurrecting-open-source-projects/packit
> 
> The following changes were *not* tested:
> * SKB_DROP_REASON_BRIDGE_NO_EGRESS_PORT in br_multicast_flood(). I could
>   not find an easy way to make a crafted packet get there.
> * SKB_DROP_REASON_BRIDGE_INGRESS_PORT_NFWD in br_handle_frame_finish()
>   when the port state is BR_STATE_DISABLED, because in that case the
>   frame is already dropped in the switch/case block at the end of
>   br_handle_frame().
> 
>     +---+---+
>     |  br0  |
>     +---+---+
>         |
>     +---+---+  veth pair  +-------+
>     | veth0 +-------------+ xeth0 |
>     +-------+             +-------+
> 
> SKB_DROP_REASON_MAC_INVALID_SOURCE - br_handle_frame()
> packit -t UDP -s 192.168.0.1 -d 192.168.0.2 -S 8000 -D 8000 \
>   -e 01:22:33:44:55:66 -E aa:bb:cc:dd:ee:ff -c 1 \
>   -p '0x de ad be ef' -i xeth0
> 
> SKB_DROP_REASON_MAC_IEEE_MAC_CONTROL - br_handle_frame()
> packit -t UDP -s 192.168.0.1 -d 192.168.0.2 -S 8000 -D 8000 \
>   -e 02:22:33:44:55:66 -E 01:80:c2:00:00:01 -c 1 \
>   -p '0x de ad be ef' -i xeth0
> 
> SKB_DROP_REASON_BRIDGE_INGRESS_PORT_NFWD - br_handle_frame()
> bridge link set dev veth0 state 0 # disabled
> packit -t UDP -s 192.168.0.1 -d 192.168.0.2 -S 8000 -D 8000 \
>   -e 02:22:33:44:55:66 -E aa:bb:cc:dd:ee:ff -c 1 \
>   -p '0x de ad be ef' -i xeth0
> 
> SKB_DROP_REASON_BRIDGE_INGRESS_PORT_NFWD - br_handle_frame_finish()
> bridge link set dev veth0 state 2 # learning
> packit -t UDP -s 192.168.0.1 -d 192.168.0.2 -S 8000 -D 8000 \
>   -e 02:22:33:44:55:66 -E aa:bb:cc:dd:ee:ff -c 1 \
>   -p '0x de ad be ef' -i xeth0
> 
> SKB_DROP_REASON_BRIDGE_NO_EGRESS_PORT - br_flood()
> packit -t UDP -s 192.168.0.1 -d 192.168.0.2 -S 8000 -D 8000 \
>   -e 02:22:33:44:55:66 -E aa:bb:cc:dd:ee:ff -c 1 \
>   -p '0x de ad be ef' -i xeth0
> 
> Signed-off-by: Radu Rendec <rrendec@redhat.com>
> ---
>  include/net/dropreason-core.h | 18 ++++++++++++++++++
>  net/bridge/br_forward.c       |  4 ++--
>  net/bridge/br_input.c         | 24 +++++++++++++++---------
>  3 files changed, 35 insertions(+), 11 deletions(-)
> 

Hi,
Thanks for working on this, a few comments below.

> diff --git a/include/net/dropreason-core.h b/include/net/dropreason-core.h
> index c29282fabae6..1f2ae5b387c1 100644
> --- a/include/net/dropreason-core.h
> +++ b/include/net/dropreason-core.h
> @@ -108,6 +108,9 @@
>  	FN(TUNNEL_TXINFO)		\
>  	FN(LOCAL_MAC)			\
>  	FN(ARP_PVLAN_DISABLE)		\
> +	FN(MAC_IEEE_MAC_CONTROL)	\
> +	FN(BRIDGE_INGRESS_PORT_NFWD)	\
> +	FN(BRIDGE_NO_EGRESS_PORT)	\
>  	FNe(MAX)
>  
>  /**
> @@ -502,6 +505,21 @@ enum skb_drop_reason {
>  	 * enabled.
>  	 */
>  	SKB_DROP_REASON_ARP_PVLAN_DISABLE,
> +	/**
> +	 * @SKB_DROP_REASON_MAC_IEEE_MAC_CONTROL: the destination MAC address
> +	 * is an IEEE MAC Control address.
> +	 */
> +	SKB_DROP_REASON_MAC_IEEE_MAC_CONTROL,
> +	/**
> +	 * @SKB_DROP_REASON_BRIDGE_INGRESS_PORT_NFWD: the STP state of the
> +	 * ingress bridge port does not allow frames to be forwarded.
> +	 */
> +	SKB_DROP_REASON_BRIDGE_INGRESS_PORT_NFWD,

Since this is used only when the port state causes the packet to drop, why not
rename it to something that suggests it was the state?

> +	/**
> +	 * SKB_DROP_REASON_BRIDGE_NO_EGRESS_PORT: no eligible egress port was
> +	 * found while attempting to flood the frame.
> +	 */
> +	SKB_DROP_REASON_BRIDGE_NO_EGRESS_PORT,
>  	/**
>  	 * @SKB_DROP_REASON_MAX: the maximum of core drop reasons, which
>  	 * shouldn't be used as a real 'reason' - only for tracing code gen
> diff --git a/net/bridge/br_forward.c b/net/bridge/br_forward.c
> index e19b583ff2c6..e33e2f4fc3d9 100644
> --- a/net/bridge/br_forward.c
> +++ b/net/bridge/br_forward.c
> @@ -249,7 +249,7 @@ void br_flood(struct net_bridge *br, struct sk_buff *skb,
>  
>  out:
>  	if (!local_rcv)
> -		kfree_skb(skb);
> +		kfree_skb_reason(skb, SKB_DROP_REASON_BRIDGE_NO_EGRESS_PORT);

This is not entirely correct, we can get here if we had an error forwarding
the packet to some port, but it may already have been forwarded to others.
The reason should distinguish between those two cases.

>  }
>  
>  #ifdef CONFIG_BRIDGE_IGMP_SNOOPING
> @@ -349,6 +349,6 @@ void br_multicast_flood(struct net_bridge_mdb_entry *mdst,
>  
>  out:
>  	if (!local_rcv)
> -		kfree_skb(skb);
> +		kfree_skb_reason(skb, SKB_DROP_REASON_BRIDGE_NO_EGRESS_PORT);

Same comment as above (br_flood).

>  }
>  #endif
> diff --git a/net/bridge/br_input.c b/net/bridge/br_input.c
> index ceaa5a89b947..fc00e172e1e1 100644
> --- a/net/bridge/br_input.c
> +++ b/net/bridge/br_input.c
> @@ -96,8 +96,10 @@ int br_handle_frame_finish(struct net *net, struct sock *sk, struct sk_buff *skb
>  	if (br_mst_is_enabled(br)) {
>  		state = BR_STATE_FORWARDING;
>  	} else {
> -		if (p->state == BR_STATE_DISABLED)
> -			goto drop;
> +		if (p->state == BR_STATE_DISABLED) {
> +			kfree_skb_reason(skb, SKB_DROP_REASON_BRIDGE_INGRESS_PORT_NFWD);
> +			return 0;
> +		}
>  
>  		state = p->state;
>  	}
> @@ -155,8 +157,10 @@ int br_handle_frame_finish(struct net *net, struct sock *sk, struct sk_buff *skb
>  		}
>  	}
>  
> -	if (state == BR_STATE_LEARNING)
> -		goto drop;
> +	if (state == BR_STATE_LEARNING) {
> +		kfree_skb_reason(skb, SKB_DROP_REASON_BRIDGE_INGRESS_PORT_NFWD);
> +		return 0;
> +	}>  
>  	BR_INPUT_SKB_CB(skb)->brdev = br->dev;
>  	BR_INPUT_SKB_CB(skb)->src_port_isolated = !!(p->flags & BR_ISOLATED);
> @@ -331,8 +335,10 @@ static rx_handler_result_t br_handle_frame(struct sk_buff **pskb)
>  	if (unlikely(skb->pkt_type == PACKET_LOOPBACK))
>  		return RX_HANDLER_PASS;
>  
> -	if (!is_valid_ether_addr(eth_hdr(skb)->h_source))
> -		goto drop;
> +	if (!is_valid_ether_addr(eth_hdr(skb)->h_source)) {
> +		kfree_skb_reason(skb, SKB_DROP_REASON_MAC_INVALID_SOURCE);
> +		return RX_HANDLER_CONSUMED;
> +	}
>  
>  	skb = skb_share_check(skb, GFP_ATOMIC);
>  	if (!skb)
> @@ -374,7 +380,8 @@ static rx_handler_result_t br_handle_frame(struct sk_buff **pskb)
>  			return RX_HANDLER_PASS;
>  
>  		case 0x01:	/* IEEE MAC (Pause) */
> -			goto drop;
> +			kfree_skb_reason(skb, SKB_DROP_REASON_MAC_IEEE_MAC_CONTROL);
> +			return RX_HANDLER_CONSUMED;
>  
>  		case 0x0E:	/* 802.1AB LLDP */
>  			fwd_mask |= p->br->group_fwd_mask;
> @@ -423,8 +430,7 @@ static rx_handler_result_t br_handle_frame(struct sk_buff **pskb)
>  
>  		return nf_hook_bridge_pre(skb, pskb);
>  	default:
> -drop:
> -		kfree_skb(skb);
> +		kfree_skb_reason(skb, SKB_DROP_REASON_BRIDGE_INGRESS_PORT_NFWD);
>  	}
>  	return RX_HANDLER_CONSUMED;
>  }

Cheers,
 Nik

Re: [PATCH net-next] net/bridge: Add skb drop reasons to the most common drop points

[Ido Schimmel]

On Sun, Dec 08, 2024 at 05:18:05PM -0500, Radu Rendec wrote:
> The bridge input code may drop frames for various reasons and at various
> points in the ingress handling logic. Currently kfree_skb() is used
> everywhere, and therefore no drop reason is specified. Add drop reasons
> to the most common drop points.
> 
> The purpose of this patch is to address the most common drop points on
> the bridge ingress path. It does not exhaustively add drop reasons to
> the entire bridge code. The intention here is to incrementally add drop
> reasons to the rest of the bridge code in follow up patches.
> 
> Most of the skb drop points that are addressed in this patch can be
> easily tested by sending crafted packets. The diagram below shows a
> simple test configuration, and some examples using `packit`(*) are
> also included. The bridge is set up with STP disabled.
> (*) https://github.com/resurrecting-open-source-projects/packit
> 
> The following changes were *not* tested:
> * SKB_DROP_REASON_BRIDGE_NO_EGRESS_PORT in br_multicast_flood(). I could
>   not find an easy way to make a crafted packet get there.
> * SKB_DROP_REASON_BRIDGE_INGRESS_PORT_NFWD in br_handle_frame_finish()
>   when the port state is BR_STATE_DISABLED, because in that case the
>   frame is already dropped in the switch/case block at the end of
>   br_handle_frame().
> 
>     +---+---+
>     |  br0  |
>     +---+---+
>         |
>     +---+---+  veth pair  +-------+
>     | veth0 +-------------+ xeth0 |
>     +-------+             +-------+
> 
> SKB_DROP_REASON_MAC_INVALID_SOURCE - br_handle_frame()
> packit -t UDP -s 192.168.0.1 -d 192.168.0.2 -S 8000 -D 8000 \
>   -e 01:22:33:44:55:66 -E aa:bb:cc:dd:ee:ff -c 1 \
>   -p '0x de ad be ef' -i xeth0
> 
> SKB_DROP_REASON_MAC_IEEE_MAC_CONTROL - br_handle_frame()
> packit -t UDP -s 192.168.0.1 -d 192.168.0.2 -S 8000 -D 8000 \
>   -e 02:22:33:44:55:66 -E 01:80:c2:00:00:01 -c 1 \
>   -p '0x de ad be ef' -i xeth0
> 
> SKB_DROP_REASON_BRIDGE_INGRESS_PORT_NFWD - br_handle_frame()
> bridge link set dev veth0 state 0 # disabled
> packit -t UDP -s 192.168.0.1 -d 192.168.0.2 -S 8000 -D 8000 \
>   -e 02:22:33:44:55:66 -E aa:bb:cc:dd:ee:ff -c 1 \
>   -p '0x de ad be ef' -i xeth0
> 
> SKB_DROP_REASON_BRIDGE_INGRESS_PORT_NFWD - br_handle_frame_finish()
> bridge link set dev veth0 state 2 # learning
> packit -t UDP -s 192.168.0.1 -d 192.168.0.2 -S 8000 -D 8000 \
>   -e 02:22:33:44:55:66 -E aa:bb:cc:dd:ee:ff -c 1 \
>   -p '0x de ad be ef' -i xeth0
> 
> SKB_DROP_REASON_BRIDGE_NO_EGRESS_PORT - br_flood()
> packit -t UDP -s 192.168.0.1 -d 192.168.0.2 -S 8000 -D 8000 \
>   -e 02:22:33:44:55:66 -E aa:bb:cc:dd:ee:ff -c 1 \
>   -p '0x de ad be ef' -i xeth0
> 
> Signed-off-by: Radu Rendec <rrendec@redhat.com>
> ---
>  include/net/dropreason-core.h | 18 ++++++++++++++++++
>  net/bridge/br_forward.c       |  4 ++--
>  net/bridge/br_input.c         | 24 +++++++++++++++---------
>  3 files changed, 35 insertions(+), 11 deletions(-)
> 
> diff --git a/include/net/dropreason-core.h b/include/net/dropreason-core.h
> index c29282fabae6..1f2ae5b387c1 100644
> --- a/include/net/dropreason-core.h
> +++ b/include/net/dropreason-core.h
> @@ -108,6 +108,9 @@
>  	FN(TUNNEL_TXINFO)		\
>  	FN(LOCAL_MAC)			\
>  	FN(ARP_PVLAN_DISABLE)		\
> +	FN(MAC_IEEE_MAC_CONTROL)	\
> +	FN(BRIDGE_INGRESS_PORT_NFWD)	\
> +	FN(BRIDGE_NO_EGRESS_PORT)	\
>  	FNe(MAX)
>  
>  /**
> @@ -502,6 +505,21 @@ enum skb_drop_reason {
>  	 * enabled.
>  	 */
>  	SKB_DROP_REASON_ARP_PVLAN_DISABLE,
> +	/**
> +	 * @SKB_DROP_REASON_MAC_IEEE_MAC_CONTROL: the destination MAC address
> +	 * is an IEEE MAC Control address.
> +	 */

IMO, dropping pause frames is not among "the most common drop points".
Are you planning on reusing this reason in other modules? If not, then I
prefer removing it. My understanding is that we should not try to
document every obscure drop with these reasons.

> +	SKB_DROP_REASON_MAC_IEEE_MAC_CONTROL,
> +	/**
> +	 * @SKB_DROP_REASON_BRIDGE_INGRESS_PORT_NFWD: the STP state of the
> +	 * ingress bridge port does not allow frames to be forwarded.
> +	 */
> +	SKB_DROP_REASON_BRIDGE_INGRESS_PORT_NFWD,

Are you intending on reusing this for other ingress drops (e.g., VLAN,
locked port) or is this specific to ingress STP filtering? I think it
will be useful to distinguish between the different cases, so I suggest
renaming this reason to make it clear it is about ingress STP.

> +	/**
> +	 * SKB_DROP_REASON_BRIDGE_NO_EGRESS_PORT: no eligible egress port was
> +	 * found while attempting to flood the frame.
> +	 */
> +	SKB_DROP_REASON_BRIDGE_NO_EGRESS_PORT,
>  	/**
>  	 * @SKB_DROP_REASON_MAX: the maximum of core drop reasons, which
>  	 * shouldn't be used as a real 'reason' - only for tracing code gen
> diff --git a/net/bridge/br_forward.c b/net/bridge/br_forward.c
> index e19b583ff2c6..e33e2f4fc3d9 100644
> --- a/net/bridge/br_forward.c
> +++ b/net/bridge/br_forward.c
> @@ -249,7 +249,7 @@ void br_flood(struct net_bridge *br, struct sk_buff *skb,
>  
>  out:
>  	if (!local_rcv)
> -		kfree_skb(skb);
> +		kfree_skb_reason(skb, SKB_DROP_REASON_BRIDGE_NO_EGRESS_PORT);
>  }
>  
>  #ifdef CONFIG_BRIDGE_IGMP_SNOOPING
> @@ -349,6 +349,6 @@ void br_multicast_flood(struct net_bridge_mdb_entry *mdst,
>  
>  out:
>  	if (!local_rcv)
> -		kfree_skb(skb);
> +		kfree_skb_reason(skb, SKB_DROP_REASON_BRIDGE_NO_EGRESS_PORT);
>  }
>  #endif
> diff --git a/net/bridge/br_input.c b/net/bridge/br_input.c
> index ceaa5a89b947..fc00e172e1e1 100644
> --- a/net/bridge/br_input.c
> +++ b/net/bridge/br_input.c
> @@ -96,8 +96,10 @@ int br_handle_frame_finish(struct net *net, struct sock *sk, struct sk_buff *skb
>  	if (br_mst_is_enabled(br)) {
>  		state = BR_STATE_FORWARDING;
>  	} else {
> -		if (p->state == BR_STATE_DISABLED)
> -			goto drop;
> +		if (p->state == BR_STATE_DISABLED) {
> +			kfree_skb_reason(skb, SKB_DROP_REASON_BRIDGE_INGRESS_PORT_NFWD);
> +			return 0;
> +		}
>  
>  		state = p->state;
>  	}
> @@ -155,8 +157,10 @@ int br_handle_frame_finish(struct net *net, struct sock *sk, struct sk_buff *skb
>  		}
>  	}
>  
> -	if (state == BR_STATE_LEARNING)
> -		goto drop;
> +	if (state == BR_STATE_LEARNING) {
> +		kfree_skb_reason(skb, SKB_DROP_REASON_BRIDGE_INGRESS_PORT_NFWD);
> +		return 0;
> +	}
>  
>  	BR_INPUT_SKB_CB(skb)->brdev = br->dev;
>  	BR_INPUT_SKB_CB(skb)->src_port_isolated = !!(p->flags & BR_ISOLATED);
> @@ -331,8 +335,10 @@ static rx_handler_result_t br_handle_frame(struct sk_buff **pskb)
>  	if (unlikely(skb->pkt_type == PACKET_LOOPBACK))
>  		return RX_HANDLER_PASS;
>  
> -	if (!is_valid_ether_addr(eth_hdr(skb)->h_source))
> -		goto drop;
> +	if (!is_valid_ether_addr(eth_hdr(skb)->h_source)) {
> +		kfree_skb_reason(skb, SKB_DROP_REASON_MAC_INVALID_SOURCE);
> +		return RX_HANDLER_CONSUMED;
> +	}
>  
>  	skb = skb_share_check(skb, GFP_ATOMIC);
>  	if (!skb)
> @@ -374,7 +380,8 @@ static rx_handler_result_t br_handle_frame(struct sk_buff **pskb)
>  			return RX_HANDLER_PASS;
>  
>  		case 0x01:	/* IEEE MAC (Pause) */
> -			goto drop;
> +			kfree_skb_reason(skb, SKB_DROP_REASON_MAC_IEEE_MAC_CONTROL);
> +			return RX_HANDLER_CONSUMED;
>  
>  		case 0x0E:	/* 802.1AB LLDP */
>  			fwd_mask |= p->br->group_fwd_mask;
> @@ -423,8 +430,7 @@ static rx_handler_result_t br_handle_frame(struct sk_buff **pskb)
>  
>  		return nf_hook_bridge_pre(skb, pskb);
>  	default:
> -drop:
> -		kfree_skb(skb);
> +		kfree_skb_reason(skb, SKB_DROP_REASON_BRIDGE_INGRESS_PORT_NFWD);
>  	}
>  	return RX_HANDLER_CONSUMED;
>  }
> -- 
> 2.47.1
> 
> 

Re: [PATCH net-next] net/bridge: Add skb drop reasons to the most common drop points

[Ido Schimmel]

On Tue, Dec 10, 2024 at 11:18:06AM +0200, Nikolay Aleksandrov wrote:
> On 12/9/24 00:18, Radu Rendec wrote:
> > +	/**
> > +	 * SKB_DROP_REASON_BRIDGE_NO_EGRESS_PORT: no eligible egress port was
> > +	 * found while attempting to flood the frame.
> > +	 */
> > +	SKB_DROP_REASON_BRIDGE_NO_EGRESS_PORT,
> >  	/**
> >  	 * @SKB_DROP_REASON_MAX: the maximum of core drop reasons, which
> >  	 * shouldn't be used as a real 'reason' - only for tracing code gen
> > diff --git a/net/bridge/br_forward.c b/net/bridge/br_forward.c
> > index e19b583ff2c6..e33e2f4fc3d9 100644
> > --- a/net/bridge/br_forward.c
> > +++ b/net/bridge/br_forward.c
> > @@ -249,7 +249,7 @@ void br_flood(struct net_bridge *br, struct sk_buff *skb,
> >  
> >  out:
> >  	if (!local_rcv)
> > -		kfree_skb(skb);
> > +		kfree_skb_reason(skb, SKB_DROP_REASON_BRIDGE_NO_EGRESS_PORT);
> 
> This is not entirely correct, we can get here if we had an error forwarding
> the packet to some port, but it may already have been forwarded to others.
> The reason should distinguish between those two cases.

Regarding 'SKB_DROP_REASON_BRIDGE_NO_EGRESS_PORT', there is a similar
reason in VXLAN called 'SKB_DROP_REASON_VXLAN_NO_REMOTE' which basically
means the same thing. Maybe we can rename it to
'SKB_DROP_REASON_NO_TX_TARGET' (or something similar) and reuse it here?

> 
> >  }
> >  
> >  #ifdef CONFIG_BRIDGE_IGMP_SNOOPING
> > @@ -349,6 +349,6 @@ void br_multicast_flood(struct net_bridge_mdb_entry *mdst,
> >  
> >  out:
> >  	if (!local_rcv)
> > -		kfree_skb(skb);
> > +		kfree_skb_reason(skb, SKB_DROP_REASON_BRIDGE_NO_EGRESS_PORT);
> 
> Same comment as above (br_flood).
> 
> >  }
> >  #endif
> > diff --git a/net/bridge/br_input.c b/net/bridge/br_input.c
> > index ceaa5a89b947..fc00e172e1e1 100644
> > --- a/net/bridge/br_input.c
> > +++ b/net/bridge/br_input.c
> > @@ -96,8 +96,10 @@ int br_handle_frame_finish(struct net *net, struct sock *sk, struct sk_buff *skb
> >  	if (br_mst_is_enabled(br)) {
> >  		state = BR_STATE_FORWARDING;
> >  	} else {
> > -		if (p->state == BR_STATE_DISABLED)
> > -			goto drop;
> > +		if (p->state == BR_STATE_DISABLED) {
> > +			kfree_skb_reason(skb, SKB_DROP_REASON_BRIDGE_INGRESS_PORT_NFWD);
> > +			return 0;
> > +		}
> >  
> >  		state = p->state;
> >  	}
> > @@ -155,8 +157,10 @@ int br_handle_frame_finish(struct net *net, struct sock *sk, struct sk_buff *skb
> >  		}
> >  	}
> >  
> > -	if (state == BR_STATE_LEARNING)
> > -		goto drop;
> > +	if (state == BR_STATE_LEARNING) {
> > +		kfree_skb_reason(skb, SKB_DROP_REASON_BRIDGE_INGRESS_PORT_NFWD);
> > +		return 0;
> > +	}>  
> >  	BR_INPUT_SKB_CB(skb)->brdev = br->dev;
> >  	BR_INPUT_SKB_CB(skb)->src_port_isolated = !!(p->flags & BR_ISOLATED);
> > @@ -331,8 +335,10 @@ static rx_handler_result_t br_handle_frame(struct sk_buff **pskb)
> >  	if (unlikely(skb->pkt_type == PACKET_LOOPBACK))
> >  		return RX_HANDLER_PASS;
> >  
> > -	if (!is_valid_ether_addr(eth_hdr(skb)->h_source))
> > -		goto drop;
> > +	if (!is_valid_ether_addr(eth_hdr(skb)->h_source)) {
> > +		kfree_skb_reason(skb, SKB_DROP_REASON_MAC_INVALID_SOURCE);
> > +		return RX_HANDLER_CONSUMED;
> > +	}
> >  
> >  	skb = skb_share_check(skb, GFP_ATOMIC);
> >  	if (!skb)
> > @@ -374,7 +380,8 @@ static rx_handler_result_t br_handle_frame(struct sk_buff **pskb)
> >  			return RX_HANDLER_PASS;
> >  
> >  		case 0x01:	/* IEEE MAC (Pause) */
> > -			goto drop;
> > +			kfree_skb_reason(skb, SKB_DROP_REASON_MAC_IEEE_MAC_CONTROL);
> > +			return RX_HANDLER_CONSUMED;
> >  
> >  		case 0x0E:	/* 802.1AB LLDP */
> >  			fwd_mask |= p->br->group_fwd_mask;
> > @@ -423,8 +430,7 @@ static rx_handler_result_t br_handle_frame(struct sk_buff **pskb)
> >  
> >  		return nf_hook_bridge_pre(skb, pskb);
> >  	default:
> > -drop:
> > -		kfree_skb(skb);
> > +		kfree_skb_reason(skb, SKB_DROP_REASON_BRIDGE_INGRESS_PORT_NFWD);
> >  	}
> >  	return RX_HANDLER_CONSUMED;
> >  }
> 
> Cheers,
>  Nik
> 
> 

Re: [PATCH net-next] net/bridge: Add skb drop reasons to the most common drop points

[Radu Rendec]

On Tue, 2024-12-10 at 11:18 +0200, Nikolay Aleksandrov wrote:
> On 12/9/24 00:18, Radu Rendec wrote:
> > The bridge input code may drop frames for various reasons and at various
> > points in the ingress handling logic. Currently kfree_skb() is used
> > everywhere, and therefore no drop reason is specified. Add drop reasons
> > to the most common drop points.
> > 
> > The purpose of this patch is to address the most common drop points on
> > the bridge ingress path. It does not exhaustively add drop reasons to
> > the entire bridge code. The intention here is to incrementally add drop
> > reasons to the rest of the bridge code in follow up patches.
> > 
> > Most of the skb drop points that are addressed in this patch can be
> > easily tested by sending crafted packets. The diagram below shows a
> > simple test configuration, and some examples using `packit`(*) are
> > also included. The bridge is set up with STP disabled.
> > (*) https://github.com/resurrecting-open-source-projects/packit
> > 
> > The following changes were *not* tested:
> > * SKB_DROP_REASON_BRIDGE_NO_EGRESS_PORT in br_multicast_flood(). I could
> >   not find an easy way to make a crafted packet get there.
> > * SKB_DROP_REASON_BRIDGE_INGRESS_PORT_NFWD in br_handle_frame_finish()
> >   when the port state is BR_STATE_DISABLED, because in that case the
> >   frame is already dropped in the switch/case block at the end of
> >   br_handle_frame().
> > 
> >     +---+---+
> >     |  br0  |
> >     +---+---+
> >         |
> >     +---+---+  veth pair  +-------+
> >     | veth0 +-------------+ xeth0 |
> >     +-------+             +-------+
> > 
> > SKB_DROP_REASON_MAC_INVALID_SOURCE - br_handle_frame()
> > packit -t UDP -s 192.168.0.1 -d 192.168.0.2 -S 8000 -D 8000 \
> >   -e 01:22:33:44:55:66 -E aa:bb:cc:dd:ee:ff -c 1 \
> >   -p '0x de ad be ef' -i xeth0
> > 
> > SKB_DROP_REASON_MAC_IEEE_MAC_CONTROL - br_handle_frame()
> > packit -t UDP -s 192.168.0.1 -d 192.168.0.2 -S 8000 -D 8000 \
> >   -e 02:22:33:44:55:66 -E 01:80:c2:00:00:01 -c 1 \
> >   -p '0x de ad be ef' -i xeth0
> > 
> > SKB_DROP_REASON_BRIDGE_INGRESS_PORT_NFWD - br_handle_frame()
> > bridge link set dev veth0 state 0 # disabled
> > packit -t UDP -s 192.168.0.1 -d 192.168.0.2 -S 8000 -D 8000 \
> >   -e 02:22:33:44:55:66 -E aa:bb:cc:dd:ee:ff -c 1 \
> >   -p '0x de ad be ef' -i xeth0
> > 
> > SKB_DROP_REASON_BRIDGE_INGRESS_PORT_NFWD - br_handle_frame_finish()
> > bridge link set dev veth0 state 2 # learning
> > packit -t UDP -s 192.168.0.1 -d 192.168.0.2 -S 8000 -D 8000 \
> >   -e 02:22:33:44:55:66 -E aa:bb:cc:dd:ee:ff -c 1 \
> >   -p '0x de ad be ef' -i xeth0
> > 
> > SKB_DROP_REASON_BRIDGE_NO_EGRESS_PORT - br_flood()
> > packit -t UDP -s 192.168.0.1 -d 192.168.0.2 -S 8000 -D 8000 \
> >   -e 02:22:33:44:55:66 -E aa:bb:cc:dd:ee:ff -c 1 \
> >   -p '0x de ad be ef' -i xeth0
> > 
> > Signed-off-by: Radu Rendec <rrendec@redhat.com>
> > ---
> >  include/net/dropreason-core.h | 18 ++++++++++++++++++
> >  net/bridge/br_forward.c       |  4 ++--
> >  net/bridge/br_input.c         | 24 +++++++++++++++---------
> >  3 files changed, 35 insertions(+), 11 deletions(-)
> > 
> 
> Hi,
> Thanks for working on this, a few comments below.

Sure, thanks for reviewing! Please see my comments below.

> > diff --git a/include/net/dropreason-core.h b/include/net/dropreason-core.h
> > index c29282fabae6..1f2ae5b387c1 100644
> > --- a/include/net/dropreason-core.h
> > +++ b/include/net/dropreason-core.h
> > @@ -108,6 +108,9 @@
> >  	FN(TUNNEL_TXINFO)		\
> >  	FN(LOCAL_MAC)			\
> >  	FN(ARP_PVLAN_DISABLE)		\
> > +	FN(MAC_IEEE_MAC_CONTROL)	\
> > +	FN(BRIDGE_INGRESS_PORT_NFWD)	\
> > +	FN(BRIDGE_NO_EGRESS_PORT)	\
> >  	FNe(MAX)
> >  
> >  /**
> > @@ -502,6 +505,21 @@ enum skb_drop_reason {
> >  	 * enabled.
> >  	 */
> >  	SKB_DROP_REASON_ARP_PVLAN_DISABLE,
> > +	/**
> > +	 * @SKB_DROP_REASON_MAC_IEEE_MAC_CONTROL: the destination MAC address
> > +	 * is an IEEE MAC Control address.
> > +	 */
> > +	SKB_DROP_REASON_MAC_IEEE_MAC_CONTROL,
> > +	/**
> > +	 * @SKB_DROP_REASON_BRIDGE_INGRESS_PORT_NFWD: the STP state of the
> > +	 * ingress bridge port does not allow frames to be forwarded.
> > +	 */
> > +	SKB_DROP_REASON_BRIDGE_INGRESS_PORT_NFWD,
> 
> Since this is used only when the port state causes the packet to drop, why not
> rename it to something that suggests it was the state?

Yes, Ido had a similar suggestion [1], so it's clear that it must be
renamed. I will go with SKB_DROP_REASON_BRIDGE_INGRESS_STP_STATE in the
next version, unless you have a better idea.

> > +	/**
> > +	 * SKB_DROP_REASON_BRIDGE_NO_EGRESS_PORT: no eligible egress port was
> > +	 * found while attempting to flood the frame.
> > +	 */
> > +	SKB_DROP_REASON_BRIDGE_NO_EGRESS_PORT,
> >  	/**
> >  	 * @SKB_DROP_REASON_MAX: the maximum of core drop reasons, which
> >  	 * shouldn't be used as a real 'reason' - only for tracing code gen
> > diff --git a/net/bridge/br_forward.c b/net/bridge/br_forward.c
> > index e19b583ff2c6..e33e2f4fc3d9 100644
> > --- a/net/bridge/br_forward.c
> > +++ b/net/bridge/br_forward.c
> > @@ -249,7 +249,7 @@ void br_flood(struct net_bridge *br, struct sk_buff *skb,
> >  
> >  out:
> >  	if (!local_rcv)
> > -		kfree_skb(skb);
> > +		kfree_skb_reason(skb, SKB_DROP_REASON_BRIDGE_NO_EGRESS_PORT);
> 
> This is not entirely correct, we can get here if we had an error forwarding
> the packet to some port, but it may already have been forwarded to others.
> The reason should distinguish between those two cases.

I will follow Ido's suggestion [2] and rename SKB_DROP_REASON_VXLAN_NO_REMOTE
to SKB_DROP_REASON_NO_TX_TARGET, and then use that.

But it will only cover the case when there are no errors, so I still
need a different reason for the error case. I looked, and I couldn't
find an existing one that's close enough, so I think I should create a
new one. How about SKB_DROP_REASON_TX_ERROR? I would not use "BRIDGE"
in the name because I'm thinking it may be reused elsewhere, outside
the bridge module.

> >  }
> >  
> >  #ifdef CONFIG_BRIDGE_IGMP_SNOOPING
> > @@ -349,6 +349,6 @@ void br_multicast_flood(struct net_bridge_mdb_entry *mdst,
> >  
> >  out:
> >  	if (!local_rcv)
> > -		kfree_skb(skb);
> > +		kfree_skb_reason(skb, SKB_DROP_REASON_BRIDGE_NO_EGRESS_PORT);
> 
> Same comment as above (br_flood).
> 
> >  }
> >  #endif
> > diff --git a/net/bridge/br_input.c b/net/bridge/br_input.c
> > index ceaa5a89b947..fc00e172e1e1 100644
> > --- a/net/bridge/br_input.c
> > +++ b/net/bridge/br_input.c
> > @@ -96,8 +96,10 @@ int br_handle_frame_finish(struct net *net, struct sock *sk, struct sk_buff *skb
> >  	if (br_mst_is_enabled(br)) {
> >  		state = BR_STATE_FORWARDING;
> >  	} else {
> > -		if (p->state == BR_STATE_DISABLED)
> > -			goto drop;
> > +		if (p->state == BR_STATE_DISABLED) {
> > +			kfree_skb_reason(skb, SKB_DROP_REASON_BRIDGE_INGRESS_PORT_NFWD);
> > +			return 0;
> > +		}
> >  
> >  		state = p->state;
> >  	}
> > @@ -155,8 +157,10 @@ int br_handle_frame_finish(struct net *net, struct sock *sk, struct sk_buff *skb
> >  		}
> >  	}
> >  
> > -	if (state == BR_STATE_LEARNING)
> > -		goto drop;
> > +	if (state == BR_STATE_LEARNING) {
> > +		kfree_skb_reason(skb, SKB_DROP_REASON_BRIDGE_INGRESS_PORT_NFWD);
> > +		return 0;
> > +	}>  
> >  	BR_INPUT_SKB_CB(skb)->brdev = br->dev;
> >  	BR_INPUT_SKB_CB(skb)->src_port_isolated = !!(p->flags & BR_ISOLATED);
> > @@ -331,8 +335,10 @@ static rx_handler_result_t br_handle_frame(struct sk_buff **pskb)
> >  	if (unlikely(skb->pkt_type == PACKET_LOOPBACK))
> >  		return RX_HANDLER_PASS;
> >  
> > -	if (!is_valid_ether_addr(eth_hdr(skb)->h_source))
> > -		goto drop;
> > +	if (!is_valid_ether_addr(eth_hdr(skb)->h_source)) {
> > +		kfree_skb_reason(skb, SKB_DROP_REASON_MAC_INVALID_SOURCE);
> > +		return RX_HANDLER_CONSUMED;
> > +	}
> >  
> >  	skb = skb_share_check(skb, GFP_ATOMIC);
> >  	if (!skb)
> > @@ -374,7 +380,8 @@ static rx_handler_result_t br_handle_frame(struct sk_buff **pskb)
> >  			return RX_HANDLER_PASS;
> >  
> >  		case 0x01:	/* IEEE MAC (Pause) */
> > -			goto drop;
> > +			kfree_skb_reason(skb, SKB_DROP_REASON_MAC_IEEE_MAC_CONTROL);
> > +			return RX_HANDLER_CONSUMED;
> >  
> >  		case 0x0E:	/* 802.1AB LLDP */
> >  			fwd_mask |= p->br->group_fwd_mask;
> > @@ -423,8 +430,7 @@ static rx_handler_result_t br_handle_frame(struct sk_buff **pskb)
> >  
> >  		return nf_hook_bridge_pre(skb, pskb);
> >  	default:
> > -drop:
> > -		kfree_skb(skb);
> > +		kfree_skb_reason(skb, SKB_DROP_REASON_BRIDGE_INGRESS_PORT_NFWD);
> >  	}
> >  	return RX_HANDLER_CONSUMED;
> >  }

[1] https://lore.kernel.org/bridge/Z1sLyqZQCjbcCOde@shredder/
[2] https://lore.kernel.org/bridge/Z1sUsSFfBC9GoiIA@shredder/

--
Best regards,
Radu

Re: [PATCH net-next] net/bridge: Add skb drop reasons to the most common drop points

[Radu Rendec]

On Thu, 2024-12-12 at 18:14 +0200, Ido Schimmel wrote:
> On Sun, Dec 08, 2024 at 05:18:05PM -0500, Radu Rendec wrote:
> > The bridge input code may drop frames for various reasons and at various
> > points in the ingress handling logic. Currently kfree_skb() is used
> > everywhere, and therefore no drop reason is specified. Add drop reasons
> > to the most common drop points.
> > 
> > The purpose of this patch is to address the most common drop points on
> > the bridge ingress path. It does not exhaustively add drop reasons to
> > the entire bridge code. The intention here is to incrementally add drop
> > reasons to the rest of the bridge code in follow up patches.
> > 
> > Most of the skb drop points that are addressed in this patch can be
> > easily tested by sending crafted packets. The diagram below shows a
> > simple test configuration, and some examples using `packit`(*) are
> > also included. The bridge is set up with STP disabled.
> > (*) https://github.com/resurrecting-open-source-projects/packit
> > 
> > The following changes were *not* tested:
> > * SKB_DROP_REASON_BRIDGE_NO_EGRESS_PORT in br_multicast_flood(). I could
> >   not find an easy way to make a crafted packet get there.
> > * SKB_DROP_REASON_BRIDGE_INGRESS_PORT_NFWD in br_handle_frame_finish()
> >   when the port state is BR_STATE_DISABLED, because in that case the
> >   frame is already dropped in the switch/case block at the end of
> >   br_handle_frame().
> > 
> >     +---+---+
> >     |  br0  |
> >     +---+---+
> >         |
> >     +---+---+  veth pair  +-------+
> >     | veth0 +-------------+ xeth0 |
> >     +-------+             +-------+
> > 
> > SKB_DROP_REASON_MAC_INVALID_SOURCE - br_handle_frame()
> > packit -t UDP -s 192.168.0.1 -d 192.168.0.2 -S 8000 -D 8000 \
> >   -e 01:22:33:44:55:66 -E aa:bb:cc:dd:ee:ff -c 1 \
> >   -p '0x de ad be ef' -i xeth0
> > 
> > SKB_DROP_REASON_MAC_IEEE_MAC_CONTROL - br_handle_frame()
> > packit -t UDP -s 192.168.0.1 -d 192.168.0.2 -S 8000 -D 8000 \
> >   -e 02:22:33:44:55:66 -E 01:80:c2:00:00:01 -c 1 \
> >   -p '0x de ad be ef' -i xeth0
> > 
> > SKB_DROP_REASON_BRIDGE_INGRESS_PORT_NFWD - br_handle_frame()
> > bridge link set dev veth0 state 0 # disabled
> > packit -t UDP -s 192.168.0.1 -d 192.168.0.2 -S 8000 -D 8000 \
> >   -e 02:22:33:44:55:66 -E aa:bb:cc:dd:ee:ff -c 1 \
> >   -p '0x de ad be ef' -i xeth0
> > 
> > SKB_DROP_REASON_BRIDGE_INGRESS_PORT_NFWD - br_handle_frame_finish()
> > bridge link set dev veth0 state 2 # learning
> > packit -t UDP -s 192.168.0.1 -d 192.168.0.2 -S 8000 -D 8000 \
> >   -e 02:22:33:44:55:66 -E aa:bb:cc:dd:ee:ff -c 1 \
> >   -p '0x de ad be ef' -i xeth0
> > 
> > SKB_DROP_REASON_BRIDGE_NO_EGRESS_PORT - br_flood()
> > packit -t UDP -s 192.168.0.1 -d 192.168.0.2 -S 8000 -D 8000 \
> >   -e 02:22:33:44:55:66 -E aa:bb:cc:dd:ee:ff -c 1 \
> >   -p '0x de ad be ef' -i xeth0
> > 
> > Signed-off-by: Radu Rendec <rrendec@redhat.com>
> > ---
> >  include/net/dropreason-core.h | 18 ++++++++++++++++++
> >  net/bridge/br_forward.c       |  4 ++--
> >  net/bridge/br_input.c         | 24 +++++++++++++++---------
> >  3 files changed, 35 insertions(+), 11 deletions(-)
> > 
> > diff --git a/include/net/dropreason-core.h b/include/net/dropreason-core.h
> > index c29282fabae6..1f2ae5b387c1 100644
> > --- a/include/net/dropreason-core.h
> > +++ b/include/net/dropreason-core.h
> > @@ -108,6 +108,9 @@
> >  	FN(TUNNEL_TXINFO)		\
> >  	FN(LOCAL_MAC)			\
> >  	FN(ARP_PVLAN_DISABLE)		\
> > +	FN(MAC_IEEE_MAC_CONTROL)	\
> > +	FN(BRIDGE_INGRESS_PORT_NFWD)	\
> > +	FN(BRIDGE_NO_EGRESS_PORT)	\
> >  	FNe(MAX)
> >  
> >  /**
> > @@ -502,6 +505,21 @@ enum skb_drop_reason {
> >  	 * enabled.
> >  	 */
> >  	SKB_DROP_REASON_ARP_PVLAN_DISABLE,
> > +	/**
> > +	 * @SKB_DROP_REASON_MAC_IEEE_MAC_CONTROL: the destination MAC address
> > +	 * is an IEEE MAC Control address.
> > +	 */
> 
> IMO, dropping pause frames is not among "the most common drop points".
> Are you planning on reusing this reason in other modules? If not, then I
> prefer removing it. My understanding is that we should not try to
> document every obscure drop with these reasons.

Fair enough. I don't have an immediate plan to reuse this reason, and
to be honest, I'm not that familiar with the networking stack to be
able to tell off hand if it's likely to be useful elsewhere.

Would you prefer to stick to not specifying a drop reason at all at
that particular drop point, or to reuse an existing reason? Two
existing reasons that could be used (although they are not entirely
accurate) are:
SKB_DROP_REASON_UNHANDLED_PROTO
SKB_DROP_REASON_MAC_INVALID_SOURCE

> > +	SKB_DROP_REASON_MAC_IEEE_MAC_CONTROL,
> > +	/**
> > +	 * @SKB_DROP_REASON_BRIDGE_INGRESS_PORT_NFWD: the STP state of the
> > +	 * ingress bridge port does not allow frames to be forwarded.
> > +	 */
> > +	SKB_DROP_REASON_BRIDGE_INGRESS_PORT_NFWD,
> 
> Are you intending on reusing this for other ingress drops (e.g., VLAN,
> locked port) or is this specific to ingress STP filtering? I think it
> will be useful to distinguish between the different cases, so I suggest
> renaming this reason to make it clear it is about ingress STP.

No, it's specific to ingress STP filtering. I will rename it to
SKB_DROP_REASON_BRIDGE_INGRESS_STP_STATE as I said in the other thread.

> > +	/**
> > +	 * SKB_DROP_REASON_BRIDGE_NO_EGRESS_PORT: no eligible egress port was
> > +	 * found while attempting to flood the frame.
> > +	 */
> > +	SKB_DROP_REASON_BRIDGE_NO_EGRESS_PORT,
> >  	/**
> >  	 * @SKB_DROP_REASON_MAX: the maximum of core drop reasons, which
> >  	 * shouldn't be used as a real 'reason' - only for tracing code gen
> > diff --git a/net/bridge/br_forward.c b/net/bridge/br_forward.c
> > index e19b583ff2c6..e33e2f4fc3d9 100644
> > --- a/net/bridge/br_forward.c
> > +++ b/net/bridge/br_forward.c
> > @@ -249,7 +249,7 @@ void br_flood(struct net_bridge *br, struct sk_buff *skb,
> >  
> >  out:
> >  	if (!local_rcv)
> > -		kfree_skb(skb);
> > +		kfree_skb_reason(skb, SKB_DROP_REASON_BRIDGE_NO_EGRESS_PORT);
> >  }
> >  
> >  #ifdef CONFIG_BRIDGE_IGMP_SNOOPING
> > @@ -349,6 +349,6 @@ void br_multicast_flood(struct net_bridge_mdb_entry *mdst,
> >  
> >  out:
> >  	if (!local_rcv)
> > -		kfree_skb(skb);
> > +		kfree_skb_reason(skb, SKB_DROP_REASON_BRIDGE_NO_EGRESS_PORT);
> >  }
> >  #endif
> > diff --git a/net/bridge/br_input.c b/net/bridge/br_input.c
> > index ceaa5a89b947..fc00e172e1e1 100644
> > --- a/net/bridge/br_input.c
> > +++ b/net/bridge/br_input.c
> > @@ -96,8 +96,10 @@ int br_handle_frame_finish(struct net *net, struct sock *sk, struct sk_buff *skb
> >  	if (br_mst_is_enabled(br)) {
> >  		state = BR_STATE_FORWARDING;
> >  	} else {
> > -		if (p->state == BR_STATE_DISABLED)
> > -			goto drop;
> > +		if (p->state == BR_STATE_DISABLED) {
> > +			kfree_skb_reason(skb, SKB_DROP_REASON_BRIDGE_INGRESS_PORT_NFWD);
> > +			return 0;
> > +		}
> >  
> >  		state = p->state;
> >  	}
> > @@ -155,8 +157,10 @@ int br_handle_frame_finish(struct net *net, struct sock *sk, struct sk_buff *skb
> >  		}
> >  	}
> >  
> > -	if (state == BR_STATE_LEARNING)
> > -		goto drop;
> > +	if (state == BR_STATE_LEARNING) {
> > +		kfree_skb_reason(skb, SKB_DROP_REASON_BRIDGE_INGRESS_PORT_NFWD);
> > +		return 0;
> > +	}
> >  
> >  	BR_INPUT_SKB_CB(skb)->brdev = br->dev;
> >  	BR_INPUT_SKB_CB(skb)->src_port_isolated = !!(p->flags & BR_ISOLATED);
> > @@ -331,8 +335,10 @@ static rx_handler_result_t br_handle_frame(struct sk_buff **pskb)
> >  	if (unlikely(skb->pkt_type == PACKET_LOOPBACK))
> >  		return RX_HANDLER_PASS;
> >  
> > -	if (!is_valid_ether_addr(eth_hdr(skb)->h_source))
> > -		goto drop;
> > +	if (!is_valid_ether_addr(eth_hdr(skb)->h_source)) {
> > +		kfree_skb_reason(skb, SKB_DROP_REASON_MAC_INVALID_SOURCE);
> > +		return RX_HANDLER_CONSUMED;
> > +	}
> >  
> >  	skb = skb_share_check(skb, GFP_ATOMIC);
> >  	if (!skb)
> > @@ -374,7 +380,8 @@ static rx_handler_result_t br_handle_frame(struct sk_buff **pskb)
> >  			return RX_HANDLER_PASS;
> >  
> >  		case 0x01:	/* IEEE MAC (Pause) */
> > -			goto drop;
> > +			kfree_skb_reason(skb, SKB_DROP_REASON_MAC_IEEE_MAC_CONTROL);
> > +			return RX_HANDLER_CONSUMED;
> >  
> >  		case 0x0E:	/* 802.1AB LLDP */
> >  			fwd_mask |= p->br->group_fwd_mask;
> > @@ -423,8 +430,7 @@ static rx_handler_result_t br_handle_frame(struct sk_buff **pskb)
> >  
> >  		return nf_hook_bridge_pre(skb, pskb);
> >  	default:
> > -drop:
> > -		kfree_skb(skb);
> > +		kfree_skb_reason(skb, SKB_DROP_REASON_BRIDGE_INGRESS_PORT_NFWD);
> >  	}
> >  	return RX_HANDLER_CONSUMED;
> >  }
> > -- 
> > 2.47.1
> 

Thanks for reviewing!

-- 
Best regards,
Radu

Re: [PATCH net-next] net/bridge: Add skb drop reasons to the most common drop points

[Ido Schimmel]

On Fri, Dec 13, 2024 at 03:44:49PM -0500, Radu Rendec wrote:
> On Thu, 2024-12-12 at 18:14 +0200, Ido Schimmel wrote:
> > On Sun, Dec 08, 2024 at 05:18:05PM -0500, Radu Rendec wrote:
> > > The bridge input code may drop frames for various reasons and at various
> > > points in the ingress handling logic. Currently kfree_skb() is used
> > > everywhere, and therefore no drop reason is specified. Add drop reasons
> > > to the most common drop points.
> > > 
> > > The purpose of this patch is to address the most common drop points on
> > > the bridge ingress path. It does not exhaustively add drop reasons to
> > > the entire bridge code. The intention here is to incrementally add drop
> > > reasons to the rest of the bridge code in follow up patches.
> > > 
> > > Most of the skb drop points that are addressed in this patch can be
> > > easily tested by sending crafted packets. The diagram below shows a
> > > simple test configuration, and some examples using `packit`(*) are
> > > also included. The bridge is set up with STP disabled.
> > > (*) https://github.com/resurrecting-open-source-projects/packit
> > > 
> > > The following changes were *not* tested:
> > > * SKB_DROP_REASON_BRIDGE_NO_EGRESS_PORT in br_multicast_flood(). I could
> > >   not find an easy way to make a crafted packet get there.
> > > * SKB_DROP_REASON_BRIDGE_INGRESS_PORT_NFWD in br_handle_frame_finish()
> > >   when the port state is BR_STATE_DISABLED, because in that case the
> > >   frame is already dropped in the switch/case block at the end of
> > >   br_handle_frame().
> > > 
> > >     +---+---+
> > >     |  br0  |
> > >     +---+---+
> > >         |
> > >     +---+---+  veth pair  +-------+
> > >     | veth0 +-------------+ xeth0 |
> > >     +-------+             +-------+
> > > 
> > > SKB_DROP_REASON_MAC_INVALID_SOURCE - br_handle_frame()
> > > packit -t UDP -s 192.168.0.1 -d 192.168.0.2 -S 8000 -D 8000 \
> > >   -e 01:22:33:44:55:66 -E aa:bb:cc:dd:ee:ff -c 1 \
> > >   -p '0x de ad be ef' -i xeth0
> > > 
> > > SKB_DROP_REASON_MAC_IEEE_MAC_CONTROL - br_handle_frame()
> > > packit -t UDP -s 192.168.0.1 -d 192.168.0.2 -S 8000 -D 8000 \
> > >   -e 02:22:33:44:55:66 -E 01:80:c2:00:00:01 -c 1 \
> > >   -p '0x de ad be ef' -i xeth0
> > > 
> > > SKB_DROP_REASON_BRIDGE_INGRESS_PORT_NFWD - br_handle_frame()
> > > bridge link set dev veth0 state 0 # disabled
> > > packit -t UDP -s 192.168.0.1 -d 192.168.0.2 -S 8000 -D 8000 \
> > >   -e 02:22:33:44:55:66 -E aa:bb:cc:dd:ee:ff -c 1 \
> > >   -p '0x de ad be ef' -i xeth0
> > > 
> > > SKB_DROP_REASON_BRIDGE_INGRESS_PORT_NFWD - br_handle_frame_finish()
> > > bridge link set dev veth0 state 2 # learning
> > > packit -t UDP -s 192.168.0.1 -d 192.168.0.2 -S 8000 -D 8000 \
> > >   -e 02:22:33:44:55:66 -E aa:bb:cc:dd:ee:ff -c 1 \
> > >   -p '0x de ad be ef' -i xeth0
> > > 
> > > SKB_DROP_REASON_BRIDGE_NO_EGRESS_PORT - br_flood()
> > > packit -t UDP -s 192.168.0.1 -d 192.168.0.2 -S 8000 -D 8000 \
> > >   -e 02:22:33:44:55:66 -E aa:bb:cc:dd:ee:ff -c 1 \
> > >   -p '0x de ad be ef' -i xeth0
> > > 
> > > Signed-off-by: Radu Rendec <rrendec@redhat.com>
> > > ---
> > >  include/net/dropreason-core.h | 18 ++++++++++++++++++
> > >  net/bridge/br_forward.c       |  4 ++--
> > >  net/bridge/br_input.c         | 24 +++++++++++++++---------
> > >  3 files changed, 35 insertions(+), 11 deletions(-)
> > > 
> > > diff --git a/include/net/dropreason-core.h b/include/net/dropreason-core.h
> > > index c29282fabae6..1f2ae5b387c1 100644
> > > --- a/include/net/dropreason-core.h
> > > +++ b/include/net/dropreason-core.h
> > > @@ -108,6 +108,9 @@
> > >  	FN(TUNNEL_TXINFO)		\
> > >  	FN(LOCAL_MAC)			\
> > >  	FN(ARP_PVLAN_DISABLE)		\
> > > +	FN(MAC_IEEE_MAC_CONTROL)	\
> > > +	FN(BRIDGE_INGRESS_PORT_NFWD)	\
> > > +	FN(BRIDGE_NO_EGRESS_PORT)	\
> > >  	FNe(MAX)
> > >  
> > >  /**
> > > @@ -502,6 +505,21 @@ enum skb_drop_reason {
> > >  	 * enabled.
> > >  	 */
> > >  	SKB_DROP_REASON_ARP_PVLAN_DISABLE,
> > > +	/**
> > > +	 * @SKB_DROP_REASON_MAC_IEEE_MAC_CONTROL: the destination MAC address
> > > +	 * is an IEEE MAC Control address.
> > > +	 */
> > 
> > IMO, dropping pause frames is not among "the most common drop points".
> > Are you planning on reusing this reason in other modules? If not, then I
> > prefer removing it. My understanding is that we should not try to
> > document every obscure drop with these reasons.
> 
> Fair enough. I don't have an immediate plan to reuse this reason, and
> to be honest, I'm not that familiar with the networking stack to be
> able to tell off hand if it's likely to be useful elsewhere.
> 
> Would you prefer to stick to not specifying a drop reason at all at
> that particular drop point, or to reuse an existing reason? Two
> existing reasons that could be used (although they are not entirely
> accurate) are:
> SKB_DROP_REASON_UNHANDLED_PROTO
> SKB_DROP_REASON_MAC_INVALID_SOURCE

Both aren't really applicable in this case and I doubt users are hitting
this drop point in practice, but I feel like I don't have a good
argument against adding 'SKB_DROP_REASON_MAC_IEEE_MAC_CONTROL', so maybe
just keep it ^o^

Re: [PATCH net-next] net/bridge: Add skb drop reasons to the most common drop points

[Ido Schimmel]

On Fri, Dec 13, 2024 at 03:33:44PM -0500, Radu Rendec wrote:
> On Tue, 2024-12-10 at 11:18 +0200, Nikolay Aleksandrov wrote:
> > On 12/9/24 00:18, Radu Rendec wrote:
> > > The bridge input code may drop frames for various reasons and at various
> > > points in the ingress handling logic. Currently kfree_skb() is used
> > > everywhere, and therefore no drop reason is specified. Add drop reasons
> > > to the most common drop points.
> > > 
> > > The purpose of this patch is to address the most common drop points on
> > > the bridge ingress path. It does not exhaustively add drop reasons to
> > > the entire bridge code. The intention here is to incrementally add drop
> > > reasons to the rest of the bridge code in follow up patches.
> > > 
> > > Most of the skb drop points that are addressed in this patch can be
> > > easily tested by sending crafted packets. The diagram below shows a
> > > simple test configuration, and some examples using `packit`(*) are
> > > also included. The bridge is set up with STP disabled.
> > > (*) https://github.com/resurrecting-open-source-projects/packit
> > > 
> > > The following changes were *not* tested:
> > > * SKB_DROP_REASON_BRIDGE_NO_EGRESS_PORT in br_multicast_flood(). I could
> > >   not find an easy way to make a crafted packet get there.
> > > * SKB_DROP_REASON_BRIDGE_INGRESS_PORT_NFWD in br_handle_frame_finish()
> > >   when the port state is BR_STATE_DISABLED, because in that case the
> > >   frame is already dropped in the switch/case block at the end of
> > >   br_handle_frame().
> > > 
> > >     +---+---+
> > >     |  br0  |
> > >     +---+---+
> > >         |
> > >     +---+---+  veth pair  +-------+
> > >     | veth0 +-------------+ xeth0 |
> > >     +-------+             +-------+
> > > 
> > > SKB_DROP_REASON_MAC_INVALID_SOURCE - br_handle_frame()
> > > packit -t UDP -s 192.168.0.1 -d 192.168.0.2 -S 8000 -D 8000 \
> > >   -e 01:22:33:44:55:66 -E aa:bb:cc:dd:ee:ff -c 1 \
> > >   -p '0x de ad be ef' -i xeth0
> > > 
> > > SKB_DROP_REASON_MAC_IEEE_MAC_CONTROL - br_handle_frame()
> > > packit -t UDP -s 192.168.0.1 -d 192.168.0.2 -S 8000 -D 8000 \
> > >   -e 02:22:33:44:55:66 -E 01:80:c2:00:00:01 -c 1 \
> > >   -p '0x de ad be ef' -i xeth0
> > > 
> > > SKB_DROP_REASON_BRIDGE_INGRESS_PORT_NFWD - br_handle_frame()
> > > bridge link set dev veth0 state 0 # disabled
> > > packit -t UDP -s 192.168.0.1 -d 192.168.0.2 -S 8000 -D 8000 \
> > >   -e 02:22:33:44:55:66 -E aa:bb:cc:dd:ee:ff -c 1 \
> > >   -p '0x de ad be ef' -i xeth0
> > > 
> > > SKB_DROP_REASON_BRIDGE_INGRESS_PORT_NFWD - br_handle_frame_finish()
> > > bridge link set dev veth0 state 2 # learning
> > > packit -t UDP -s 192.168.0.1 -d 192.168.0.2 -S 8000 -D 8000 \
> > >   -e 02:22:33:44:55:66 -E aa:bb:cc:dd:ee:ff -c 1 \
> > >   -p '0x de ad be ef' -i xeth0
> > > 
> > > SKB_DROP_REASON_BRIDGE_NO_EGRESS_PORT - br_flood()
> > > packit -t UDP -s 192.168.0.1 -d 192.168.0.2 -S 8000 -D 8000 \
> > >   -e 02:22:33:44:55:66 -E aa:bb:cc:dd:ee:ff -c 1 \
> > >   -p '0x de ad be ef' -i xeth0
> > > 
> > > Signed-off-by: Radu Rendec <rrendec@redhat.com>
> > > ---
> > >  include/net/dropreason-core.h | 18 ++++++++++++++++++
> > >  net/bridge/br_forward.c       |  4 ++--
> > >  net/bridge/br_input.c         | 24 +++++++++++++++---------
> > >  3 files changed, 35 insertions(+), 11 deletions(-)
> > > 
> > 
> > Hi,
> > Thanks for working on this, a few comments below.
> 
> Sure, thanks for reviewing! Please see my comments below.
> 
> > > diff --git a/include/net/dropreason-core.h b/include/net/dropreason-core.h
> > > index c29282fabae6..1f2ae5b387c1 100644
> > > --- a/include/net/dropreason-core.h
> > > +++ b/include/net/dropreason-core.h
> > > @@ -108,6 +108,9 @@
> > >  	FN(TUNNEL_TXINFO)		\
> > >  	FN(LOCAL_MAC)			\
> > >  	FN(ARP_PVLAN_DISABLE)		\
> > > +	FN(MAC_IEEE_MAC_CONTROL)	\
> > > +	FN(BRIDGE_INGRESS_PORT_NFWD)	\
> > > +	FN(BRIDGE_NO_EGRESS_PORT)	\
> > >  	FNe(MAX)
> > >  
> > >  /**
> > > @@ -502,6 +505,21 @@ enum skb_drop_reason {
> > >  	 * enabled.
> > >  	 */
> > >  	SKB_DROP_REASON_ARP_PVLAN_DISABLE,
> > > +	/**
> > > +	 * @SKB_DROP_REASON_MAC_IEEE_MAC_CONTROL: the destination MAC address
> > > +	 * is an IEEE MAC Control address.
> > > +	 */
> > > +	SKB_DROP_REASON_MAC_IEEE_MAC_CONTROL,
> > > +	/**
> > > +	 * @SKB_DROP_REASON_BRIDGE_INGRESS_PORT_NFWD: the STP state of the
> > > +	 * ingress bridge port does not allow frames to be forwarded.
> > > +	 */
> > > +	SKB_DROP_REASON_BRIDGE_INGRESS_PORT_NFWD,
> > 
> > Since this is used only when the port state causes the packet to drop, why not
> > rename it to something that suggests it was the state?
> 
> Yes, Ido had a similar suggestion [1], so it's clear that it must be
> renamed. I will go with SKB_DROP_REASON_BRIDGE_INGRESS_STP_STATE in the
> next version, unless you have a better idea.
> 
> > > +	/**
> > > +	 * SKB_DROP_REASON_BRIDGE_NO_EGRESS_PORT: no eligible egress port was
> > > +	 * found while attempting to flood the frame.
> > > +	 */
> > > +	SKB_DROP_REASON_BRIDGE_NO_EGRESS_PORT,
> > >  	/**
> > >  	 * @SKB_DROP_REASON_MAX: the maximum of core drop reasons, which
> > >  	 * shouldn't be used as a real 'reason' - only for tracing code gen
> > > diff --git a/net/bridge/br_forward.c b/net/bridge/br_forward.c
> > > index e19b583ff2c6..e33e2f4fc3d9 100644
> > > --- a/net/bridge/br_forward.c
> > > +++ b/net/bridge/br_forward.c
> > > @@ -249,7 +249,7 @@ void br_flood(struct net_bridge *br, struct sk_buff *skb,
> > >  
> > >  out:
> > >  	if (!local_rcv)
> > > -		kfree_skb(skb);
> > > +		kfree_skb_reason(skb, SKB_DROP_REASON_BRIDGE_NO_EGRESS_PORT);
> > 
> > This is not entirely correct, we can get here if we had an error forwarding
> > the packet to some port, but it may already have been forwarded to others.
> > The reason should distinguish between those two cases.
> 
> I will follow Ido's suggestion [2] and rename SKB_DROP_REASON_VXLAN_NO_REMOTE
> to SKB_DROP_REASON_NO_TX_TARGET, and then use that.
> 
> But it will only cover the case when there are no errors, so I still
> need a different reason for the error case. I looked, and I couldn't
> find an existing one that's close enough, so I think I should create a
> new one. How about SKB_DROP_REASON_TX_ERROR? I would not use "BRIDGE"
> in the name because I'm thinking it may be reused elsewhere, outside
> the bridge module.

AFAICT the only possible error is skb_clone() failure and this is
supposed to be covered by 'SKB_DROP_REASON_NOMEM'.

> 
> > >  }
> > >  
> > >  #ifdef CONFIG_BRIDGE_IGMP_SNOOPING
> > > @@ -349,6 +349,6 @@ void br_multicast_flood(struct net_bridge_mdb_entry *mdst,
> > >  
> > >  out:
> > >  	if (!local_rcv)
> > > -		kfree_skb(skb);
> > > +		kfree_skb_reason(skb, SKB_DROP_REASON_BRIDGE_NO_EGRESS_PORT);
> > 
> > Same comment as above (br_flood).
> > 
> > >  }
> > >  #endif
> > > diff --git a/net/bridge/br_input.c b/net/bridge/br_input.c
> > > index ceaa5a89b947..fc00e172e1e1 100644
> > > --- a/net/bridge/br_input.c
> > > +++ b/net/bridge/br_input.c
> > > @@ -96,8 +96,10 @@ int br_handle_frame_finish(struct net *net, struct sock *sk, struct sk_buff *skb
> > >  	if (br_mst_is_enabled(br)) {
> > >  		state = BR_STATE_FORWARDING;
> > >  	} else {
> > > -		if (p->state == BR_STATE_DISABLED)
> > > -			goto drop;
> > > +		if (p->state == BR_STATE_DISABLED) {
> > > +			kfree_skb_reason(skb, SKB_DROP_REASON_BRIDGE_INGRESS_PORT_NFWD);
> > > +			return 0;
> > > +		}
> > >  
> > >  		state = p->state;
> > >  	}
> > > @@ -155,8 +157,10 @@ int br_handle_frame_finish(struct net *net, struct sock *sk, struct sk_buff *skb
> > >  		}
> > >  	}
> > >  
> > > -	if (state == BR_STATE_LEARNING)
> > > -		goto drop;
> > > +	if (state == BR_STATE_LEARNING) {
> > > +		kfree_skb_reason(skb, SKB_DROP_REASON_BRIDGE_INGRESS_PORT_NFWD);
> > > +		return 0;
> > > +	}>  
> > >  	BR_INPUT_SKB_CB(skb)->brdev = br->dev;
> > >  	BR_INPUT_SKB_CB(skb)->src_port_isolated = !!(p->flags & BR_ISOLATED);
> > > @@ -331,8 +335,10 @@ static rx_handler_result_t br_handle_frame(struct sk_buff **pskb)
> > >  	if (unlikely(skb->pkt_type == PACKET_LOOPBACK))
> > >  		return RX_HANDLER_PASS;
> > >  
> > > -	if (!is_valid_ether_addr(eth_hdr(skb)->h_source))
> > > -		goto drop;
> > > +	if (!is_valid_ether_addr(eth_hdr(skb)->h_source)) {
> > > +		kfree_skb_reason(skb, SKB_DROP_REASON_MAC_INVALID_SOURCE);
> > > +		return RX_HANDLER_CONSUMED;
> > > +	}
> > >  
> > >  	skb = skb_share_check(skb, GFP_ATOMIC);
> > >  	if (!skb)
> > > @@ -374,7 +380,8 @@ static rx_handler_result_t br_handle_frame(struct sk_buff **pskb)
> > >  			return RX_HANDLER_PASS;
> > >  
> > >  		case 0x01:	/* IEEE MAC (Pause) */
> > > -			goto drop;
> > > +			kfree_skb_reason(skb, SKB_DROP_REASON_MAC_IEEE_MAC_CONTROL);
> > > +			return RX_HANDLER_CONSUMED;
> > >  
> > >  		case 0x0E:	/* 802.1AB LLDP */
> > >  			fwd_mask |= p->br->group_fwd_mask;
> > > @@ -423,8 +430,7 @@ static rx_handler_result_t br_handle_frame(struct sk_buff **pskb)
> > >  
> > >  		return nf_hook_bridge_pre(skb, pskb);
> > >  	default:
> > > -drop:
> > > -		kfree_skb(skb);
> > > +		kfree_skb_reason(skb, SKB_DROP_REASON_BRIDGE_INGRESS_PORT_NFWD);
> > >  	}
> > >  	return RX_HANDLER_CONSUMED;
> > >  }
> 
> [1] https://lore.kernel.org/bridge/Z1sLyqZQCjbcCOde@shredder/
> [2] https://lore.kernel.org/bridge/Z1sUsSFfBC9GoiIA@shredder/
> 
> --
> Best regards,
> Radu
>