From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-1.0 required=3.0 tests=HEADER_FROM_DIFFERENT_DOMAINS, MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS autolearn=no autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 1B200ECDE20 for ; Wed, 11 Sep 2019 09:51:44 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id EC07A20CC7 for ; Wed, 11 Sep 2019 09:51:43 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1727496AbfIKJvn (ORCPT ); Wed, 11 Sep 2019 05:51:43 -0400 Received: from s3.sipsolutions.net ([144.76.43.62]:34792 "EHLO sipsolutions.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1726341AbfIKJvl (ORCPT ); Wed, 11 Sep 2019 05:51:41 -0400 Received: by sipsolutions.net with esmtpsa (TLS1.3:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.92) (envelope-from ) id 1i7zHm-0003Eu-Q0; Wed, 11 Sep 2019 11:51:38 +0200 Message-ID: Subject: Re: [PATCH] cfg80211: Purge frame registrations on iftype change From: Johannes Berg To: Denis Kenzior , linux-wireless@vger.kernel.org Cc: stable@vger.kernel.org Date: Wed, 11 Sep 2019 11:51:37 +0200 In-Reply-To: (sfid-20190830_172839_535654_F8184AE6) References: <20190828211110.15005-1-denkenz@gmail.com> <5dc694c33759a32eb3796668a8b396c0133e1ebe.camel@sipsolutions.net> (sfid-20190830_172839_535654_F8184AE6) Content-Type: text/plain; charset="UTF-8" User-Agent: Evolution 3.30.5 (3.30.5-1.fc29) MIME-Version: 1.0 Content-Transfer-Encoding: 7bit Sender: stable-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: stable@vger.kernel.org Hi, On Fri, 2019-08-30 at 01:32 -0500, Denis Kenzior wrote: > Hi Johannes, > > On 8/30/19 3:53 AM, Johannes Berg wrote: > > On Wed, 2019-08-28 at 16:11 -0500, Denis Kenzior wrote: > > > Currently frame registrations are not purged, even when changing the > > > interface type. This can lead to potentially weird / dangerous > > > situations where frames possibly not relevant to a given interface > > > type remain registered and mgmt_frame_register is not called for the > > > no-longer-relevant frame types. > > > > I'd argue really just "weird and non-working", hardly dangerous. I think I may just have found a way that's sort of "dangerous" in the sense of breaking all of our tests, but hey. > > Even in > > the mac80211 design where we want to not let you intercept e.g. AUTH > > frames in client mode - if you did, then you'd just end up with a non- > > working interface. Not sure I see any "dangerous situation". Not really > > an all that important distinction though. > > Fair enough, I'm happy to drop / reword this language. It seemed fishy > to me since the unregistration operation was not called at all, and the > driver does go to some lengths to set up the valid frame registration > types. Sure. > > However, I do wonder if we should make this more transactional, and hang > > on to them if the type switching fails. We're not notifying userspace > > that the registrations have disappeared, so if type switching fails and > > it continues to work with the old type rather than throwing its hands up > > and quitting or something, it'd make a possibly bigger mess to just > > silently have removed them already. > > I do like that idea, not sure how to go about implementing it though? > The failure case is a bit hard to deal with. Something like > NL80211_EXT_FEATURE_LIVE_IFTYPE_CHANGE would help, particularly if > nl80211/cfg80211 actually checked it prior to doing anything (e.g. > disconnecting, etc). That would then take care of the majority of the > 'typical' failure paths. I didn't add such checking in the other patch > set since I felt you might find it overly intrusive on userspace. But > maybe we really should do this? As I just said on the other patch, I think we probably should do that there, if just to be able to advertise a correct set of interface types that you can switch between there. I don't see how it'd be more intrusive to userspace than failing later? :-) > So playing devil's advocate, another argument might be that by the time > we got here, we've already tore down a bunch of state. E.g. > disconnected the station, stopped AP, etc. So we've already > side-effected state in a bunch of ways, what's one more? True, fair point. > > I *think* it should be safe to just move this after the switching > > succeeds, since the switching can pretty much only be done at a point > > where nothing is happening on the interface anyway, though that might > > confuse the driver when the remove happens. > > > > I would concur as that is what happens today. But should it? Well, dunno, what should happen? If you ask drivers they might want to remove & re-register after, for those registrations that are still possible. > It isn't currently clear to me if there are any guarantees on the driver > operation call sequence that cfg80211 provides. E.g. can the driver > expect rdev_change_virtual_intf to be called only once all the old > registrations are purged and the new registrations are performed after > the fact? Or should it expect things to just happen in any order? Well, evidently it cannot rely on anything today, and for the most part I guess this is implemented in the software paths where it doesn't really matter (the same way that mac80211 implements it). But it probably should be defined better. > > What do you think? > > > > A big part of me thinks that just wiping the slate clean and having > userspace set it up from scratch isn't that much to ask and it might > want to do that anyway. It might (a big maybe?) also make the driver's > life easier if it can rely on certain guarantees from cfg80211. E.g. > that all invalid registrations are purged. Yeah, fair point. > I have seen wpa_s perform a bunch of register commands which bounce off > with an -EALREADY. So it may already be erring on the side of caution > and assuming that it needs to reset the state fully? Not sure. I'm pretty sure that it does in fact go through a full reset (re-setup) after switching things around. > But if the kernel wants to be nice and spends some cycles figuring out > which frame registrations to keep and re-register them, that is also > fine with me. Let's not then. I've applied this patch now. johannes