linux-kernel - Re: [PATCH 2/2] staging: rtl8192u: use min_t/max

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] [day] [month] [year] [list]

Date:   Thu, 7 Apr 2022 10:49:45 +0300
From:   Dan Carpenter <dan.carpenter@...cle.com>
To:     Rebecca Mckeever <remckee0@...il.com>
Cc:     outreachy@...ts.linux.dev,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        linux-staging@...ts.linux.dev, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 2/2] staging: rtl8192u: use min_t/max_t macros instead of
 if else

On Wed, Apr 06, 2022 at 07:09:44PM -0500, Rebecca Mckeever wrote:
> Replace if else statement with min_t or max_t macros to increase
> readability and conform to Linux kernel coding style. The _t versions
> of the macros must be used to avoid applying typeof to the bit fields
> pPeerHTCap->MaxRxAMPDUFactor, and pPeerHTCap->MPDUDensity.
> Found with minmax coccinelle script.
> 
> Signed-off-by: Rebecca Mckeever <remckee0@...il.com>
> ---
>  drivers/staging/rtl8192u/ieee80211/rtl819x_HTProc.c | 13 +++++--------
>  1 file changed, 5 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/staging/rtl8192u/ieee80211/rtl819x_HTProc.c b/drivers/staging/rtl8192u/ieee80211/rtl819x_HTProc.c
> index dba3f2db9f48..8f2cd669c5d4 100644
> --- a/drivers/staging/rtl8192u/ieee80211/rtl819x_HTProc.c
> +++ b/drivers/staging/rtl8192u/ieee80211/rtl819x_HTProc.c
> @@ -940,10 +940,8 @@ void HTOnAssocRsp(struct ieee80211_device *ieee)
>  			else
>  				pHTInfo->CurrentAMPDUFactor = HT_AGG_SIZE_64K;
>  		} else {
> -			if (pPeerHTCap->MaxRxAMPDUFactor < HT_AGG_SIZE_32K)
> -				pHTInfo->CurrentAMPDUFactor = pPeerHTCap->MaxRxAMPDUFactor;
> -			else
> -				pHTInfo->CurrentAMPDUFactor = HT_AGG_SIZE_32K;
> +			pHTInfo->CurrentAMPDUFactor = min_t(u8, pPeerHTCap->MaxRxAMPDUFactor,
> +							    HT_AGG_SIZE_32K);

The u8 cast doesn't cause a problem, but it makes me nervous so I have
to look up the types involved.

pPeerHTCap->MaxRxAMPDUFactor is 2 bits of a u8.
HT_AGG_SIZE_32K is an enum, which in real life terms of the compilers we
use is going to be a uint.

With min_t() you want to do the casts to a large enough unsigned type
so that it doesn't truncate.  And so u8 works.  But it would be less
scary if you cast it to a u32 or uint because a read will assume that
probably pPeerHTCap->MaxRxAMPDUFactor is a u32 and HT_AGG_SIZE_32K is an
int.  These are false assumptions, but probably that's what most people
will assume.

The reader in this case will feel safe and not worry about truncation so
they won't have to look up the types.

regards,
dan carpenter