mac80211: Take bitrates into account when building IEs.

From: Ben Greear <[email protected]>

When sending the association request, pay attention to the legacy rates configured by the user, and do not advertise support for any that are not configured.

This makes the assoc request packet look more correct when making modern hardware act like a /b mode station, for instance.

From: Ben Greear <[email protected]>

If a user restricts the rateset for some reason, then the probe requests should not advertise rates that are not selected by the user.

To implement this, we save the requested bitrates at the mac80211 level and take it into account when building the IEs.

This allows one to create a more realistic /b mode station on modern hardware, for instance. Good for testing, and likely good for other things as well.

Any comments on these patches?

include/uapi/linux/nl80211.h enum nl80211_attrs { · line 2133

Quoted 7 lines · On Tue, 2015-10-20 at 10:24 -0700, [email protected] wrote:

include/uapi/linux/nl80211.h +1 -0

@@ -2130,6 +2130,8 @@ enum nl80211_attrs {
 
  	NL80211_ATTR_REG_INDOOR,
 
+	NL80211_ATTR_TX_ADVERT_RATEMASK,

First of all, this is missing documentation. It's a FLAG as far as I can tell, but if it's set should it affect only the advertised or both?

I also think you added this because I had commented that we might not want to do it unconditionally, but is there really a good reason not to do it unconditionally? I was probably more thinking out loud what would happen with P2P, but there we already say "no cck" for scanning so it shouldn't really have any effect.

But given that we have NL80211_ATTR_SCAN_SUPP_RATES, where does your patch even have an effect, other than this not handling HT/VHT?

I'm a bit wary that we're introducing two ways of achieving a very similar thing.

Also, adding these overrides all over the code seems very complex. Perhaps we can achieve the goal of being able to pretend to have limited hardware capabilities by actually restricting hardware capabilities instead? That would also avoid having to play whack-a-mole with code paths using the device capabilities.

I'd imagine an API along the lines of doing some kind of higher-layer re-register of the wiphy in mac80211 with limited capabilites. At that point you'd allocate a copy of the original capabilities (as far as the new restricted ones overwrite the data), and remove the device from the system as far as higher layers like cfg80211 are concerned.

Would something like that work for you?

johannes

As far as I can tell, that will not work, because I want to have multiple station devices per radio, and have each of them be able to use a different configuration.  So, one station may be /g, and another /n and another /AC.  Same with APs.  In addition, some stations may want to use all available rates for their mode, and others may want to use a fixed rate or subset of available rates.

Quoted 6 lines · Ben Greear

As far as I can tell, that will not work, because I want to have multiple station devices per radio, and have each of them be able to use a different configuration. So, one station may be /g, and another /n and another /AC. Same with APs. In addition, some stations may want to use all available rates for their mode, and others may want to use a fixed rate or subset of available rates.

So let's agree that we're splitting the *used* rates (which we have today) and the *advertised* rates/modes/...

Even if we can't modify the wiphy capabilities, perhaps we could still handle it more consistently at mac80211 level, e.g. by introducing and using "sdata->capa.bands" (and similar for all the other wiphy capabilities you want to manipulate), and then allowing that to point to something modified?

Quoted 10 lines · Johannes Berg

Quoted 7 lines · Ben Greear

As far as I can tell, that will not work, because I want to have multiple station devices per radio, and have each of them be able to
use a different configuration.  So, one station may be /g, and another /n and another /AC.  Same with APs.  In addition, some stations may want to use all available rates for their mode, and others may want to use a fixed rate or subset of available rates.

Quoted 2 lines

So let's agree that we're splitting the *used* rates (which we have today) and the *advertised* rates/modes/...

Yes, I think that will work well, and unless I mis-understand, that is basically what I implemented so far.

Copied state might be tricky.  I think if we hold any copies of capabilities data in the sdata, then it should be logically compared with a mask and then treated as an AND with whatever the wiphy has.

  I'm reluctant to propose any serious mac80211 change at this point, though perhaps as more of this type of features are added, then it will become more obvious how to nicely consolidate things in mac80211.

To be honest, I thought my -v2 patches were fairly non-invasive compared to my normal hackings :)

Quoted 11 lines · Ben Greear

Quoted 10 lines · Johannes Berg

Quoted 7 lines · Ben Greear

As far as I can tell, that will not work, because I want to have multiple station devices per radio, and have each of them be able to
use a different configuration. So, one station may be /g, and another /n and another /AC. Same with APs. In addition, some stations may want to use all available rates for their mode, and others may want to use a fixed rate or subset of available rates.

Quoted 5 lines

Quoted 2 lines

So let's agree that we're splitting the *used* rates (which we have today) and the *advertised* rates/modes/...

Yes, I think that will work well, and unless I mis-understand, that is basically what I implemented so far.

Yes.

Quoted 3 lines

Copied state might be tricky. I think if we hold any copies of capabilities data in the sdata, then it should be logically compared with a mask and then treated as an AND with whatever the wiphy has.

Not sure what you're saying here. I was thinking we'd simply do one of these two:

1) sdata->sband[5GHZ].vht = wiphy->sband[5GHZ].vht & user-config (semantically, not implementation of course)
2) sdata->sband[5GHZ].vht = wiphy->sband[5GHZ].vht

and change mac80211 to use sdata->sband instead of wiphy->sband wherever the latter is used today. That way, we can avoid touching all these things.

I think, for example, that you missed TDLS in your changes. Changing everything throughout would mean that grepping for "wiphy->sband" would immediately show such bugs, making it far easier to maintain.

Quoted 4 lines

I'm reluctant to propose any serious mac80211 change at this point,
though perhaps as more of this type of features are added, then it will become more obvious how to nicely consolidate things in mac80211.

I don't really think this would be a "serious" change? It's basically pointering changes, you could (and perhaps should, to catch it all) use an spatch to make the initial change.

Quoted 3 lines

To be honest, I thought my -v2 patches were fairly non-invasive compared to my normal hackings :)

:)

johannes

Quoted 3 lines

1) sdata->sband[5GHZ].vht = wiphy->sband[5GHZ].vht & user-config (semantically, not implementation of course)
2) sdata->sband[5GHZ].vht = wiphy->sband[5GHZ].vht

I think option 1 might be better.  For option 2, I'd be worried about someone changing the wiphy somehow and vdevs getting out of sync.

Any opinions on the lifetime for the sdata->sband_user_config data?

For instance, could be cleared when station dis-associates, or we could make it persist until changed or until vdev goes away?

And, while this would take care of some of the issues (3x3 vs 2x2?), it still wouldn't let someone have full control over the advertised rates vs configured-rates and such, would it?

Quoted 6 lines · Ben Greear

Quoted 3 lines

1) sdata->sband[5GHZ].vht = wiphy->sband[5GHZ].vht & user-config (semantically, not implementation of course)
2) sdata->sband[5GHZ].vht = wiphy->sband[5GHZ].vht

I think option 1 might be better. For option 2, I'd be worried about someone changing the wiphy somehow and vdevs getting out of sync.

Huh? I'm confused. Those two options above weren't meant as options, they were meant as two sides of an "if (have-user-config)" branch :)

Quoted 5 lines

Any opinions on the lifetime for the sdata->sband_user_config data?

For instance, could be cleared when station dis-associates, or we could make it persist until changed or until vdev goes away?

Should probably persist.

Quoted 4 lines

And, while this would take care of some of the issues (3x3 vs 2x2?), it still wouldn't let someone have full control over the advertised rates vs configured-rates and such, would it?

Why not? The sband also contains the rates list. I was just using VHT as an example.

Oh.  In that case, what if this happens:

wiphy created and init
vdev created, vht copied from wiphy
....
wiphy vht info changes?  Can this ever happen?  I'm thinking maybe when
  a user uses 'iw' to set the rx/rx chainmask on a phy?  Possibly nasty debugfs hacks that have worked so far?

So, I think it should be more like:

if (vdata->have-user-config) {
   return sdata->user_config_vht & sdata->wiphy->vht; }
else {
   return sdata->wiphy->vht;
}

Point of the above 'code' is that we do not want to ever have duplicate state (ie, a copy of wiphy->vht in sdata->vht, because then they can get out of sync.  But, we can have a set of over-ride data in sdata, and since we logically AND that with wiphy data, then we should never exceed the capabilities of either sdata or wiphy.

That make sense?

Ok, the details of mac80211 have leaked out of my skull since the start of this thread...I'll see if I can make a stab at implementing this proposal.  But, likely it will be a bit, as I'm swamped with other things at the moment.  If someone else is interested in trying this, then of course be my guest!

Quoted 9 lines · Ben Greear

Oh. In that case, what if this happens:

wiphy created and init
vdev created, vht copied from wiphy
....
wiphy vht info changes? Can this ever happen? I'm thinking maybe when
a user uses 'iw' to set the rx/rx chainmask on a phy? Possibly
nasty debugfs hacks that have worked so far?

Ah, but I don't think this is something that's supposed to happen.

Then again, there were some people talking about this last week, with a use case that seemed valid (shared antennas and reconfiguration or so), but we can require that to cause some action to propagate it. My gut reaction was to just re-register the entire wiphy, but that may be too big of a hammer.

In any case, I think this isn't something we really need to consider too much. If there are bugs in this area then we'll fix them one way or another (or perhaps we'll just never find them since I don't expect many people to use this functionality).

Quoted 15 lines

So, I think it should be more like:

if (vdata->have-user-config) {
    return sdata->user_config_vht & sdata->wiphy->vht;
}
else {
    return sdata->wiphy->vht;
}

Point of the above 'code' is that we do not want to ever have duplicate state (ie, a copy of wiphy->vht in sdata->vht, because then they can get out of sync. But, we can have a set of over-ride data in sdata, and since we logically AND that with wiphy data, then we should never exceed the capabilities of either sdata or wiphy.

Quoted 1 line

That make sense?

Yeah, it does, at least if you assume that the capabilities can actually change. We assume they don't though, hostapd/wpa_s for example will never re-query them after startup.

I also don't like the whole "call a function to get the capabilities" not much more than what you have now in the code, since then we'll have to deal with allocations (or stack memory) for the data we want and it just generally makes the code using the capabilities much more complex (for a pretty fringe use case.)

I'd much rather have the data structure that we can use as today without additional hoops to jump through, just possibly "redirecting" it for the use cases you have in mind.

Quoted 6 lines

Ok, the details of mac80211 have leaked out of my skull since the start of this thread...I'll see if I can make a stab at implementing this proposal. But, likely it will be a bit, as I'm swamped with other things at the moment. If someone else is interested in trying this, then of course be my guest!

Looked like Cedric Debarge had a similar idea, I've pointed him to this thread (as you probably saw.)

Quoted 3 lines

Yeah, it does, at least if you assume that the capabilities can actually change. We assume they don't though, hostapd/wpa_s for example will never re-query them after startup.

Restarting supplicant, even if it came to that, might be less of an issue than having to re-create the vdev to have it grab proper new settings from the wiphy...

Well, it would be easy enough to make a method that updated an vdev/sdata->vht from wiphy, so could just call that as needed (ie, when certain things set or are changed by wiphy).

Quoted 7 lines · Ben Greear

Quoted 3 lines

Yeah, it does, at least if you assume that the capabilities can actually change. We assume they don't though, hostapd/wpa_s for example will never re-query them after startup.

Restarting supplicant, even if it came to that, might be less of an issue than having to re-create the vdev to have it grab proper new settings from the wiphy...

Quoted 3 lines

Well, it would be easy enough to make a method that updated an vdev/sdata->vht from wiphy, so could just call that as needed (ie, when certain things set or are changed by wiphy).

Yeah, I think we'll just have to deal with this when it comes along. This multi-antenna-sharing case (unfortunately I forgot who was talking about it) is the only thing that comes to mind where we really may have to deal with this.

First, are you proposing that I make a copy of the entire  local-

Quoted 1 line

hw.wiphy->bands array and store it locally in sdata?

And then, I would provide some API to modify the bands[i]->bitrates and other variables to properly select the advertised features?

I am a bit concerned about making copies (and then changing) the driver's bitrates array.  Likely it will work with ath10k, but it seems fragile at best.

Quoted 2 lines · Ben Greear

First, are you proposing that I make a copy of the entire local-

Quoted 1 line

hw.wiphy->bands array and store it locally in sdata?

No, I think it should be pointers there, say

sdata->bands[X] pointing to the same thing as local->hw.wiphy->bands[X] is pointing to.

So the first patch would introduce that, and replace each and every use of local->hw.wiphy->bands[] with sdata->bands[], if necessary annotating the remaining ones with why they're OK and should stay.

Quoted 2 lines

And then, I would provide some API to modify the bands[i]->bitrates and other variables to properly select the advertised features?

Kinda. You'd provide some kind of helper function or API to take the local->hw.wiphy->bands[X] and duplicate it into a newly allocated buffer, modifying it along the way, before assigning it to the per- sdata stuff in sdata->bands[X].

Quoted 3 lines

I am a bit concerned about making copies (and then changing) the driver's bitrates array. Likely it will work with ath10k, but it seems fragile at best.

That's an interesting point, we currently use the rate *index* a lot, so we have to find some way of preserving that. Perhaps we need to introduce a flag indicating a given rate is disabled?

The logic I wrote is basically exactly this.  It uses the configured rates to specify which of the hardware's rates are allowed and disabled.

Quoted 5 lines · Ben Greear

The logic I wrote is basically exactly this. It uses the configured rates to specify which of the hardware's rates are allowed and disabled.

I understand that. I just take issue with the fact that we have to sprinkle "magic pixie dust" (in form of the override function calls) everywhere throughout the code.

johannes

Quoted 13 lines · Johannes Berg

Quoted 6 lines · Ben Greear

The logic I wrote is basically exactly this.  It uses the configured
rates to specify which of the hardware's rates are allowed and disabled.

I understand that. I just take issue with the fact that we have to sprinkle "magic pixie dust" (in form of the override function calls)
everywhere throughout the code.

Quoted 2 lines

johannes

So, I never could think up a way to make this more acceptable and meet my own goals.

Any chance I could get just the netlink enum in place, even if it is called something like 'api_placeholder_x'?

Quoted 14 lines · Ben Greear

Quoted 13 lines · Johannes Berg

Quoted 6 lines · Ben Greear

The logic I wrote is basically exactly this. It uses the configured
rates to specify which of the hardware's rates are allowed and disabled.

I understand that. I just take issue with the fact that we have to sprinkle "magic pixie dust" (in form of the override function calls)
everywhere throughout the code.

Quoted 9 lines

Quoted 2 lines

johannes

So, I never could think up a way to make this more acceptable and meet my own goals.

Any chance I could get just the netlink enum in place, even if it is called something like 'api_placeholder_x'?

No, I don't think I want to do that.

But if you're not too worried about space consumption, you could pick a higher number:

	/* add attributes here, update the policy in nl80211.c */

	NL80211_ATTR_TX_ADVERT_RATEMASK = 300,

	__NL80211_ATTR_AFTER_LAST,

We currently use about 230 attributes, so that'll give you lots of breathing room.

With regard to my original patch, are there any other things that I could do to make it more acceptable without making copies of the driver data?