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

[PATCH net-next v2 0/2] 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 series 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 series 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_NOMEM in br_flood(). It's not easy to trigger an OOM
  condition for testing purposes, while everything else works correctly.
* All drop reasons in br_multicast_flood(). I could not find an easy way
  to make a crafted packet get there.
* SKB_DROP_REASON_BRIDGE_INGRESS_STP_STATE 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_STP_STATE - 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_STP_STATE - 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_NO_TX_TARGET - 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

Changes since v1:
- Add patch #1, which makes it possible to reuse the existing
  SKB_DROP_REASON_VXLAN_NO_REMOTE drop reason from vxlan in the bridge
  module, where a similar drop case exists.
- Don't add SKB_DROP_REASON_BRIDGE_NO_EGRESS_PORT. Instead, use either
  SKB_DROP_REASON_NO_TX_TARGET or SKB_DROP_REASON_NOMEM, depending on
  the case.
- For better clarity, rename SKB_DROP_REASON_BRIDGE_INGRESS_PORT_NFWD to
  SKB_DROP_REASON_BRIDGE_INGRESS_STP_STATE.

Radu Rendec (2):
  net: vxlan: rename SKB_DROP_REASON_VXLAN_NO_REMOTE
  net: bridge: add skb drop reasons to the most common drop points

 drivers/net/vxlan/vxlan_core.c |  4 ++--
 drivers/net/vxlan/vxlan_mdb.c  |  2 +-
 include/net/dropreason-core.h  | 18 +++++++++++++++---
 net/bridge/br_forward.c        | 16 ++++++++++++----
 net/bridge/br_input.c          | 24 +++++++++++++++---------
 5 files changed, 45 insertions(+), 19 deletions(-)

-- 
2.47.1

[PATCH net-next v2 1/2] net: vxlan: rename SKB_DROP_REASON_VXLAN_NO_REMOTE

[Radu Rendec]

The SKB_DROP_REASON_VXLAN_NO_REMOTE skb drop reason was introduced in
the specific context of vxlan. As it turns out, there are similar cases
when a packet needs to be dropped in other parts of the network stack,
such as the bridge module.

Rename SKB_DROP_REASON_VXLAN_NO_REMOTE and give it a more generic name,
so that it can be used in other parts of the network stack. This is not
a functional change, and the numeric value of the drop reason even
remains unchanged.

Signed-off-by: Radu Rendec <rrendec@redhat.com>
---
 drivers/net/vxlan/vxlan_core.c | 4 ++--
 drivers/net/vxlan/vxlan_mdb.c  | 2 +-
 include/net/dropreason-core.h  | 6 +++---
 3 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/net/vxlan/vxlan_core.c b/drivers/net/vxlan/vxlan_core.c
index 0c356e0a61ef0..05c10acb2a57e 100644
--- a/drivers/net/vxlan/vxlan_core.c
+++ b/drivers/net/vxlan/vxlan_core.c
@@ -2798,7 +2798,7 @@ static netdev_tx_t vxlan_xmit(struct sk_buff *skb, struct net_device *dev)
 			dev_dstats_tx_dropped(dev);
 			vxlan_vnifilter_count(vxlan, vni, NULL,
 					      VXLAN_VNI_STATS_TX_DROPS, 0);
-			kfree_skb_reason(skb, SKB_DROP_REASON_VXLAN_NO_REMOTE);
+			kfree_skb_reason(skb, SKB_DROP_REASON_NO_TX_TARGET);
 			return NETDEV_TX_OK;
 		}
 	}
@@ -2821,7 +2821,7 @@ static netdev_tx_t vxlan_xmit(struct sk_buff *skb, struct net_device *dev)
 		if (fdst)
 			vxlan_xmit_one(skb, dev, vni, fdst, did_rsc);
 		else
-			kfree_skb_reason(skb, SKB_DROP_REASON_VXLAN_NO_REMOTE);
+			kfree_skb_reason(skb, SKB_DROP_REASON_NO_TX_TARGET);
 	}
 
 	return NETDEV_TX_OK;
diff --git a/drivers/net/vxlan/vxlan_mdb.c b/drivers/net/vxlan/vxlan_mdb.c
index 8735891ee1286..816ab1aa05262 100644
--- a/drivers/net/vxlan/vxlan_mdb.c
+++ b/drivers/net/vxlan/vxlan_mdb.c
@@ -1712,7 +1712,7 @@ netdev_tx_t vxlan_mdb_xmit(struct vxlan_dev *vxlan,
 		vxlan_xmit_one(skb, vxlan->dev, src_vni,
 			       rcu_dereference(fremote->rd), false);
 	else
-		kfree_skb_reason(skb, SKB_DROP_REASON_VXLAN_NO_REMOTE);
+		kfree_skb_reason(skb, SKB_DROP_REASON_NO_TX_TARGET);
 
 	return NETDEV_TX_OK;
 }
diff --git a/include/net/dropreason-core.h b/include/net/dropreason-core.h
index ead4170a1d0a9..be58c97c64a1b 100644
--- a/include/net/dropreason-core.h
+++ b/include/net/dropreason-core.h
@@ -106,7 +106,7 @@
 	FN(VXLAN_VNI_NOT_FOUND)		\
 	FN(MAC_INVALID_SOURCE)		\
 	FN(VXLAN_ENTRY_EXISTS)		\
-	FN(VXLAN_NO_REMOTE)		\
+	FN(NO_TX_TARGET)		\
 	FN(IP_TUNNEL_ECN)		\
 	FN(TUNNEL_TXINFO)		\
 	FN(LOCAL_MAC)			\
@@ -497,8 +497,8 @@ enum skb_drop_reason {
 	 * entry or an entry pointing to a nexthop.
 	 */
 	SKB_DROP_REASON_VXLAN_ENTRY_EXISTS,
-	/** @SKB_DROP_REASON_VXLAN_NO_REMOTE: no remote found for xmit */
-	SKB_DROP_REASON_VXLAN_NO_REMOTE,
+	/** @SKB_DROP_REASON_NO_TX_TARGET: no remote found for xmit */
+	SKB_DROP_REASON_NO_TX_TARGET,
 	/**
 	 * @SKB_DROP_REASON_IP_TUNNEL_ECN: skb is dropped according to
 	 * RFC 6040 4.2, see __INET_ECN_decapsulate() for detail.
-- 
2.47.1

Re: [PATCH net-next v2 1/2] net: vxlan: rename SKB_DROP_REASON_VXLAN_NO_REMOTE

[Ido Schimmel]

On Tue, Dec 17, 2024 at 06:07:10PM -0500, Radu Rendec wrote:
> @@ -497,8 +497,8 @@ enum skb_drop_reason {
>  	 * entry or an entry pointing to a nexthop.
>  	 */
>  	SKB_DROP_REASON_VXLAN_ENTRY_EXISTS,
> -	/** @SKB_DROP_REASON_VXLAN_NO_REMOTE: no remote found for xmit */
> -	SKB_DROP_REASON_VXLAN_NO_REMOTE,
> +	/** @SKB_DROP_REASON_NO_TX_TARGET: no remote found for xmit */

Looks good, but I suggest replacing "remote" with "target" since
"remote" is VXLAN specific.

> +	SKB_DROP_REASON_NO_TX_TARGET,
>  	/**
>  	 * @SKB_DROP_REASON_IP_TUNNEL_ECN: skb is dropped according to
>  	 * RFC 6040 4.2, see __INET_ECN_decapsulate() for detail.
> -- 
> 2.47.1
> 

[PATCH net-next v2 2/2] 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.

Drop reasons are not added exhaustively to the entire bridge code. The
intention is to incrementally add drop reasons to the rest of the bridge
code in follow up patches.

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

diff --git a/include/net/dropreason-core.h b/include/net/dropreason-core.h
index be58c97c64a1b..eeb7c67586431 100644
--- a/include/net/dropreason-core.h
+++ b/include/net/dropreason-core.h
@@ -111,6 +111,8 @@
 	FN(TUNNEL_TXINFO)		\
 	FN(LOCAL_MAC)			\
 	FN(ARP_PVLAN_DISABLE)		\
+	FN(MAC_IEEE_MAC_CONTROL)	\
+	FN(BRIDGE_INGRESS_STP_STATE)	\
 	FNe(MAX)
 
 /**
@@ -520,6 +522,16 @@ 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_STP_STATE,
 	/**
 	 * @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 e19b583ff2c6d..3e9b462809b0e 100644
--- a/net/bridge/br_forward.c
+++ b/net/bridge/br_forward.c
@@ -201,6 +201,7 @@ void br_flood(struct net_bridge *br, struct sk_buff *skb,
 	      enum br_pkt_type pkt_type, bool local_rcv, bool local_orig,
 	      u16 vid)
 {
+	enum skb_drop_reason reason = SKB_DROP_REASON_NO_TX_TARGET;
 	struct net_bridge_port *prev = NULL;
 	struct net_bridge_port *p;
 
@@ -234,8 +235,11 @@ void br_flood(struct net_bridge *br, struct sk_buff *skb,
 			continue;
 
 		prev = maybe_deliver(prev, p, skb, local_orig);
-		if (IS_ERR(prev))
+		if (IS_ERR(prev)) {
+			WARN_ON_ONCE(PTR_ERR(prev) != -ENOMEM);
+			reason = SKB_DROP_REASON_NOMEM;
 			goto out;
+		}
 	}
 
 	if (!prev)
@@ -249,7 +253,7 @@ void br_flood(struct net_bridge *br, struct sk_buff *skb,
 
 out:
 	if (!local_rcv)
-		kfree_skb(skb);
+		kfree_skb_reason(skb, reason);
 }
 
 #ifdef CONFIG_BRIDGE_IGMP_SNOOPING
@@ -289,6 +293,7 @@ void br_multicast_flood(struct net_bridge_mdb_entry *mdst,
 			struct net_bridge_mcast *brmctx,
 			bool local_rcv, bool local_orig)
 {
+	enum skb_drop_reason reason = SKB_DROP_REASON_NO_TX_TARGET;
 	struct net_bridge_port *prev = NULL;
 	struct net_bridge_port_group *p;
 	bool allow_mode_include = true;
@@ -329,8 +334,11 @@ void br_multicast_flood(struct net_bridge_mdb_entry *mdst,
 		}
 
 		prev = maybe_deliver(prev, port, skb, local_orig);
-		if (IS_ERR(prev))
+		if (IS_ERR(prev)) {
+			WARN_ON_ONCE(PTR_ERR(prev) != -ENOMEM);
+			reason = SKB_DROP_REASON_NOMEM;
 			goto out;
+		}
 delivered:
 		if ((unsigned long)lport >= (unsigned long)port)
 			p = rcu_dereference(p->next);
@@ -349,6 +357,6 @@ void br_multicast_flood(struct net_bridge_mdb_entry *mdst,
 
 out:
 	if (!local_rcv)
-		kfree_skb(skb);
+		kfree_skb_reason(skb, reason);
 }
 #endif
diff --git a/net/bridge/br_input.c b/net/bridge/br_input.c
index ceaa5a89b947f..0adad3986c77d 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_STP_STATE);
+			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_STP_STATE);
+		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_STP_STATE);
 	}
 	return RX_HANDLER_CONSUMED;
 }
-- 
2.47.1

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

[Ido Schimmel]

On Tue, Dec 17, 2024 at 06:07:11PM -0500, Radu Rendec wrote:
> @@ -520,6 +522,16 @@ 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

s/SKB_DROP_REASON_BRIDGE_INGRESS_PORT_NFWD/SKB_DROP_REASON_BRIDGE_INGRESS_STP_STATE/

> +	 * ingress bridge port does not allow frames to be forwarded.
> +	 */
> +	SKB_DROP_REASON_BRIDGE_INGRESS_STP_STATE,
>  	/**
>  	 * @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 e19b583ff2c6d..3e9b462809b0e 100644
> --- a/net/bridge/br_forward.c
> +++ b/net/bridge/br_forward.c
> @@ -201,6 +201,7 @@ void br_flood(struct net_bridge *br, struct sk_buff *skb,
>  	      enum br_pkt_type pkt_type, bool local_rcv, bool local_orig,
>  	      u16 vid)
>  {
> +	enum skb_drop_reason reason = SKB_DROP_REASON_NO_TX_TARGET;
>  	struct net_bridge_port *prev = NULL;
>  	struct net_bridge_port *p;
>  
> @@ -234,8 +235,11 @@ void br_flood(struct net_bridge *br, struct sk_buff *skb,
>  			continue;
>  
>  		prev = maybe_deliver(prev, p, skb, local_orig);
> -		if (IS_ERR(prev))
> +		if (IS_ERR(prev)) {
> +			WARN_ON_ONCE(PTR_ERR(prev) != -ENOMEM);

I don't think we want to see a stack trace just because someone forgot
to adjust the drop reason to the error code. Maybe just set it to
'NOMEM' if error code is '-ENOMEM', otherwise to 'NOT_SPECIFIED'.

> +			reason = SKB_DROP_REASON_NOMEM;
>  			goto out;
> +		}
>  	}
>  
>  	if (!prev)
> @@ -249,7 +253,7 @@ void br_flood(struct net_bridge *br, struct sk_buff *skb,
>  
>  out:
>  	if (!local_rcv)
> -		kfree_skb(skb);
> +		kfree_skb_reason(skb, reason);
>  }
>  
>  #ifdef CONFIG_BRIDGE_IGMP_SNOOPING
> @@ -289,6 +293,7 @@ void br_multicast_flood(struct net_bridge_mdb_entry *mdst,
>  			struct net_bridge_mcast *brmctx,
>  			bool local_rcv, bool local_orig)
>  {
> +	enum skb_drop_reason reason = SKB_DROP_REASON_NO_TX_TARGET;
>  	struct net_bridge_port *prev = NULL;
>  	struct net_bridge_port_group *p;
>  	bool allow_mode_include = true;
> @@ -329,8 +334,11 @@ void br_multicast_flood(struct net_bridge_mdb_entry *mdst,
>  		}
>  
>  		prev = maybe_deliver(prev, port, skb, local_orig);
> -		if (IS_ERR(prev))
> +		if (IS_ERR(prev)) {
> +			WARN_ON_ONCE(PTR_ERR(prev) != -ENOMEM);

Likewise

> +			reason = SKB_DROP_REASON_NOMEM;
>  			goto out;
> +		}
>  delivered:
>  		if ((unsigned long)lport >= (unsigned long)port)
>  			p = rcu_dereference(p->next);
> @@ -349,6 +357,6 @@ void br_multicast_flood(struct net_bridge_mdb_entry *mdst,
>  
>  out:
>  	if (!local_rcv)
> -		kfree_skb(skb);
> +		kfree_skb_reason(skb, reason);
>  }
>  #endif
> diff --git a/net/bridge/br_input.c b/net/bridge/br_input.c
> index ceaa5a89b947f..0adad3986c77d 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_STP_STATE);
> +			return 0;
> +		}

It would be good to keep the error path consolidated with 'goto drop' in
case we ever want to increment a drop counter or do something else that
is common to all the drops.

Did you consider adding a 'reason' variable that is initialized to
'SKB_DROP_REASON_NOT_SPECIFIED' and setting it to the appropriate reason
before 'goto drop'? Seems like a common pattern.

Same in br_handle_frame().

>  
>  		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_STP_STATE);
> +		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_STP_STATE);
>  	}
>  	return RX_HANDLER_CONSUMED;
>  }
> -- 
> 2.47.1
> 

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

[Radu Rendec]

On Wed, 2024-12-18 at 19:19 +0200, Ido Schimmel wrote:
> On Tue, Dec 17, 2024 at 06:07:11PM -0500, Radu Rendec wrote:
> > @@ -520,6 +522,16 @@ 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
> 
> s/SKB_DROP_REASON_BRIDGE_INGRESS_PORT_NFWD/SKB_DROP_REASON_BRIDGE_INGRESS_STP_STATE/

Oops :) Good catch!

> > +	 * ingress bridge port does not allow frames to be forwarded.
> > +	 */
> > +	SKB_DROP_REASON_BRIDGE_INGRESS_STP_STATE,
> >  	/**
> >  	 * @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 e19b583ff2c6d..3e9b462809b0e 100644
> > --- a/net/bridge/br_forward.c
> > +++ b/net/bridge/br_forward.c
> > @@ -201,6 +201,7 @@ void br_flood(struct net_bridge *br, struct sk_buff *skb,
> >  	      enum br_pkt_type pkt_type, bool local_rcv, bool local_orig,
> >  	      u16 vid)
> >  {
> > +	enum skb_drop_reason reason = SKB_DROP_REASON_NO_TX_TARGET;
> >  	struct net_bridge_port *prev = NULL;
> >  	struct net_bridge_port *p;
> >  
> > @@ -234,8 +235,11 @@ void br_flood(struct net_bridge *br, struct sk_buff *skb,
> >  			continue;
> >  
> >  		prev = maybe_deliver(prev, p, skb, local_orig);
> > -		if (IS_ERR(prev))
> > +		if (IS_ERR(prev)) {
> > +			WARN_ON_ONCE(PTR_ERR(prev) != -ENOMEM);
> 
> I don't think we want to see a stack trace just because someone forgot
> to adjust the drop reason to the error code. Maybe just set it to
> 'NOMEM' if error code is '-ENOMEM', otherwise to 'NOT_SPECIFIED'.

Sure, that was my first choice too, but then I changed my mind. I don't
think there's a 100% clean way of doing this because maybe_deliver()
can return only -ENOMEM today, but that may change in the future. I
will change it back to what I had initially, which is essentially the
same as you suggested.

> > +			reason = SKB_DROP_REASON_NOMEM;
> >  			goto out;
> > +		}
> >  	}
> >  
> >  	if (!prev)
> > @@ -249,7 +253,7 @@ void br_flood(struct net_bridge *br, struct sk_buff *skb,
> >  
> >  out:
> >  	if (!local_rcv)
> > -		kfree_skb(skb);
> > +		kfree_skb_reason(skb, reason);
> >  }
> >  
> >  #ifdef CONFIG_BRIDGE_IGMP_SNOOPING
> > @@ -289,6 +293,7 @@ void br_multicast_flood(struct net_bridge_mdb_entry *mdst,
> >  			struct net_bridge_mcast *brmctx,
> >  			bool local_rcv, bool local_orig)
> >  {
> > +	enum skb_drop_reason reason = SKB_DROP_REASON_NO_TX_TARGET;
> >  	struct net_bridge_port *prev = NULL;
> >  	struct net_bridge_port_group *p;
> >  	bool allow_mode_include = true;
> > @@ -329,8 +334,11 @@ void br_multicast_flood(struct net_bridge_mdb_entry *mdst,
> >  		}
> >  
> >  		prev = maybe_deliver(prev, port, skb, local_orig);
> > -		if (IS_ERR(prev))
> > +		if (IS_ERR(prev)) {
> > +			WARN_ON_ONCE(PTR_ERR(prev) != -ENOMEM);
> 
> Likewise
> 
> > +			reason = SKB_DROP_REASON_NOMEM;
> >  			goto out;
> > +		}
> >  delivered:
> >  		if ((unsigned long)lport >= (unsigned long)port)
> >  			p = rcu_dereference(p->next);
> > @@ -349,6 +357,6 @@ void br_multicast_flood(struct net_bridge_mdb_entry *mdst,
> >  
> >  out:
> >  	if (!local_rcv)
> > -		kfree_skb(skb);
> > +		kfree_skb_reason(skb, reason);
> >  }
> >  #endif
> > diff --git a/net/bridge/br_input.c b/net/bridge/br_input.c
> > index ceaa5a89b947f..0adad3986c77d 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_STP_STATE);
> > +			return 0;
> > +		}
> 
> It would be good to keep the error path consolidated with 'goto drop' in
> case we ever want to increment a drop counter or do something else that
> is common to all the drops.
> 
> Did you consider adding a 'reason' variable that is initialized to
> 'SKB_DROP_REASON_NOT_SPECIFIED' and setting it to the appropriate reason
> before 'goto drop'? Seems like a common pattern.

I did not consider it because I didn't realize there was an intention
to keep the error path consolidated. I did that for the two "flood"
functions though. And now that you explained it, I can see why you'd
want to do it here. I will refactor it and post v3 soon if I don't see
any new comments on v2.

> Same in br_handle_frame().
> 
> >  
> >  		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_STP_STATE);
> > +		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_STP_STATE);
> >  	}
> >  	return RX_HANDLER_CONSUMED;
> >  }
> > -- 
> > 2.47.1
> > 
> 

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

[Nikolay Aleksandrov]

On 12/18/24 19:19, Ido Schimmel wrote:
> On Tue, Dec 17, 2024 at 06:07:11PM -0500, Radu Rendec wrote:
>> @@ -520,6 +522,16 @@ 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
> 
> s/SKB_DROP_REASON_BRIDGE_INGRESS_PORT_NFWD/SKB_DROP_REASON_BRIDGE_INGRESS_STP_STATE/
> 
>> +	 * ingress bridge port does not allow frames to be forwarded.
>> +	 */
>> +	SKB_DROP_REASON_BRIDGE_INGRESS_STP_STATE,
>>  	/**
>>  	 * @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 e19b583ff2c6d..3e9b462809b0e 100644
>> --- a/net/bridge/br_forward.c
>> +++ b/net/bridge/br_forward.c
>> @@ -201,6 +201,7 @@ void br_flood(struct net_bridge *br, struct sk_buff *skb,
>>  	      enum br_pkt_type pkt_type, bool local_rcv, bool local_orig,
>>  	      u16 vid)
>>  {
>> +	enum skb_drop_reason reason = SKB_DROP_REASON_NO_TX_TARGET;
>>  	struct net_bridge_port *prev = NULL;
>>  	struct net_bridge_port *p;
>>  
>> @@ -234,8 +235,11 @@ void br_flood(struct net_bridge *br, struct sk_buff *skb,
>>  			continue;
>>  
>>  		prev = maybe_deliver(prev, p, skb, local_orig);
>> -		if (IS_ERR(prev))
>> +		if (IS_ERR(prev)) {
>> +			WARN_ON_ONCE(PTR_ERR(prev) != -ENOMEM);
> 
> I don't think we want to see a stack trace just because someone forgot
> to adjust the drop reason to the error code. Maybe just set it to
> 'NOMEM' if error code is '-ENOMEM', otherwise to 'NOT_SPECIFIED'.
> 

+1