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@xxxxxxxxxx> > > > --- > > > 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^
