[PATCH v2 1/2] ath10k: Add support for ath10k_sta_statistics support

[Michal Kazior]

On 17 March 2016 at 12:20, Mohammed Shafi Shajakhan
<mohammed at codeaurora.org> wrote:
> Hi Michal,
>
> On Thu, Mar 17, 2016 at 12:12:31PM +0100, Michal Kazior wrote:
>> On 17 March 2016 at 11:48, Mohammed Shafi Shajakhan
>> <mohammed at qti.qualcomm.com> wrote:
>> [...]
>> > +void ath10k_sta_statistics(struct ieee80211_hw *hw, struct ieee80211_vif *vif,
>> > +                          struct ieee80211_sta *sta,
>> > +                          struct station_info *sinfo)
>> > +{
>> > +       struct ath10k_sta *arsta = (struct ath10k_sta *)sta->drv_priv;
>> > +       struct ath10k *ar = arsta->arvif->ar;
>> > +
>> > +       mutex_lock(&ar->conf_mutex);
>> > +
>> > +       if (ar->state != ATH10K_STATE_ON &&
>> > +           ar->state != ATH10K_STATE_RESTARTED)
>> > +               goto out;
>>
>> Do you really need mutex and ar->state check in this function?
>>
>
> [shafi] By default peer stats will be disabled, we are enabling this by debugfs
> (hw-restart) so i thought these checks are needed , please advise .. Do you say
> they  will be never hit

Hmm.. I think mac80211 shouldn't call sta_statistics() before
sta_state() during recovery (it makes no sense). In practice I think
this isn't enforced in which case it's a mac80211 bug.

Anyway, this isn't much of a problem now. You only read out u64 from
`arsta` (sta->drv_priv). Even if it's uninitialized/undefined there's
no way for you to crash the system (it's not a list, spinlock or any
other complex data structure). Worst case userspace will get garbage
rx_duration value if it happens to get_station() while hw-restart is
ongoing.

It'd be good to verify this is actually a problem and - assuming you
want to guarantee correct readouts at all times - to fix the problem
in mac80211.


Michał