linux-wireless.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Johannes Berg <johannes@sipsolutions.net>
To: Denis Kenzior <denkenz@gmail.com>, linux-wireless@vger.kernel.org
Cc: stable@vger.kernel.org
Subject: Re: [PATCH] cfg80211: Purge frame registrations on iftype change
Date: Fri, 30 Aug 2019 10:53:01 +0200	[thread overview]
Message-ID: <5dc694c33759a32eb3796668a8b396c0133e1ebe.camel@sipsolutions.net> (raw)
In-Reply-To: <20190828211110.15005-1-denkenz@gmail.com> (sfid-20190828_231636_661927_AAE3C4AB)

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. 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.

Depending on the design, it may also just be that those registrations
are *ignored*, because e.g. firmware intercepts the AUTH frame already,
which would just (maybe) confuse userspace - but that seems unlikely
since it switched interface type and has no real need for those frames
then.

> The kernel currently relies on userspace apps to actually purge the
> registrations themselves, e.g. by closing the nl80211 socket associated
> with those frames.  However, this requires multiple nl80211 sockets to
> be open by the userspace app, and for userspace to be aware of all state
> changes.  This is not something that the kernel should rely on.

I tend to agree with that the kernel shouldn't rely on it.

> This commit adds a call to cfg80211_mlme_purge_registrations() to
> forcefully remove any registrations left over prior to switching the
> iftype.

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 *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.

Also, perhaps it'd be better to actually hang on to those registrations
that *are* still possible afterwards? But to not confuse the driver I
guess that might require unregister/re-register to happen, all of which
requires hanging on to the list and going through it after the type
switch completed?

What do you think?

johannes


  reply	other threads:[~2019-08-30  8:53 UTC|newest]

Thread overview: 5+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-08-28 21:11 [PATCH] cfg80211: Purge frame registrations on iftype change Denis Kenzior
2019-08-30  8:53 ` Johannes Berg [this message]
2019-08-30  6:32   ` Denis Kenzior
2019-09-11  9:51     ` Johannes Berg
2019-09-11 12:12       ` Denis Kenzior

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=5dc694c33759a32eb3796668a8b396c0133e1ebe.camel@sipsolutions.net \
    --to=johannes@sipsolutions.net \
    --cc=denkenz@gmail.com \
    --cc=linux-wireless@vger.kernel.org \
    --cc=stable@vger.kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).