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. > + 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 > >
