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

[Radu Rendec <rrendec@redhat.com>]

On Wed, 2024-12-18 at 19:19 +0200, Ido Schimmel wrote:
> 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/

Oops :) Good catch!

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

Sure, that was my first choice too, but then I changed my mind. I don't
think there's a 100% clean way of doing this because maybe_deliver()
can return only -ENOMEM today, but that may change in the future. I
will change it back to what I had initially, which is essentially the
same as you suggested.

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

I did not consider it because I didn't realize there was an intention
to keep the error path consolidated. I did that for the two "flood"
functions though. And now that you explained it, I can see why you'd
want to do it here. I will refactor it and post v3 soon if I don't see
any new comments on v2.

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