From:   Jose Ignacio Tornos Martinez <jtornosm@redhat.com>
To:     johannes.berg@intel.com
Cc:     gregory.greenman@intel.com, kvalo@kernel.org,
        linux-wireless@vger.kernel.org, pstourac@redhat.com
Subject: [PATCH] iwlwifi: mvm: fix double list_add at iwl_mvm_mac_wake_tx_queue (roaming)
Date:   Tue, 14 Mar 2023 15:52:09 +0100
Message-Id: <20230314145209.401875-1-jtornosm@redhat.com>
In-Reply-To: <20230314103840.30771-1-jtornosm@redhat.com>
References: <20230314103840.30771-1-jtornosm@redhat.com>
MIME-Version: 1.0
Content-Transfer-Encoding: 8bit
Precedence: bulk

Hi,

>> Issues like this, in scenarios with continuous roaming, have been reported:
> Can you say where? Any public reports?
We have some Bugzilla (i.e. 
https://bugzilla.redhat.com/show_bug.cgi?id=2152168).
You only have to create a user for this tool.

>> This can be reproduced with a single script from the station:
>>     while true; do
>>         wpa_cli -i wlp3s0 roam 34:13:E8:B1:DB:9A
>>         sleep 2
>>         wpa_cli -i wlp3s0 roam 34:13:E8:3C:FB:DB
>>         sleep 2
>>     done
>> And flooding with tx traffic.
>Oh, nice to have a reproducer.
It is not immediate but I can reproduce here like this.

> Funny thing is, I was _just_ looking at this exact bug, because we were
> discussing all this concurrency over in
So more people is struggling this this, good to get the best solution.

> https://lore.kernel.org/r/20230313201542.72325-1-alexander@wetzel-home.de

>> +++ b/drivers/net/wireless/intel/iwlwifi/mvm/mac80211.c
>> @@ -787,11 +787,18 @@ static void iwl_mvm_mac_wake_tx_queue(struct ieee80211_hw *hw,
>>               return;
>>       }
>>  
>> +     spin_lock_bh(&mvmtxq->list_lock);
>> +
>>       /* The list is being deleted only after the queue is fully allocated. */
>> -     if (!list_empty(&mvmtxq->list))
>> +     if (!list_empty(&mvmtxq->list)) {
>> +             spin_unlock_bh(&mvmtxq->list_lock);
>>               return;
>> +     }
>>  
>>       list_add_tail(&mvmtxq->list, &mvm->add_stream_txqs);
>> +
>> +     spin_unlock_bh(&mvmtxq->list_lock);


> While this might fix the issue as far as you could observe, that is
> clearly not sufficient, since you don't protect the list on the other
> side, where the items are removed from it again.
Ok, I thought about that as well but I was not able to find any problem with
the other side. Anyway, the better the solution is made the better.

> Below are the two patches that I've come up with so far, if anyone wants
> to try them. Please ignore all the extra metadata, I exported this
> directly from our internal code base.
Of course, I can test the soutions here in order to be sure.
Do you prefer I reply with the result here or in the other thread that you have
commented me before?

Thanks

Best regards
Jose Ignacio