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

[Ido Schimmel <idosch@idosch.org>]

On Tue, Dec 10, 2024 at 11:18:06AM +0200, Nikolay Aleksandrov wrote:
> On 12/9/24 00:18, Radu Rendec wrote:
> > +	/**
> > +	 * 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);
> 
> This is not entirely correct, we can get here if we had an error forwarding
> the packet to some port, but it may already have been forwarded to others.
> The reason should distinguish between those two cases.

Regarding 'SKB_DROP_REASON_BRIDGE_NO_EGRESS_PORT', there is a similar
reason in VXLAN called 'SKB_DROP_REASON_VXLAN_NO_REMOTE' which basically
means the same thing. Maybe we can rename it to
'SKB_DROP_REASON_NO_TX_TARGET' (or something similar) and reuse it here?

> 
> >  }
> >  
> >  #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);
> 
> Same comment as above (br_flood).
> 
> >  }
> >  #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;
> >  }
> 
> Cheers,
>  Nik
> 
>