| Message ID | 20170310093454.27664-1-sven.eckelmann@openmesh.com (mailing list archive) |
|---|---|
| State | Rejected |
| Delegated to: | Johannes Berg |
| Headers | show |
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
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
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
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
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
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 --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);