linux-kernel - Re: [PATCH 2/2] staging: r8188eu: convert rtw_set_802_11_add

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 for Android: free password hash cracker in your pocket

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

Date:   Fri, 29 Jul 2022 09:48:03 +0300
From:   Dan Carpenter <dan.carpenter@...cle.com>
To:     Phillip Potter <phil@...lpotter.co.uk>
Cc:     gregkh@...uxfoundation.org, Larry.Finger@...inger.net,
        paskripkin@...il.com, martin@...ser.cx, straube.linux@...il.com,
        fmdefrancesco@...il.com, abdun.nihaal@...il.com,
        linux-staging@...ts.linux.dev, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 2/2] staging: r8188eu: convert rtw_set_802_11_add_wep
 error code semantics

On Fri, Jul 29, 2022 at 12:11:50AM +0100, Phillip Potter wrote:
> -u8 rtw_set_802_11_add_wep(struct adapter *padapter, struct ndis_802_11_wep *wep)
> +int rtw_set_802_11_add_wep(struct adapter *padapter, struct ndis_802_11_wep *wep)
>  {
>  	int		keyid, res;
>  	struct security_priv *psecuritypriv = &padapter->securitypriv;
> -	u8		ret = _SUCCESS;
> +	int		ret = 0;
>  
>  	keyid = wep->KeyIndex & 0x3fffffff;
>  
>  	if (keyid >= 4) {
> -		ret = false;
> +		ret = -EOPNOTSUPP;
>  		goto exit;
>  	}
>  
> @@ -424,7 +424,7 @@ u8 rtw_set_802_11_add_wep(struct adapter *padapter, struct ndis_802_11_wep *wep)
>  	res = rtw_set_key(padapter, psecuritypriv, keyid, 1);
>  
>  	if (res == _FAIL)
> -		ret = false;
> +		ret = -ENOMEM;
>  exit:
>  
>  	return ret;

No, this isn't right.  This now returns 1 on success and negative
error codes on error.

There are a couple anti-patterns here:

1) Do nothing gotos
2) Mixing error paths and success paths.

If you avoid mixing error paths and success paths then you get a pattern
called: "Solid return zero."  This is where the end of the function has
a very chunky "return 0;" to mark that it is successful.  You want that.
Some people do a "if (ret == 0) return ret;".  Nope.  "return ret;" is
not chunky.

regards,
dan carpenter