diff mbox

mac80211: Allow multiple listeners for management frames.

Message ID 20170310093454.27664-1-sven.eckelmann@openmesh.com (mailing list archive)
State Rejected
Delegated to: Johannes Berg
Headers show

Commit Message

Sven Eckelmann March 10, 2017, 9:34 a.m. UTC
From: Benjamin Berg <benjamin@sipsolutions.net>

Previously it was not possible to have multiple listeners for management
frames in userspace. This is a bit weird as multiple processes in
userspace might try to request management frame reporting but only
the first request was accepted.

Signed-off-by: Benjamin Berg <benjamin@sipsolutions.net>
Signed-off-by: Sven Eckelmann <sven.eckelmann@openmesh.com>
---
It is currently unknown why Benjamin never forwarded it. I am doing this
now to have a chance that this private patch doesn't have to be rebased
anymore by OpenMesh.
---
 net/wireless/mlme.c | 30 ++++++++++++++++++++++++++++--
 1 file changed, 28 insertions(+), 2 deletions(-)

Comments

Johannes Berg March 14, 2017, 1:51 p.m. UTC | #1
On Fri, 2017-03-10 at 10:34 +0100, Sven Eckelmann wrote:
> From: Benjamin Berg <benjamin@sipsolutions.net>
> 
> Previously it was not possible to have multiple listeners for
> management frames in userspace. This is a bit weird as multiple
> processes in userspace might try to request management frame
> reporting but only the first request was accepted.

I vaguely remember discussing this with Benjamin.

> It is currently unknown why Benjamin never forwarded it. I am doing
> this now to have a chance that this private patch doesn't have to be
> rebased anymore by OpenMesh.

I'm not sure I mentioned it to him, or even remembered it when we were
discussing it, but I don't think this patch is a good idea, at least
for action frames.

For non-action frames, I don't see any problem.

However, for action frames, if you subscribe you get the responsibility
to return if unhandled (setting 0x80 in the action code fields).

With multiple subscribers, that becomes impossible.

If you restrict it to non-action I can live with it, but I don't know
what you really want to do with this.

johannes
Sven Eckelmann March 14, 2017, 2:06 p.m. UTC | #2
On Dienstag, 14. März 2017 14:51:01 CET Johannes Berg wrote:
[...]
> I'm not sure I mentioned it to him, or even remembered it when we were
> discussing it, but I don't think this patch is a good idea, at least
> for action frames.
[...]
> If you restrict it to non-action I can live with it, but I don't know
> what you really want to do with this.

I think he required it only for probe requests.

Kind regards,
	Sven
Simon Wunderlich March 14, 2017, 2:10 p.m. UTC | #3
On Tuesday, March 14, 2017 3:06:35 PM CET Sven Eckelmann wrote:
> On Dienstag, 14. März 2017 14:51:01 CET Johannes Berg wrote:
> [...]
> 
> > I'm not sure I mentioned it to him, or even remembered it when we were
> > discussing it, but I don't think this patch is a good idea, at least
> > for action frames.
> 
> [...]
> 
> > If you restrict it to non-action I can live with it, but I don't know
> > what you really want to do with this.
> 
> I think he required it only for probe requests.
> 

The idea was to grab probe requests from userspace with a program running next 
to hostapd.

Cheers,
      Simon
Johannes Berg March 14, 2017, 2:13 p.m. UTC | #4
On Tue, 2017-03-14 at 15:10 +0100, Simon Wunderlich wrote:
> On Tuesday, March 14, 2017 3:06:35 PM CET Sven Eckelmann wrote:
> > On Dienstag, 14. März 2017 14:51:01 CET Johannes Berg wrote:
> > [...]
> > 
> > > I'm not sure I mentioned it to him, or even remembered it when we
> > > were
> > > discussing it, but I don't think this patch is a good idea, at
> > > least
> > > for action frames.
> > 
> > [...]
> > 
> > > If you restrict it to non-action I can live with it, but I don't
> > > know
> > > what you really want to do with this.
> > 
> > I think he required it only for probe requests.

Similar situation for probe requests though - who answers them? :-)

Though at least in that case the kernel doesn't care (unlike in the
action frame case) since it never does.

> The idea was to grab probe requests from userspace with a program
> running next to hostapd.

I guess there are some efficiency problems with that right now, but a
monitor mode interface should work just as well. Perhaps we can allow
attaching a BPF program to a monitor mode interface, and run that
without cloning the SKB etc.?

johannes
Sven Eckelmann March 14, 2017, 2:21 p.m. UTC | #5
On Dienstag, 14. März 2017 15:13:12 CET Johannes Berg wrote:
> > The idea was to grab probe requests from userspace with a program
> > running next to hostapd.
> 
> I guess there are some efficiency problems with that right now, but a
> monitor mode interface should work just as well. Perhaps we can allow
> attaching a BPF program to a monitor mode interface, and run that
> without cloning the SKB etc.?

This has also other problems. For example the ath10k fw will crash a lot
when a monitor interface is active. I think that we saw also some
performance regressions caused by the ath9k filtering behavior when a
monitor mode interface is active (without actually listening on it).

Kind regards,
	Sven
Johannes Berg March 14, 2017, 2:27 p.m. UTC | #6
On Tue, 2017-03-14 at 15:21 +0100, Sven Eckelmann wrote:
> On Dienstag, 14. März 2017 15:13:12 CET Johannes Berg wrote:
> > > The idea was to grab probe requests from userspace with a program
> > > running next to hostapd.
> > 
> > I guess there are some efficiency problems with that right now, but
> > a
> > monitor mode interface should work just as well. Perhaps we can
> > allow
> > attaching a BPF program to a monitor mode interface, and run that
> > without cloning the SKB etc.?
> 
> This has also other problems. For example the ath10k fw will crash a
> lot when a monitor interface is active.

That's not an argument I can accept - the driver shouldn't even have to
*care* if one is active or not, especially not if you set the flags to
none. I'm not even sure we tell the driver about it in that case, but
it definitely sounds like something that should be fixed in the driver.

> I think that we saw also some
> performance regressions caused by the ath9k filtering behavior when a
> monitor mode interface is active (without actually listening on it).

Ditto, the filtering can be fixed by passing "flags none" with iw (or
directly when creating with nl80211). The driver should be fixed to not
care about the interface at all in that case.

Regardless though, there *will* be some performance impact of this.
Hence my suggestion to add a BPF filter not to the socket, but to the
monitor interface itself.

Now I'm also wondering if we could implement cooked monitor that way,
might cut down some special code that we only keep for backwards
compatibility ...

Either way, due to the action frame issue I don't really want to apply
this patch. With probe requests it's a bit borderline, if you promise
to never send anything it would probably be OK, but it's still rather
strange to use this for monitor purposes.

johannes
diff mbox

Patch

diff --git a/net/wireless/mlme.c b/net/wireless/mlme.c
index 22b3d9990065..f8ce18dcaa61 100644
--- a/net/wireless/mlme.c
+++ b/net/wireless/mlme.c
@@ -467,6 +467,7 @@  int cfg80211_mlme_register_mgmt(struct wireless_dev *wdev, u32 snd_portid,
 	struct cfg80211_registered_device *rdev = wiphy_to_rdev(wiphy);
 	struct cfg80211_mgmt_registration *reg, *nreg;
 	int err = 0;
+	int registered = 0;
 	u16 mgmt_type;
 
 	if (!wdev->wiphy->mgmt_stypes)
@@ -494,6 +495,12 @@  int cfg80211_mlme_register_mgmt(struct wireless_dev *wdev, u32 snd_portid,
 		if (frame_type != le16_to_cpu(reg->frame_type))
 			continue;
 
+		registered = 1;
+
+		/* Allow duplicate registrations on different ports */
+		if (snd_portid != reg->nlportid)
+			continue;
+
 		if (memcmp(reg->match, match_data, mlen) == 0) {
 			err = -EALREADY;
 			break;
@@ -516,7 +523,7 @@  int cfg80211_mlme_register_mgmt(struct wireless_dev *wdev, u32 snd_portid,
 	/* process all unregistrations to avoid driver confusion */
 	cfg80211_process_mlme_unregistrations(rdev);
 
-	if (rdev->ops->mgmt_frame_register)
+	if (rdev->ops->mgmt_frame_register && !registered)
 		rdev_mgmt_frame_register(rdev, wdev, frame_type, true);
 
 	return 0;
@@ -536,6 +543,26 @@  void cfg80211_mlme_unregister_socket(struct wireless_dev *wdev, u32 nlportid)
 	spin_lock_bh(&wdev->mgmt_registrations_lock);
 
 	list_for_each_entry_safe(reg, tmp, &wdev->mgmt_registrations, list) {
+		struct cfg80211_mgmt_registration *inner;
+		int keep_registration = 0;
+
+		/* Do not remove registration as long as there is one other
+		 * registration for the frame type. Note that this registration
+		 * might even be on the same nlportid with a different match.
+		 */
+		list_for_each_entry(inner, &wdev->mgmt_registrations, list) {
+			if (inner == reg)
+				continue;
+
+			if (reg->frame_type == inner->frame_type) {
+				keep_registration = 1;
+				break;
+			}
+		}
+
+		if (keep_registration)
+			continue;
+
 		if (reg->nlportid != nlportid)
 			continue;
 
@@ -735,7 +762,6 @@  bool cfg80211_rx_mgmt(struct wireless_dev *wdev, int freq, int sig_mbm,
 			continue;
 
 		result = true;
-		break;
 	}
 
 	spin_unlock_bh(&wdev->mgmt_registrations_lock);