linux-kernel - Re: [PATCH v2] wifi: mac80211: re-order unassigning channel in activate links

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: <9c497dca795ed9f62f0505daf7f9311a803334c8.camel@sipsolutions.net>
Date: Fri, 06 Dec 2024 11:07:19 +0100
From: Johannes Berg <johannes@...solutions.net>
To: Aditya Kumar Singh <quic_adisi@...cinc.com>
Cc: linux-wireless@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2] wifi: mac80211: re-order unassigning channel in
 activate links

On Thu, 2024-12-05 at 20:43 +0530, Aditya Kumar Singh wrote:
> On 12/5/24 18:30, Johannes Berg wrote:
> > On Thu, 2024-12-05 at 12:43 +0100, Johannes Berg wrote:
> > > > 
> > > > Therefore, re-order the logic so that stations are handled first and then
> > > > channel is unassigned.
> > > > 
> > > 
> > > This causes memory leaks in my tests with iwlwifi.
> > > 
> > 
> > And also firmware crashes because the station is removed while it's
> > still being used.
> > 
> 
> So is this exposing some underlying issue with iwlwifi?

I don't think so?

> Or this change 
> will break drivers which does not group multiple hardware into single 
> wiphy?

Not necessarily, but it breaks iwlwifi because of the changed order of
operations, and what it does with the firmware.

I think the issue here is that we treat link active == link has channel
context in iwlwifi, and an active link in client mode requires a station
in firmware. Otherwise you cannot even deactivate a link, since that
requires sending an NDP to the AP, but if you don't have the AP STA you
can't do that ...

I guess the driver could be changed to treat station links as active
when they have the AP STA entry, but that seems ... difficult and
strange, it would make it different between AP and client modes?

Looking at your commit message more, I wonder if it really even makes
sense to *delete* the link when the channel context is unassigned,
rather than (similarly to iwlwifi) deactivating it and deleting it later
when it's actually removed (change_vif_links)? You do know which
hardware it is/was on, after all. And these two operations can *never*
be atomic. Removing the STAs first might be something that's appropriate
for AP mode, but I guess I'm more with iwlwifi here in that it doesn't
seem quite right for client mode?

> Also, how about non-ML scenario in iwlwifi? There, first station is 
> removed and then the interface goes down right?

It's not so much about the interface but the link, it seems.

johannes