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
>
