linux-kernel - Re: [PATCH wireless-next 6/8] wifi: wlcore: add pn16 support

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: Windows password security audit tool. GUI, reports in PDF.

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

Date: Thu, 30 May 2024 15:25:59 +0300
From: "Nemanov, Michael" <michael.nemanov@...com>
To: "Russell King (Oracle)" <linux@...linux.org.uk>
CC: Kalle Valo <kvalo@...nel.org>, Johannes Berg <johannes.berg@...el.com>,
        <linux-kernel@...r.kernel.org>, <linux-wireless@...r.kernel.org>
Subject: Re: [PATCH wireless-next 6/8] wifi: wlcore: add pn16 support



On 5/30/2024 11:20 AM, Russell King (Oracle) wrote:
[...]
> > The original code was > tx_lnk_free_pkts = 
> status->counters.tx_lnk_free_pkts[i]; > diff = (tx_lnk_free_pkts - 
> lnk->prev_freed_pkts) & 0xff; > > if (diff == 0) > continue; > > I 
> wonder if comparing tx_lnk_free_pkts to 0 was added intentionally? 
> This is > monotonously incremented counter so 0 is not significant, 
> unlike the diff. > Have I missed something? You are... While you're 
> correct about the original code, your quote is somewhat incomplete. + 
> if ( (isSta == true) && (i == wlvifSta->sta.hlid) && 
> (test_bit(WLVIF_FLAG_STA_AUTHORIZED, &wlvifSta->flags)) && 
> (status->counters.tx_lnk_free_pkts[i] > 0) ) ... + } + if ( (isAp == 
> true) && (test_bit(i, &wlvifAp->ap.sta_hlid_map[0])) && 
> (test_bit(WLVIF_FLAG_AP_STARTED, &wlvifAp->flags)) && 
> (wlvifAp->inconn_count == 0) && (status->counters.tx_lnk_free_pkts[i] 
> > 0) ) ... + } }

Sorry, considered only the diff with base branch, not the original patch.

> Note that both of these if() conditions can only be executed if the 
> final condition in each is true. Both check for the same thing, which 
> is: status->counters.tx_lnk_free_pkts[i] > 0 In my patch, 
> tx_lnk_free_pkts is status->counters.tx_lnk_free_pkts. Therefore, 
> there is no point in evaluating either of these excessively long if() 
> conditions in the original code when tx_lnk_free_pkts is less than 
> zero or zero - and thus the logic between TI's original patch and my 
> change is preserved. Whether that condition in the original patch is 
> correct or not is the subject of that FIXME comment - I believe TI's 
> code is incorrect, since it is possible that tx_lnk_free_pkts, which 
> is a u8 that is incremented by the number of free packets, will hit 
> zero at some point just as a matter of one extra packet being freed 
> when the counter was 255. Moving it out of those two if() statements 
> makes the issue very obvious. It would be nice to get a view from TI 
> on whether the original patch is actually correct in this regard. I 
> believe TI's original patch is buggy.

After consulting with the author I do not believe it is buggy. It was 
the most painless way to prevent issues with the recovery flow.
Indeed there will be case where tx_lnk_free_pkts is 0 again in which 
case the internal counters will not be updated. This was considered OK 
as this is usually a transient state and the counter will advance 
eventually.
For the unlikely case where FW crashed just after update was skipped, 
well, there will be a small rollback in the PN after recovery which 
means a few frames will get lost. This as considered acceptable.

Michael.