linux-kernel - Re: [PATCH 2/2] ath11k: Handle errors if peer creation fails

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: <87pn4pfm19.fsf@codeaurora.org>
Date:   Sat, 07 Nov 2020 13:23:30 +0200
From:   Kalle Valo <kvalo@...eaurora.org>
To:     Alex Dewar <alex.dewar90@...il.com>
Cc:     netdev@...r.kernel.org, Carl Huang <cjhuang@...eaurora.org>,
        linux-wireless@...r.kernel.org, linux-kernel@...r.kernel.org,
        "David S. Miller" <davem@...emloft.net>,
        ath11k@...ts.infradead.org, Jakub Kicinski <kuba@...nel.org>
Subject: Re: [PATCH 2/2] ath11k: Handle errors if peer creation fails

Alex Dewar <alex.dewar90@...il.com> writes:

> On Tue, Oct 06, 2020 at 10:26:28AM +0300, Kalle Valo wrote:
>> Alex Dewar <alex.dewar90@...il.com> writes:
>> 
>> > ath11k_peer_create() is called without its return value being checked,
>> > meaning errors will be unhandled. Add missing check and, as the mutex is
>> > unconditionally unlocked on leaving this function, simplify the exit
>> > path.
>> >
>> > Addresses-Coverity-ID: 1497531 ("Code maintainability issues")
>> > Fixes: 701e48a43e15 ("ath11k: add packet log support for QCA6390")
>> > Signed-off-by: Alex Dewar <alex.dewar90@...il.com>
>> > ---
>> >  drivers/net/wireless/ath/ath11k/mac.c | 21 +++++++++------------
>> >  1 file changed, 9 insertions(+), 12 deletions(-)
>> >
>> > diff --git a/drivers/net/wireless/ath/ath11k/mac.c b/drivers/net/wireless/ath/ath11k/mac.c
>> > index 7f8dd47d2333..58db1b57b941 100644
>> > --- a/drivers/net/wireless/ath/ath11k/mac.c
>> > +++ b/drivers/net/wireless/ath/ath11k/mac.c
>> > @@ -5211,7 +5211,7 @@ ath11k_mac_op_assign_vif_chanctx(struct ieee80211_hw *hw,
>> >  	struct ath11k *ar = hw->priv;
>> >  	struct ath11k_base *ab = ar->ab;
>> >  	struct ath11k_vif *arvif = (void *)vif->drv_priv;
>> > -	int ret;
>> > +	int ret = 0;
>> 
>> I prefer not to initialise the ret variable.
>> 
>> >  	arvif->is_started = true;
>> >  
>> >  	/* TODO: Setup ps and cts/rts protection */
>> >  
>> > -	mutex_unlock(&ar->conf_mutex);
>> > -
>> > -	return 0;
>> > -
>> > -err:
>> > +unlock:
>> >  	mutex_unlock(&ar->conf_mutex);
>> >  
>> >  	return ret;
>> 
>> So in the pending branch I changed this to:
>> 
>> 	ret = 0;
>> 
>> out:
>> 	mutex_unlock(&ar->conf_mutex);
>> 
>> 	return ret;
>> 
>> Please check.
>
> I'm afraid you've introduced a bug ;). The body of the first if-statement
> in the function doesn't set ret because no error has occurred. So now
> it'll jump to the label and the function will return ret uninitialized.

Ouch, so I did. Good catch! I would have hoped that GCC warns about that
but it didn't.

I fixed the bug and added also a warning messages if peer_create()
fails:

https://git.kernel.org/pub/scm/linux/kernel/git/kvalo/ath.git/commit/?h=pending&id=e3e7b8072fa6bb0928b9066cf76e19e6bd2ec663

Does this look better now? :)

> With the gcc extension, ret will be initialised to zero anyway, so we're
> not saving anything by explicitly assigning to ret later in the
> function.

I prefer not to initialise ret in the beginning of the function and I
try to maintain that style in ath11k. I think it's more readable that
the error value is assigned just before the goto.

-- 
https://patchwork.kernel.org/project/linux-wireless/list/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches