DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 1B86D20827
Message-ID: <f9be46875c90f00a79f03838944bbf23f05d2c0d.camel@sipsolutions.net>
Subject: Re: [PATCH 3/3] mac80211: Implement functionality to monitor
 station's signal stregnth
From:   Johannes Berg <johannes@sipsolutions.net>
To:     Tamizh chelvam <tamizhr@codeaurora.org>
Cc:     linux-wireless@vger.kernel.org
Date:   Fri, 09 Nov 2018 12:55:22 +0100
In-Reply-To: <1539626250-769-4-git-send-email-tamizhr@codeaurora.org>
References: <1539626250-769-1-git-send-email-tamizhr@codeaurora.org>
         <1539626250-769-4-git-send-email-tamizhr@codeaurora.org>
Content-Type: text/plain; charset="UTF-8"
Mime-Version: 1.0
Content-Transfer-Encoding: 7bit
Sender: linux-wireless-owner@vger.kernel.org
Precedence: bulk

Oh, umm, that patch is still here ...

I guess we can combine 2 and 3 too.


> +	if (sta->rssi_low && bss_conf->enable_beacon) {
> +		int last_event =
> +			sta->last_rssi_event_value;
> +		int sig = -ewma_signal_read(&sta->rx_stats_avg.signal);
> +		int low = sta->rssi_low;
> +		int high = sta->rssi_high;

This doesn't really support a list, like in patch 2 where you store
sta_info::rssi_tholds?

> +		if (sig < low &&
> +		    (last_event == 0 || last_event >= low)) {
> +			sta->last_rssi_event_value = sig;
> +			cfg80211_sta_mon_rssi_notify(
> +				rx->sdata->dev, sta->addr,
> +				NL80211_CQM_RSSI_THRESHOLD_EVENT_LOW,
> +				sig, GFP_ATOMIC);
> +			rssi_cross = true;
> +		} else if (sig > high &&
> +			   (last_event == 0 || last_event <= high)) {
> +			sta->last_rssi_event_value = sig;
> +			cfg80211_sta_mon_rssi_notify(
> +				rx->sdata->dev, sta->addr,
> +				NL80211_CQM_RSSI_THRESHOLD_EVENT_HIGH,
> +				sig, GFP_ATOMIC);
> +			rssi_cross = true;
> +		}
> +	}
> +
> +	if (rssi_cross) {
> +		ieee80211_update_rssi_config(sta);
> +		rssi_cross = false;
> +	}
> +}

Ah, but it does, just hard to understand.

However, this does mean what I suspected on the other patch is true -
you're calling ieee80211_update_rssi_config() here, and that uses the
sta->rssi_tholds array without any protection, while a concurrent change
of configuration can free the data.

You need to sort that out. I would suggest to stick all the sta->rssi_*
fields you add into a new struct (you even had an empty one), and
protect that struct using RCU. That also saves the memory in case it's
not assigned at all. Something like

struct sta_mon_rssi_config {
	struct rcu_head rcu_head;
	int n_thresholds;
	s32 low, high;
	u32 hyst;
	s32 last_value;
	s32 thresholds[];
};

then you can kfree_rcu() it, and just do a single allocation using
struct_size() for however many entries you need.

> + * @count_rx_signal: Number of data frames used in averaging station signal.
> + *	This can be used to avoid generating less reliable station rssi cross
> + *	events that would be based only on couple of received frames.
>   */
>  struct sta_info {
>  	/* General information, mostly static */
> @@ -600,6 +603,7 @@ struct sta_info {
>  	s32 rssi_high;
>  	u32 rssi_hyst;
>  	s32 last_rssi_event_value;
> +	unsigned int count_rx_signal;

I guess that would also move into the new struct.

johannes