linux-kernel - Re: [EXTERNAL] [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]

Message-ID: <e6ae6dfa-6554-4e88-abb0-31dbbd8df03f@ti.com>
Date: Thu, 30 May 2024 11:06:40 +0300
From: "Nemanov, Michael" <michael.nemanov@...com>
To: "Russell King (Oracle)" <rmk+kernel@...linux.org.uk>,
        Kalle Valo
	<kvalo@...nel.org>
CC: Johannes Berg <johannes.berg@...el.com>, <linux-kernel@...r.kernel.org>,
        <linux-wireless@...r.kernel.org>
Subject: Re: [EXTERNAL] [PATCH wireless-next 6/8] wifi: wlcore: add pn16
 support


On 5/28/2024 12:18 PM, Russell King (Oracle) wrote:

[...]

>    
>    static int wlcore_fw_status(struct wl1271 *wl, struct wl_fw_status *status)
>    {
> +	struct wl12xx_vif *wlvifsta;
> +	struct wl12xx_vif *wlvifap;
>    	struct wl12xx_vif *wlvif;
>    	u32 old_tx_blk_count = wl->tx_blocks_available;
>    	int avail, freed_blocks;
> @@ -410,23 +412,100 @@ static int wlcore_fw_status(struct wl1271 *wl, struct wl_fw_status *status)
>    		wl->tx_pkts_freed[i] = status->counters.tx_released_pkts[i];
>    	}
>    
[...]
>    	for_each_set_bit(i, wl->links_map, wl->num_links) {
> +		u16 diff16, sec_pn16;
>    		u8 diff, tx_lnk_free_pkts;
> +
>    		lnk = &wl->links[i];
>    
>    		/* prevent wrap-around in freed-packets counter */
>    		tx_lnk_free_pkts = status->counters.tx_lnk_free_pkts[i];
>    		diff = (tx_lnk_free_pkts - lnk->prev_freed_pkts) & 0xff;
>    
> -		if (diff == 0)
> +		if (diff) {
> +			lnk->allocated_pkts -= diff;
> +			lnk->prev_freed_pkts = tx_lnk_free_pkts;
> +		}
> +
> +		/* Get the current sec_pn16 value if present */
> +		if (status->counters.tx_lnk_sec_pn16)
> +			sec_pn16 = __le16_to_cpu(status->counters.tx_lnk_sec_pn16[i]);
> +		else
> +			sec_pn16 = 0;
> +		/* prevent wrap-around in pn16 counter */
> +		diff16 = (sec_pn16 - lnk->prev_sec_pn16) & 0xffff;
> +
> +		/* FIXME: since free_pkts is a 8-bit counter of packets that
> +		 * rolls over, it can become zero. If it is zero, then we
> +		 * omit processing below. Is that really correct?
> +		 */
> +		if (tx_lnk_free_pkts <= 0)
>    			continue;
>    
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?

Michael.