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

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:

quoted

On Sun, Dec 08, 2024 at 05:18:05PM -0500, Radu Rendec wrote:

quoted

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 <redacted>
---
 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^