LinuxLists.cc - [PATCH v2 1/4] cfg80211: leave invalid channels on regdomain change

[permalink] [raw]

Subject: Re: [PATCH v2 1/4] cfg80211: leave invalid channels on regdomain change

On Thu, Nov 20, 2014 at 09:38:53PM +0100, Johannes Berg wrote:
> On Thu, 2014-11-20 at 21:35 +0100, Luis R. Rodriguez wrote:
>
> > It really should only kick you off of channels you are not allowed to use,
> > I welcome this change and think it is important as a standard feature.
> > Please rename to REGULATORY_IGNORE_STALE_KICKOFF and make the behaviour
> > change to ignore the kick off rather than opt-in. We take measures to
> > operate properly regulatory and this change helps in that direction, we
> > want opt-in features to let folks that know what they are doing ingore
> > certain fatures.
>
> > > I don't want to add modes I cannot test with HW I have. I think that's
> > > irresponsible, especially when the side-effects are disconnections.
> > > I can add IBSS as well with the HW I have, but that's about it.
> >
> > I'd like to see IBSS and Mesh considered and addressed.
>
> And I want a cookie :-)
>
> I outlined what I'll accept in my other email, and since I think it's
> unreasonable to force Arik to revamp the whole thing the behaviour will
> have to stay opt-in, at least until all modes are addressed, at which
> point the flag could even go away.

Fine..

Luis

2014-11-21 23:38:25

[permalink] [raw]

Subject: Re: [PATCH v2 4/4] cfg80211: Allow usermode to query wiphy specific regd info

On Fri, Nov 21, 2014 at 11:33:43AM +0200, Arik Nemtsov wrote:
> On Thu, Nov 20, 2014 at 10:54 PM, Luis R. Rodriguez <[email protected]> wrote:
> >>
> >> Then it gets the global one, and it knows it via a wiphy attribute:
> >>
> >> (wiphy->regulatory_flags & REGULATORY_WIPHY_SELF_MANAGED) &&
> >> + (nla_put_flag(msg, NL80211_ATTR_WIPHY_SELF_MANAGED_REG)))
> >> + goto nla_put_failure;
> >>
> >> (we won't do this put_flag if it's global)
> >
> > You can still follow this on wpa_s for REGULATORY_WIPHY_SELF_MANAGED, and
> > the other type that uses wiphy->regd would still follow the global regdomain.
> > The other flag I'm looking for is more informational for userspace in particular
> > 'iw reg get' (for central and all devices) or 'iw reg get dev wlan0'.
>
> I'm not sure why another flag is needed for userspace.

wiphy->regd will be used for REGULATORY_WIPHY_SELF_MANAGED and also on
devices that regulatory_hint() is called. Even though users of regulatory_hint()
will still abide by an intersection its useful to display that device's
regulatory domain in userspace, for that you'd need to distinguish it from
REGULATORY_WIPHY_SELF_MANAGED so another flag is needed. Showing that
regulatory domain will be informational.

> If we have
> SELF_MANAGED, we'll return the per-wiphy one (if a wiphy_idx is given
> to us of course). For validation purposes, the global one is used in
> other cases. And for CUSTOM_REG, we'll return the global one (we don't
> have the per-wiphy information in cfg80211).

Atheros cards always use CUSTOM_REG first, then later if they determine
the card was programmed with an alpha2 regulatory_hint() is called and
the CUSTOM_REG flag cleared. Regardless if wiphy->regd is set it would
be useful to have that information available.

Luis

2014-11-16 11:00:36

[permalink] [raw]

Subject: Re: [PATCH v2 1/4] cfg80211: leave invalid channels on regdomain change

On Fri, Nov 14, 2014 at 12:45 AM, Luis R. Rodriguez <[email protected]> wrote:
>> + * @REGULATORY_ENFORCE_CHANNELS: the regulatory core will make sure all
>> + * interfaces on this wiphy reside on allowed channels. Upon a regdomain
>> + * change, the interfaces are given a grace period to disconnect or move
>> + * to an allowed channels. Interfaces on forbidden channels are forcibly
>> + * disconnected.
>
> I don't like this name, it would seem folks not using this don't
> get to enforce channels, and that's not right, this is a feature,
> and in fact I am not sure why this is being implemented as optional
> rather than a standard feature. Care to explain the reasoning there?

This is a big change in behavior. It can hurt certification tests etc.
I believe a chip vendor should opt-in for this change. Otherwise we
risk bad user experience.

>
>> diff --git a/net/wireless/reg.c b/net/wireless/reg.c
>> index 7449a8c..6459ddd 100644
>> --- a/net/wireless/reg.c
>> +++ b/net/wireless/reg.c
>> @@ -56,6 +56,7 @@
>> #include <net/cfg80211.h>
>> #include "core.h"
>> #include "reg.h"
>> +#include "rdev-ops.h"
>> #include "regdb.h"
>> #include "nl80211.h"
>>
>> @@ -66,6 +67,12 @@
>> #define REG_DBG_PRINT(args...)
>> #endif
>>
>> +/*
>> + * Grace period we give before making sure all current interfaces reside on
>> + * channels allowed by the current regulatory domain.
>> + */
>> +#define REG_ENFORCE_GRACE_MS 60000
>> +
>> /**
>> * enum reg_request_treatment - regulatory request treatment
>> *
>> @@ -210,6 +217,9 @@ struct reg_beacon {
>> struct ieee80211_channel chan;
>> };
>>
>> +static void reg_check_chans_work(struct work_struct *work);
>> +static DECLARE_DELAYED_WORK(reg_check_chans, reg_check_chans_work);
>> +
>> static void reg_todo(struct work_struct *work);
>> static DECLARE_WORK(reg_work, reg_todo);
>>
>> @@ -1518,6 +1528,90 @@ static void reg_call_notifier(struct wiphy *wiphy,
>> wiphy->reg_notifier(wiphy, request);
>> }
>>
>> +static bool reg_wdev_chan_valid(struct wiphy *wiphy, struct wireless_dev *wdev)
>> +{
>> + struct ieee80211_channel *ch;
>> + struct cfg80211_chan_def chandef;
>> + struct cfg80211_registered_device *rdev = wiphy_to_rdev(wiphy);
>> + bool ret = true;
>> +
>> + wdev_lock(wdev);
>> +
>> + if (!wdev->netdev || !netif_running(wdev->netdev))
>> + goto out;
>> +
>> + switch (wdev->iftype) {
>> + case NL80211_IFTYPE_AP:
>> + case NL80211_IFTYPE_P2P_GO:
>> + if (!wdev->beacon_interval)
>> + goto out;
>> +
>> + ret = cfg80211_reg_can_beacon(wiphy,
>> + &wdev->chandef, wdev->iftype);
>> + break;
>> + case NL80211_IFTYPE_STATION:
>> + case NL80211_IFTYPE_P2P_CLIENT:
>> + if (!wdev->current_bss ||
>> + !wdev->current_bss->pub.channel)
>> + goto out;
>> +
>> + ch = wdev->current_bss->pub.channel;
>> + if (rdev->ops->get_channel &&
>> + !rdev_get_channel(rdev, wdev, &chandef))
>> + ret = cfg80211_chandef_usable(wiphy, &chandef,
>> + IEEE80211_CHAN_DISABLED);
>> + else
>> + ret = !(ch->flags & IEEE80211_CHAN_DISABLED);
>> + break;
>> + default:
>> + /* others not implemented for now */
>> + pr_info("Regulatory channel check not implemented for mode %d\n",
>> + wdev->iftype);
>
> I feel you are being lazy here, come on, think of it and address it.
> It can't be that hard. In fact cfg80211_leave() already deals with
> all the logic to leave properly for all types of interfaces, you
> just have to come up with the logic to know things should kick
> the device off. Its not that hard.

I don't want to add modes I cannot test with HW I have. I think that's
irresponsible, especially when the side-effects are disconnections.
I can add IBSS as well with the HW I have, but that's about it.

Arik

2014-11-16 11:06:17

[permalink] [raw]

Subject: Re: [PATCH v2 3/4] cfg80211: allow wiphy specific regdomain management

On Fri, Nov 14, 2014 at 1:11 AM, Luis R. Rodriguez <[email protected]> wrote:
> On Thu, Nov 13, 2014 at 06:13:38PM +0200, Arik Nemtsov wrote:
>> From: Jonathan Doron <[email protected]>
>>
>> Add a new regulatory flag that allows a driver to manage regdomain
>> changes/updates for its own wiphy.
>> In this case the regdomain is local to the driver, and it does not use
>> the shared cfg80211 regdomain. It also implies that the driver does not
>> wish to get regulatory updates generated by other wiphys or by usermode.
>
> I'd like you to be clearer on this both on the commit log and also
> on the documentation:
>
> --
> Using this mean you are using your very own regulatory rule
> interpretations, vetting of your own regulatory rules then is
> completely on the driver / firmware using this. Any heuristics
> about regulatory that cfg80211 can help with is completely
> ignored.
> --
>
> There are gains by using cfg80211 and the work we have put into
> cfg80211's regulatory infrastructure, we want folks to evaluate
> not using it before discarding it, and ensure folks understand
> the risks and issues that might come up if they don't gain any
> help from cfg80211.
>
>> A new API lets the driver send a complete regdomain, to be applied on
>> its wiphy only.
>>
>> After a wiphy-specific regdomain change takes place, usermode will get
>> a new type of change notification. The regulatory core also takes care
>> enforce regulatory restrictions, in case some interfaces are on
>> forbidden channels.
>
> I want you to also address what happens when two different devices
> are used on a system that don't use the same mechanism. For instance,
> one device may use this feature, another set may use the cfg80211
> regulatory code. The implications are that the devices using this
> new feature are not learning or gaining any information from the
> core, its own its own. This is another reason that not using the
> cfg80211 regulatory information is a poor architectural choice,
> it doesn't scale well. I last considered simply disallowing such
> combinations because of this very reason -- but since you are
> separating this now I am OK with it -- I just want to ensure it is
> crystal clear of the downfall to this long term architecturally.
>
> In short: its fucking stupid.
>
> I want folks to think very fucking hard before being stupid.
>
> Please help with that.

I'll try to beef up the commit message.

>> Signed-off-by: Jonathan Doron <[email protected]>
>> Signed-off-by: Arik Nemtsov <[email protected]>
>> ---
>> include/net/cfg80211.h | 14 ++++++++
>> include/net/regulatory.h | 9 +++++
>> include/uapi/linux/nl80211.h | 5 +++
>> net/wireless/core.c | 7 ++++
>> net/wireless/nl80211.c | 80 ++++++++++++++++++++++++++++++++++----------
>> net/wireless/nl80211.h | 1 +
>> net/wireless/reg.c | 59 ++++++++++++++++++++++++++++++++
>> 7 files changed, 157 insertions(+), 18 deletions(-)
>>
>> diff --git a/include/net/cfg80211.h b/include/net/cfg80211.h
>> index 220d5f5..dd4f811 100644
>> --- a/include/net/cfg80211.h
>> +++ b/include/net/cfg80211.h
>> @@ -3728,6 +3728,20 @@ const u8 *cfg80211_find_vendor_ie(unsigned int oui, u8 oui_type,
>> int regulatory_hint(struct wiphy *wiphy, const char *alpha2);
>>
>> /**
>> + * regulatory_set_wiphy_regd_rtnl - set regdom info for self managed drivers
>> + * @wiphy: the wireless device we want to process the regulatory domain on
>> + * @rd: the regulatory domain informatoin to use for this wiphy
>> + *
>> + * Set the regulatory domain information for self managed drivers, only they
>> + * can use this function.
>> + * This function requires the caller to hold the rtnl_lock.
>
> This documentation is lacking a lot of information. Please fell it with
> information as to reasoning for why folks may go down this path as well
> as well as caveats. The combination of devices is of particular concern
> and it should be made clear.

I'll clarify it.

>
>> + *
>> + * Return: 0 on success. -EINVAL, -EPERM
>> + */
>> +int regulatory_set_wiphy_regd_rtnl(struct wiphy *wiphy,
>> + struct ieee80211_regdomain *rd);
>> +
>> +/**
>> * wiphy_apply_custom_regulatory - apply a custom driver regulatory domain
>> * @wiphy: the wireless device we want to process the regulatory domain on
>> * @regd: the custom regulatory domain to use for this wiphy
>> diff --git a/include/net/regulatory.h b/include/net/regulatory.h
>> index 701177c..42345f4 100644
>> --- a/include/net/regulatory.h
>> +++ b/include/net/regulatory.h
>> @@ -141,6 +141,14 @@ struct regulatory_request {
>> * change, the interfaces are given a grace period to disconnect or move
>> * to an allowed channels. Interfaces on forbidden channels are forcibly
>> * disconnected.
>> + * @REGULATORY_WIPHY_SELF_MANAGED: for devices that employ wiphy-specific
>> + * regdom management. These devices will ignore all regdom changes not
>> + * originating from their own wiphy. This flag is incompatible with the
>> + * flags: %REGULATORY_CUSTOM_REG, %REGULATORY_STRICT_REG,
>> + * %REGULATORY_COUNTRY_IE_FOLLOW_POWER, %REGULATORY_COUNTRY_IE_IGNORE and
>> + * %REGULATORY_DISABLE_BEACON_HINTS. Mixing any of the above flags with
>> + * this flag will result in a failure to register the wiphy. This flag
>> + * implies %REGULATORY_DISABLE_BEACON_HINTS.
>
> Please discourage such use.

Will do.

>
>> */
>> enum ieee80211_regulatory_flags {
>> REGULATORY_CUSTOM_REG = BIT(0),
>> @@ -150,6 +158,7 @@ enum ieee80211_regulatory_flags {
>> REGULATORY_COUNTRY_IE_IGNORE = BIT(4),
>> REGULATORY_ENABLE_RELAX_NO_IR = BIT(5),
>> REGULATORY_ENFORCE_CHANNELS = BIT(6),
>> + REGULATORY_WIPHY_SELF_MANAGED = BIT(7),
>> };
>>
>> struct ieee80211_freq_range {
>> diff --git a/include/uapi/linux/nl80211.h b/include/uapi/linux/nl80211.h
>> index 442369f..f050dcb 100644
>> --- a/include/uapi/linux/nl80211.h
>> +++ b/include/uapi/linux/nl80211.h
>> @@ -762,6 +762,9 @@
>> * @NL80211_CMD_LEAVE_OCB: Leave the OCB network -- no special arguments, the
>> * network is determined by the network interface.
>> *
>> + * @NL80211_CMD_WIPHY_REG_CHANGE: Similar to %NL80211_CMD_REG_CHANGE, but used
>> + * for indicating changes for devices with wiphy-specific regdom management
>> + *
>> * @NL80211_CMD_MAX: highest used command number
>> * @__NL80211_CMD_AFTER_LAST: internal use
>> */
>> @@ -943,6 +946,8 @@ enum nl80211_commands {
>>
>> NL80211_CMD_CH_SWITCH_STARTED_NOTIFY,
>>
>> + NL80211_CMD_WIPHY_REG_CHANGE,
>> +
>> /* add new commands above here */
>>
>> /* used to define NL80211_CMD_MAX below */
>> diff --git a/net/wireless/core.c b/net/wireless/core.c
>> index a4d2792..656a1b1 100644
>> --- a/net/wireless/core.c
>> +++ b/net/wireless/core.c
>> @@ -541,6 +541,13 @@ int wiphy_register(struct wiphy *wiphy)
>> !wiphy->wowlan->tcp))
>> return -EINVAL;
>> #endif
>> + if (WARN_ON((wiphy->regulatory_flags & REGULATORY_WIPHY_SELF_MANAGED) &&
>> + (wiphy->regulatory_flags &
>> + (REGULATORY_CUSTOM_REG | REGULATORY_STRICT_REG |
>> + REGULATORY_COUNTRY_IE_FOLLOW_POWER |
>> + REGULATORY_COUNTRY_IE_IGNORE |
>> + REGULATORY_DISABLE_BEACON_HINTS))))
>> + return -EINVAL;
>
> Look at all those heuristics go away... That's alot. The documetnation should
> reflect all this not being used because of this decision. I also want you to
> think of the issues that may come up when combining devices that, one that
> uses this feature and one that does not.

Since this is a private regdomain, I guess this just means the
cfg80211 using device will be alone in the system for all regulatory
purposes.
I don't really see possible interoperability issues here. Am I missing
something?

Arik

2014-11-13 22:45:58

[permalink] [raw]

Subject: Re: [PATCH v2 1/4] cfg80211: leave invalid channels on regdomain change

Johannes, Jouni, please review the comment about WLAN_REASON_DEAUTH_LEAVING
below.

On Thu, Nov 13, 2014 at 06:13:36PM +0200, Arik Nemtsov wrote:
> When the regulatory settings change, some channels might become invalid.
> Disconnect interfaces acting on these channels, after giving userspace
> code a grace period to leave them.
>
> Signed-off-by: Arik Nemtsov <[email protected]>
> Reviewed-by: Johannes Berg <[email protected]>
> ---
> include/net/regulatory.h | 6 +++
> net/wireless/reg.c | 101 ++++++++++++++++++++++++++++++++++++++++++++++-
> 2 files changed, 106 insertions(+), 1 deletion(-)
>
> diff --git a/include/net/regulatory.h b/include/net/regulatory.h
> index dad7ab2..701177c 100644
> --- a/include/net/regulatory.h
> +++ b/include/net/regulatory.h
> @@ -136,6 +136,11 @@ struct regulatory_request {
> * otherwise initiating radiation is not allowed. This will enable the
> * relaxations enabled under the CFG80211_REG_RELAX_NO_IR configuration
> * option
> + * @REGULATORY_ENFORCE_CHANNELS: the regulatory core will make sure all
> + * interfaces on this wiphy reside on allowed channels. Upon a regdomain
> + * change, the interfaces are given a grace period to disconnect or move
> + * to an allowed channels. Interfaces on forbidden channels are forcibly
> + * disconnected.

I don't like this name, it would seem folks not using this don't
get to enforce channels, and that's not right, this is a feature,
and in fact I am not sure why this is being implemented as optional
rather than a standard feature. Care to explain the reasoning there?

> diff --git a/net/wireless/reg.c b/net/wireless/reg.c
> index 7449a8c..6459ddd 100644
> --- a/net/wireless/reg.c
> +++ b/net/wireless/reg.c
> @@ -56,6 +56,7 @@
> #include <net/cfg80211.h>
> #include "core.h"
> #include "reg.h"
> +#include "rdev-ops.h"
> #include "regdb.h"
> #include "nl80211.h"
>
> @@ -66,6 +67,12 @@
> #define REG_DBG_PRINT(args...)
> #endif
>
> +/*
> + * Grace period we give before making sure all current interfaces reside on
> + * channels allowed by the current regulatory domain.
> + */
> +#define REG_ENFORCE_GRACE_MS 60000
> +
> /**
> * enum reg_request_treatment - regulatory request treatment
> *
> @@ -210,6 +217,9 @@ struct reg_beacon {
> struct ieee80211_channel chan;
> };
>
> +static void reg_check_chans_work(struct work_struct *work);
> +static DECLARE_DELAYED_WORK(reg_check_chans, reg_check_chans_work);
> +
> static void reg_todo(struct work_struct *work);
> static DECLARE_WORK(reg_work, reg_todo);
>
> @@ -1518,6 +1528,90 @@ static void reg_call_notifier(struct wiphy *wiphy,
> wiphy->reg_notifier(wiphy, request);
> }
>
> +static bool reg_wdev_chan_valid(struct wiphy *wiphy, struct wireless_dev *wdev)
> +{
> + struct ieee80211_channel *ch;
> + struct cfg80211_chan_def chandef;
> + struct cfg80211_registered_device *rdev = wiphy_to_rdev(wiphy);
> + bool ret = true;
> +
> + wdev_lock(wdev);
> +
> + if (!wdev->netdev || !netif_running(wdev->netdev))
> + goto out;
> +
> + switch (wdev->iftype) {
> + case NL80211_IFTYPE_AP:
> + case NL80211_IFTYPE_P2P_GO:
> + if (!wdev->beacon_interval)
> + goto out;
> +
> + ret = cfg80211_reg_can_beacon(wiphy,
> + &wdev->chandef, wdev->iftype);
> + break;
> + case NL80211_IFTYPE_STATION:
> + case NL80211_IFTYPE_P2P_CLIENT:
> + if (!wdev->current_bss ||
> + !wdev->current_bss->pub.channel)
> + goto out;
> +
> + ch = wdev->current_bss->pub.channel;
> + if (rdev->ops->get_channel &&
> + !rdev_get_channel(rdev, wdev, &chandef))
> + ret = cfg80211_chandef_usable(wiphy, &chandef,
> + IEEE80211_CHAN_DISABLED);
> + else
> + ret = !(ch->flags & IEEE80211_CHAN_DISABLED);
> + break;
> + default:
> + /* others not implemented for now */
> + pr_info("Regulatory channel check not implemented for mode %d\n",
> + wdev->iftype);

I feel you are being lazy here, come on, think of it and address it.
It can't be that hard. In fact cfg80211_leave() already deals with
all the logic to leave properly for all types of interfaces, you
just have to come up with the logic to know things should kick
the device off. Its not that hard.

> + break;
> + }
> +
> +out:
> + wdev_unlock(wdev);
> + return ret;
> +}
> +
> +static void reg_leave_invalid_chans(struct wiphy *wiphy)
> +{
> + struct wireless_dev *wdev;
> + struct cfg80211_registered_device *rdev = wiphy_to_rdev(wiphy);
> +
> + ASSERT_RTNL();
> +
> + list_for_each_entry(wdev, &rdev->wdev_list, list)
> + if (!reg_wdev_chan_valid(wiphy, wdev))
> + cfg80211_leave(rdev, wdev);
> +}

Kind of sad we only have WLAN_REASON_DEAUTH_LEAVING and we
only use this for STAs. You are providing an event for
this trigger so could be understood by the supplicant that
this happened for this reason but I think it'd be a lot
nicer to unify this as part of a disconnect. Johannes,
Jouni, thought?

Luis

2014-11-21 09:17:22

[permalink] [raw]

Subject: Re: [PATCH v2 3/4] cfg80211: allow wiphy specific regdomain management

On Thu, Nov 20, 2014 at 10:27 PM, Luis R. Rodriguez <[email protected]> wrote:
> On Sun, Nov 16, 2014 at 01:06:00PM +0200, Arik Nemtsov wrote:
>> On Fri, Nov 14, 2014 at 1:11 AM, Luis R. Rodriguez <[email protected]> wrote:
>> >> index a4d2792..656a1b1 100644
>> >> --- a/net/wireless/core.c
>> >> +++ b/net/wireless/core.c
>> >> @@ -541,6 +541,13 @@ int wiphy_register(struct wiphy *wiphy)
>> >> !wiphy->wowlan->tcp))
>> >> return -EINVAL;
>> >> #endif
>> >> + if (WARN_ON((wiphy->regulatory_flags & REGULATORY_WIPHY_SELF_MANAGED) &&
>> >> + (wiphy->regulatory_flags &
>> >> + (REGULATORY_CUSTOM_REG | REGULATORY_STRICT_REG |
>> >> + REGULATORY_COUNTRY_IE_FOLLOW_POWER |
>> >> + REGULATORY_COUNTRY_IE_IGNORE |
>> >> + REGULATORY_DISABLE_BEACON_HINTS))))
>> >> + return -EINVAL;
>> >
>> > Look at all those heuristics go away... That's alot. The documetnation should
>> > reflect all this not being used because of this decision. I also want you to
>> > think of the issues that may come up when combining devices that, one that
>> > uses this feature and one that does not.
>>
>> Since this is a private regdomain, I guess this just means the
>> cfg80211 using device will be alone in the system for all regulatory
>> purposes.
>> I don't really see possible interoperability issues here. Am I missing
>> something?
>
> It means you can technically end up with two devices that operate with
> different interpretation of rules, this can mean for example that some
> expectations of having two devices may fail and since this will be all hard
> coded you can't fix it. The worst of the issues will be caused by the fact
> that we simply won't know what issues will creep up until the two data sets
> conflict and create an unexpected user facing issue. This is precicely why
> having support for querying information about all regulatory data is critical,
> and I'm glad you are doing that work.
>
> What will happen when say a user / user interface wants to restrict all devices
> to say a country like Israel, 'iw reg set IL' is used, so the cfg80211 regulatory
> abiding devices follow the rules, but this Intel device does not?

It's nice that you've used IL in your example :)

An Intel device would basically stay in mode "00" until the FW decides
via a modem or other indication that it is in IL. At that point we
will send regulatory_hint_regd() notification.
The Intel device doesn't use/trust other devices.

Arik

2014-11-20 15:22:08

[permalink] [raw]

Subject: Re: [PATCH v2 4/4] cfg80211: Allow usermode to query wiphy specific regd info

On Sun, 2014-11-16 at 13:06 +0200, Arik Nemtsov wrote:

> We intend to add a patch to wpa_s to always add the wiphy_idx to
> NL80211_CMD_GET_REG. With the current approach only drivers with
> SELF_MANAGED_REG will get wiphy->regd back. This is ok since these are
> new drivers, which are familiar with this API.
>
> But if we use your suggestion and always return wiphy->regd, then some
> driver like ath9k that uses regulatory_hint() will now get it's
> private regd returned to the wpa_s that manages it. I'm not saying
> it's necessarily bad, but it's different than what was returned
> before. The cfg80211 regdomain is intersected with wiphy->regd, so now
> ath9k will start getting more permissive channels in usermode.
>
> So we thought it's best to enable the new behavior only if the driver
> explicitly wants it, using a new regulatory flag.

How does this work the other way around - i.e. a newer wpa_s requesting
per-wiphy information but it not being present?

It seems to me that either way what the kernel should return is the
information that will actually be applied when validated, which is of
course not possible when there are wiphy-specific regdomains and a
global one is requested (unless there's just one wiphy, which might be
something to consider making work?)

I also don't actually see a driver regulatory flag in this patch, as
expected, so not sure what exactly you're talking about above?

johannes

2014-11-21 09:34:02

[permalink] [raw]

Subject: Re: [PATCH v2 4/4] cfg80211: Allow usermode to query wiphy specific regd info

On Thu, Nov 20, 2014 at 10:54 PM, Luis R. Rodriguez <[email protected]> wrote:
>>
>> Then it gets the global one, and it knows it via a wiphy attribute:
>>
>> (wiphy->regulatory_flags & REGULATORY_WIPHY_SELF_MANAGED) &&
>> + (nla_put_flag(msg, NL80211_ATTR_WIPHY_SELF_MANAGED_REG)))
>> + goto nla_put_failure;
>>
>> (we won't do this put_flag if it's global)
>
> You can still follow this on wpa_s for REGULATORY_WIPHY_SELF_MANAGED, and
> the other type that uses wiphy->regd would still follow the global regdomain.
> The other flag I'm looking for is more informational for userspace in particular
> 'iw reg get' (for central and all devices) or 'iw reg get dev wlan0'.

I'm not sure why another flag is needed for userspace. If we have
SELF_MANAGED, we'll return the per-wiphy one (if a wiphy_idx is given
to us of course). For validation purposes, the global one is used in
other cases. And for CUSTOM_REG, we'll return the global one (we don't
have the per-wiphy information in cfg80211).

Arik

2014-11-20 20:35:27

[permalink] [raw]

Subject: Re: [PATCH v2 1/4] cfg80211: leave invalid channels on regdomain change

On Sun, Nov 16, 2014 at 01:00:16PM +0200, Arik Nemtsov wrote:
> On Fri, Nov 14, 2014 at 12:45 AM, Luis R. Rodriguez <[email protected]> wrote:
> >> + * @REGULATORY_ENFORCE_CHANNELS: the regulatory core will make sure all
> >> + * interfaces on this wiphy reside on allowed channels. Upon a regdomain
> >> + * change, the interfaces are given a grace period to disconnect or move
> >> + * to an allowed channels. Interfaces on forbidden channels are forcibly
> >> + * disconnected.
> >
> > I don't like this name, it would seem folks not using this don't
> > get to enforce channels, and that's not right, this is a feature,
> > and in fact I am not sure why this is being implemented as optional
> > rather than a standard feature. Care to explain the reasoning there?
>
> This is a big change in behavior. It can hurt certification tests etc.
> I believe a chip vendor should opt-in for this change. Otherwise we
> risk bad user experience.

It really should only kick you off of channels you are not allowed to use,
I welcome this change and think it is important as a standard feature.
Please rename to REGULATORY_IGNORE_STALE_KICKOFF and make the behaviour
change to ignore the kick off rather than opt-in. We take measures to
operate properly regulatory and this change helps in that direction, we
want opt-in features to let folks that know what they are doing ingore
certain fatures.

> >> diff --git a/net/wireless/reg.c b/net/wireless/reg.c
> >> index 7449a8c..6459ddd 100644
> >> --- a/net/wireless/reg.c
> >> +++ b/net/wireless/reg.c
> >> @@ -56,6 +56,7 @@
> >> #include <net/cfg80211.h>
> >> #include "core.h"
> >> #include "reg.h"
> >> +#include "rdev-ops.h"
> >> #include "regdb.h"
> >> #include "nl80211.h"
> >>
> >> @@ -66,6 +67,12 @@
> >> #define REG_DBG_PRINT(args...)
> >> #endif
> >>
> >> +/*
> >> + * Grace period we give before making sure all current interfaces reside on
> >> + * channels allowed by the current regulatory domain.
> >> + */
> >> +#define REG_ENFORCE_GRACE_MS 60000
> >> +
> >> /**
> >> * enum reg_request_treatment - regulatory request treatment
> >> *
> >> @@ -210,6 +217,9 @@ struct reg_beacon {
> >> struct ieee80211_channel chan;
> >> };
> >>
> >> +static void reg_check_chans_work(struct work_struct *work);
> >> +static DECLARE_DELAYED_WORK(reg_check_chans, reg_check_chans_work);
> >> +
> >> static void reg_todo(struct work_struct *work);
> >> static DECLARE_WORK(reg_work, reg_todo);
> >>
> >> @@ -1518,6 +1528,90 @@ static void reg_call_notifier(struct wiphy *wiphy,
> >> wiphy->reg_notifier(wiphy, request);
> >> }
> >>
> >> +static bool reg_wdev_chan_valid(struct wiphy *wiphy, struct wireless_dev *wdev)
> >> +{
> >> + struct ieee80211_channel *ch;
> >> + struct cfg80211_chan_def chandef;
> >> + struct cfg80211_registered_device *rdev = wiphy_to_rdev(wiphy);
> >> + bool ret = true;
> >> +
> >> + wdev_lock(wdev);
> >> +
> >> + if (!wdev->netdev || !netif_running(wdev->netdev))
> >> + goto out;
> >> +
> >> + switch (wdev->iftype) {
> >> + case NL80211_IFTYPE_AP:
> >> + case NL80211_IFTYPE_P2P_GO:
> >> + if (!wdev->beacon_interval)
> >> + goto out;
> >> +
> >> + ret = cfg80211_reg_can_beacon(wiphy,
> >> + &wdev->chandef, wdev->iftype);
> >> + break;
> >> + case NL80211_IFTYPE_STATION:
> >> + case NL80211_IFTYPE_P2P_CLIENT:
> >> + if (!wdev->current_bss ||
> >> + !wdev->current_bss->pub.channel)
> >> + goto out;
> >> +
> >> + ch = wdev->current_bss->pub.channel;
> >> + if (rdev->ops->get_channel &&
> >> + !rdev_get_channel(rdev, wdev, &chandef))
> >> + ret = cfg80211_chandef_usable(wiphy, &chandef,
> >> + IEEE80211_CHAN_DISABLED);
> >> + else
> >> + ret = !(ch->flags & IEEE80211_CHAN_DISABLED);
> >> + break;
> >> + default:
> >> + /* others not implemented for now */
> >> + pr_info("Regulatory channel check not implemented for mode %d\n",
> >> + wdev->iftype);
> >
> > I feel you are being lazy here, come on, think of it and address it.
> > It can't be that hard. In fact cfg80211_leave() already deals with
> > all the logic to leave properly for all types of interfaces, you
> > just have to come up with the logic to know things should kick
> > the device off. Its not that hard.
>
> I don't want to add modes I cannot test with HW I have. I think that's
> irresponsible, especially when the side-effects are disconnections.
> I can add IBSS as well with the HW I have, but that's about it.

I'd like to see IBSS and Mesh considered and addressed.

Luis

2014-11-13 16:13:46

[permalink] [raw]

Subject: [PATCH v2 3/4] cfg80211: allow wiphy specific regdomain management

From: Jonathan Doron <[email protected]>

Add a new regulatory flag that allows a driver to manage regdomain
changes/updates for its own wiphy.
In this case the regdomain is local to the driver, and it does not use
the shared cfg80211 regdomain. It also implies that the driver does not
wish to get regulatory updates generated by other wiphys or by usermode.

A new API lets the driver send a complete regdomain, to be applied on
its wiphy only.

After a wiphy-specific regdomain change takes place, usermode will get
a new type of change notification. The regulatory core also takes care
enforce regulatory restrictions, in case some interfaces are on
forbidden channels.

Signed-off-by: Jonathan Doron <[email protected]>
Signed-off-by: Arik Nemtsov <[email protected]>
---
include/net/cfg80211.h | 14 ++++++++
include/net/regulatory.h | 9 +++++
include/uapi/linux/nl80211.h | 5 +++
net/wireless/core.c | 7 ++++
net/wireless/nl80211.c | 80 ++++++++++++++++++++++++++++++++++----------
net/wireless/nl80211.h | 1 +
net/wireless/reg.c | 59 ++++++++++++++++++++++++++++++++
7 files changed, 157 insertions(+), 18 deletions(-)

diff --git a/include/net/cfg80211.h b/include/net/cfg80211.h
index 220d5f5..dd4f811 100644
--- a/include/net/cfg80211.h
+++ b/include/net/cfg80211.h
@@ -3728,6 +3728,20 @@ const u8 *cfg80211_find_vendor_ie(unsigned int oui, u8 oui_type,
int regulatory_hint(struct wiphy *wiphy, const char *alpha2);

/**
+ * regulatory_set_wiphy_regd_rtnl - set regdom info for self managed drivers
+ * @wiphy: the wireless device we want to process the regulatory domain on
+ * @rd: the regulatory domain informatoin to use for this wiphy
+ *
+ * Set the regulatory domain information for self managed drivers, only they
+ * can use this function.
+ * This function requires the caller to hold the rtnl_lock.
+ *
+ * Return: 0 on success. -EINVAL, -EPERM
+ */
+int regulatory_set_wiphy_regd_rtnl(struct wiphy *wiphy,
+ struct ieee80211_regdomain *rd);
+
+/**
* wiphy_apply_custom_regulatory - apply a custom driver regulatory domain
* @wiphy: the wireless device we want to process the regulatory domain on
* @regd: the custom regulatory domain to use for this wiphy
diff --git a/include/net/regulatory.h b/include/net/regulatory.h
index 701177c..42345f4 100644
--- a/include/net/regulatory.h
+++ b/include/net/regulatory.h
@@ -141,6 +141,14 @@ struct regulatory_request {
* change, the interfaces are given a grace period to disconnect or move
* to an allowed channels. Interfaces on forbidden channels are forcibly
* disconnected.
+ * @REGULATORY_WIPHY_SELF_MANAGED: for devices that employ wiphy-specific
+ * regdom management. These devices will ignore all regdom changes not
+ * originating from their own wiphy. This flag is incompatible with the
+ * flags: %REGULATORY_CUSTOM_REG, %REGULATORY_STRICT_REG,
+ * %REGULATORY_COUNTRY_IE_FOLLOW_POWER, %REGULATORY_COUNTRY_IE_IGNORE and
+ * %REGULATORY_DISABLE_BEACON_HINTS. Mixing any of the above flags with
+ * this flag will result in a failure to register the wiphy. This flag
+ * implies %REGULATORY_DISABLE_BEACON_HINTS.
*/
enum ieee80211_regulatory_flags {
REGULATORY_CUSTOM_REG = BIT(0),
@@ -150,6 +158,7 @@ enum ieee80211_regulatory_flags {
REGULATORY_COUNTRY_IE_IGNORE = BIT(4),
REGULATORY_ENABLE_RELAX_NO_IR = BIT(5),
REGULATORY_ENFORCE_CHANNELS = BIT(6),
+ REGULATORY_WIPHY_SELF_MANAGED = BIT(7),
};

struct ieee80211_freq_range {
diff --git a/include/uapi/linux/nl80211.h b/include/uapi/linux/nl80211.h
index 442369f..f050dcb 100644
--- a/include/uapi/linux/nl80211.h
+++ b/include/uapi/linux/nl80211.h
@@ -762,6 +762,9 @@
* @NL80211_CMD_LEAVE_OCB: Leave the OCB network -- no special arguments, the
* network is determined by the network interface.
*
+ * @NL80211_CMD_WIPHY_REG_CHANGE: Similar to %NL80211_CMD_REG_CHANGE, but used
+ * for indicating changes for devices with wiphy-specific regdom management
+ *
* @NL80211_CMD_MAX: highest used command number
* @__NL80211_CMD_AFTER_LAST: internal use
*/
@@ -943,6 +946,8 @@ enum nl80211_commands {

NL80211_CMD_CH_SWITCH_STARTED_NOTIFY,

+ NL80211_CMD_WIPHY_REG_CHANGE,
+
/* add new commands above here */

/* used to define NL80211_CMD_MAX below */
diff --git a/net/wireless/core.c b/net/wireless/core.c
index a4d2792..656a1b1 100644
--- a/net/wireless/core.c
+++ b/net/wireless/core.c
@@ -541,6 +541,13 @@ int wiphy_register(struct wiphy *wiphy)
!wiphy->wowlan->tcp))
return -EINVAL;
#endif
+ if (WARN_ON((wiphy->regulatory_flags & REGULATORY_WIPHY_SELF_MANAGED) &&
+ (wiphy->regulatory_flags &
+ (REGULATORY_CUSTOM_REG | REGULATORY_STRICT_REG |
+ REGULATORY_COUNTRY_IE_FOLLOW_POWER |
+ REGULATORY_COUNTRY_IE_IGNORE |
+ REGULATORY_DISABLE_BEACON_HINTS))))
+ return -EINVAL;

if (WARN_ON(wiphy->coalesce &&
(!wiphy->coalesce->n_rules ||
diff --git a/net/wireless/nl80211.c b/net/wireless/nl80211.c
index d0a8361..3a422e6 100644
--- a/net/wireless/nl80211.c
+++ b/net/wireless/nl80211.c
@@ -10659,25 +10659,9 @@ void nl80211_send_sched_scan(struct cfg80211_registered_device *rdev,
NL80211_MCGRP_SCAN, GFP_KERNEL);
}

-/*
- * This can happen on global regulatory changes or device specific settings
- * based on custom world regulatory domains.
- */
-void nl80211_send_reg_change_event(struct regulatory_request *request)
+static bool nl80211_reg_change_event_fill(struct sk_buff *msg,
+ struct regulatory_request *request)
{
- struct sk_buff *msg;
- void *hdr;
-
- msg = nlmsg_new(NLMSG_DEFAULT_SIZE, GFP_KERNEL);
- if (!msg)
- return;
-
- hdr = nl80211hdr_put(msg, 0, 0, 0, NL80211_CMD_REG_CHANGE);
- if (!hdr) {
- nlmsg_free(msg);
- return;
- }
-
/* Userspace can always count this one always being set */
if (nla_put_u8(msg, NL80211_ATTR_REG_INITIATOR, request->initiator))
goto nla_put_failure;
@@ -10707,6 +10691,66 @@ void nl80211_send_reg_change_event(struct regulatory_request *request)
nla_put_u32(msg, NL80211_ATTR_WIPHY, request->wiphy_idx))
goto nla_put_failure;

+ return true;
+
+nla_put_failure:
+ return false;
+}
+
+/*
+ * This can happen on global regulatory changes or device specific settings
+ * based on custom world regulatory domains.
+ */
+void nl80211_send_reg_change_event(struct regulatory_request *request)
+{
+ struct sk_buff *msg;
+ void *hdr;
+
+ msg = nlmsg_new(NLMSG_DEFAULT_SIZE, GFP_KERNEL);
+ if (!msg)
+ return;
+
+ hdr = nl80211hdr_put(msg, 0, 0, 0, NL80211_CMD_REG_CHANGE);
+ if (!hdr) {
+ nlmsg_free(msg);
+ return;
+ }
+
+ if (nl80211_reg_change_event_fill(msg, request) == false)
+ goto nla_put_failure;
+
+ genlmsg_end(msg, hdr);
+
+ rcu_read_lock();
+ genlmsg_multicast_allns(&nl80211_fam, msg, 0,
+ NL80211_MCGRP_REGULATORY, GFP_ATOMIC);
+ rcu_read_unlock();
+
+ return;
+
+nla_put_failure:
+ genlmsg_cancel(msg, hdr);
+ nlmsg_free(msg);
+}
+
+void nl80211_send_wiphy_reg_change_event(struct regulatory_request *request)
+{
+ struct sk_buff *msg;
+ void *hdr;
+
+ msg = nlmsg_new(NLMSG_DEFAULT_SIZE, GFP_KERNEL);
+ if (!msg)
+ return;
+
+ hdr = nl80211hdr_put(msg, 0, 0, 0, NL80211_CMD_WIPHY_REG_CHANGE);
+ if (!hdr) {
+ nlmsg_free(msg);
+ return;
+ }
+
+ if (nl80211_reg_change_event_fill(msg, request) == false)
+ goto nla_put_failure;
+
genlmsg_end(msg, hdr);

rcu_read_lock();
diff --git a/net/wireless/nl80211.h b/net/wireless/nl80211.h
index 7ad70d6..b91b9c5 100644
--- a/net/wireless/nl80211.h
+++ b/net/wireless/nl80211.h
@@ -18,6 +18,7 @@ void nl80211_send_sched_scan(struct cfg80211_registered_device *rdev,
void nl80211_send_sched_scan_results(struct cfg80211_registered_device *rdev,
struct net_device *netdev);
void nl80211_send_reg_change_event(struct regulatory_request *request);
+void nl80211_send_wiphy_reg_change_event(struct regulatory_request *request);
void nl80211_send_rx_auth(struct cfg80211_registered_device *rdev,
struct net_device *netdev,
const u8 *buf, size_t len, gfp_t gfp);
diff --git a/net/wireless/reg.c b/net/wireless/reg.c
index 174d8f82..4ee282d 100644
--- a/net/wireless/reg.c
+++ b/net/wireless/reg.c
@@ -1307,6 +1307,9 @@ static bool ignore_reg_update(struct wiphy *wiphy,
{
struct regulatory_request *lr = get_last_request();

+ if (wiphy->regulatory_flags & REGULATORY_WIPHY_SELF_MANAGED)
+ return true;
+
if (!lr) {
REG_DBG_PRINT("Ignoring regulatory request set by %s "
"since last_request is not set\n",
@@ -1370,6 +1373,9 @@ static void handle_reg_beacon(struct wiphy *wiphy, unsigned int chan_idx,
sband = wiphy->bands[reg_beacon->chan.band];
chan = &sband->channels[chan_idx];

+ if (wiphy->regulatory_flags & REGULATORY_WIPHY_SELF_MANAGED)
+ return;
+
if (likely(chan->center_freq != reg_beacon->chan.center_freq))
return;

@@ -2426,6 +2432,9 @@ static void restore_regulatory_settings(bool reset_user)
world_alpha2[1] = cfg80211_world_regdom->alpha2[1];

list_for_each_entry(rdev, &cfg80211_rdev_list, list) {
+ if (rdev->wiphy.regulatory_flags &
+ REGULATORY_WIPHY_SELF_MANAGED)
+ continue;
if (rdev->wiphy.regulatory_flags & REGULATORY_CUSTOM_REG)
restore_custom_reg_settings(&rdev->wiphy);
}
@@ -2829,6 +2838,56 @@ int set_regdom(const struct ieee80211_regdomain *rd)
return 0;
}

+int regulatory_set_wiphy_regd_rtnl(struct wiphy *wiphy,
+ struct ieee80211_regdomain *rd)
+{
+ const struct ieee80211_regdomain *regd;
+ const struct ieee80211_regdomain *tmp;
+ enum ieee80211_band band;
+ struct regulatory_request request = {};
+
+ if (WARN_ON(!wiphy || !rd))
+ return -EINVAL;
+
+ if (WARN(wiphy->regulatory_flags & REGULATORY_WIPHY_SELF_MANAGED,
+ "wiphy should have REGULATORY_WIPHY_SELF_MANAGED\n"))
+ return -EPERM;
+
+ WARN_ON_ONCE(!lockdep_rtnl_is_held());
+
+ if (WARN(!is_valid_rd(rd), "Invalid regulatory domain detected\n")) {
+ print_regdomain_info(rd);
+ return -EINVAL;
+ }
+
+ regd = reg_copy_regd(rd);
+ if (IS_ERR(regd))
+ return PTR_ERR(regd);
+
+ tmp = get_wiphy_regdom(wiphy);
+ rcu_assign_pointer(wiphy->regd, regd);
+ rcu_free_regdom(tmp);
+
+ for (band = 0; band < IEEE80211_NUM_BANDS; band++) {
+ handle_band_custom(wiphy,
+ wiphy->bands[band],
+ regd);
+ }
+
+ reg_process_ht_flags(wiphy);
+
+ request.wiphy_idx = get_wiphy_idx(wiphy);
+ request.alpha2[0] = rd->alpha2[0];
+ request.alpha2[1] = rd->alpha2[1];
+ request.initiator = NL80211_REGDOM_SET_BY_DRIVER;
+
+ nl80211_send_wiphy_reg_change_event(&request);
+
+ reg_check_channels();
+ return 0;
+}
+EXPORT_SYMBOL(regulatory_set_wiphy_regd_rtnl);
+
void wiphy_regulatory_register(struct wiphy *wiphy)
{
struct regulatory_request *lr;
--
1.9.1

2014-11-13 22:55:29

[permalink] [raw]

Subject: Re: [PATCH v2 2/4] cfg80211: update missing fields in custom regulatory path

On Thu, Nov 13, 2014 at 06:13:37PM +0200, Arik Nemtsov wrote:
> From: Jonathan Doron <[email protected]>
>
> Some channels fields were not being updated in the custom regulatory
> path. Update them according to the code in handle_channel().
>
> Signed-off-by: Jonathan Doron <[email protected]>
> Signed-off-by: Arik Nemtsov <[email protected]>
> ---
> net/wireless/reg.c | 13 +++++++++++++
> 1 file changed, 13 insertions(+)
>
> diff --git a/net/wireless/reg.c b/net/wireless/reg.c
> index 6459ddd..174d8f82 100644
> --- a/net/wireless/reg.c
> +++ b/net/wireless/reg.c
> @@ -1693,10 +1693,23 @@ static void handle_channel_custom(struct wiphy *wiphy,
> if (max_bandwidth_khz < MHZ_TO_KHZ(160))
> bw_flags |= IEEE80211_CHAN_NO_160MHZ;
>
> + chan->dfs_state = NL80211_DFS_USABLE;

NL80211_DFS_USABLE is 0 so this is not needed. Being explicit about it
is OK I suppose though.

> + chan->dfs_state_entered = jiffies;

OK.

> +
> + chan->beacon_found = false;

false is 0 so this is not needed but being explicit about it is OK.

> chan->flags |= map_regdom_flags(reg_rule->flags) | bw_flags;
> chan->max_antenna_gain = (int) MBI_TO_DBI(power_rule->max_antenna_gain);
> chan->max_reg_power = chan->max_power =
> (int) MBM_TO_DBM(power_rule->max_eirp);
> +
> + if (chan->flags & IEEE80211_CHAN_RADAR) {
> + if (reg_rule->dfs_cac_ms)
> + chan->dfs_cac_ms = reg_rule->dfs_cac_ms;
> + else
> + chan->dfs_cac_ms = IEEE80211_DFS_MIN_CAC_TIME_MS;
> + }
> +
> + chan->max_power = chan->max_reg_power;
> }

I rather you split this up into the stuff not required (things which
are good to be explicit about) Vs possible fixes, these last are
good fixes. Also put them as part of your first set of patches
and add my Acked-by so Johannes can already merge them. If you want
them merged faster just send them separately now.

Acked-by: Luis R. Rodriguez <[email protected]>

Luis

2014-11-16 11:01:27

[permalink] [raw]

Subject: Re: [PATCH v2 2/4] cfg80211: update missing fields in custom regulatory path

On Fri, Nov 14, 2014 at 12:55 AM, Luis R. Rodriguez <[email protected]> wrote:
> On Thu, Nov 13, 2014 at 06:13:37PM +0200, Arik Nemtsov wrote:
>> From: Jonathan Doron <[email protected]>
>>
>> Some channels fields were not being updated in the custom regulatory
>> path. Update them according to the code in handle_channel().
>>
>> Signed-off-by: Jonathan Doron <[email protected]>
>> Signed-off-by: Arik Nemtsov <[email protected]>
>> ---
>> net/wireless/reg.c | 13 +++++++++++++
>> 1 file changed, 13 insertions(+)
>>
>> diff --git a/net/wireless/reg.c b/net/wireless/reg.c
>> index 6459ddd..174d8f82 100644
>> --- a/net/wireless/reg.c
>> +++ b/net/wireless/reg.c
>> @@ -1693,10 +1693,23 @@ static void handle_channel_custom(struct wiphy *wiphy,
>> if (max_bandwidth_khz < MHZ_TO_KHZ(160))
>> bw_flags |= IEEE80211_CHAN_NO_160MHZ;
>>
>> + chan->dfs_state = NL80211_DFS_USABLE;
>
> NL80211_DFS_USABLE is 0 so this is not needed. Being explicit about it
> is OK I suppose though.
>
>> + chan->dfs_state_entered = jiffies;
>
> OK.
>
>> +
>> + chan->beacon_found = false;
>
> false is 0 so this is not needed but being explicit about it is OK.
>
>> chan->flags |= map_regdom_flags(reg_rule->flags) | bw_flags;
>> chan->max_antenna_gain = (int) MBI_TO_DBI(power_rule->max_antenna_gain);
>> chan->max_reg_power = chan->max_power =
>> (int) MBM_TO_DBM(power_rule->max_eirp);
>> +
>> + if (chan->flags & IEEE80211_CHAN_RADAR) {
>> + if (reg_rule->dfs_cac_ms)
>> + chan->dfs_cac_ms = reg_rule->dfs_cac_ms;
>> + else
>> + chan->dfs_cac_ms = IEEE80211_DFS_MIN_CAC_TIME_MS;
>> + }
>> +
>> + chan->max_power = chan->max_reg_power;
>> }
>
> I rather you split this up into the stuff not required (things which
> are good to be explicit about) Vs possible fixes, these last are
> good fixes. Also put them as part of your first set of patches
> and add my Acked-by so Johannes can already merge them. If you want
> them merged faster just send them separately now.
>
> Acked-by: Luis R. Rodriguez <[email protected]>

Thanks.

I'll split into 2 and send then.

Arik

2014-11-20 20:27:02

[permalink] [raw]

Subject: Re: [PATCH v2 3/4] cfg80211: allow wiphy specific regdomain management

On Sun, Nov 16, 2014 at 01:06:00PM +0200, Arik Nemtsov wrote:
> On Fri, Nov 14, 2014 at 1:11 AM, Luis R. Rodriguez <[email protected]> wrote:
> >> index a4d2792..656a1b1 100644
> >> --- a/net/wireless/core.c
> >> +++ b/net/wireless/core.c
> >> @@ -541,6 +541,13 @@ int wiphy_register(struct wiphy *wiphy)
> >> !wiphy->wowlan->tcp))
> >> return -EINVAL;
> >> #endif
> >> + if (WARN_ON((wiphy->regulatory_flags & REGULATORY_WIPHY_SELF_MANAGED) &&
> >> + (wiphy->regulatory_flags &
> >> + (REGULATORY_CUSTOM_REG | REGULATORY_STRICT_REG |
> >> + REGULATORY_COUNTRY_IE_FOLLOW_POWER |
> >> + REGULATORY_COUNTRY_IE_IGNORE |
> >> + REGULATORY_DISABLE_BEACON_HINTS))))
> >> + return -EINVAL;
> >
> > Look at all those heuristics go away... That's alot. The documetnation should
> > reflect all this not being used because of this decision. I also want you to
> > think of the issues that may come up when combining devices that, one that
> > uses this feature and one that does not.
>
> Since this is a private regdomain, I guess this just means the
> cfg80211 using device will be alone in the system for all regulatory
> purposes.
> I don't really see possible interoperability issues here. Am I missing
> something?

It means you can technically end up with two devices that operate with
different interpretation of rules, this can mean for example that some
expectations of having two devices may fail and since this will be all hard
coded you can't fix it. The worst of the issues will be caused by the fact
that we simply won't know what issues will creep up until the two data sets
conflict and create an unexpected user facing issue. This is precicely why
having support for querying information about all regulatory data is critical,
and I'm glad you are doing that work.

What will happen when say a user / user interface wants to restrict all devices
to say a country like Israel, 'iw reg set IL' is used, so the cfg80211 regulatory
abiding devices follow the rules, but this Intel device does not?

Luis

2014-11-20 15:17:04

[permalink] [raw]

Subject: Re: [PATCH v2 1/4] cfg80211: leave invalid channels on regdomain change

On Thu, 2014-11-13 at 18:13 +0200, Arik Nemtsov wrote:
> When the regulatory settings change, some channels might become invalid.
> Disconnect interfaces acting on these channels, after giving userspace
> code a grace period to leave them.

So as we discussed, I think you should add a note here that the flag
only works for some interface types, and prohibit setting the flag when
unsupported ones are supported. Then you can also remove the message and
just put in a WARN_ON(1) or something since you can't really get to that
code path.

IMHO that's a reasonable trade-off between generalisation and not doing
all the work now especially since you can't validate it over Intel hw
for other interface types - if anyone else needs/wants the flag then
they can add those interface types and change the conditions.

johannes

2014-11-20 15:17:51

[permalink] [raw]

Subject: Re: [PATCH v2 1/4] cfg80211: leave invalid channels on regdomain change

On Sun, 2014-11-16 at 13:00 +0200, Arik Nemtsov wrote:

> I don't want to add modes I cannot test with HW I have. I think that's
> irresponsible, especially when the side-effects are disconnections.
> I can add IBSS as well with the HW I have, but that's about it.

Oops, my other reply was meant to go here...

johannes

2014-11-13 23:11:39

[permalink] [raw]

Subject: Re: [PATCH v2 3/4] cfg80211: allow wiphy specific regdomain management

On Thu, Nov 13, 2014 at 06:13:38PM +0200, Arik Nemtsov wrote:
> From: Jonathan Doron <[email protected]>
>
> Add a new regulatory flag that allows a driver to manage regdomain
> changes/updates for its own wiphy.
> In this case the regdomain is local to the driver, and it does not use
> the shared cfg80211 regdomain. It also implies that the driver does not
> wish to get regulatory updates generated by other wiphys or by usermode.

I'd like you to be clearer on this both on the commit log and also
on the documentation:

--
Using this mean you are using your very own regulatory rule
interpretations, vetting of your own regulatory rules then is
completely on the driver / firmware using this. Any heuristics
about regulatory that cfg80211 can help with is completely
ignored.
--

There are gains by using cfg80211 and the work we have put into
cfg80211's regulatory infrastructure, we want folks to evaluate
not using it before discarding it, and ensure folks understand
the risks and issues that might come up if they don't gain any
help from cfg80211.

> A new API lets the driver send a complete regdomain, to be applied on
> its wiphy only.
>
> After a wiphy-specific regdomain change takes place, usermode will get
> a new type of change notification. The regulatory core also takes care
> enforce regulatory restrictions, in case some interfaces are on
> forbidden channels.

I want you to also address what happens when two different devices
are used on a system that don't use the same mechanism. For instance,
one device may use this feature, another set may use the cfg80211
regulatory code. The implications are that the devices using this
new feature are not learning or gaining any information from the
core, its own its own. This is another reason that not using the
cfg80211 regulatory information is a poor architectural choice,
it doesn't scale well. I last considered simply disallowing such
combinations because of this very reason -- but since you are
separating this now I am OK with it -- I just want to ensure it is
crystal clear of the downfall to this long term architecturally.

In short: its fucking stupid.

I want folks to think very fucking hard before being stupid.

Please help with that.

> Signed-off-by: Jonathan Doron <[email protected]>
> Signed-off-by: Arik Nemtsov <[email protected]>
> ---
> include/net/cfg80211.h | 14 ++++++++
> include/net/regulatory.h | 9 +++++
> include/uapi/linux/nl80211.h | 5 +++
> net/wireless/core.c | 7 ++++
> net/wireless/nl80211.c | 80 ++++++++++++++++++++++++++++++++++----------
> net/wireless/nl80211.h | 1 +
> net/wireless/reg.c | 59 ++++++++++++++++++++++++++++++++
> 7 files changed, 157 insertions(+), 18 deletions(-)
>
> diff --git a/include/net/cfg80211.h b/include/net/cfg80211.h
> index 220d5f5..dd4f811 100644
> --- a/include/net/cfg80211.h
> +++ b/include/net/cfg80211.h
> @@ -3728,6 +3728,20 @@ const u8 *cfg80211_find_vendor_ie(unsigned int oui, u8 oui_type,
> int regulatory_hint(struct wiphy *wiphy, const char *alpha2);
>
> /**
> + * regulatory_set_wiphy_regd_rtnl - set regdom info for self managed drivers
> + * @wiphy: the wireless device we want to process the regulatory domain on
> + * @rd: the regulatory domain informatoin to use for this wiphy
> + *
> + * Set the regulatory domain information for self managed drivers, only they
> + * can use this function.
> + * This function requires the caller to hold the rtnl_lock.

This documentation is lacking a lot of information. Please fell it with
information as to reasoning for why folks may go down this path as well
as well as caveats. The combination of devices is of particular concern
and it should be made clear.

> + *
> + * Return: 0 on success. -EINVAL, -EPERM
> + */
> +int regulatory_set_wiphy_regd_rtnl(struct wiphy *wiphy,
> + struct ieee80211_regdomain *rd);
> +
> +/**
> * wiphy_apply_custom_regulatory - apply a custom driver regulatory domain
> * @wiphy: the wireless device we want to process the regulatory domain on
> * @regd: the custom regulatory domain to use for this wiphy
> diff --git a/include/net/regulatory.h b/include/net/regulatory.h
> index 701177c..42345f4 100644
> --- a/include/net/regulatory.h
> +++ b/include/net/regulatory.h
> @@ -141,6 +141,14 @@ struct regulatory_request {
> * change, the interfaces are given a grace period to disconnect or move
> * to an allowed channels. Interfaces on forbidden channels are forcibly
> * disconnected.
> + * @REGULATORY_WIPHY_SELF_MANAGED: for devices that employ wiphy-specific
> + * regdom management. These devices will ignore all regdom changes not
> + * originating from their own wiphy. This flag is incompatible with the
> + * flags: %REGULATORY_CUSTOM_REG, %REGULATORY_STRICT_REG,
> + * %REGULATORY_COUNTRY_IE_FOLLOW_POWER, %REGULATORY_COUNTRY_IE_IGNORE and
> + * %REGULATORY_DISABLE_BEACON_HINTS. Mixing any of the above flags with
> + * this flag will result in a failure to register the wiphy. This flag
> + * implies %REGULATORY_DISABLE_BEACON_HINTS.

Please discourage such use.

> */
> enum ieee80211_regulatory_flags {
> REGULATORY_CUSTOM_REG = BIT(0),
> @@ -150,6 +158,7 @@ enum ieee80211_regulatory_flags {
> REGULATORY_COUNTRY_IE_IGNORE = BIT(4),
> REGULATORY_ENABLE_RELAX_NO_IR = BIT(5),
> REGULATORY_ENFORCE_CHANNELS = BIT(6),
> + REGULATORY_WIPHY_SELF_MANAGED = BIT(7),
> };
>
> struct ieee80211_freq_range {
> diff --git a/include/uapi/linux/nl80211.h b/include/uapi/linux/nl80211.h
> index 442369f..f050dcb 100644
> --- a/include/uapi/linux/nl80211.h
> +++ b/include/uapi/linux/nl80211.h
> @@ -762,6 +762,9 @@
> * @NL80211_CMD_LEAVE_OCB: Leave the OCB network -- no special arguments, the
> * network is determined by the network interface.
> *
> + * @NL80211_CMD_WIPHY_REG_CHANGE: Similar to %NL80211_CMD_REG_CHANGE, but used
> + * for indicating changes for devices with wiphy-specific regdom management
> + *
> * @NL80211_CMD_MAX: highest used command number
> * @__NL80211_CMD_AFTER_LAST: internal use
> */
> @@ -943,6 +946,8 @@ enum nl80211_commands {
>
> NL80211_CMD_CH_SWITCH_STARTED_NOTIFY,
>
> + NL80211_CMD_WIPHY_REG_CHANGE,
> +
> /* add new commands above here */
>
> /* used to define NL80211_CMD_MAX below */
> diff --git a/net/wireless/core.c b/net/wireless/core.c
> index a4d2792..656a1b1 100644
> --- a/net/wireless/core.c
> +++ b/net/wireless/core.c
> @@ -541,6 +541,13 @@ int wiphy_register(struct wiphy *wiphy)
> !wiphy->wowlan->tcp))
> return -EINVAL;
> #endif
> + if (WARN_ON((wiphy->regulatory_flags & REGULATORY_WIPHY_SELF_MANAGED) &&
> + (wiphy->regulatory_flags &
> + (REGULATORY_CUSTOM_REG | REGULATORY_STRICT_REG |
> + REGULATORY_COUNTRY_IE_FOLLOW_POWER |
> + REGULATORY_COUNTRY_IE_IGNORE |
> + REGULATORY_DISABLE_BEACON_HINTS))))
> + return -EINVAL;

Look at all those heuristics go away... That's alot. The documetnation should
reflect all this not being used because of this decision. I also want you to
think of the issues that may come up when combining devices that, one that
uses this feature and one that does not.

Luis

2014-11-13 16:13:47

[permalink] [raw]

Subject: [PATCH v2 4/4] cfg80211: Allow usermode to query wiphy specific regd info

From: Jonathan Doron <[email protected]>

Allow usermode to query wiphy-specific regd info, for drivers that use
wiphy-specific regulatory management.

Use the existing API for sending regdomain info to usermode, but return
the wiphy-specific regd in case wiphy index is provided and the driver
employs wiphy-specific management. This implies user and kernel-mode
support for the feature and is backward compatible.

Signed-off-by: Jonathan Doron <[email protected]>
Signed-off-by: Arik Nemtsov <[email protected]>
---
include/uapi/linux/nl80211.h | 6 +++++
net/wireless/nl80211.c | 54 +++++++++++++++++++++++++++++++++++---------
net/wireless/reg.c | 2 +-
net/wireless/reg.h | 1 +
4 files changed, 51 insertions(+), 12 deletions(-)

diff --git a/include/uapi/linux/nl80211.h b/include/uapi/linux/nl80211.h
index f050dcb..9c996bc 100644
--- a/include/uapi/linux/nl80211.h
+++ b/include/uapi/linux/nl80211.h
@@ -1674,6 +1674,10 @@ enum nl80211_commands {
* @NL80211_ATTR_SMPS_MODE: SMPS mode to use (ap mode). see
* &enum nl80211_smps_mode.
*
+ * @NL80211_ATTR_WIPHY_SELF_MANAGED_REG: flag attribute indicating the
+ * regulatory information came from the driver and not from the global
+ * cfg80211 regulatory domain information.
+ *
* @NL80211_ATTR_MAX: highest attribute number currently defined
* @__NL80211_ATTR_AFTER_LAST: internal use
*/
@@ -2026,6 +2030,8 @@ enum nl80211_attrs {

NL80211_ATTR_SMPS_MODE,

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

__NL80211_ATTR_AFTER_LAST,
diff --git a/net/wireless/nl80211.c b/net/wireless/nl80211.c
index 3a422e6..f1f632c 100644
--- a/net/wireless/nl80211.c
+++ b/net/wireless/nl80211.c
@@ -395,6 +395,7 @@ static const struct nla_policy nl80211_policy[NL80211_ATTR_MAX+1] = {
[NL80211_ATTR_USER_PRIO] = { .type = NLA_U8 },
[NL80211_ATTR_ADMITTED_TIME] = { .type = NLA_U16 },
[NL80211_ATTR_SMPS_MODE] = { .type = NLA_U8 },
+ [NL80211_ATTR_WIPHY_SELF_MANAGED_REG] = { .type = NLA_FLAG },
};

/* policy for the key attributes */
@@ -5287,14 +5288,24 @@ static int nl80211_update_mesh_config(struct sk_buff *skb,

static int nl80211_get_reg(struct sk_buff *skb, struct genl_info *info)
{
- const struct ieee80211_regdomain *regdom;
+ const struct ieee80211_regdomain *regdom = NULL;
+ struct cfg80211_registered_device *rdev;
struct sk_buff *msg;
void *hdr = NULL;
struct nlattr *nl_reg_rules;
unsigned int i;

- if (!cfg80211_regdomain)
- return -EINVAL;
+ if (info->attrs[NL80211_ATTR_WIPHY] != NULL) {
+ struct wiphy *wiphy;
+
+ rdev = cfg80211_get_dev_from_info(genl_info_net(info), info);
+ if (IS_ERR(rdev))
+ return PTR_ERR(rdev);
+
+ wiphy = &rdev->wiphy;
+ if (wiphy->regulatory_flags & REGULATORY_WIPHY_SELF_MANAGED)
+ regdom = get_wiphy_regdom(wiphy);
+ }

msg = nlmsg_new(NLMSG_DEFAULT_SIZE, GFP_KERNEL);
if (!msg)
@@ -5305,13 +5316,27 @@ static int nl80211_get_reg(struct sk_buff *skb, struct genl_info *info)
if (!hdr)
goto put_failure;

- if (reg_last_request_cell_base() &&
- nla_put_u32(msg, NL80211_ATTR_USER_REG_HINT_TYPE,
- NL80211_USER_REG_HINT_CELL_BASE))
- goto nla_put_failure;
+ if (!regdom) {
+ if (!cfg80211_regdomain) {
+ nlmsg_free(msg);
+ return -EINVAL;
+ }
+
+ if (reg_last_request_cell_base() &&
+ nla_put_u32(msg, NL80211_ATTR_USER_REG_HINT_TYPE,
+ NL80211_USER_REG_HINT_CELL_BASE))
+ goto nla_put_failure;
+ } else {
+ if (nla_put_flag(msg, NL80211_ATTR_WIPHY_SELF_MANAGED_REG))
+ goto nla_put_failure;
+ if (nla_put_u32(msg, NL80211_ATTR_WIPHY, rdev->wiphy_idx))
+ goto nla_put_failure;
+ }

rcu_read_lock();
- regdom = rcu_dereference(cfg80211_regdomain);
+
+ if (regdom == NULL)
+ regdom = rcu_dereference(cfg80211_regdomain);

if (nla_put_string(msg, NL80211_ATTR_REG_ALPHA2, regdom->alpha2) ||
(regdom->dfs_region &&
@@ -10687,9 +10712,16 @@ static bool nl80211_reg_change_event_fill(struct sk_buff *msg,
goto nla_put_failure;
}

- if (request->wiphy_idx != WIPHY_IDX_INVALID &&
- nla_put_u32(msg, NL80211_ATTR_WIPHY, request->wiphy_idx))
- goto nla_put_failure;
+ if (request->wiphy_idx != WIPHY_IDX_INVALID) {
+ struct wiphy *wiphy;
+
+ wiphy = wiphy_idx_to_wiphy(request->wiphy_idx);
+ if ((wiphy != NULL) &&
+ nla_put_u32(msg, NL80211_ATTR_WIPHY, request->wiphy_idx) &&
+ (wiphy->regulatory_flags & REGULATORY_WIPHY_SELF_MANAGED) &&
+ (nla_put_flag(msg, NL80211_ATTR_WIPHY_SELF_MANAGED_REG)))
+ goto nla_put_failure;
+ }

return true;

diff --git a/net/wireless/reg.c b/net/wireless/reg.c
index 4ee282d..a5f32d5 100644
--- a/net/wireless/reg.c
+++ b/net/wireless/reg.c
@@ -142,7 +142,7 @@ static const struct ieee80211_regdomain *get_cfg80211_regdom(void)
return rtnl_dereference(cfg80211_regdomain);
}

-static const struct ieee80211_regdomain *get_wiphy_regdom(struct wiphy *wiphy)
+const struct ieee80211_regdomain *get_wiphy_regdom(struct wiphy *wiphy)
{
return rtnl_dereference(wiphy->regd);
}
diff --git a/net/wireless/reg.h b/net/wireless/reg.h
index 5e48031..4b45d6e 100644
--- a/net/wireless/reg.h
+++ b/net/wireless/reg.h
@@ -38,6 +38,7 @@ unsigned int reg_get_max_bandwidth(const struct ieee80211_regdomain *rd,
const struct ieee80211_reg_rule *rule);

bool reg_last_request_cell_base(void);
+const struct ieee80211_regdomain *get_wiphy_regdom(struct wiphy *wiphy);

/**
* regulatory_hint_found_beacon - hints a beacon was found on a channel
--
1.9.1

2014-11-20 20:55:01

[permalink] [raw]

Subject: Re: [PATCH v2 4/4] cfg80211: Allow usermode to query wiphy specific regd info

On Thu, Nov 20, 2014 at 06:47:24PM +0200, Arik Nemtsov wrote:
> On Thu, Nov 20, 2014 at 5:22 PM, Johannes Berg
> <[email protected]> wrote:
> > On Sun, 2014-11-16 at 13:06 +0200, Arik Nemtsov wrote:
> >
> >> We intend to add a patch to wpa_s to always add the wiphy_idx to
> >> NL80211_CMD_GET_REG. With the current approach only drivers with
> >> SELF_MANAGED_REG will get wiphy->regd back. This is ok since these are
> >> new drivers, which are familiar with this API.
> >>
> >> But if we use your suggestion and always return wiphy->regd, then some
> >> driver like ath9k that uses regulatory_hint() will now get it's
> >> private regd returned to the wpa_s that manages it. I'm not saying
> >> it's necessarily bad, but it's different than what was returned
> >> before. The cfg80211 regdomain is intersected with wiphy->regd, so now
> >> ath9k will start getting more permissive channels in usermode.
> >>
> >> So we thought it's best to enable the new behavior only if the driver
> >> explicitly wants it, using a new regulatory flag.
> >
> > How does this work the other way around - i.e. a newer wpa_s requesting
> > per-wiphy information but it not being present?
>
> Then it gets the global one, and it knows it via a wiphy attribute:
>
> (wiphy->regulatory_flags & REGULATORY_WIPHY_SELF_MANAGED) &&
> + (nla_put_flag(msg, NL80211_ATTR_WIPHY_SELF_MANAGED_REG)))
> + goto nla_put_failure;
>
> (we won't do this put_flag if it's global)

You can still follow this on wpa_s for REGULATORY_WIPHY_SELF_MANAGED, and
the other type that uses wiphy->regd would still follow the global regdomain.
The other flag I'm looking for is more informational for userspace in particular
'iw reg get' (for central and all devices) or 'iw reg get dev wlan0'.

> The supplicant patches are not up yet, but I think for now we will
> just act according to the global one. Anyway it has the option to
> change policy, since it knows.
>
> >
> > It seems to me that either way what the kernel should return is the
> > information that will actually be applied when validated, which is of
> > course not possible when there are wiphy-specific regdomains and a
> > global one is requested (unless there's just one wiphy, which might be
> > something to consider making work?)
>
> Well the cfg80211 regdomain is always intersected with all wiphy->regd
> in the system, so the global one is always less permissive.
> This is of course true before REGULATORY_WIPHY_SELF_MANAGED - in this
> case wiphy->regd will not affect the global regd.

The discrepancy is a long term issue when device combing but it seems folks
dealing with REGULATORY_WIPHY_SELF_MANAGED are OK with supporting whatever
issues come up.

> With CUSTOM_REG we have a special case, since the regulatory code
> doesn't set wiphy->regd, it just applies a user supplied regd to the
> wiphy channels (which get validated).

Its a custom world regdom, its a minimum base set. New information is
always welcomed to help it comply further, and that's what happens.

> If the user also happens to set wiphy->regd himself with CUSTOM_REG,
> then it makes sense to return the per-wiphy one. The global regd is
> meaningless anyway for CUSTOM_REG, and the per-wiphy one might be too.
> But I guess we can return it if it's there?

When CUSTOM_REG is used we already have code that deals with what is
allowed or now given new hints from different sources, wpa_s can
and should rely on the channel information state it is in already.
All the intersection logic is already dealt for the device. The only
thing wpa_s could probably care more about from the regulatory data is
the actual ISO3166 alpha2 used for country IEs.

Luis

2014-11-16 11:07:06

[permalink] [raw]

Subject: Re: [PATCH v2 4/4] cfg80211: Allow usermode to query wiphy specific regd info

On Fri, Nov 14, 2014 at 1:13 AM, Luis R. Rodriguez <[email protected]> wrote:
> On Thu, Nov 13, 2014 at 06:13:39PM +0200, Arik Nemtsov wrote:
>> From: Jonathan Doron <[email protected]>
>>
>> Allow usermode to query wiphy-specific regd info, for drivers that use
>> wiphy-specific regulatory management.
>>
>> Use the existing API for sending regdomain info to usermode, but return
>> the wiphy-specific regd in case wiphy index is provided and the driver
>> employs wiphy-specific management. This implies user and kernel-mode
>> support for the feature and is backward compatible.
>
> This patch does not address my feedback about making this generic
> to any wiphy->regd.

Copy-pasting my previous reply:

Well always sending wiphy->regd whenever it is set is easy, but it
might be problematic I guess:

We intend to add a patch to wpa_s to always add the wiphy_idx to
NL80211_CMD_GET_REG. With the current approach only drivers with
SELF_MANAGED_REG will get wiphy->regd back. This is ok since these are
new drivers, which are familiar with this API.

But if we use your suggestion and always return wiphy->regd, then some
driver like ath9k that uses regulatory_hint() will now get it's
private regd returned to the wpa_s that manages it. I'm not saying
it's necessarily bad, but it's different than what was returned
before. The cfg80211 regdomain is intersected with wiphy->regd, so now
ath9k will start getting more permissive channels in usermode.

So we thought it's best to enable the new behavior only if the driver
explicitly wants it, using a new regulatory flag.

Arik

2014-11-13 16:13:45

[permalink] [raw]

Subject: [PATCH v2 2/4] cfg80211: update missing fields in custom regulatory path

From: Jonathan Doron <[email protected]>

Some channels fields were not being updated in the custom regulatory
path. Update them according to the code in handle_channel().

Signed-off-by: Jonathan Doron <[email protected]>
Signed-off-by: Arik Nemtsov <[email protected]>
---
net/wireless/reg.c | 13 +++++++++++++
1 file changed, 13 insertions(+)

diff --git a/net/wireless/reg.c b/net/wireless/reg.c
index 6459ddd..174d8f82 100644
--- a/net/wireless/reg.c
+++ b/net/wireless/reg.c
@@ -1693,10 +1693,23 @@ static void handle_channel_custom(struct wiphy *wiphy,
if (max_bandwidth_khz < MHZ_TO_KHZ(160))
bw_flags |= IEEE80211_CHAN_NO_160MHZ;

+ chan->dfs_state = NL80211_DFS_USABLE;
+ chan->dfs_state_entered = jiffies;
+
+ chan->beacon_found = false;
chan->flags |= map_regdom_flags(reg_rule->flags) | bw_flags;
chan->max_antenna_gain = (int) MBI_TO_DBI(power_rule->max_antenna_gain);
chan->max_reg_power = chan->max_power =
(int) MBM_TO_DBM(power_rule->max_eirp);
+
+ if (chan->flags & IEEE80211_CHAN_RADAR) {
+ if (reg_rule->dfs_cac_ms)
+ chan->dfs_cac_ms = reg_rule->dfs_cac_ms;
+ else
+ chan->dfs_cac_ms = IEEE80211_DFS_MIN_CAC_TIME_MS;
+ }
+
+ chan->max_power = chan->max_reg_power;
}

static void handle_band_custom(struct wiphy *wiphy,
--
1.9.1

2014-11-20 20:39:01

[permalink] [raw]

Subject: Re: [PATCH v2 1/4] cfg80211: leave invalid channels on regdomain change

On Thu, 2014-11-20 at 21:35 +0100, Luis R. Rodriguez wrote:

> It really should only kick you off of channels you are not allowed to use,
> I welcome this change and think it is important as a standard feature.
> Please rename to REGULATORY_IGNORE_STALE_KICKOFF and make the behaviour
> change to ignore the kick off rather than opt-in. We take measures to
> operate properly regulatory and this change helps in that direction, we
> want opt-in features to let folks that know what they are doing ingore
> certain fatures.

> > I don't want to add modes I cannot test with HW I have. I think that's
> > irresponsible, especially when the side-effects are disconnections.
> > I can add IBSS as well with the HW I have, but that's about it.
>
> I'd like to see IBSS and Mesh considered and addressed.

And I want a cookie :-)

I outlined what I'll accept in my other email, and since I think it's
unreasonable to force Arik to revamp the whole thing the behaviour will
have to stay opt-in, at least until all modes are addressed, at which
point the flag could even go away.

johannes

2014-11-20 16:47:41

[permalink] [raw]

Subject: Re: [PATCH v2 4/4] cfg80211: Allow usermode to query wiphy specific regd info

On Thu, Nov 20, 2014 at 5:22 PM, Johannes Berg
<[email protected]> wrote:
> On Sun, 2014-11-16 at 13:06 +0200, Arik Nemtsov wrote:
>
>> We intend to add a patch to wpa_s to always add the wiphy_idx to
>> NL80211_CMD_GET_REG. With the current approach only drivers with
>> SELF_MANAGED_REG will get wiphy->regd back. This is ok since these are
>> new drivers, which are familiar with this API.
>>
>> But if we use your suggestion and always return wiphy->regd, then some
>> driver like ath9k that uses regulatory_hint() will now get it's
>> private regd returned to the wpa_s that manages it. I'm not saying
>> it's necessarily bad, but it's different than what was returned
>> before. The cfg80211 regdomain is intersected with wiphy->regd, so now
>> ath9k will start getting more permissive channels in usermode.
>>
>> So we thought it's best to enable the new behavior only if the driver
>> explicitly wants it, using a new regulatory flag.
>
> How does this work the other way around - i.e. a newer wpa_s requesting
> per-wiphy information but it not being present?

Then it gets the global one, and it knows it via a wiphy attribute:

(wiphy->regulatory_flags & REGULATORY_WIPHY_SELF_MANAGED) &&
+ (nla_put_flag(msg, NL80211_ATTR_WIPHY_SELF_MANAGED_REG)))
+ goto nla_put_failure;

(we won't do this put_flag if it's global)

The supplicant patches are not up yet, but I think for now we will
just act according to the global one. Anyway it has the option to
change policy, since it knows.

>
> It seems to me that either way what the kernel should return is the
> information that will actually be applied when validated, which is of
> course not possible when there are wiphy-specific regdomains and a
> global one is requested (unless there's just one wiphy, which might be
> something to consider making work?)

Well the cfg80211 regdomain is always intersected with all wiphy->regd
in the system, so the global one is always less permissive.
This is of course true before REGULATORY_WIPHY_SELF_MANAGED - in this
case wiphy->regd will not affect the global regd.

With CUSTOM_REG we have a special case, since the regulatory code
doesn't set wiphy->regd, it just applies a user supplied regd to the
wiphy channels (which get validated).
If the user also happens to set wiphy->regd himself with CUSTOM_REG,
then it makes sense to return the per-wiphy one. The global regd is
meaningless anyway for CUSTOM_REG, and the per-wiphy one might be too.
But I guess we can return it if it's there?

>
> I also don't actually see a driver regulatory flag in this patch, as
> expected, so not sure what exactly you're talking about above?

The regulatory flag is in the previous patch and is called
REGULATORY_WIPHY_SELF_MANAGED. The relevant code here is:

+ if (wiphy->regulatory_flags & REGULATORY_WIPHY_SELF_MANAGED)
+ regdom = get_wiphy_regdom(wiphy);

Arik

2014-11-13 23:13:24

[permalink] [raw]

Subject: Re: [PATCH v2 4/4] cfg80211: Allow usermode to query wiphy specific regd info

On Thu, Nov 13, 2014 at 06:13:39PM +0200, Arik Nemtsov wrote:
> From: Jonathan Doron <[email protected]>
>
> Allow usermode to query wiphy-specific regd info, for drivers that use
> wiphy-specific regulatory management.
>
> Use the existing API for sending regdomain info to usermode, but return
> the wiphy-specific regd in case wiphy index is provided and the driver
> employs wiphy-specific management. This implies user and kernel-mode
> support for the feature and is backward compatible.

This patch does not address my feedback about making this generic
to any wiphy->regd.

Luis

2014-11-20 20:38:08