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

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

Message-ID: <YuV452xuR1S0WyJi@OEMBP14.local>
Date:   Sat, 30 Jul 2022 19:36:57 +0100
From:   Phillip Potter <phil@...lpotter.co.uk>
To:     Dan Carpenter <dan.carpenter@...cle.com>
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 09:48:03AM +0300, Dan Carpenter wrote:
> 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
> 

Hi Dan,

Thank you for the review firstly, much appreciated.

I'm happy of course to rewrite this to address any concerns, but
I was hoping I could clarify what you've said though? Apologies if I've
missed it, but how is this function now returning 1 on success? It sets
ret to 0 (success) at the start and then sets it to one of two negative
error codes depending on what happens. Am I missing something here?
(Perfectly possible that I am).

In terms of do nothing gotos, do you mean gotos that just set an error
code then jump to the end? If you'd prefer, as the function just returns
right after the exit label, I can just return the codes directly and have
a 'return 0;' like you say above?

Thanks as always for your insight.

Regards,
Phil