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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <34e43da3694e2d627555af0149ebe438e1ed2938.camel@sipsolutions.net>
Date:   Wed, 29 Mar 2023 23:56:31 +0200
From:   Johannes Berg <johannes@...solutions.net>
To:     linux-wireless@...r.kernel.org, netdev@...r.kernel.org
Subject: Re: [RFC PATCH 2/2] mac80211: use the new drop reasons
 infrastructure

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