On 17 March 2016 at 12:20, Mohammed Shafi Shajakhan
<mohammed@xxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxxx> 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ł
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
