linux-kernel - Re: [PATCH] ath9k: unlock rcu read when returning early

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]

Date:   Tue, 13 Dec 2016 14:52:49 +0100
From:   Felix Fietkau <nbd@....name>
To:     Tobias Klausmann <tobias.johannes.klausmann@....thm.de>,
        kvalo@...eaurora.org, helgaas@...nel.org,
        linux-kernel@...r.kernel.org, linux-pci@...r.kernel.org,
        marc.zyngier@....com, Janusz.Dziedzic@...to.com,
        rmanohar@....qualcomm.com, ath9k-devel@....qualcomm.com,
        linux-wireless@...r.kernel.org, rmanohar@....qualcomm.com,
        bharat.kumar.gogada@...inx.com
Subject: Re: [PATCH] ath9k: unlock rcu read when returning early

On 2016-12-13 14:41, Tobias Klausmann wrote:
> On 13.12.2016 11:41, Felix Fietkau wrote:
>> On 2016-12-12 19:50, Tobias Klausmann wrote:
>>> diff --git a/drivers/net/wireless/ath/ath9k/xmit.c b/drivers/net/wireless/ath/ath9k/xmit.c
>>> index 52bfbb988611..857d5ae09a1d 100644
>>> --- a/drivers/net/wireless/ath/ath9k/xmit.c
>>> +++ b/drivers/net/wireless/ath/ath9k/xmit.c
>>> @@ -2787,6 +2787,7 @@ void ath_tx_edma_tasklet(struct ath_softc *sc)
>>>   		fifo_list = &txq->txq_fifo[txq->txq_tailidx];
>>>   		if (list_empty(fifo_list)) {
>>>   			ath_txq_unlock(sc, txq);
>>> +			rcu_read_unlock();
>> Technically this is fine as well, but I'd prefer a fix where you replace
>> the 'return' with 'break', thus avoiding the duplication of
>> rcu_read_unlock()
> 
> Actually if you want to avoid it, maybe skipping over the rest is better 
> (as originally intended):
> 
> ...
> 
> ath_txq_unlock(sc, txq);
> 
> 
> goto unlock;
> }
> ...
> 
> unlock:
> rcu_read_unlock();
There are already other places that skip to the rcu_read_unlock() part
by using 'break'. I don't see how adding an unnecessary goto makes
things any better.

- Felix