Re: [RFC] First CRDA integration work

Johannes Berg
2008-06-12T08:43:44Z

[-- Attachment #1: Type: text/plain, Size: 9954 bytes --]

> + * @NL80211_CMD_GET_REG: Get current regulatory domain, this is a query
> + * to the kernel wireless core, the wireless core returns currently
> + * set alpha2 by %NL80211_ATTR_REG_ALPHA2,.

Please specify what command it comes in, probably _NEW_REG that you
haven't added.

> + * @NL80211_CMD_SET_REG: Set current regulatory domain. CRDA sets this after
sends

> + * being queried by the kernel.

How?

> CRDA replies by sending a regulatory domain
> + * structure which consists of %NL80211_ATTR_REG_ALPHA set to our current alpha2
> + * if it found a match.

Why set the alpha2 again? I thought the kernel queried for that?

> It also provides %NL80211_ATTR_REG_NUM,

What is that for?

> + * %NL80211_ATTR_REG_PGP_SIGNATURE_CHECK,

It's not PGP either way, and why is that required in the kernel?

> and a regulatory rule. The regulatory
> + * rule consists of set of frequency ranges given by
> + * %NL80211_ATTR_REG_RULE_FREQ_[START|END] with an attached power rule given
> + * by %NL80211_ATTR_REG_RULE_POWER_MAX_ANT_GAIN and
> + * %NL80211_ATTR_REG_RULE_POWER_MAX_EIRP. Each regulatory rule also has
> + * an attached %NL80211_ATTR_REG_RULE_FLAGS.

How are these attributes used? Nested in some array?

> + * @NL80211_ATTR_REG_ALPHA2: an ISO-3166-alpha2 country code for which the
> + * current regulatory domain should be set to or is already set to.
> + * For example, 'CR', for Costa Rica.

Heh :)

> This attribute is used by the kernel
> + * to query the CRDA userspace agent to retrieve one regulatory domain.
> + * This attribute can also be used by userspace to query the kernel for
> + * the currently set regulatory domain.

???

> + * @NL80211_ATTR_REG_NUM_RULES: number of regulatory rules passed

Not necessary.

> + * @NL80211_ATTR_REG_PGP_SIGNATURE_CHECK: whether or not CRDA has blessed
> + * this information through a PGP signature check. If its not blessed
> + * we go ahead and regulatory-taint the kernel. This is obviously
> + * hackable but the purpose is to allow distributions to be able to use
> + * a CRDA with a regulatory database signed off by the community
> + * and get the communities' blessing on this. Vendors can also
> + * support their own custom databases for custom solutions (AP, military).
> + * Start to shift liablity to the user, where it should be.

Not PGP. It's just an RSA signature. If liability is at the user, why do
we need to know this in the kernel?

> + * @NL80211_ATTR_REG_RULE_FLAGS: a set of flags which specify additional
> + * considerations for a given frequency range. These are the
> + * &enum nl80211_reg_rule_flags.

Should we use flag attributes here nested into something? That would be
more netlink-like and allow easier expansion above 32 should it become
necessary.

> + * @NL80211_ATTR_REG_RULE_FREQ_START: starting frequencry for the regulatory
> + * rule in KHz. This is not a center of frequency but an actual regulatory
^^ remove
> + * band edge.
> + * @NL80211_ATTR_REG_RULE_FREQ_END: ending frequency for the regulatory rule
> + * in KHz. This is not a center a frequency but an actual regulatory
^^ remove
> + * band edge.
> + * @NL80211_ATTR_REG_RULE_FREQ_MAX_BW: maximum allowed bandwidth for this
> + * frequency range, in KHz.
> + * @NL80211_ATTR_REG_RULE_POWER_MAX_ANT_GAIN: the maximum allowed antenna gain
> + * for a given frequency range. The value is in mBi (100 * dBi).

Can be left out.

> +/* Central wireless core regulatory domains, we only need two,
> + * the current one and a world regulatory domain in case we have no
> + * information to give us an alpha2 */
> +const struct ieee80211_regdomain *cfg80211_regdomain;

const?

> +const struct ieee80211_regdomain *cfg80211_world_regdom;
> +DEFINE_MUTEX(cfg80211_reg_mutex);
> +
> /* RCU might be appropriate here since we usually
> * only read the list, and that can happen quite
> * often because we need to do it for each command */
> @@ -402,9 +410,30 @@ static struct notifier_block cfg80211_netdev_notifier = {
> .notifier_call = cfg80211_netdev_notifier_call,
> };
>
> +
> static int cfg80211_init(void)
> {
> - int err = wiphy_sysfs_init();
> +#define PASSIVE_AND_NO_IBSS RRF_PASSIVE_SCAN | RRF_NO_IBSS
> + int err;
> + static const struct ieee80211_regdomain world_regdom = {
> + .n_reg_rules = 7, /* I hope we can count */

ARRAY_SIZE() helps you count.

> + .pgp_signature_check = 1, /* Duh, we're already here */
> + .alpha2 = { '0', '0' }, /* Got any better ideas? */

"\0\0" so we can't actually ever use this manually?

> + .reg_rules = {
> + REG_RULE(2402, 2472, 40, 0, 20, RRF_PASSIVE_SCAN),
> + REG_RULE(2474, 2494, 40, 0, 20, PASSIVE_AND_NO_IBSS),
> + REG_RULE(5160, 5240, 40, 0, 30, PASSIVE_AND_NO_IBSS),
> + REG_RULE(5250, 5330, 40, 0, 30, PASSIVE_AND_NO_IBSS),
> + REG_RULE(5735, 5835, 40, 0, 30, PASSIVE_AND_NO_IBSS),
> + REG_RULE(5490, 5710, 40, 0, 30, PASSIVE_AND_NO_IBSS),
> + REG_RULE(5490, 5710, 40, 0, 30, PASSIVE_AND_NO_IBSS)
> + }
> + };

#undef PASSIVE_AND_NOIBSS

why not declare this statically in the file rather than statically in
the init function?

> + cfg80211_regdomain = NULL;
> + cfg80211_world_regdom = (struct ieee80211_regdomain *) &world_regdom;

What's with the cast?

> --- a/net/wireless/core.h
> +++ b/net/wireless/core.h
> @@ -11,6 +11,7 @@
> #include <net/genetlink.h>
> #include <net/wireless.h>
> #include <net/cfg80211.h>
> +#include "reg.h"

Hmm?

> --- a/net/wireless/nl80211.c
> +++ b/net/wireless/nl80211.c
> @@ -18,6 +18,7 @@
> #include <net/cfg80211.h>
> #include "core.h"
> #include "nl80211.h"
> +#include "reg.h"
>
> /* the netlink family */
> static struct genl_family nl80211_fam = {
> @@ -87,6 +88,16 @@ static struct nla_policy nl80211_policy[NL80211_ATTR_MAX+1] __read_mostly = {
> [NL80211_ATTR_MESH_ID] = { .type = NLA_BINARY,
> .len = IEEE80211_MAX_MESH_ID_LEN },
> [NL80211_ATTR_MPATH_NEXT_HOP] = { .type = NLA_U32 },
> +
> + [NL80211_ATTR_REG_RULE_ALPHA2] = { .type = NLA_NUL_STRING, .len = 3 },

Why does it have to be a NUL-string? It is always only two bytes, so
please just do that. No need to null-terminate.

> +static int nl80211_get_reg(struct sk_buff *skb, struct genl_info *info)
> +{
> + return -EOPNOTSUPP;
> +}
> +
> +static int nl80211_set_reg(struct sk_buff *skb, struct genl_info *info)
> +{
> + return -EOPNOTSUPP;
> +}

-ECODEMISSING :P

> @@ -9,137 +10,93 @@
> */
>
> /*
> - * This regulatory domain control implementation is highly incomplete, it
> - * only exists for the purpose of not regressing mac80211.
> - *
> - * For now, drivers can restrict the set of allowed channels by either
> - * not registering those channels or setting the IEEE80211_CHAN_DISABLED
> - * flag; that flag will only be *set* by this code, never *cleared.
> - *
> * The usual implementation is for a driver to read a device EEPROM to
> * determine which regulatory domain it should be operating under, then
> * looking up the allowable channels in a driver-local table and finally
> * registering those channels in the wiphy structure.
> *
> - * Alternatively, drivers that trust the regulatory domain control here
> - * will register a complete set of capabilities and the control code
> - * will restrict the set by setting the IEEE80211_CHAN_* flags.

I think you shouldn't remove that but just rewrite it to make the driver
trust crda instead of this code.

> + * Another set of compliance enforcement is for drivers to use their
> + * own compliance limits which can be stored on the EEPROM. The host
> + * driver or firmware may ensure these are used.
> + *
> + * In addition to all this we provide an extra layer of regulatory
> + * compliance, however, that of what is provided by CRDA. This tries
> + * to ensure we remain complaint by not letting the user up front try
^^^^^^^^^ heh

> + * to set their wireless device to settings we know we shouldn't be
> + * setting to. We also disable channels on the driver which we know we
> + * shouldn't be using in current regulatory domain. This is because
> + * some drivers may have outdated EEPROMs or no regulatory control at all.

> +static int freq_in_range(const struct ieee80211_freq_range *freq_range, u32 freq)
> +{
> + if (freq >= freq_range->start_freq &&
> + freq <= freq_range->end_freq)
> + return 1;
> + return 0;
> }

That needs a bandwidth check I think.

> +++ b/net/wireless/reg.h
> @@ -0,0 +1,56 @@
> +#ifndef __NET_WIRELESS_REG_H
> +#define __NET_WIRELESS_REG_H
> +
> +extern const struct ieee80211_regdomain *cfg80211_regdomain;
> +extern const struct ieee80211_regdomain *cfg80211_world_regdom;
> +
> +struct ieee80211_freq_range {
> + u32 start_freq;
> + u32 end_freq;
> + u32 max_bandwidth;
> +};
> +
> +struct ieee80211_power_rule {
> + u32 max_antenna_gain;
> + u32 max_eirp;
> +};
> +
> +/* These must match CRDA regdb.h reg_rule_flags. nl80211
> + * must also be synced. */

I think we should use the internal flags directly in nl80211 and have
nl80211 use flag-attributes in a nested flags attribute (see how monitor
flags work) and not rely on any shared bit meanings with userspace.

> +struct ieee80211_reg_rule {
> + struct ieee80211_freq_range freq_range;
> + struct ieee80211_power_rule power_rule;
> + u32 flags;
> +};
> +
> +struct ieee80211_regdomain {
> + u32 n_reg_rules;
> + int pgp_signature_check;
> + char alpha2[2];
> + struct ieee80211_reg_rule reg_rules[];

Make that a pointer instead so we can declare the array and point to it
and use ARRAY_SIZE().

johannes

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]