Re: [RFC PATCH 2/2] mac80211: use the new drop reasons infrastructure

From: Johannes Berg <johannes@sipsolutions.net>
To: linux-wireless@vger.kernel.org, netdev@vger.kernel.org
Subject: Re: [RFC PATCH 2/2] mac80211: use the new drop reasons infrastructure
Date: Wed, 29 Mar 2023 23:56:31 +0200	[thread overview]
Message-ID: <34e43da3694e2d627555af0149ebe438e1ed2938.camel@sipsolutions.net> (raw)
In-Reply-To: <20230329234613.5bcb4d8dcade.Iea29d70af97ce2ed590a00dbebee2ab4d013dfd5@changeid>

Couple of comments that I didn't want to inline into the patch...

> 
> +	/** @SKB_DROP_REASON_SUBSYS_MAC80211_UNUSABLE: mac80211 drop reasons
> +	 * for unusable frames, see net/mac80211/drop.h

Not sure if that should be in net/mac80211/drop.h, or better
include/net/mac80211-drop.h or something.

> +static const char * const drop_reasons_monitor[] = {
> +#define V(x)	#x,
> +	[0] = "RX_DROP_MONITOR",
> +	MAC80211_DROP_REASONS_MONITOR(V)

We could, and perhaps should, add some prefix here, so the strings
become something like SKB_DROP_REASON_MAC80211_MONITOR_...

The only annoying thing with that is we'd probably then want to generate
the "RX_DROP_M_" prefix for the constants in the DEF() macros in the
header file, which might make elixir/ctags/... even worse - but it's
probably already pretty bad for it anyway.

> +	drop_reasons_unregister_subsys(SKB_DROP_REASON_SUBSYS_MAC80211_MONITOR);
> +	drop_reasons_unregister_subsys(SKB_DROP_REASON_SUBSYS_MAC80211_UNUSABLE);
> +
>  	rcu_barrier();

This is making me think that perhaps we don't want synchronize_rcu()
inside drop_reasons_unregister_subsys(), since I have two now and also
already have an rcu_barrier() ... so maybe just document that it's
needed?

> +++ b/net/mac80211/rx.c
> @@ -1826,7 +1826,7 @@ ieee80211_rx_h_sta_process(struct ieee80211_rx_data *rx)
>  				cfg80211_rx_unexpected_4addr_frame(
>  					rx->sdata->dev, sta->sta.addr,
>  					GFP_ATOMIC);
> -			return RX_DROP_MONITOR;
> +			return RX_DROP_M_4ADDR_NDP;

This was coded up too hastily, it should've been called
RX_DROP_M_UNEXPECTED_4ADDR.

> +++ b/net/mac80211/wpa.c
> @@ -550,7 +550,7 @@ ieee80211_crypto_ccmp_decrypt(struct ieee80211_rx_data *rx,
>  		if (res < 0 ||
>  		    (!res && !(status->flag & RX_FLAG_ALLOW_SAME_PN))) {
>  			key->u.ccmp.replays++;
> -			return RX_DROP_UNUSABLE;
> +			return RX_DROP_U_REPLAY;

I did wonder if we should distinguish CCMP/GCMP/... for replays, MIC
failures etc., but haven't really quite decided yet. With drop_monitor
you'd have the frame (I think?) and that makes it easy to see what it
was. It's also not really all that relevant for the drop reasons
infrastructure discussion.

johannes