Re: [PATCH v2] wireless: mark expected switch fall-throughs

[Johannes Berg]

On Tue, 2018-10-23 at 12:58 +0200, Gustavo A. R. Silva wrote:
> On 10/23/18 10:59 AM, Gustavo A. R. Silva wrote:
> > 
> > On 10/23/18 9:01 AM, Johannes Berg wrote:
> > > On Tue, 2018-10-23 at 02:13 +0200, Gustavo A. R. Silva wrote:
> > > > In preparation to enabling -Wimplicit-fallthrough, mark switch cases
> > > > where we are expecting to fall through.
> > > > 
> > > > Warning level 3 was used: -Wimplicit-fallthrough=3
> > > > 
> > > > This code was not tested and GCC 7.2.0 was used to compile it.
> > > 
> > > Look, I'm not going to make this any clearer: I'm not applying patches
> > > like that where you've invested no effort whatsoever on verifying that
> > > they're correct.
> > > 
> > 
> > How do you suggest me to verify that every part is correct in this type
> > of patches?
> > 
> 
> BTW... I'm under the impression you think that I don't even look at
> the code. Is that correct?

That's what your commit log looks like, yes. This is your full commit
log:

   In preparation to enabling -Wimplicit-fallthrough, mark switch cases
   where we are expecting to fall through.

   Warning level 3 was used: -Wimplicit-fallthrough=3

   This code was not tested and GCC 7.2.0 was used to compile it.

   For all I know, you could've run spatch to add the comments wherever
   there was no break, and then compiled it once.

   > I've been working on this for quite a while, and in every case I try to
> understand the code in terms of the context in which every warning is
> reported.

That's great.

> Here is a bug I found yesterday at 
> drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c
> 
> 5690 case WLAN_CIPHER_SUITE_CCMP:
> 5691 key->flags |= IEEE80211_KEY_FLAG_SW_MGMT_TX;
> 5692 break;
> 5693 case WLAN_CIPHER_SUITE_TKIP:
> 5694 key->flags |= IEEE80211_KEY_FLAG_GENERATE_MMIC;
> 5695 default:
> 5696 return -EOPNOTSUPP;
> 5697 }

Indeed, that looks like a bug, although kinda benign since it just means
TKIP will always use software crypto and TKIP is slow anyway ;-)

> I do this analysis for every warning.  Now, when I say I haven't tested the 
> code
> is because I don't have any log as evidence for anything. Not that I haven't 
> put
> any effort on trying to understand it and its context.  When I started 
> working on
> this task there were more than 2000 of these issues, now there are around 600 
> left.
> 
> I have fixed many bugs on the way, so a good amount of work is being invested 
> on
> this, and it's paying off. :)

:-)

> Now, let me ask you this question:
> 
> It would be easier for you to review this patch if I turn it into a series?
> 
> I can do that without a problem.

I'd be happy if you were to actually just mention that you've looked at
them, and found them to be expected fall throughs. I'll still review
them, but without that information I feel like I'm doing the first round
of reviews this code ever got.

johannes

pull-request: mac80211 2018-09-27

[Johannes Berg]

Hi Dave,

Here's another - unfortunately pretty large - set of fixes
for the current cycle. The changes are pretty simple or even
trivial though.

Please pull and let me know if there's any problem.

Thanks,
johannes



The following changes since commit 28619527b8a712590c93d0a9e24b4425b9376a8c:

  Merge git://git.kernel.org/pub/scm/linux/kernel/git/davem/net (2018-09-04 
12:45:11 -0700)

are available in the Git repository at:

  git://git.kernel.org/pub/scm/linux/kernel/git/jberg/mac80211.git 
tags/mac80211-for-davem-2018-09-27

for you to fetch changes up to 1222a16014888ed9733c11e221730d4a8196222b:

  nl80211: Fix possible Spectre-v1 for CQM RSSI thresholds (2018-09-27 11:44:44 
+0200)


More patches than I'd like perhaps, but each seems reasonable:
 * two new spectre-v1 mitigations in nl80211
 * TX status fix in general, and mesh in particular
 * powersave vs. offchannel fix
 * regulatory initialization fix
 * fix for a queue hang due to a bad return value
 * allocate TXQs for active monitor interfaces, fixing my
   earlier patch to avoid unnecessary allocations where I
   missed this case needed them
 * fix TDLS data frames priority assignment
 * fix scan results processing to take into account duplicate
   channel numbers (over different operating classes, but we
   don't necessarily know the operating class)
 * various hwsim fixes for radio destruction and new radio
   announcement messages
 * remove an extraneous kernel-doc line


Andrei Otcheretianski (3):
  mac80211: Always report TX status
  mac80211: Don't wake up from PS for offchannel TX
  cfg80211: reg: Init wiphy_idx in regulatory_hint_core()

Bob Copeland (1):
  mac80211: fix pending queue hang due to TX_DROP

Felix Fietkau (1):
  mac80211: allocate TXQs for active monitor interfaces

Johannes Berg (1):
  mac80211: TDLS: fix skb queue/priority assignment

Jouni Malinen (1):
  cfg80211: Address some corner cases in scan result channel updating

Martin Willi (3):
  mac80211_hwsim: fix locking when iterating radios during ns exit
  mac80211_hwsim: fix race in radio destruction from netlink notifier
  mac80211_hwsim: do not omit multicast announce of first added radio

Masashi Honma (2):
  nl80211: Fix possible Spectre-v1 for NL80211_TXRATE_HT
  nl80211: Fix possible Spectre-v1 for CQM RSSI thresholds

Randy Dunlap (1):
  cfg80211: fix reg_query_regdb_wmm kernel-doc

Yuan-Chi Pang (1):
  mac80211: fix TX status reporting for ieee80211s

 drivers/net/wireless/mac80211_hwsim.c | 36 ++
 include/net/cfg80211.h|  2 --
 net/mac80211/iface.c  |  3 +-
 net/mac80211/mesh.h   |  3 +-
 net/mac80211/mesh_hwmp.c  |  9 ++
 net/mac80211/status.c | 11 +++
 net/mac80211/tdls.c   |  8 ++---
 net/mac80211/tx.c |  6 +++-
 net/wireless/nl80211.c| 20 +---
 net/wireless/reg.c|  1 +
 net/wireless/scan.c   | 58 +--
 11 files changed, 103 insertions(+), 54 deletions(-)

Re: [PATCH v3 0/5] netlink: nested policy validation

[Johannes Berg]

On Wed, 2018-09-26 at 10:21 -0700, David Miller wrote:
> From: Johannes Berg 
> Date: Wed, 26 Sep 2018 11:15:29 +0200
> 
> > This adds nested policy validation, which lets you specify the
> > nested attribute type, e.g. NLA_NESTED with sub-policy, or the
> > new NLA_NESTED_ARRAY with sub-sub-policy.
> > 
> > 
> > Changes in v2:
> >  * move setting the bad attr pointer/message into validate_nla()
> >  * remove the recursion patch since that's no longer needed
> >  * simply skip the generic bad attr pointer/message setting in
> >case of nested nla_validate() failing since that could fail
> >only due to validate_nla() failing inside, which already sets
> >the extack information
> > 
> > Changes in v3:
> >  * fix NLA_REJECT to have an error message if none is in policy
> 
> Looks great Johannes, series applied.

Sorry to nag, but I see patches that you replied to later than this in
the tree, but not this.

Or did you see something wrong with this later and dropped it?

johannes

[PATCH] netlink: fix typo in nla_parse_nested() comment

[Johannes Berg]

From: Johannes Berg 

Fix a simple typo: attribuets -> attributes

Signed-off-by: Johannes Berg 
---
 include/net/netlink.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/net/netlink.h b/include/net/netlink.h
index ddabc832febc..69866f5a97f2 100644
--- a/include/net/netlink.h
+++ b/include/net/netlink.h
@@ -153,7 +153,7 @@
  *   nla_find()find attribute in stream of 
attributes
  *   nla_find_nested() find attribute in nested attributes
  *   nla_parse()   parse and validate stream of attrs
- *   nla_parse_nested()parse nested attribuets
+ *   nla_parse_nested()parse nested attributes
  *   nla_for_each_attr()   loop over all attributes
  *   nla_for_each_nested() loop over the nested attributes
  *=
-- 
2.14.4

[PATCH v3 3/5] netlink: move extack setting into validate_nla()

[Johannes Berg]

From: Johannes Berg 

This unifies the code between nla_parse() which sets the bad
attribute pointer and an error message, and nla_validate()
which only sets the bad attribute pointer.

It also cleans up the code for NLA_REJECT and paves the way
for nested policy validation, as it will allow us to easily
skip setting the "generic" message without any extra args
like the **error_msg now, just passing the extack through is
now enough.

While at it, remove the unnecessary label in nla_parse().

Suggested-by: David Ahern 
Signed-off-by: Johannes Berg 
---
 lib/nlattr.c | 68 
 1 file changed, 36 insertions(+), 32 deletions(-)

diff --git a/lib/nlattr.c b/lib/nlattr.c
index e2e5b38394d5..6e03d650bec4 100644
--- a/lib/nlattr.c
+++ b/lib/nlattr.c
@@ -69,10 +69,11 @@ static int validate_nla_bitfield32(const struct nlattr *nla,
 
 static int validate_nla(const struct nlattr *nla, int maxtype,
const struct nla_policy *policy,
-   const char **error_msg)
+   struct netlink_ext_ack *extack)
 {
const struct nla_policy *pt;
int minlen = 0, attrlen = nla_len(nla), type = nla_type(nla);
+   int err = -ERANGE;
 
if (type <= 0 || type > maxtype)
return 0;
@@ -90,24 +91,31 @@ static int validate_nla(const struct nlattr *nla, int 
maxtype,
switch (pt->type) {
case NLA_EXACT_LEN:
if (attrlen != pt->len)
-   return -ERANGE;
+   goto out_err;
break;
 
case NLA_REJECT:
-   if (pt->validation_data && error_msg)
-   *error_msg = pt->validation_data;
-   return -EINVAL;
+   if (extack && pt->validation_data) {
+   NL_SET_BAD_ATTR(extack, nla);
+   extack->_msg = pt->validation_data;
+   return -EINVAL;
+   }
+   err = -EINVAL;
+   goto out_err;
 
case NLA_FLAG:
if (attrlen > 0)
-   return -ERANGE;
+   goto out_err;
break;
 
case NLA_BITFIELD32:
if (attrlen != sizeof(struct nla_bitfield32))
-   return -ERANGE;
+   goto out_err;
 
-   return validate_nla_bitfield32(nla, pt->validation_data);
+   err = validate_nla_bitfield32(nla, pt->validation_data);
+   if (err)
+   goto out_err;
+   break;
 
case NLA_NUL_STRING:
if (pt->len)
@@ -115,13 +123,15 @@ static int validate_nla(const struct nlattr *nla, int 
maxtype,
else
minlen = attrlen;
 
-   if (!minlen || memchr(nla_data(nla), '\0', minlen) == NULL)
-   return -EINVAL;
+   if (!minlen || memchr(nla_data(nla), '\0', minlen) == NULL) {
+   err = -EINVAL;
+   goto out_err;
+   }
/* fall through */
 
case NLA_STRING:
if (attrlen < 1)
-   return -ERANGE;
+   goto out_err;
 
if (pt->len) {
char *buf = nla_data(nla);
@@ -130,13 +140,13 @@ static int validate_nla(const struct nlattr *nla, int 
maxtype,
attrlen--;
 
if (attrlen > pt->len)
-   return -ERANGE;
+   goto out_err;
}
break;
 
case NLA_BINARY:
if (pt->len && attrlen > pt->len)
-   return -ERANGE;
+   goto out_err;
break;
 
case NLA_NESTED:
@@ -152,10 +162,13 @@ static int validate_nla(const struct nlattr *nla, int 
maxtype,
minlen = nla_attr_minlen[pt->type];
 
if (attrlen < minlen)
-   return -ERANGE;
+   goto out_err;
}
 
return 0;
+out_err:
+   NL_SET_ERR_MSG_ATTR(extack, nla, "Attribute failed policy validation");
+   return err;
 }
 
 /**
@@ -180,12 +193,10 @@ int nla_validate(const struct nlattr *head, int len, int 
maxtype,
int rem;
 
nla_for_each_attr(nla, head, len, rem) {
-   int err = validate_nla(nla, maxtype, policy, NULL);
+   int err = validate_nla(nla, maxtype, policy, extack);
 
-   if (err < 0) {
-   NL_SET_BAD_ATTR(extack, nla);
+   if (err < 0)
return err;
-   }
}
 
return 0;
@@ -241,7 +252,7 @@ int nla_parse(struct nlattr **tb, int ma

[PATCH v3 5/5] netlink: add nested array policy validation

[Johannes Berg]

From: Johannes Berg 

Sometimes nested netlink attributes are just used as arrays, with
the nla_type() of each not being used; we have this in nl80211 and
e.g. NFTA_SET_ELEM_LIST_ELEMENTS.

Add the ability to validate this type of message directly in the
policy, by adding the type NLA_NESTED_ARRAY which does exactly
this: require a first level of nesting but ignore the attribute
type, and then inside each require a second level of nested and
validate those attributes against a given policy (if present).

Note that some nested array types actually require that all of
the entries have the same index, this is possible to express in
a nested policy already, apart from the validation that only the
one allowed type is used.

Signed-off-by: Johannes Berg 
---
 include/net/netlink.h | 12 +++-
 lib/nlattr.c  | 51 +++
 2 files changed, 62 insertions(+), 1 deletion(-)

diff --git a/include/net/netlink.h b/include/net/netlink.h
index 91907852da1c..3698ca8ff92c 100644
--- a/include/net/netlink.h
+++ b/include/net/netlink.h
@@ -172,6 +172,7 @@ enum {
NLA_FLAG,
NLA_MSECS,
NLA_NESTED,
+   NLA_NESTED_ARRAY,
NLA_NUL_STRING,
NLA_BINARY,
NLA_S8,
@@ -200,7 +201,8 @@ enum {
  *NLA_NUL_STRING   Maximum length of string (excluding NUL)
  *NLA_FLAG Unused
  *NLA_BINARY   Maximum length of attribute payload
- *NLA_NESTED   Length verification is done by checking len of
+ *NLA_NESTED,
+ *NLA_NESTED_ARRAY Length verification is done by checking len of
  * nested header (or empty); len field is used if
  * validation_data is also used, for the max attr
  * number in the nested policy.
@@ -230,6 +232,12 @@ enum {
  * `len' to the max attribute number.
  * Note that nla_parse() will validate, but of course 
not
  * parse, the nested sub-policies.
+ *NLA_NESTED_ARRAY Points to a nested policy to validate, must also set
+ * `len' to the max attribute number. The difference to
+ * NLA_NESTED is the structure - NLA_NESTED has the
+ * nested attributes directly inside, while an array 
has
+ * the nested attributes at another level down and the
+ * attributes directly in the nesting don't matter.
  *All otherUnused
  *
  * Example:
@@ -255,6 +263,8 @@ struct nla_policy {
 
 #define NLA_POLICY_NESTED(maxattr, policy) \
{ .type = NLA_NESTED, .validation_data = policy, .len = maxattr }
+#define NLA_POLICY_NESTED_ARRAY(maxattr, policy) \
+   { .type = NLA_NESTED_ARRAY, .validation_data = policy, .len = maxattr }
 
 /**
  * struct nl_info - netlink source information
diff --git a/lib/nlattr.c b/lib/nlattr.c
index 04750f88477c..2f8feff669a7 100644
--- a/lib/nlattr.c
+++ b/lib/nlattr.c
@@ -67,6 +67,34 @@ static int validate_nla_bitfield32(const struct nlattr *nla,
return 0;
 }
 
+static int nla_validate_array(const struct nlattr *head, int len, int maxtype,
+ const struct nla_policy *policy,
+ struct netlink_ext_ack *extack)
+{
+   const struct nlattr *entry;
+   int rem;
+
+   nla_for_each_attr(entry, head, len, rem) {
+   int ret;
+
+   if (nla_len(entry) == 0)
+   continue;
+
+   if (nla_len(entry) < NLA_HDRLEN) {
+   NL_SET_ERR_MSG_ATTR(extack, entry,
+   "Array element too short");
+   return -ERANGE;
+   }
+
+   ret = nla_validate(nla_data(entry), nla_len(entry),
+  maxtype, policy, extack);
+   if (ret < 0)
+   return ret;
+   }
+
+   return 0;
+}
+
 static int validate_nla(const struct nlattr *nla, int maxtype,
const struct nla_policy *policy,
struct netlink_ext_ack *extack)
@@ -169,6 +197,29 @@ static int validate_nla(const struct nlattr *nla, int 
maxtype,
}
}
break;
+   case NLA_NESTED_ARRAY:
+   /* a nested array attribute is allowed to be empty; if its not,
+* it must have a size of at least NLA_HDRLEN.
+*/
+   if (attrlen == 0)
+   break;
+   if (attrlen < NLA_HDRLEN)
+   goto out_err;
+   if (pt->validation_data) {
+   int err;
+
+   err = nla_validate_array(nla_data(nla), nla_len(nla),
+pt->

[PATCH v3 1/5] netlink: remove NLA_NESTED_COMPAT

[Johannes Berg]

From: Johannes Berg 

This isn't used anywhere, so we might as well get rid of it.

Reviewed-by: David Ahern 
Signed-off-by: Johannes Berg 
---
 include/net/netlink.h |  2 --
 lib/nlattr.c  | 11 ---
 2 files changed, 13 deletions(-)

diff --git a/include/net/netlink.h b/include/net/netlink.h
index 318b1ded3833..b680fe365e91 100644
--- a/include/net/netlink.h
+++ b/include/net/netlink.h
@@ -172,7 +172,6 @@ enum {
NLA_FLAG,
NLA_MSECS,
NLA_NESTED,
-   NLA_NESTED_COMPAT,
NLA_NUL_STRING,
NLA_BINARY,
NLA_S8,
@@ -203,7 +202,6 @@ enum {
  *NLA_BINARY   Maximum length of attribute payload
  *NLA_NESTED   Don't use `len' field -- length verification is
  * done by checking len of nested header (or empty)
- *NLA_NESTED_COMPATMinimum length of structure payload
  *NLA_U8, NLA_U16,
  *NLA_U32, NLA_U64,
  *NLA_S8, NLA_S16,
diff --git a/lib/nlattr.c b/lib/nlattr.c
index bb6fe5ed4ecf..120ad569e13d 100644
--- a/lib/nlattr.c
+++ b/lib/nlattr.c
@@ -140,17 +140,6 @@ static int validate_nla(const struct nlattr *nla, int 
maxtype,
return -ERANGE;
break;
 
-   case NLA_NESTED_COMPAT:
-   if (attrlen < pt->len)
-   return -ERANGE;
-   if (attrlen < NLA_ALIGN(pt->len))
-   break;
-   if (attrlen < NLA_ALIGN(pt->len) + NLA_HDRLEN)
-   return -ERANGE;
-   nla = nla_data(nla) + NLA_ALIGN(pt->len);
-   if (attrlen < NLA_ALIGN(pt->len) + NLA_HDRLEN + nla_len(nla))
-   return -ERANGE;
-   break;
case NLA_NESTED:
/* a nested attributes is allowed to be empty; if its not,
 * it must have a size of at least NLA_HDRLEN.
-- 
2.14.4

[PATCH v3 2/5] netlink: make validation_data const

[Johannes Berg]

From: Johannes Berg 

The validation data is only used within the policy that
should usually already be const, and isn't changed in any
code that uses it. Therefore, make the validation_data
pointer const.

While at it, remove the duplicate variable in the bitfield
validation that I'd otherwise have to change to const.

Reviewed-by: David Ahern 
Signed-off-by: Johannes Berg 
---
 include/net/netlink.h | 2 +-
 lib/nlattr.c  | 5 ++---
 2 files changed, 3 insertions(+), 4 deletions(-)

diff --git a/include/net/netlink.h b/include/net/netlink.h
index b680fe365e91..0d698215d4d9 100644
--- a/include/net/netlink.h
+++ b/include/net/netlink.h
@@ -237,7 +237,7 @@ enum {
 struct nla_policy {
u16 type;
u16 len;
-   void*validation_data;
+   const void *validation_data;
 };
 
 #define NLA_POLICY_EXACT_LEN(_len) { .type = NLA_EXACT_LEN, .len = _len }
diff --git a/lib/nlattr.c b/lib/nlattr.c
index 120ad569e13d..e2e5b38394d5 100644
--- a/lib/nlattr.c
+++ b/lib/nlattr.c
@@ -45,12 +45,11 @@ static const u8 nla_attr_minlen[NLA_TYPE_MAX+1] = {
 };
 
 static int validate_nla_bitfield32(const struct nlattr *nla,
-  u32 *valid_flags_allowed)
+  const u32 *valid_flags_mask)
 {
const struct nla_bitfield32 *bf = nla_data(nla);
-   u32 *valid_flags_mask = valid_flags_allowed;
 
-   if (!valid_flags_allowed)
+   if (!valid_flags_mask)
return -EINVAL;
 
/*disallow invalid bit selector */
-- 
2.14.4

[PATCH v3 4/5] netlink: allow NLA_NESTED to specify nested policy to validate

[Johannes Berg]

From: Johannes Berg 

Now that we have a validation_data pointer, and the len field in
the policy is unused for NLA_NESTED, we can allow using them both
to have nested validation. This can be nice in code, although we
still have to use nla_parse_nested() or similar which would also
take a policy; however, it also serves as documentation in the
policy without requiring a look at the code.

Signed-off-by: Johannes Berg 
---
 include/net/netlink.h | 13 +++--
 lib/nlattr.c  | 14 ++
 2 files changed, 25 insertions(+), 2 deletions(-)

diff --git a/include/net/netlink.h b/include/net/netlink.h
index 0d698215d4d9..91907852da1c 100644
--- a/include/net/netlink.h
+++ b/include/net/netlink.h
@@ -200,8 +200,10 @@ enum {
  *NLA_NUL_STRING   Maximum length of string (excluding NUL)
  *NLA_FLAG Unused
  *NLA_BINARY   Maximum length of attribute payload
- *NLA_NESTED   Don't use `len' field -- length verification is
- * done by checking len of nested header (or empty)
+ *NLA_NESTED   Length verification is done by checking len of
+ * nested header (or empty); len field is used if
+ * validation_data is also used, for the max attr
+ * number in the nested policy.
  *NLA_U8, NLA_U16,
  *NLA_U32, NLA_U64,
  *NLA_S8, NLA_S16,
@@ -224,6 +226,10 @@ enum {
  *NLA_REJECT   This attribute is always rejected and validation 
data
  * may point to a string to report as the error instead
  * of the generic one in extended ACK.
+ *NLA_NESTED   Points to a nested policy to validate, must also set
+ * `len' to the max attribute number.
+ * Note that nla_parse() will validate, but of course 
not
+ * parse, the nested sub-policies.
  *All otherUnused
  *
  * Example:
@@ -247,6 +253,9 @@ struct nla_policy {
 #define NLA_POLICY_ETH_ADDRNLA_POLICY_EXACT_LEN(ETH_ALEN)
 #define NLA_POLICY_ETH_ADDR_COMPAT NLA_POLICY_EXACT_LEN_WARN(ETH_ALEN)
 
+#define NLA_POLICY_NESTED(maxattr, policy) \
+   { .type = NLA_NESTED, .validation_data = policy, .len = maxattr }
+
 /**
  * struct nl_info - netlink source information
  * @nlh: Netlink message header of original request
diff --git a/lib/nlattr.c b/lib/nlattr.c
index 6e03d650bec4..04750f88477c 100644
--- a/lib/nlattr.c
+++ b/lib/nlattr.c
@@ -155,6 +155,20 @@ static int validate_nla(const struct nlattr *nla, int 
maxtype,
 */
if (attrlen == 0)
break;
+   if (attrlen < NLA_HDRLEN)
+   goto out_err;
+   if (pt->validation_data) {
+   err = nla_validate(nla_data(nla), nla_len(nla), pt->len,
+  pt->validation_data, extack);
+   if (err < 0) {
+   /*
+* return directly to preserve the inner
+* error message/attribute pointer
+*/
+   return err;
+   }
+   }
+   break;
default:
if (pt->len)
minlen = pt->len;
-- 
2.14.4

[PATCH v3 0/5] netlink: nested policy validation

[Johannes Berg]

This adds nested policy validation, which lets you specify the
nested attribute type, e.g. NLA_NESTED with sub-policy, or the
new NLA_NESTED_ARRAY with sub-sub-policy.


Changes in v2:
 * move setting the bad attr pointer/message into validate_nla()
 * remove the recursion patch since that's no longer needed
 * simply skip the generic bad attr pointer/message setting in
   case of nested nla_validate() failing since that could fail
   only due to validate_nla() failing inside, which already sets
   the extack information

Changes in v3:
 * fix NLA_REJECT to have an error message if none is in policy

johannes

Re: [RFC 4/5] netlink: prepare validate extack setting for recursion

[Johannes Berg]

On Wed, 2018-09-19 at 18:10 -0300, Marcelo Ricardo Leitner wrote:

> > FWIW, if you do think that there's a need for distinguishing this, then
> > I'd argue that perhaps the right way to address this would be to extend
> > this all the way to userspace and have two separate attributes for
> > errors and warnings in the extended ACK message?
> 
> Likely, yes. And perhaps even support multiple messages. That way we
> could, for example, parse all attributes and return a list of the all
> the offending ones, instead of returning one at a time. Net benefit?
> Not clear.. over engineering? Possibly.

Not sure I'd want to continue parsing after hitting something that's
considered garbage? It might be that the attribute length field is
corrupted and the data is actually fine, or something, and then
continuing to parse would just lead to more errors over and over again.

Also, I'd worry about being able to "blow up" the message, if we get a
text & bad attribute for everything that's wrong, it's pretty easy to
create a sort of "message amplification" and we'd probably have to
defend against that in terms of limiting memory allocation for all the
possible errors etc. Not sure I'd want to go there.

> I agree with you that in general we should not need this.

:-)

> > I'm still not really sure what the use case for a warning is, so not
> > sure I can really comment on this.
> 
> Good point. From iproute POV, a warning is a non-fatal message. From
> an user perspective, that probably translates are nothing because in
> the end the command actually worked. :-)
> 
> Seriously, I do think it's more of a hint for developers than anyone
> else.

I guess we also have the (ratelimited) messages from the kernel, like
the one saying you have extra bytes after your attributes at the end of
the message. Not sure which makes more sense, depends on the specific
case you'd use this in I guess.


Anyway - we got into this discussion because of all the extra recursion
stuff I was adding. With the change suggested by David we don't need
that now at all, so I guess it'd be better to propose a patch if you (or
perhaps I will see a need later) need such a facility for multiple
messages or multiple message levels?

johannes

[PATCH] smc: generic netlink family should be __ro_after_init

[Johannes Berg]

From: Johannes Berg 

The generic netlink family is only initialized during module init,
so it should be __ro_after_init like all other generic netlink
families.

Signed-off-by: Johannes Berg 
---
 net/smc/smc_pnet.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/smc/smc_pnet.c b/net/smc/smc_pnet.c
index 01c6ce042a1c..7cb3e4f07c10 100644
--- a/net/smc/smc_pnet.c
+++ b/net/smc/smc_pnet.c
@@ -461,7 +461,7 @@ static const struct genl_ops smc_pnet_ops[] = {
 };
 
 /* SMC_PNETID family definition */
-static struct genl_family smc_pnet_nl_family = {
+static struct genl_family smc_pnet_nl_family __ro_after_init = {
.hdrsize = 0,
.name = SMCR_GENL_FAMILY_NAME,
.version = SMCR_GENL_FAMILY_VERSION,
-- 
2.14.4

Re: [RFC 4/5] netlink: prepare validate extack setting for recursion

[Johannes Berg]

On Wed, 2018-09-19 at 15:46 -0300, Marcelo Ricardo Leitner wrote:

> > NL_SET_ERR_MSG(extack, "warning: deprecated command");
> > err = nla_parse(..., extack);
> > if (err)
> > return err;
> > /* do something */
> > return 0;
> > 
> > Here you could consider the message there a warning that's transported
> > out even if we return 0, but if we return with a failure from
> > nla_parse() (or nla_validate instead if you wish), then that failure
> > message "wins".
> 
> Agree. This is the core issue here, IMHO. Once out of the context that
> set the message, we have no way of knowing if we can nor should
> overwrite the message that is already in there.

True.

I'm not really sure I'd go as far as calling it an issue though.

IMHO, we really get into this situation if the code is badly structured
to start with. Taking the above code, if you write it as

err = nla_parse(...);
if (err)
return err;
/* do something */
NLA_SET_ERR_MSG(extack, "warning: deprecated command");
return 0;

instead, then you have no such problems.

Well, perhaps you do, if "/* do something */" might *also* set the
message, but basically you have to statically decide which one wins by
ordering the code correctly.

I'm still not convinced, btw, that we actually have any instance in the
kernel today of the issue we've been discussing - namely that some code
does something like the original quoted code at the top of the email.

> > I suppose we could - technically - make that generic, in that we could
> > have both
> > 
> >   NLA_SET_WARN_MSG(extack, "...");
> >   NLA_SET_ERR_MSG(extack, "...");
> 
> I like this.

I'm not really sure what for though :-)

FWIW, if you do think that there's a need for distinguishing this, then
I'd argue that perhaps the right way to address this would be to extend
this all the way to userspace and have two separate attributes for
errors and warnings in the extended ACK message?

> > and keep track of warning vs. error; however, just like my first version
> > of the NLA_REJECT patch, that would break existing code.
> 
> Hm, I may have missed something but I think the discussion in there
> was for a different context. For an extack msg to be set by
> when validate_nla() call returns on nla_parse(), the previous message
> had to be a "warning" because otherwise the parsing wouldn't be even
> attempted. So in that case, we are safe to simply overwrite it.

Yes, arguably, if you really had an "error" then you'd probably have
never gotten to the parsing. It's possible - although sort of stupid -
to write this code too though:

NL_SET_ERR_MSG(extack, "error: unsupported command");
err = nla_parse(...);
return err ? : -EOPNOTSUPP;

Which one should win in that case?

Again, in my opinion we've got enough flexibility if we let
nla_parse/nla_validate behave as today (mostly, nla_validate doesn't set
a message and I'd like to change that, it does set the error attribute
pointer/offset) and let the calling code sort it out.

In many cases nla_parse() will be called by generic code (e.g.
genetlink) and you'd never get into this situation. Once you're past
that, you  can do whatever you like.

> While for the situation you are describing here, it will set a generic
> error message in case the inner code didn't do it.

Yes, but that's not a change in semantics as far as the caller of
nla_parse/nla_validate is concerned - it'll continue to unconditionally
set a message if an error occurred, only the internal behaviour as to
which message seems more relevant is at issue, and the whole recursion
thing and avoiding an outer one overwriting an inner one is IMHO more
useful because that's the more specific error.

> Using the semantics of NLA_SET_WARN_MSG and ERR, then WARN would
> replace other WARNs but not ERRs, and ERR would replace other WARNs
> too but not other ERRs. All we need to do handle this is a bit in
> extack saying if the message is considered a warning or not, or an
> error/fatal message or not.

I'm still not really sure what the use case for a warning is, so not
sure I can really comment on this.

> Okay but we have split parsing of netlink messages and this could be
> useful in there too:
> In cls_api.c, tc_new_tfilter() calls nmmsg_parse() and do some
> processing, and then handle it to a classifier. cls_flower, for
> example, will then do another parsing. If, for whatever reason, flower
> failed and did not set an extack msg, tc_new_tfilter() could set a
> default error message, but at this moment we can't tell if the msg
> already set was just a warning from the first parsing (or any other
> code before engaging flower) (which we can overwrite) or if it a
> message that flower set (which we should not overwrite).
> 
> Hopefully my description clear.. 8-)
> 
> I think this is the same situation as with the nested parsing you're
> proposing.

Yes, I admit that's the same, just not part of pure policy checking, but

Re: [RFC 4/5] netlink: prepare validate extack setting for recursion

[Johannes Berg]

On Wed, 2018-09-19 at 11:28 +0200, Jiri Benc wrote:

> > It might be possible to do this differently, in theory, but all the ways
> > I've tried to come up with so far made the code vastly more complex.
> 
> Wouldn't still make sense to store the flag in the struct
> netlink_ext_ack, though?

Does it make sense to store a flag there that only has a single,
localized, user? I'd say no.

> The way the parameters are passed around in
> this patch looks ugly. 

Yeah, it's not the best in some ways.

I considered having a cleared new extack on the stack in the outer-most
call (nla_parse/nla_validate), and then we can "set if not already set",
and at the end unconditionally copy/set to the real one... But then I
either need to duplicate that code in both, or pass another argument to
nla_validate_parse() anyway to indicate whether to do this or not (since
the inner calls to it shouldn't, since that would defeat the purpose),
or add another level of function call indirection?

I'm not really sure any of these are better ... and more complex, in
some ways, since we have to copy all the data around.

> And adding more users of the flag later (there
> may be other cases when we want the earlier messages to be preserved)
> would mean adding parameters all around, while the flag in the struct
> would be readily available.

I can't really think of any other users of such a thing?

johannes

Re: [RFC 4/5] netlink: prepare validate extack setting for recursion

[Johannes Berg]

On Wed, 2018-09-19 at 00:37 -0300, Marcelo Ricardo Leitner wrote:

> Did you consider indicating the message level, and only overwrite the
> message that is already in there if the new message level is higher
> than the current one?

Hmm, no, I guess I didn't - I'm not even sure I understand what you're
saying.

This code in itself generates no "warning" messages; that was just a
construct we discussed in the NLA_REJECT thread, e.g. if you say (like I
just also wrote in my reply to Jiri):

NL_SET_ERR_MSG(extack, "warning: deprecated command");
err = nla_parse(..., extack);
if (err)
return err;
/* do something */
return 0;

Here you could consider the message there a warning that's transported
out even if we return 0, but if we return with a failure from
nla_parse() (or nla_validate instead if you wish), then that failure
message "wins".

> This way the first to set an Error message will have it, and Warning
> messages would be overwritten by such if needed. But it also would
> cause the first warning to be held, and not the last one, as it does
> today. We want the first error, but the last warning otherwise.
> 
> It would not be possible to overwrite if new_msglvl >= cur_msglvl
> because then it would trigger the initial issue again, so some extra
> logic would be needed to solve this.

That sounds way more complex than what I'm doing now?

Note, like I said above, this isn't *generic* in any way. This code here
will only ever set error messages that should "win".

I suppose we could - technically - make that generic, in that we could
have both

  NLA_SET_WARN_MSG(extack, "...");
  NLA_SET_ERR_MSG(extack, "...");

and keep track of warning vs. error; however, just like my first version
of the NLA_REJECT patch, that would break existing code.

I also don't think that we actually *need* this complexity in general.
It should almost always be possible (and realistically, pretty easy) to
structure your code in a way that warning messages only go out if no
error message overwrites them. The only reason we were ever even
discussing this was that in NLA_REJECT I missed the fact that somebody
could've set a message before and thus would keep it rather than
overwrite it, which was a change in behaviour.

Now, with this patch, all I'm doing is changing the internal behaviour
of nla_parse/nla_validate - externally, it still overwrites any existing
message if an error occurs, but internally it keeps the inner-most
error.

Why is this? Consider this:

static const struct nla_policy inner_policy[] = {
[INNER_FLAG] = { .type = NLA_REJECT,
 .validation_data = "must not set this flag" }
};

static const struct nla_policy outer_policy[] = {
[OUTER_NESTING] = { .type = NLA_NESTED, .len = INNER_MAX,
.validation_data = inner_policy,
};

Now if you invoke nla_parse/nla_validate with a message like this

[ OUTER_NESTING => [ INNER_FLAG, ... ], ... ]

you'd get "must not set this flag" with the error offset pointing to
that; if I didn't do this construction here with inner messages winning,
you'd get "Attribute failed policy validation" with the error offset
pointing to the "OUTER_NESTING" attribute, that's pretty useless.

>From an external API POV though, nla_validate/nla_parse will continue to
unconditionally overwrite any existing "warning" messages with errors,
if such occurred. They just won't overwrite their own messages when
returning from a nested policy validation.

johannes

Re: [RFC 4/5] netlink: prepare validate extack setting for recursion

[Johannes Berg]

On Wed, 2018-09-19 at 11:10 +0200, Jiri Benc wrote:
> On Tue, 18 Sep 2018 15:12:11 +0200, Johannes Berg wrote:
> >  static int validate_nla(const struct nlattr *nla, int maxtype,
> > const struct nla_policy *policy,
> > -   const char **error_msg)
> > +   struct netlink_ext_ack *extack, bool *extack_set)
> 
> Can't the extack_set be included in the struct netlink_ext_ack? One less
> parameter to pass around and the NL_SET_* macros could check this on
> their own.

No, I don't think it can or should.

For one, having the NL_SET_* macros check it on their own will already
not work - as we discussed over in the NLA_REJECT thread, we do need to
be able to override the data, e.g. if somebody does

NL_SET_ERR_MSG(extack, "warning: deprecated command");
err = nla_parse(..., extack);

and nla_parse() sets a new message. Thus, hiding all the logic in there
already will not work, and is also IMHO rather unexpected. Normally,
*later* messages should win, not *earlier* ones - and that doesn't
require any tracking, just setting them unconditionally.

It's just a side effect of how we do the recursive validation here that
we want *earlier* messages to win (but only within this code piece - if
a previous message was set, we want it to be overwritten by nla_parse!).

It might be possible to do this differently, in theory, but all the ways
I've tried to come up with so far made the code vastly more complex.

johannes

Re: [RFC 2/5] netlink: set extack error message in nla_validate()

[Johannes Berg]

On Tue, 2018-09-18 at 10:18 -0700, David Ahern wrote:
> On 9/18/18 6:12 AM, Johannes Berg wrote:
> > diff --git a/lib/nlattr.c b/lib/nlattr.c
> > index 120ad569e13d..efbd6c1aff29 100644
> > --- a/lib/nlattr.c
> > +++ b/lib/nlattr.c
> > @@ -181,9 +181,13 @@ int nla_validate(const struct nlattr *head, int len, 
> > int maxtype,
> > int rem;
> >  
> > nla_for_each_attr(nla, head, len, rem) {
> > -   int err = validate_nla(nla, maxtype, policy, NULL);
> > +   static const char _msg[] = "Attribute failed policy validation";
> > +   const char *msg = _msg;
> > +   int err = validate_nla(nla, maxtype, policy, );
> >  
> > if (err < 0) {
> > +   if (extack)
> > +   extack->_msg = msg;
> > NL_SET_BAD_ATTR(extack, nla);
> > return err;
> > }
> > 
> 
> I take it this set is on top of another set - the NLA_REJECT?

Yeah, these two:

https://patchwork.ozlabs.org/project/netdev/list/?series=65982

johannes

[RFC 4/5] netlink: prepare validate extack setting for recursion

[Johannes Berg]

From: Johannes Berg 

In one of my previous patches in this area I introduced code
to pass out just the error message to store in the extack, for
use in NLA_REJECT.

Change this code now to set both the error message and the bad
attribute pointer, and carry around a boolean indicating that
the values have been set.

This will be used in the next patch to allow recursive validation
of nested policies, while preserving the innermost error message
rather than overwriting it with a generic out-level message.

Signed-off-by: Johannes Berg 
---
 lib/nlattr.c | 32 
 1 file changed, 20 insertions(+), 12 deletions(-)

diff --git a/lib/nlattr.c b/lib/nlattr.c
index 46a6d79cf2d1..fecc7b834706 100644
--- a/lib/nlattr.c
+++ b/lib/nlattr.c
@@ -70,7 +70,7 @@ static int validate_nla_bitfield32(const struct nlattr *nla,
 
 static int validate_nla(const struct nlattr *nla, int maxtype,
const struct nla_policy *policy,
-   const char **error_msg)
+   struct netlink_ext_ack *extack, bool *extack_set)
 {
const struct nla_policy *pt;
int minlen = 0, attrlen = nla_len(nla), type = nla_type(nla);
@@ -95,8 +95,11 @@ static int validate_nla(const struct nlattr *nla, int 
maxtype,
break;
 
case NLA_REJECT:
-   if (pt->validation_data && error_msg)
-   *error_msg = pt->validation_data;
+   if (pt->validation_data && extack && !*extack_set) {
+   *extack_set = true;
+   extack->_msg = pt->validation_data;
+   NL_SET_BAD_ATTR(extack, nla);
+   }
return -EINVAL;
 
case NLA_FLAG:
@@ -161,24 +164,25 @@ static int validate_nla(const struct nlattr *nla, int 
maxtype,
 
 static int nla_validate_parse(const struct nlattr *head, int len, int maxtype,
  const struct nla_policy *policy,
- struct netlink_ext_ack *extack,
+ struct netlink_ext_ack *extack, bool *extack_set,
  struct nlattr **tb)
 {
const struct nlattr *nla;
int rem;
 
nla_for_each_attr(nla, head, len, rem) {
-   static const char _msg[] = "Attribute failed policy validation";
-   const char *msg = _msg;
u16 type = nla_type(nla);
 
if (policy) {
-   int err = validate_nla(nla, maxtype, policy, );
+   int err = validate_nla(nla, maxtype, policy,
+  extack, extack_set);
 
if (err < 0) {
-   if (extack)
-   extack->_msg = msg;
-   NL_SET_BAD_ATTR(extack, nla);
+   if (!*extack_set) {
+   *extack_set = true;
+   NL_SET_ERR_MSG_ATTR(extack, nla,
+   "Attribute failed 
policy validation");
+   }
return err;
}
}
@@ -208,9 +212,11 @@ int nla_validate(const struct nlattr *head, int len, int 
maxtype,
 const struct nla_policy *policy,
 struct netlink_ext_ack *extack)
 {
+   bool extack_set = false;
int rem;
 
-   rem = nla_validate_parse(head, len, maxtype, policy, extack, NULL);
+   rem = nla_validate_parse(head, len, maxtype, policy,
+extack, _set, NULL);
 
if (rem < 0)
return rem;
@@ -267,11 +273,13 @@ int nla_parse(struct nlattr **tb, int maxtype, const 
struct nlattr *head,
  int len, const struct nla_policy *policy,
  struct netlink_ext_ack *extack)
 {
+   bool extack_set = false;
int rem;
 
memset(tb, 0, sizeof(struct nlattr *) * (maxtype + 1));
 
-   rem = nla_validate_parse(head, len, maxtype, policy, extack, tb);
+   rem = nla_validate_parse(head, len, maxtype, policy,
+extack, _set, tb);
if (rem < 0)
return rem;
 
-- 
2.14.4

[RFC 2/5] netlink: set extack error message in nla_validate()

[Johannes Berg]

From: Johannes Berg 

In nla_parse() we already set this, but it makes sense to
also do it in nla_validate() which already also sets the
bad attribute pointer.

CC: David Ahern 
Signed-off-by: Johannes Berg 
---
 lib/nlattr.c | 6 +-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/lib/nlattr.c b/lib/nlattr.c
index 120ad569e13d..efbd6c1aff29 100644
--- a/lib/nlattr.c
+++ b/lib/nlattr.c
@@ -181,9 +181,13 @@ int nla_validate(const struct nlattr *head, int len, int 
maxtype,
int rem;
 
nla_for_each_attr(nla, head, len, rem) {
-   int err = validate_nla(nla, maxtype, policy, NULL);
+   static const char _msg[] = "Attribute failed policy validation";
+   const char *msg = _msg;
+   int err = validate_nla(nla, maxtype, policy, );
 
if (err < 0) {
+   if (extack)
+   extack->_msg = msg;
NL_SET_BAD_ATTR(extack, nla);
return err;
}
-- 
2.14.4

[RFC 5/5] netlink: allow NLA_NESTED to specify nested policy to validate

[Johannes Berg]

From: Johannes Berg 

Now that we have a validation_data pointer, and the len field in
the policy is unused for NLA_NESTED, we can allow using them both
to have nested validation. This can be nice in code, although we
still have to use nla_parse_nested() or similar which would also
take a policy; however, it also serves as documentation in the
policy without requiring a look at the code.

Signed-off-by: Johannes Berg 
---
 include/net/netlink.h | 10 --
 lib/nlattr.c  | 17 +
 2 files changed, 25 insertions(+), 2 deletions(-)

diff --git a/include/net/netlink.h b/include/net/netlink.h
index b680fe365e91..6efa25a004f5 100644
--- a/include/net/netlink.h
+++ b/include/net/netlink.h
@@ -200,8 +200,10 @@ enum {
  *NLA_NUL_STRING   Maximum length of string (excluding NUL)
  *NLA_FLAG Unused
  *NLA_BINARY   Maximum length of attribute payload
- *NLA_NESTED   Don't use `len' field -- length verification is
- * done by checking len of nested header (or empty)
+ *NLA_NESTED   Length verification is done by checking len of
+ * nested header (or empty); len field is used if
+ * validation_data is also used, for the max attr
+ * number in the nested policy.
  *NLA_U8, NLA_U16,
  *NLA_U32, NLA_U64,
  *NLA_S8, NLA_S16,
@@ -224,6 +226,10 @@ enum {
  *NLA_REJECT   This attribute is always rejected and validation 
data
  * may point to a string to report as the error instead
  * of the generic one in extended ACK.
+ *NLA_NESTED   Points to a nested policy to validate, must also set
+ * `len' to the max attribute number.
+ * Note that nla_parse() will validate, but of course 
not
+ * parse, the nested sub-policies.
  *All otherUnused
  *
  * Example:
diff --git a/lib/nlattr.c b/lib/nlattr.c
index fecc7b834706..4c8c4fffb20d 100644
--- a/lib/nlattr.c
+++ b/lib/nlattr.c
@@ -68,6 +68,11 @@ static int validate_nla_bitfield32(const struct nlattr *nla,
return 0;
 }
 
+static int nla_validate_parse(const struct nlattr *head, int len, int maxtype,
+ const struct nla_policy *policy,
+ struct netlink_ext_ack *extack, bool *extack_set,
+ struct nlattr **tb);
+
 static int validate_nla(const struct nlattr *nla, int maxtype,
const struct nla_policy *policy,
struct netlink_ext_ack *extack, bool *extack_set)
@@ -149,6 +154,18 @@ static int validate_nla(const struct nlattr *nla, int 
maxtype,
 */
if (attrlen == 0)
break;
+   if (attrlen < NLA_HDRLEN)
+   return -ERANGE;
+   if (pt->validation_data) {
+   int err;
+
+   err = nla_validate_parse(nla_data(nla), nla_len(nla),
+pt->len, pt->validation_data,
+extack, extack_set, NULL);
+   if (err < 0)
+   return err;
+   }
+   break;
default:
if (pt->len)
minlen = pt->len;
-- 
2.14.4

[RFC 1/5] netlink: remove NLA_NESTED_COMPAT

[Johannes Berg]

From: Johannes Berg 

This isn't used anywhere, so we might as well get rid of it.

Signed-off-by: Johannes Berg 
---
 include/net/netlink.h |  2 --
 lib/nlattr.c  | 11 ---
 2 files changed, 13 deletions(-)

diff --git a/include/net/netlink.h b/include/net/netlink.h
index 318b1ded3833..b680fe365e91 100644
--- a/include/net/netlink.h
+++ b/include/net/netlink.h
@@ -172,7 +172,6 @@ enum {
NLA_FLAG,
NLA_MSECS,
NLA_NESTED,
-   NLA_NESTED_COMPAT,
NLA_NUL_STRING,
NLA_BINARY,
NLA_S8,
@@ -203,7 +202,6 @@ enum {
  *NLA_BINARY   Maximum length of attribute payload
  *NLA_NESTED   Don't use `len' field -- length verification is
  * done by checking len of nested header (or empty)
- *NLA_NESTED_COMPATMinimum length of structure payload
  *NLA_U8, NLA_U16,
  *NLA_U32, NLA_U64,
  *NLA_S8, NLA_S16,
diff --git a/lib/nlattr.c b/lib/nlattr.c
index bb6fe5ed4ecf..120ad569e13d 100644
--- a/lib/nlattr.c
+++ b/lib/nlattr.c
@@ -140,17 +140,6 @@ static int validate_nla(const struct nlattr *nla, int 
maxtype,
return -ERANGE;
break;
 
-   case NLA_NESTED_COMPAT:
-   if (attrlen < pt->len)
-   return -ERANGE;
-   if (attrlen < NLA_ALIGN(pt->len))
-   break;
-   if (attrlen < NLA_ALIGN(pt->len) + NLA_HDRLEN)
-   return -ERANGE;
-   nla = nla_data(nla) + NLA_ALIGN(pt->len);
-   if (attrlen < NLA_ALIGN(pt->len) + NLA_HDRLEN + nla_len(nla))
-   return -ERANGE;
-   break;
case NLA_NESTED:
/* a nested attributes is allowed to be empty; if its not,
 * it must have a size of at least NLA_HDRLEN.
-- 
2.14.4

[RFC 3/5] netlink: combine validate/parse functions

[Johannes Berg]

From: Johannes Berg 

The parse function of course contains validate, but it's
implemented a second time, sharing just the validation
of a single attribute.

Introduce nla_validate_parse() that can be used for both
parsing/validation and only validation, to share code.

Signed-off-by: Johannes Berg 
---
 lib/nlattr.c | 76 +++-
 1 file changed, 39 insertions(+), 37 deletions(-)

diff --git a/lib/nlattr.c b/lib/nlattr.c
index efbd6c1aff29..46a6d79cf2d1 100644
--- a/lib/nlattr.c
+++ b/lib/nlattr.c
@@ -159,6 +159,37 @@ static int validate_nla(const struct nlattr *nla, int 
maxtype,
return 0;
 }
 
+static int nla_validate_parse(const struct nlattr *head, int len, int maxtype,
+ const struct nla_policy *policy,
+ struct netlink_ext_ack *extack,
+ struct nlattr **tb)
+{
+   const struct nlattr *nla;
+   int rem;
+
+   nla_for_each_attr(nla, head, len, rem) {
+   static const char _msg[] = "Attribute failed policy validation";
+   const char *msg = _msg;
+   u16 type = nla_type(nla);
+
+   if (policy) {
+   int err = validate_nla(nla, maxtype, policy, );
+
+   if (err < 0) {
+   if (extack)
+   extack->_msg = msg;
+   NL_SET_BAD_ATTR(extack, nla);
+   return err;
+   }
+   }
+
+   if (tb && type > 0 && type <= maxtype)
+   tb[type] = (struct nlattr *)nla;
+   }
+
+   return rem;
+}
+
 /**
  * nla_validate - Validate a stream of attributes
  * @head: head of attribute stream
@@ -177,21 +208,12 @@ int nla_validate(const struct nlattr *head, int len, int 
maxtype,
 const struct nla_policy *policy,
 struct netlink_ext_ack *extack)
 {
-   const struct nlattr *nla;
int rem;
 
-   nla_for_each_attr(nla, head, len, rem) {
-   static const char _msg[] = "Attribute failed policy validation";
-   const char *msg = _msg;
-   int err = validate_nla(nla, maxtype, policy, );
+   rem = nla_validate_parse(head, len, maxtype, policy, extack, NULL);
 
-   if (err < 0) {
-   if (extack)
-   extack->_msg = msg;
-   NL_SET_BAD_ATTR(extack, nla);
-   return err;
-   }
-   }
+   if (rem < 0)
+   return rem;
 
return 0;
 }
@@ -245,39 +267,19 @@ int nla_parse(struct nlattr **tb, int maxtype, const 
struct nlattr *head,
  int len, const struct nla_policy *policy,
  struct netlink_ext_ack *extack)
 {
-   const struct nlattr *nla;
-   int rem, err;
+   int rem;
 
memset(tb, 0, sizeof(struct nlattr *) * (maxtype + 1));
 
-   nla_for_each_attr(nla, head, len, rem) {
-   u16 type = nla_type(nla);
-
-   if (type > 0 && type <= maxtype) {
-   static const char _msg[] = "Attribute failed policy 
validation";
-   const char *msg = _msg;
-
-   if (policy) {
-   err = validate_nla(nla, maxtype, policy, );
-   if (err < 0) {
-   NL_SET_BAD_ATTR(extack, nla);
-   if (extack)
-   extack->_msg = msg;
-   goto errout;
-   }
-   }
-
-   tb[type] = (struct nlattr *)nla;
-   }
-   }
+   rem = nla_validate_parse(head, len, maxtype, policy, extack, tb);
+   if (rem < 0)
+   return rem;
 
if (unlikely(rem > 0))
pr_warn_ratelimited("netlink: %d bytes leftover after parsing 
attributes in process `%s'.\n",
rem, current->comm);
 
-   err = 0;
-errout:
-   return err;
+   return 0;
 }
 EXPORT_SYMBOL(nla_parse);
 
-- 
2.14.4

[PATCH v2 2/2] netlink: add ethernet address policy types

[Johannes Berg]

From: Johannes Berg 

Commonly, ethernet addresses are just using a policy of
{ .len = ETH_ALEN }
which leaves userspace free to send more data than it should,
which may hide bugs.

Introduce NLA_EXACT_LEN which checks for exact size, rejecting
the attribute if it's not exactly that length. Also add
NLA_EXACT_LEN_WARN which requires the minimum length and will
warn on longer attributes, for backward compatibility.

Use these to define NLA_POLICY_ETH_ADDR (new strict policy) and
NLA_POLICY_ETH_ADDR_COMPAT (compatible policy with warning);
these are used like this:

static const struct nla_policy [...] = {
[NL_ATTR_NAME] = NLA_POLICY_ETH_ADDR,
...
};

Signed-off-by: Johannes Berg 
---
v2: add only NLA_EXACT_LEN/NLA_EXACT_LEN_WARN and build on top
of that for ethernet address validation, so it can be extended
for other types (e.g. IPv6 addresses)
---
 include/net/netlink.h | 13 +
 lib/nlattr.c  |  8 +++-
 2 files changed, 20 insertions(+), 1 deletion(-)

diff --git a/include/net/netlink.h b/include/net/netlink.h
index b318b0a9f6c3..318b1ded3833 100644
--- a/include/net/netlink.h
+++ b/include/net/netlink.h
@@ -181,6 +181,8 @@ enum {
NLA_S64,
NLA_BITFIELD32,
NLA_REJECT,
+   NLA_EXACT_LEN,
+   NLA_EXACT_LEN_WARN,
__NLA_TYPE_MAX,
 };
 
@@ -211,6 +213,10 @@ enum {
  * just like "All other"
  *NLA_BITFIELD32   Unused
  *NLA_REJECT   Unused
+ *NLA_EXACT_LENAttribute must have exactly this length, otherwise
+ * it is rejected.
+ *NLA_EXACT_LEN_WARN   Attribute should have exactly this length, a warning
+ * is logged if it is longer, shorter is rejected.
  *All otherMinimum length of attribute payload
  *
  * Meaning of `validation_data' field:
@@ -236,6 +242,13 @@ struct nla_policy {
void*validation_data;
 };
 
+#define NLA_POLICY_EXACT_LEN(_len) { .type = NLA_EXACT_LEN, .len = _len }
+#define NLA_POLICY_EXACT_LEN_WARN(_len){ .type = NLA_EXACT_LEN_WARN, \
+ .len = _len }
+
+#define NLA_POLICY_ETH_ADDRNLA_POLICY_EXACT_LEN(ETH_ALEN)
+#define NLA_POLICY_ETH_ADDR_COMPAT NLA_POLICY_EXACT_LEN_WARN(ETH_ALEN)
+
 /**
  * struct nl_info - netlink source information
  * @nlh: Netlink message header of original request
diff --git a/lib/nlattr.c b/lib/nlattr.c
index 36d74b079151..bb6fe5ed4ecf 100644
--- a/lib/nlattr.c
+++ b/lib/nlattr.c
@@ -82,12 +82,18 @@ static int validate_nla(const struct nlattr *nla, int 
maxtype,
 
BUG_ON(pt->type > NLA_TYPE_MAX);
 
-   if (nla_attr_len[pt->type] && attrlen != nla_attr_len[pt->type]) {
+   if ((nla_attr_len[pt->type] && attrlen != nla_attr_len[pt->type]) ||
+   (pt->type == NLA_EXACT_LEN_WARN && attrlen != pt->len)) {
pr_warn_ratelimited("netlink: '%s': attribute type %d has an 
invalid length.\n",
current->comm, type);
}
 
switch (pt->type) {
+   case NLA_EXACT_LEN:
+   if (attrlen != pt->len)
+   return -ERANGE;
+   break;
+
case NLA_REJECT:
if (pt->validation_data && error_msg)
*error_msg = pt->validation_data;
-- 
2.14.4

[PATCH v2 1/2] netlink: add NLA_REJECT policy type

[Johannes Berg]

From: Johannes Berg 

In some situations some netlink attributes may be used for output
only (kernel->userspace) or may be reserved for future use. It's
then helpful to be able to prevent userspace from using them in
messages sent to the kernel, since they'd otherwise be ignored and
any future will become impossible if this happens.

Add NLA_REJECT to the policy which does nothing but reject (with
EINVAL) validation of any messages containing this attribute.
Allow for returning a specific extended ACK error message in the
validation_data pointer.

While at it clear up the documentation a bit - the NLA_BITFIELD32
documentation was added to the list of len field descriptions.

Also, use NL_SET_BAD_ATTR() in one place where it's open-coded.

The specific case I have in mind now is a shared nested attribute
containing request/response data, and it would be pointless and
potentially confusing to have userspace include response data in
the messages that actually contain a request.

Signed-off-by: Johannes Berg 
---
v2: preserve behaviour of overwriting the extack message, with
either the generic or the specific one now
---
 include/net/netlink.h | 13 -
 lib/nlattr.c  | 23 ---
 2 files changed, 28 insertions(+), 8 deletions(-)

diff --git a/include/net/netlink.h b/include/net/netlink.h
index 0c154f98e987..b318b0a9f6c3 100644
--- a/include/net/netlink.h
+++ b/include/net/netlink.h
@@ -180,6 +180,7 @@ enum {
NLA_S32,
NLA_S64,
NLA_BITFIELD32,
+   NLA_REJECT,
__NLA_TYPE_MAX,
 };
 
@@ -208,9 +209,19 @@ enum {
  *NLA_MSECSLeaving the length field zero will verify the
  * given type fits, using it verifies minimum length
  * just like "All other"
- *NLA_BITFIELD32  A 32-bit bitmap/bitselector attribute
+ *NLA_BITFIELD32   Unused
+ *NLA_REJECT   Unused
  *All otherMinimum length of attribute payload
  *
+ * Meaning of `validation_data' field:
+ *NLA_BITFIELD32   This is a 32-bit bitmap/bitselector attribute and
+ * validation data must point to a u32 value of valid
+ * flags
+ *NLA_REJECT   This attribute is always rejected and validation 
data
+ * may point to a string to report as the error instead
+ * of the generic one in extended ACK.
+ *All otherUnused
+ *
  * Example:
  * static const struct nla_policy my_policy[ATTR_MAX+1] = {
  * [ATTR_FOO] = { .type = NLA_U16 },
diff --git a/lib/nlattr.c b/lib/nlattr.c
index e335bcafa9e4..36d74b079151 100644
--- a/lib/nlattr.c
+++ b/lib/nlattr.c
@@ -69,7 +69,8 @@ static int validate_nla_bitfield32(const struct nlattr *nla,
 }
 
 static int validate_nla(const struct nlattr *nla, int maxtype,
-   const struct nla_policy *policy)
+   const struct nla_policy *policy,
+   const char **error_msg)
 {
const struct nla_policy *pt;
int minlen = 0, attrlen = nla_len(nla), type = nla_type(nla);
@@ -87,6 +88,11 @@ static int validate_nla(const struct nlattr *nla, int 
maxtype,
}
 
switch (pt->type) {
+   case NLA_REJECT:
+   if (pt->validation_data && error_msg)
+   *error_msg = pt->validation_data;
+   return -EINVAL;
+
case NLA_FLAG:
if (attrlen > 0)
return -ERANGE;
@@ -180,11 +186,10 @@ int nla_validate(const struct nlattr *head, int len, int 
maxtype,
int rem;
 
nla_for_each_attr(nla, head, len, rem) {
-   int err = validate_nla(nla, maxtype, policy);
+   int err = validate_nla(nla, maxtype, policy, NULL);
 
if (err < 0) {
-   if (extack)
-   extack->bad_attr = nla;
+   NL_SET_BAD_ATTR(extack, nla);
return err;
}
}
@@ -250,11 +255,15 @@ int nla_parse(struct nlattr **tb, int maxtype, const 
struct nlattr *head,
u16 type = nla_type(nla);
 
if (type > 0 && type <= maxtype) {
+   static const char _msg[] = "Attribute failed policy 
validation";
+   const char *msg = _msg;
+
if (policy) {
-   err = validate_nla(nla, maxtype, policy);
+   err = validate_nla(nla, maxtype, policy, );
if (err < 0) {
-   NL_SET_ERR_MSG_ATTR(extack, nla,
-   "Attribute failed 
policy validation");
+   NL_SET_BAD_ATTR(extack, nla);
+ 

[PATCH v2] socket: fix struct ifreq size in compat ioctl

[Johannes Berg]

From: Johannes Berg 

As reported by Reobert O'Callahan, since Viro's commit to kill
dev_ifsioc() we attempt to copy too much data in compat mode,
which may lead to EFAULT when the 32-bit version of struct ifreq
sits at/near the end of a page boundary, and the next page isn't
mapped.

Fix this by passing the approprate compat/non-compat size to copy
and using that, as before the dev_ifsioc() removal. This works
because only the embedded "struct ifmap" has different size, and
this is only used in SIOCGIFMAP/SIOCSIFMAP which has a different
handler. All other parts of the union are naturally compatible.

This fixes https://bugzilla.kernel.org/show_bug.cgi?id=199469.

Fixes: bf4405737f9f ("kill dev_ifsioc()")
Reported-by: Robert O'Callahan 
Signed-off-by: Johannes Berg 
---
 net/socket.c | 22 ++
 1 file changed, 14 insertions(+), 8 deletions(-)

diff --git a/net/socket.c b/net/socket.c
index e6945e318f02..01f3f8f32d6f 100644
--- a/net/socket.c
+++ b/net/socket.c
@@ -941,7 +941,8 @@ void dlci_ioctl_set(int (*hook) (unsigned int, void __user 
*))
 EXPORT_SYMBOL(dlci_ioctl_set);
 
 static long sock_do_ioctl(struct net *net, struct socket *sock,
-unsigned int cmd, unsigned long arg)
+ unsigned int cmd, unsigned long arg,
+ unsigned int ifreq_size)
 {
int err;
void __user *argp = (void __user *)arg;
@@ -967,11 +968,11 @@ static long sock_do_ioctl(struct net *net, struct socket 
*sock,
} else {
struct ifreq ifr;
bool need_copyout;
-   if (copy_from_user(, argp, sizeof(struct ifreq)))
+   if (copy_from_user(, argp, ifreq_size))
return -EFAULT;
err = dev_ioctl(net, cmd, , _copyout);
if (!err && need_copyout)
-   if (copy_to_user(argp, , sizeof(struct ifreq)))
+   if (copy_to_user(argp, , ifreq_size))
return -EFAULT;
}
return err;
@@ -1070,7 +1071,8 @@ static long sock_ioctl(struct file *file, unsigned cmd, 
unsigned long arg)
err = open_related_ns(>ns, get_net_ns);
break;
default:
-   err = sock_do_ioctl(net, sock, cmd, arg);
+   err = sock_do_ioctl(net, sock, cmd, arg,
+   sizeof(struct ifreq));
break;
}
return err;
@@ -2750,7 +2752,8 @@ static int do_siocgstamp(struct net *net, struct socket 
*sock,
int err;
 
set_fs(KERNEL_DS);
-   err = sock_do_ioctl(net, sock, cmd, (unsigned long));
+   err = sock_do_ioctl(net, sock, cmd, (unsigned long),
+   sizeof(struct compat_ifreq));
set_fs(old_fs);
if (!err)
err = compat_put_timeval(, up);
@@ -2766,7 +2769,8 @@ static int do_siocgstampns(struct net *net, struct socket 
*sock,
int err;
 
set_fs(KERNEL_DS);
-   err = sock_do_ioctl(net, sock, cmd, (unsigned long));
+   err = sock_do_ioctl(net, sock, cmd, (unsigned long),
+   sizeof(struct compat_ifreq));
set_fs(old_fs);
if (!err)
err = compat_put_timespec(, up);
@@ -3072,7 +3076,8 @@ static int routing_ioctl(struct net *net, struct socket 
*sock,
}
 
set_fs(KERNEL_DS);
-   ret = sock_do_ioctl(net, sock, cmd, (unsigned long) r);
+   ret = sock_do_ioctl(net, sock, cmd, (unsigned long) r,
+   sizeof(struct compat_ifreq));
set_fs(old_fs);
 
 out:
@@ -3185,7 +3190,8 @@ static int compat_sock_ioctl_trans(struct file *file, 
struct socket *sock,
case SIOCBONDSETHWADDR:
case SIOCBONDCHANGEACTIVE:
case SIOCGIFNAME:
-   return sock_do_ioctl(net, sock, cmd, arg);
+   return sock_do_ioctl(net, sock, cmd, arg,
+sizeof(struct compat_ifreq));
}
 
return -ENOIOCTLCMD;
-- 
2.14.4

Re: [PATCH] socket: fix struct ifreq size in compat ioctl

[Johannes Berg]

On Thu, 2018-09-13 at 13:13 +0100, Al Viro wrote:
> On Thu, Sep 13, 2018 at 01:49:09PM +0200, Johannes Berg wrote:
> > From: Johannes Berg 
> > 
> > As reported by Reobert O'Callahan, since Viro's commit to kill
> > dev_ifsioc() we attempt to copy too much data in compat mode,
> > which may lead to EFAULT when the 32-bit version of struct ifreq
> > sits at/near the end of a page boundary, and the next page isn't
> > mapped.
> > 
> > Fix this by passing whether or not we're doing a compat call and
> > copying the appropriate size in/out, as we did before. This works
> > because only the embedded "struct ifmap" has different size, and
> > this is only used in SIOCGIFMAP/SIOCSIFMAP which has a different
> > handler. All other parts of the union are naturally compatible.
> 
> Might be better to pass explicit size instead of this bool compat...

Yeah, that's a bit better perhaps, I should resend to have the bugzilla
link anyway.

johannes

[PATCH] socket: fix struct ifreq size in compat ioctl

[Johannes Berg]

From: Johannes Berg 

As reported by Reobert O'Callahan, since Viro's commit to kill
dev_ifsioc() we attempt to copy too much data in compat mode,
which may lead to EFAULT when the 32-bit version of struct ifreq
sits at/near the end of a page boundary, and the next page isn't
mapped.

Fix this by passing whether or not we're doing a compat call and
copying the appropriate size in/out, as we did before. This works
because only the embedded "struct ifmap" has different size, and
this is only used in SIOCGIFMAP/SIOCSIFMAP which has a different
handler. All other parts of the union are naturally compatible.

Fixes: bf4405737f9f ("kill dev_ifsioc()")
Reported-by: Robert O'Callahan 
Signed-off-by: Johannes Berg 
---
 net/socket.c | 21 +
 1 file changed, 13 insertions(+), 8 deletions(-)

diff --git a/net/socket.c b/net/socket.c
index e6945e318f02..cef0725a2aaf 100644
--- a/net/socket.c
+++ b/net/socket.c
@@ -941,7 +941,8 @@ void dlci_ioctl_set(int (*hook) (unsigned int, void __user 
*))
 EXPORT_SYMBOL(dlci_ioctl_set);
 
 static long sock_do_ioctl(struct net *net, struct socket *sock,
-unsigned int cmd, unsigned long arg)
+ unsigned int cmd, unsigned long arg,
+ bool compat)
 {
int err;
void __user *argp = (void __user *)arg;
@@ -967,11 +968,15 @@ static long sock_do_ioctl(struct net *net, struct socket 
*sock,
} else {
struct ifreq ifr;
bool need_copyout;
-   if (copy_from_user(, argp, sizeof(struct ifreq)))
+   if (copy_from_user(, argp,
+  compat ? sizeof(struct compat_ifreq) :
+   sizeof(struct ifreq)))
return -EFAULT;
err = dev_ioctl(net, cmd, , _copyout);
if (!err && need_copyout)
-   if (copy_to_user(argp, , sizeof(struct ifreq)))
+   if (copy_to_user(argp, ,
+compat ? sizeof(struct compat_ifreq) :
+ sizeof(struct ifreq)))
return -EFAULT;
}
return err;
@@ -1070,7 +1075,7 @@ static long sock_ioctl(struct file *file, unsigned cmd, 
unsigned long arg)
err = open_related_ns(>ns, get_net_ns);
break;
default:
-   err = sock_do_ioctl(net, sock, cmd, arg);
+   err = sock_do_ioctl(net, sock, cmd, arg, false);
break;
}
return err;
@@ -2750,7 +2755,7 @@ static int do_siocgstamp(struct net *net, struct socket 
*sock,
int err;
 
set_fs(KERNEL_DS);
-   err = sock_do_ioctl(net, sock, cmd, (unsigned long));
+   err = sock_do_ioctl(net, sock, cmd, (unsigned long), true);
set_fs(old_fs);
if (!err)
err = compat_put_timeval(, up);
@@ -2766,7 +2771,7 @@ static int do_siocgstampns(struct net *net, struct socket 
*sock,
int err;
 
set_fs(KERNEL_DS);
-   err = sock_do_ioctl(net, sock, cmd, (unsigned long));
+   err = sock_do_ioctl(net, sock, cmd, (unsigned long), true);
set_fs(old_fs);
if (!err)
err = compat_put_timespec(, up);
@@ -3072,7 +3077,7 @@ static int routing_ioctl(struct net *net, struct socket 
*sock,
}
 
set_fs(KERNEL_DS);
-   ret = sock_do_ioctl(net, sock, cmd, (unsigned long) r);
+   ret = sock_do_ioctl(net, sock, cmd, (unsigned long) r, true);
set_fs(old_fs);
 
 out:
@@ -3185,7 +3190,7 @@ static int compat_sock_ioctl_trans(struct file *file, 
struct socket *sock,
case SIOCBONDSETHWADDR:
case SIOCBONDCHANGEACTIVE:
case SIOCGIFNAME:
-   return sock_do_ioctl(net, sock, cmd, arg);
+   return sock_do_ioctl(net, sock, cmd, arg, true);
}
 
return -ENOIOCTLCMD;
-- 
2.14.4

Re: [RFC v2 2/2] netlink: add ethernet address policy types

[Johannes Berg]

On Wed, 2018-09-12 at 10:49 +0200, Arend van Spriel wrote:
> On 9/12/2018 10:36 AM, Johannes Berg wrote:
> > From: Johannes Berg 
> > 
> > Commonly, ethernet addresses are just using a policy of
> > { .len = ETH_ALEN }
> > which leaves userspace free to send more data than it should,
> > which may hide bugs.
> > 
> > Introduce NLA_ETH_ADDR which checks for exact size, and rejects
> > the attribute if the length isn't ETH_ALEN.
> > 
> > Also add NLA_ETH_ADDR_COMPAT which can be used in place of the
> > policy above, but will, in addition, warn on an address that's
> > too long.
> 
> Not sure if this is correctly described here. It seems longer addresses 
> are not rejected, but only result in a warning message. I guess the 
> problem is in the reference to the "policy above" ;-)

Yeah, good point. I meant ".len = ETH_ALEN" but should clarify that.

johannes

Re: [PATCH v6 13/18] wireless/lib80211: Convert from ahash to shash

[Johannes Berg]

On Tue, 2018-07-24 at 09:49 -0700, Kees Cook wrote:
> In preparing to remove all stack VLA usage from the kernel[1], this
> removes the discouraged use of AHASH_REQUEST_ON_STACK in favor of
> the smaller SHASH_DESC_ON_STACK by converting from ahash-wrapped-shash
> to direct shash. By removing a layer of indirection this both improves
> performance and reduces stack usage. The stack allocation will be made
> a fixed size in a later patch to the crypto subsystem.
> 
I think you sent this before - it should be in net-next now.

johannes

[PATCH v5 1/3] bitfield: fix *_encode_bits()

[Johannes Berg]

There's a bug in *_encode_bits() in using ~field_multiplier() for
the check whether or not the constant value fits into the field,
this is wrong and clearly ~field_mask() was intended. This was
triggering for me for both constant and non-constant values.

Additionally, make this case actually into an compile error.
Declaring the extern function that will never exist with just a
warning is pointless as then later we'll just get a link error.

While at it, also fix the indentation in those lines I'm touching.

Finally, as suggested by Andy Shevchenko, add some tests and for
that introduce also u8 helpers. The tests don't compile without
the fix, showing that it's necessary.

Fixes: 00b0c9b82663 ("Add primitives for manipulating bitfields both in host- 
and fixed-endian.")
Reviewed-by: Andy Shevchenko 
Signed-off-by: Johannes Berg 
---
v2: replace stray tab by space
v3: u8 helpers, tests
v4: minor cleanup (Andy)
split into two patches (Andy)

If you don't mind, I'd like to take this through the networking
tree(s) as a fix since I have some pending code that depends on
it.
---
 include/linux/bitfield.h | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/include/linux/bitfield.h b/include/linux/bitfield.h
index cf2588d81148..147a7bb341dd 100644
--- a/include/linux/bitfield.h
+++ b/include/linux/bitfield.h
@@ -104,7 +104,7 @@
(typeof(_mask))(((_reg) & (_mask)) >> __bf_shf(_mask)); \
})
 
-extern void __compiletime_warning("value doesn't fit into mask")
+extern void __compiletime_error("value doesn't fit into mask")
 __field_overflow(void);
 extern void __compiletime_error("bad bitfield mask")
 __bad_mask(void);
@@ -121,8 +121,8 @@ static __always_inline u64 field_mask(u64 field)
 #define MAKE_OP(type,base,to,from) \
 static __always_inline __##type type##_encode_bits(base v, base field) \
 {  \
-if (__builtin_constant_p(v) && (v & ~field_multiplier(field))) \
-   __field_overflow(); \
+   if (__builtin_constant_p(v) && (v & ~field_mask(field)))\
+   __field_overflow(); \
return to((v & field_mask(field)) * field_multiplier(field));   \
 }  \
 static __always_inline __##type type##_replace_bits(__##type old,  \
-- 
2.14.4

[PATCH v5 3/3] bitfield: add tests

[Johannes Berg]

Add tests for the bitfield helpers. The constant ones will all
be folded to nothing by the compiler (if everything is correct
in the header file), and the variable ones do some tests against
open-coding the necessary shifts.

A few test cases that should fail/warn compilation are provided
under ifdef.

Suggested-by: Andy Shevchenko 
Reviewed-by: Andy Shevchenko 
Signed-off-by: Johannes Berg 
---
 lib/Kconfig.debug   |   7 +++
 lib/Makefile|   1 +
 lib/test_bitfield.c | 168 
 3 files changed, 176 insertions(+)
 create mode 100644 lib/test_bitfield.c

diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
index eb885942eb0f..b0870377b4d1 100644
--- a/lib/Kconfig.debug
+++ b/lib/Kconfig.debug
@@ -1799,6 +1799,13 @@ config TEST_BITMAP
 
  If unsure, say N.
 
+config TEST_BITFIELD
+   tristate "Test bitfield functions at runtime"
+   help
+ Enable this option to test the bitfield functions at boot.
+
+ If unsure, say N.
+
 config TEST_UUID
tristate "Test functions located in the uuid module at runtime"
 
diff --git a/lib/Makefile b/lib/Makefile
index 84c6dcb31fbb..02ab4c1a64d1 100644
--- a/lib/Makefile
+++ b/lib/Makefile
@@ -68,6 +68,7 @@ obj-$(CONFIG_TEST_STATIC_KEYS) += test_static_keys.o
 obj-$(CONFIG_TEST_STATIC_KEYS) += test_static_key_base.o
 obj-$(CONFIG_TEST_PRINTF) += test_printf.o
 obj-$(CONFIG_TEST_BITMAP) += test_bitmap.o
+obj-$(CONFIG_TEST_BITFIELD) += test_bitfield.o
 obj-$(CONFIG_TEST_UUID) += test_uuid.o
 obj-$(CONFIG_TEST_PARMAN) += test_parman.o
 obj-$(CONFIG_TEST_KMOD) += test_kmod.o
diff --git a/lib/test_bitfield.c b/lib/test_bitfield.c
new file mode 100644
index ..5b8f4108662d
--- /dev/null
+++ b/lib/test_bitfield.c
@@ -0,0 +1,168 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * Test cases for bitfield helpers.
+ */
+
+#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
+
+#include 
+#include 
+#include 
+
+#define CHECK_ENC_GET_U(tp, v, field, res) do {
\
+   {   \
+   u##tp _res; \
+   \
+   _res = u##tp##_encode_bits(v, field);   \
+   if (_res != res) {  \
+   pr_warn("u" #tp "_encode_bits(" #v ", " #field 
") is 0x%llx != " #res "\n",\
+   (u64)_res); \
+   return -EINVAL; \
+   }   \
+   if (u##tp##_get_bits(_res, field) != v) \
+   return -EINVAL; \
+   }   \
+   } while (0)
+
+#define CHECK_ENC_GET_LE(tp, v, field, res) do {   \
+   {   \
+   __le##tp _res;  \
+   \
+   _res = le##tp##_encode_bits(v, field);  \
+   if (_res != cpu_to_le##tp(res)) {   \
+   pr_warn("le" #tp "_encode_bits(" #v ", " #field 
") is 0x%llx != 0x%llx\n",\
+   (u64)le##tp##_to_cpu(_res), \
+   (u64)(res));\
+   return -EINVAL; \
+   }   \
+   if (le##tp##_get_bits(_res, field) != v)\
+   return -EINVAL; \
+   }   \
+   } while (0)
+
+#define CHECK_ENC_GET_BE(tp, v, field, res) do {   \
+   {   \
+   __be##tp _res;  \
+   \
+   _res = be##tp##_encode_bits(v, field);  \
+   if (_res != cpu_to_be##tp(res)) {   \
+   pr_warn("be" #tp "_encode_bits(" #v ", " #field 
") is 0x%llx != 0x%llx\n",\
+   (u64)be##tp##_to_cpu(_res), \
+   (u64)(res));\
+   return -EINVAL;

pull-request: mac80211-next 2018-05-23

[Johannes Berg]

Hi Dave,

Here's a new version of the pull request for net-next, now
with the stack size fixes included, which were the reason I
withdrew my earlier one. Other things are also included all
over the map.

Please pull and let me know if there's any problem.

Thanks,
johannes



The following changes since commit 1fe8c06c4a0d3b589f076cd00c25082840f10423:

  Merge branch 'qed-firmware-TLV' (2018-05-22 23:29:55 -0400)

are available in the Git repository at:

  git://git.kernel.org/pub/scm/linux/kernel/git/jberg/mac80211-next.git 
tags/mac80211-next-for-davem-2018-05-23

for you to fetch changes up to bad2929733635f80f99930b252757c70372356fe:

  nl80211: Reject disconnect commands except from conn_owner (2018-05-23 
11:56:26 +0200)


For this round, we have various things all over the place, notably
 * a fix for a race in aggregation, which I want to let
   bake for a bit longer before sending to stable
 * some new statistics (ACK RSSI, TXQ)
 * TXQ configuration
 * preparations for HE, particularly radiotap
 * replace confusing "country IE" by "country element" since it's
   not referring to Ireland

Note that I merged net-next to get a fix from mac80211 that got
there via net, to apply one patch that would otherwise conflict.


Alexander Wetzel (1):
  mac80211: fix TX aggregation stop race

Amar Singhal (1):
  cfg80211: Call reg_notifier for self managed hints conditionally

Andrew Zaborowski (1):
  nl80211: Reject disconnect commands except from conn_owner

Arend Van Spriel (2):
  cfg80211: use separate struct for FILS parameters
  nl80211: add FILS related parameters to ROAM event

Arend van Spriel (1):
  cfg80211: dynamically allocate per-tid stats for station info

Balaji Pothunoori (2):
  cfg80211: average ack rssi support for data frames
  mac80211: average ack rssi support for data frames

Bjoern Johansson (1):
  mac80211_hwsim: indicate support for powersave.

Colin Ian King (2):
  mac80211: ethtool: avoid 32 bit multiplication overflow
  cfg80211: fix spelling mistake: "uknown" -> "unknown"

Denis Kenzior (2):
  nl80211: Fix compilation
  nl80211: Optimize cfg80211_bss_expire invocations

Gregory Greenman (1):
  mac80211: add api to set CSA counter in mac80211

Haim Dreyfuss (1):
  nl80211: Add wmm rule attribute to NL80211_CMD_GET_WIPHY dump command

Ilan Peer (1):
  mac80211: Support adding duration for prepare_tx() callback

Johannes Berg (9):
  mac80211: rename rtap_vendor_space to rtap_space
  mac80211: clean up rate info bandwidth setting
  mac80211: ethtool: memset the whole sinfo struct to 0
  mac80211: remove pointless flags=0 assignment
  cfg80211/mac80211: revert to stack allocation for sinfo
  mac80211: allocate and fill tidstats only when needed
  cfg80211: release station info tidstats where needed
  cfg80211: add missing kernel-doc
  Merge remote-tracking branch 'net-next/master' into mac80211-next

João Paulo Rechi Vita (2):
  rfkill: Rename rfkill_any_led_trigger* functions
  rfkill: Create rfkill-none LED trigger

Toke Høiland-Jørgensen (3):
  regulatory: Rename confusing 'country IE' in log output
  cfg80211: Expose TXQ stats and parameters to userspace
  mac80211: Support the new cfg80211 TXQ stats API

Vidyullatha Kanchanapally (1):
  nl80211: Update ERP info using NL80211_CMD_UPDATE_CONNECT_PARAMS

 drivers/net/wireless/ath/ath9k/main.c |   3 +-
 drivers/net/wireless/intel/iwlwifi/mvm/mac80211.c |   6 +-
 drivers/net/wireless/mac80211_hwsim.c |   1 +
 include/net/cfg80211.h| 131 --
 include/net/mac80211.h|  18 +-
 include/uapi/linux/nl80211.h  |  99 ++-
 net/mac80211/cfg.c| 103 +++-
 net/mac80211/driver-ops.h |   8 +-
 net/mac80211/ethtool.c|  13 +-
 net/mac80211/ht.c |  44 ++--
 net/mac80211/ieee80211_i.h|   3 +
 net/mac80211/main.c   |   3 +
 net/mac80211/mlme.c   |  17 +-
 net/mac80211/rx.c |  40 ++-
 net/mac80211/sta_info.c   |  38 ++-
 net/mac80211/sta_info.h   |   5 +-
 net/mac80211/status.c |   2 +
 net/mac80211/trace.h  |  25 +-
 net/mac80211/tx.c |  45 
 net/mac80211/util.c   |   6 +-
 net/rfkill/core.c |  66 +++--
 net/wireless/core.c   |   4 +
 net/wireless/nl80211.c| 304 

Re: [v8, bpf-next, 4/9] net/wireless/iwlwifi: fix iwlwifi_dev_ucode_error tracepoint

[Johannes Berg]

On Wed, 2018-03-28 at 12:05 -0700, Alexei Starovoitov wrote:
> fix iwlwifi_dev_ucode_error tracepoint to pass pointer to a table
> instead of all 17 arguments by value.
> dvm/main.c and mvm/utils.c have 'struct iwl_error_event_table'
> defined with very similar yet subtly different fields and offsets.
> tracepoint is still common and using definition of 'struct 
> iwl_error_event_table'
> from dvm/commands.h while copying fields.
> Long term this tracepoint probably should be split into two.

It would've been nice to CC the wireless list for wireless related
patches ...

> --- a/drivers/net/wireless/intel/iwlwifi/iwl-devtrace.c
> +++ b/drivers/net/wireless/intel/iwlwifi/iwl-devtrace.c
> @@ -30,6 +30,7 @@
>  #ifndef __CHECKER__
>  #include "iwl-trans.h"
>  
> +#include "dvm/commands.h"

In particular, this breaks the whole driver abstraction.

> +++ b/drivers/net/wireless/intel/iwlwifi/mvm/utils.c
> @@ -549,12 +549,7 @@ static void iwl_mvm_dump_lmac_error_log(struct iwl_mvm 
> *mvm, u32 base)
>  
> IWL_ERR(mvm, "Loaded firmware version: %s\n", mvm->fw->fw_version);
>  
> -   trace_iwlwifi_dev_ucode_error(trans->dev, table.error_id, 
> table.tsf_low,
> - table.data1, table.data2, table.data3,
> - table.blink2, table.ilink1,
> - table.ilink2, table.bcon_time, 
> table.gp1,
> - table.gp2, table.fw_rev_type, 
> table.major,
> - table.minor, table.hw_ver, 
> table.brd_ver);
> +   trace_iwlwifi_dev_ucode_error(trans->dev, , table.hw_ver, 
> table.brd_ver);

This is also utterly wrong because mvm has - for better or worse - a
different type "struct iwl_error_event_table" in this file ...

This really should never have gotten into the tree.

johannes

pull-request: mac80211 2018-05-23

[Johannes Berg]

Hi Dave,

Just another handful of fixes as we wind down towards the
merge window.

Please pull and let me know if there's any problem.

Thanks,
johannes



The following changes since commit 6caf9fb3bda17df59de4ed6ed4950c43ca1361e3:

  Merge git://git.kernel.org/pub/scm/linux/kernel/git/bpf/bpf (2018-05-17 
23:33:52 -0400)

are available in the Git repository at:

  git://git.kernel.org/pub/scm/linux/kernel/git/jberg/mac80211.git 
tags/mac80211-for-davem-2018-05-23

for you to fetch changes up to fed4825096cfbbfd654cb292ab6eb193911aef01:

  mac80211_hwsim: Fix radio dump for radio idx 0 (2018-05-22 10:24:17 +0200)


A handful of fixes:
 * hwsim radio dump wasn't working for the first radio
 * mesh was updating statistics incorrectly
 * a netlink message allocation was possibly too short
 * wiphy name limit was still too long
 * in certain cases regdb query could find a NULL pointer


Andrew Zaborowski (1):
  mac80211_hwsim: Fix radio dump for radio idx 0

Bob Copeland (1):
  mac80211: mesh: fix premature update of rc stats

Dedy Lansky (1):
  nl80211: fix nlmsg allocation in cfg80211_ft_event

Eric Biggers (1):
  cfg80211: further limit wiphy names to 64 bytes

Haim Dreyfuss (1):
  cfg80211: fix NULL pointer derference when querying regdb

 drivers/net/wireless/mac80211_hwsim.c | 4 ++--
 include/uapi/linux/nl80211.h  | 2 +-
 net/mac80211/mesh_plink.c | 8 
 net/wireless/nl80211.c| 3 ++-
 net/wireless/reg.c| 3 +++
 5 files changed, 12 insertions(+), 8 deletions(-)

Re: About the alx WoL feature

[Johannes Berg]

+netdev

Hi,

> Sorry to write to you directly, I'm not sure if I should CC some
> public lists, please feel free to add them if that's the right way to
> do.

I think it is, adding netdev.

> I would like to add back the alx WoL feature which is reverted by you
> 5 years ago, and we have some discussions here[1] and here[2].
[...]
> 1. https://patchwork.kernel.org/patch/10331135/
> 2. https://patchwork.kernel.org/patch/10396651/

> David Miller insists to fix the wake up issue before we can add this
> feature back,

Rightfully so!

> so I'm wondering if you still have the machine which can
> reproduce the wake up issue or know who might have the machine and
> willing to help.

I still have the machine that I initially did this development on, but
it's been purposed as my NAS so testing on it is somewhat awkward.

However, as far as I remember (why the hell are you doing this 5 years
later?!) the problem never occurred on my machine. Perhaps on Johannes
Stezenbach's machine? I don't recall, but see emails in my inbox from
him related to alx WoL.

> We can try contacting Qualcomm and/or Rivet Network to sort out the issue.

Good luck with that, heh.

johannes

Re: pull-request: mac80211-next 2018-05-09

[Johannes Berg]

Hi,

Sorry, scratch that.

I forgot that this commit:

> Toke Høiland-Jørgensen (3):

>   cfg80211: Expose TXQ stats and parameters to userspace

caused a bunch of "too much stack" warnings - I should put in at least
the non-driver fix for that first, and then coordinate with Kalle to
send the driver fixes in too.

johannes

pull-request: mac80211-next 2018-05-09

[Johannes Berg]

Hi Dave,

Nothing major here either. I'm still waiting for the HE/802.11ax
stuff, but even when we do get that it'll just add rates and not
be very exciting at this point :-)

Please pull and let me know if there's any problem.

Thanks,
johannes



The following changes since commit 415787d7799f4fccbe8d49cb0b8e5811be6b0389:

  ipv6: frags: fix a lockdep false positive (2018-04-18 23:19:39 -0400)

are available in the Git repository at:

  git://git.kernel.org/pub/scm/linux/kernel/git/jberg/mac80211-next.git 
tags/mac80211-next-for-davem-2018-05-09

for you to fetch changes up to 57c6cb81717f957fb741f2e4c79bd0e2f4f55910:

  mac80211: ethtool: avoid 32 bit multiplication overflow (2018-05-08 15:02:03 
+0200)


While we're still waiting for HE/802.11ax code, we just
have a few things:
 * some new statistics (ACK RSSI, TXQ)
 * TXQ configuration
 * infrastructure to handle CSA in firmware
 * and some various cleanups/improvements


Amar Singhal (1):
  cfg80211: Call reg_notifier for self managed hints conditionally

Balaji Pothunoori (2):
  cfg80211: average ack rssi support for data frames
  mac80211: average ack rssi support for data frames

Bjoern Johansson (1):
  mac80211_hwsim: indicate support for powersave.

Colin Ian King (1):
  mac80211: ethtool: avoid 32 bit multiplication overflow

Gregory Greenman (1):
  mac80211: add api to set CSA counter in mac80211

Haim Dreyfuss (1):
  nl80211: Add wmm rule attribute to NL80211_CMD_GET_WIPHY dump command

Johannes Berg (4):
  mac80211: rename rtap_vendor_space to rtap_space
  mac80211: clean up rate info bandwidth setting
  mac80211: ethtool: memset the whole sinfo struct to 0
  mac80211: remove pointless flags=0 assignment

Toke Høiland-Jørgensen (3):
  regulatory: Rename confusing 'country IE' in log output
  cfg80211: Expose TXQ stats and parameters to userspace
  mac80211: Support the new cfg80211 TXQ stats API

 drivers/net/wireless/mac80211_hwsim.c |   1 +
 include/net/cfg80211.h|  53 +++
 include/net/mac80211.h|  13 ++
 include/uapi/linux/nl80211.h  |  93 
 net/mac80211/cfg.c|  99 +
 net/mac80211/ethtool.c|  37 +++--
 net/mac80211/ieee80211_i.h|   3 +
 net/mac80211/main.c   |   3 +
 net/mac80211/rx.c |  40 +++---
 net/mac80211/sta_info.c   |  24 +++-
 net/mac80211/sta_info.h   |   2 +
 net/mac80211/status.c |   2 +
 net/mac80211/tx.c |  45 ++
 net/mac80211/util.c   |   6 +-
 net/wireless/nl80211.c| 262 ++
 net/wireless/rdev-ops.h   |  12 ++
 net/wireless/reg.c|  37 -
 net/wireless/trace.h  |  14 ++
 net/wireless/wext-compat.c|  23 +--
 19 files changed, 683 insertions(+), 86 deletions(-)

pull-request: mac80211 2018-05-09

[Johannes Berg]

Hi Dave,

We just have a few fixes this time around.

Please pull and let me know if there's any problem.

Thanks,
johannes



The following changes since commit 64e86fec54069266ba32be551d7b7f75e88ab60c:

  net: qualcomm: rmnet: Fix warning seen with fill_info (2018-04-18 21:23:06 
-0400)

are available in the Git repository at:

  git://git.kernel.org/pub/scm/linux/kernel/git/jberg/mac80211.git 
tags/mac80211-for-davem-2018-05-09

for you to fetch changes up to 914eac248d876f9c00cd1792ffec3d182c863f13:

  mac80211: use timeout from the AddBA response instead of the request 
(2018-05-07 20:35:15 +0200)


We only have a few fixes this time:
 * WMM element validation
 * SAE timeout
 * add-BA timeout
 * docbook parsing
 * a few memory leaks in error paths


Ilan Peer (2):
  mac80211: Fix condition validating WMM IE
  mac80211: Adjust SAE authentication timeout

Johan Hovold (1):
  rfkill: gpio: fix memory leak in probe error path

Johannes Berg (1):
  cfg80211: limit wiphy names to 128 bytes

Randy Dunlap (1):
  mac80211: fix kernel-doc "bad line" warning

Sara Sharon (1):
  mac80211: use timeout from the AddBA response instead of the request

Srinivas Dasari (1):
  nl80211: Free connkeys on external authentication failure

YueHaibing (1):
  mac80211_hwsim: fix a possible memory leak in hwsim_new_radio_nl()

weiyongjun (A) (1):
  cfg80211: fix possible memory leak in regdb_query_country()

 drivers/net/wireless/mac80211_hwsim.c |  1 +
 include/net/mac80211.h|  2 +-
 include/uapi/linux/nl80211.h  |  2 ++
 net/mac80211/agg-tx.c |  4 
 net/mac80211/mlme.c   | 27 +++
 net/mac80211/tx.c |  3 ++-
 net/rfkill/rfkill-gpio.c  |  7 ++-
 net/wireless/core.c   |  3 +++
 net/wireless/nl80211.c|  1 +
 net/wireless/reg.c|  1 +
 10 files changed, 40 insertions(+), 11 deletions(-)

Re: [PATCH 09/18] net: mac80211.h: fix a bad comment line

[Johannes Berg]

On Wed, 2018-05-09 at 09:04 -0300, Mauro Carvalho Chehab wrote:
> Em Mon, 07 May 2018 14:38:26 +0200
> Johannes Berg <johan...@sipsolutions.net> escreveu:
> 
> > On Mon, 2018-05-07 at 15:37 +0300, Kalle Valo wrote:
> > > Mauro Carvalho Chehab <mchehab+sams...@kernel.org> writes:
> > >   
> > > > Sphinx produces a lot of errors like this:
> > > > ./include/net/mac80211.h:2083: warning: bad line:  >
> > > > 
> > > > Signed-off-by: Mauro Carvalho Chehab <mchehab+sams...@kernel.org>  
> > > 
> > > Randy already submitted a similar patch:
> > > 
> > > https://patchwork.kernel.org/patch/10367275/
> > > 
> > > But it seems Johannes has not applied that yet.  
> > 
> > Yeah, I've been super busy preparing for the plugfest.
> > 
> > I'll make a pass over all the patches as soon as I can, hopefully today
> > or tomorrow.
> 
> Thanks. I'll drop it from my patchset, assuming that you'll
> be applying Randy's version or mine via your tree.

Right, I did, just need to send a pull request.

johannes

Re: [PATCH] mac80211: ethtool: avoid 32 bit multiplication overflow

[Johannes Berg]

On Tue, 2018-05-08 at 13:57 +0100, Colin King wrote:
> From: Colin Ian King 
> 
> The multiplication of 10 * cfg80211_calculate_bitrate() is a 32 bit
> operation and can overflow if cfg80211_calculate_bitrate is greater
> than 42949. Although I don't believe this is occurring at present, it
> would be safer to avoid the potential overflow by making the constant
> 10 an ULL to ensure a 64 multiplication occurs.

Yeah it can't happen since mac80211 doesn't support 60 GHz devices, and
all others are limited to less than than.

Still, applied.

johannes

Re: [PATCH 09/18] net: mac80211.h: fix a bad comment line

[Johannes Berg]

On Mon, 2018-05-07 at 15:37 +0300, Kalle Valo wrote:
> Mauro Carvalho Chehab  writes:
> 
> > Sphinx produces a lot of errors like this:
> > ./include/net/mac80211.h:2083: warning: bad line:  >
> > 
> > Signed-off-by: Mauro Carvalho Chehab 
> 
> Randy already submitted a similar patch:
> 
> https://patchwork.kernel.org/patch/10367275/
> 
> But it seems Johannes has not applied that yet.

Yeah, I've been super busy preparing for the plugfest.

I'll make a pass over all the patches as soon as I can, hopefully today
or tomorrow.

johannes

Re: WARNING in kernfs_add_one

[Johannes Berg]

On Mon, 2018-05-07 at 11:33 +0200, Dmitry Vyukov wrote:
> On Mon, May 7, 2018 at 10:43 AM, Johannes Berg
> <johan...@sipsolutions.net> wrote:
> > On Sat, 2018-05-05 at 15:07 -0700, Greg KH wrote:
> > 
> > > > > > syzbot found the following crash on:
> > 
> > Maybe it should learn to differentiate warnings, if it's going to set
> > panic_on_warn :-)
> 
> How?
> Note that this is not specific to syzbot. If you see WARNINGs in a
> subsystem that you have no idea about (or you just a normal user),
> what do you do? Right, you report it to maintainers.

Yeah, no problem with that. Just some people seem to get so much more
upset about crashes ... but then again I get bug reports about WARN_ON
all the time anyway that say "my kernel crashed" so I guess it doesn't
really matter :-)

> > I get why, but still, at least differentiating in the emails wouldn't be
> > bad.
> 
> Well, the subject says "WARNING".
> But note there are _very_ bad WARNINGs too. Generally, a WARNING means
> a kernel bug just that kernel can tolerate without bringing the system
> down (as opposed to BUG).

Yeah, fair point. I sort of missed the subject I guess.

johannes

Re: WARNING in kernfs_add_one

[Johannes Berg]

On Sat, 2018-05-05 at 15:07 -0700, Greg KH wrote:

> > > > syzbot found the following crash on:

Maybe it should learn to differentiate warnings, if it's going to set
panic_on_warn :-)

I get why, but still, at least differentiating in the emails wouldn't be
bad.

> > > > kernfs: ns required in 'ieee80211' for 'phy3'

Huh. What does that even mean?

> > > > RIP: 0010:kernfs_add_one+0x406/0x4d0 fs/kernfs/dir.c:758
> > > > RSP: 0018:8801ca9eece0 EFLAGS: 00010286
> > > > RAX: 002d RBX: 87d5cee0 RCX: 8160ba7d
> > > > RDX:  RSI: 81610731 RDI: 8801ca9ee840
> > > > RBP: 8801ca9eed20 R08: 8801d9538500 R09: 0006
> > > > R10: 8801d9538500 R11:  R12: 8801ad1cb6c0
> > > > R13: 885da640 R14: 0020 R15: 
> > > >  kernfs_create_link+0x112/0x180 fs/kernfs/symlink.c:41
> > > >  sysfs_do_create_link_sd.isra.2+0x90/0x130 fs/sysfs/symlink.c:43
> > > >  sysfs_do_create_link fs/sysfs/symlink.c:79 [inline]
> > > >  sysfs_create_link+0x65/0xc0 fs/sysfs/symlink.c:91
> > > >  device_add_class_symlinks drivers/base/core.c:1612 [inline]
> > > >  device_add+0x7a0/0x16d0 drivers/base/core.c:1810
> > > >  wiphy_register+0x178a/0x2430 net/wireless/core.c:806
> > > >  ieee80211_register_hw+0x13cd/0x35d0 net/mac80211/main.c:1047
> > > >  mac80211_hwsim_new_radio+0x1d9b/0x3410
> > > > drivers/net/wireless/mac80211_hwsim.c:2772
> > > >  hwsim_new_radio_nl+0x7a7/0xa60 
> > > > drivers/net/wireless/mac80211_hwsim.c:3246
> > > >  genl_family_rcv_msg+0x889/0x1120 net/netlink/genetlink.c:599

Basically we're creating a new virtual radio, which in turn creates a
new device, which we have to register.

Something is going on with the context here that makes sysfs unhappy,
but TBH I have no idea what.

johannes

Re: [PATCH 1/3] ethtool: Support ETHTOOL_GSTATS2 command.

[Johannes Berg]

On Thu, 2018-04-19 at 08:25 -0700, Ben Greear wrote:
> 
> In order to efficiently parse lots of stats over and over again, I probe
> the stat names once on startup, map them to the variable I am trying to use
> (since different drivers may have different names for the same basic stat),
> and then I store the stat index.
> 
> On subsequent stat reads, I just grab stats and go right to the index to
> store the stat.
> 
> If the stats indexes change, that will complicate my logic quite a bit.

That's a good point.

> Maybe the flag could be called:  ETHTOOL_GS2_NO_REFRESH_FW ?

Sounds more to the point to me, yeah.

> > 
> > Also, wrt. the rest of the patch, I'd argue that it'd be worthwhile to
> > write the spatch and just add the flags argument to "get_ethtool_stats"
> > instead of adding a separate method - internally to the kernel it's not
> > that hard to change.
> 
> Maybe this could be in followup patches?  It's going to touch a lot of files,
> and might be hell to get merged all at once, and I've never used spatch, so
> just maybe someone else will volunteer that part :)

I guess you'll have to ask davem. :)

johannes

Re: [PATCH 1/3] ethtool: Support ETHTOOL_GSTATS2 command.

[Johannes Berg]

On Wed, 2018-04-18 at 14:51 -0700, Ben Greear wrote:

> > It'd be pretty hard to know which flags are firmware stats?
> 
> Yes, it is, but ethtool stats are difficult to understand in a generic
> manner anyway, so someone using them is already likely aware of low-level
> details of the driver(s) they are using.

Right. Come to think of it though,

> + * @get_ethtool_stats2: Return extended statistics about the device.
> + * This is only useful if the device maintains statistics not
> + * included in  rtnl_link_stats64.
> + *  Takes a flags argument:  0 means all (same as get_ethtool_stats),
> + *  0x1 (ETHTOOL_GS2_SKIP_FW) means skip firmware stats.
> + *  Other flags are reserved for now.
> + *  Same number of stats will be returned, but some of them might
> + *  not be as accurate/refreshed.  This is to allow not querying
> + *  firmware or other expensive-to-read stats, for instance.

"skip" vs. "don't refresh" is a bit ambiguous - I'd argue better to
either really skip and not return the non-refreshed ones (also helps
with the identifying), or rename the flag.

Also, wrt. the rest of the patch, I'd argue that it'd be worthwhile to
write the spatch and just add the flags argument to "get_ethtool_stats"
instead of adding a separate method - internally to the kernel it's not
that hard to change.

> I posted the patches to netdev, ath10k and linux-wireless.  If I had only
> posted them individually to different lists I figure I'd be hearing about how
> the netdev patch is useless because it has no driver support, etc.

Sure. I missed netdev, perhaps because it was in To, or more likely
because I was too sleepy. Sorry for the noise.

johannes

Re: [PATCH 1/3] ethtool: Support ETHTOOL_GSTATS2 command.

[Johannes Berg]

On Tue, 2018-04-17 at 18:49 -0700, gree...@candelatech.com wrote:
> 
> + * @get_ethtool_stats2: Return extended statistics about the device.
> + *   This is only useful if the device maintains statistics not
> + *   included in  rtnl_link_stats64.
> + *  Takes a flags argument:  0 means all (same as get_ethtool_stats),
> + *  0x1 (ETHTOOL_GS2_SKIP_FW) means skip firmware stats.
> + *  Other flags are reserved for now.

It'd be pretty hard to know which flags are firmware stats?

Anyway, there's no way I'm going to take this patch, so you need to
float it on netdev first (best CC us here) and get it applied there
before we can do anything on the wifi side.

johannes

Re: WARNING in add_uevent_var

[Johannes Berg]

On Sun, 2018-04-01 at 23:01 -0700, syzbot wrote:

> So far this crash happened 5 times on net-next, upstream.
> C reproducer: https://syzkaller.appspot.com/x/repro.c?id=6614377067184128
> 

Huh, fun. Looks like you're basically creating a new HWSIM radio with an
insanely long name (4k!) and nothing stops you, until we try to generate
an rfkill instance which sends a uevent and only has a 2k buffer for the
environment variables, where we put the name ...

But yeah, we should probably limit the phy name to something sane, I'll
pick 128 and send a patch.

johannes

pull-request: mac80211-next 2018-03-29

[Johannes Berg]

Hi Dave,

Last update for -next, I guess, but I wanted to get the ETSI adaptivity
requirements code and the eapol-over-nl80211 thing out - both have been
around for a while. A number of other smaller things are also there, of
course.

Please pull and let me know if there's any problem.

Thanks,
johannes



The following changes since commit 0466080c751ec2de9efae3ac6305225cc4326047:

  Merge branch 'dsa-mv88e6xxx-some-fixes' (2018-03-20 12:29:58 -0400)

are available in the Git repository at:

  git://git.kernel.org/pub/scm/linux/kernel/git/jberg/mac80211-next.git 
tags/mac80211-next-for-davem-2018-03-29

for you to fetch changes up to c470bdc1aaf36669e04ba65faf1092b2d1c6cabe:

  mac80211: don't WARN on bad WMM parameters from buggy APs (2018-03-29 
15:02:38 +0200)


We have a fair number of patches, but many of them are from the
first bullet here:
 * EAPoL-over-nl80211 from Denis - this will let us fix
   some long-standing issues with bridging, races with
   encryption and more
 * DFS offload support from the qtnfmac folks
 * regulatory database changes for the new ETSI adaptivity
   requirements
 * various other fixes and small enhancements


Benjamin Beichler (1):
  mac80211_hwsim: fix use-after-free bug in hwsim_exit_net

Denis Kenzior (11):
  cfg80211: Support all iftypes in autodisconnect_wk
  nl80211: Add SOCKET_OWNER support to JOIN_IBSS
  nl80211: Add SOCKET_OWNER support to JOIN_MESH
  nl80211: Add SOCKET_OWNER support to START_AP
  nl80211: Add CMD_CONTROL_PORT_FRAME API
  nl80211: Implement TX of control port frames
  nl80211: Add CONTROL_PORT_OVER_NL80211 attribute
  nl80211: Add control_port_over_nl80211 for ibss
  nl80211: Add control_port_over_nl80211 to mesh_setup
  mac80211: Add support for tx_control_port
  mac80211: Send control port frames over nl80211

Dmitry Lebed (4):
  cfg80211/nl80211: add CAC_STARTED event
  cfg80211/nl80211: add DFS offload flag
  cfg80211: fix CAC_STARTED event handling
  cfg80211: enable use of non-cleared DFS channels for DFS offload

Emmanuel Grumbach (1):
  mac80211: don't WARN on bad WMM parameters from buggy APs

Haim Dreyfuss (3):
  cfg80211: read wmm rules from regulatory database
  mac80211: limit wmm params to comply with ETSI requirements
  cfg80211: Add API to allow querying regdb for wmm_rule

Johannes Berg (4):
  mac80211_hwsim: fix secondary MAC address assignment
  cfg80211: don't require RTNL held for regdomain reads
  mac80211: remove shadowing duplicated variable
  Merge branch 'eapol-over-nl80211' into mac80211-next

Manikanta Pubbisetty (1):
  mac80211: allow AP_VLAN operation on crypto controlled devices

Pradeep Kumar Chitrapu (1):
  mac80211: notify driver for change in multicast rates

Tosoni (1):
  mac80211: inform wireless layer when frame RSSI is invalid

tami...@codeaurora.org (3):
  cfg80211: fix data type of sta_opmode_info parameter
  mac80211: Use proper smps_mode enum in sta opmode event
  mac80211: Use proper chan_width enum in sta opmode event

 drivers/net/wireless/mac80211_hwsim.c |  10 +-
 include/net/cfg80211.h|  76 -
 include/net/mac80211.h|   3 +
 include/net/regulatory.h  |  28 +
 include/uapi/linux/nl80211.h  |  46 +++-
 net/mac80211/cfg.c|  12 ++
 net/mac80211/ht.c |  15 +++
 net/mac80211/ibss.c   |   3 +-
 net/mac80211/ieee80211_i.h|  12 ++
 net/mac80211/iface.c  |   2 +
 net/mac80211/key.c|   8 +-
 net/mac80211/main.c   |  10 +-
 net/mac80211/mesh.c   |   3 +-
 net/mac80211/mlme.c   | 168 ++-
 net/mac80211/rx.c |  45 ++--
 net/mac80211/scan.c   |   4 +-
 net/mac80211/tx.c |  46 
 net/mac80211/util.c   |  47 +++-
 net/mac80211/vht.c|  32 +-
 net/wireless/ap.c |   1 +
 net/wireless/chan.c   |   9 +-
 net/wireless/core.h   |  12 +-
 net/wireless/ibss.c   |  27 +
 net/wireless/mesh.c   |  16 +--
 net/wireless/mlme.c   |   9 +-
 net/wireless/nl80211.c| 205 +++--
 net/wireless/rdev-ops.h   |  15 +++
 net/wireless/reg.c| 206 --
 net/wireless/sme.c|  43 +--
 net/wireless/trace.h  |  47 
 30 files changed, 979 insertions(+), 181 deletions(-)

Re: [PATCH] mac80211: aes-cmac: remove VLA usage

[Johannes Berg]

On Wed, 2018-03-21 at 08:57 -0500, Gustavo A. R. Silva wrote:
> 
> SHA_DESC_ON_STACK is currently being used in multiple places. But, yeah, 
> I think we can define multiple macros of the same kind and adjust to the 
> characteristics of each the component.
> 
> How big do you think tfm can get?

I have no idea, I guess you'll have to take that with Herbert.

johannes

Re: [PATCH] mac80211: aes-cmac: remove VLA usage

[Johannes Berg]

On Wed, 2018-03-21 at 08:42 -0500, Gustavo A. R. Silva wrote:
> In preparation to enabling -Wvla, remove VLAs and replace them
> with dynamic memory allocation instead.
> 
> The use of stack Variable Length Arrays needs to be avoided, as they
> can be a vector for stack exhaustion, which can be both a runtime bug
> or a security flaw. Also, in general, as code evolves it is easy to
> lose track of how big a VLA can get. Thus, we can end up having runtime
> failures that are hard to debug.
> 
> Also, fixed as part of the directive to remove all VLAs from
> the kernel: https://lkml.org/lkml/2018/3/7/621
> 
> Signed-off-by: Gustavo A. R. Silva 
> ---
>  net/mac80211/aes_cmac.c | 36 
>  1 file changed, 24 insertions(+), 12 deletions(-)
> 
> diff --git a/net/mac80211/aes_cmac.c b/net/mac80211/aes_cmac.c
> index 2fb6558..c9444bf 100644
> --- a/net/mac80211/aes_cmac.c
> +++ b/net/mac80211/aes_cmac.c
> @@ -27,30 +27,42 @@ static const u8 zero[CMAC_TLEN_256];
>  void ieee80211_aes_cmac(struct crypto_shash *tfm, const u8 *aad,
>   const u8 *data, size_t data_len, u8 *mic)
>  {
> - SHASH_DESC_ON_STACK(desc, tfm);
> + struct shash_desc *shash;
>   u8 out[AES_BLOCK_SIZE];
>  
> - desc->tfm = tfm;
> + shash = kmalloc(sizeof(*shash) + crypto_shash_descsize(tfm),
> + GFP_KERNEL);
> + if (!shash)
> + return;

Honestly, this seems like a really bad idea - you're now hitting
kmalloc for every TX/RX frame here.

SHA_DESC_ON_STACK() should just be fixed to not need a VLA, but take
some sort of maximum, I guess?

johannes

pull-request: mac80211 2018-03-21

[Johannes Berg]

Hi Dave,

Another few fixes - one for hwsim, so not really all that interesting,
and two patches to work around an ath9k_htc problem.

Note that I pulled your net tree today, so you may need to be careful
to not fast-forward if you don't merge anything else before this.

Please pull and let me know if there's any problem.

Thanks,
johannes



The following changes since commit 5f2fb802eee1df0810b47ea251942fe3fd36589a:

  ipv6: old_dport should be a __be16 in __ip6_datagram_connect() (2018-03-20 
12:43:43 -0400)

are available in the Git repository at:

  git://git.kernel.org/pub/scm/linux/kernel/git/jberg/mac80211.git 
tags/mac80211-for-davem-2018-03-21

for you to fetch changes up to 60b01bcce97191f473fa869df2713143936d6ef4:

  ath9k_htc: use non-QoS NDP for AP probing (2018-03-21 13:01:55 +0100)


Two more fixes (in three patches):
 * ath9k_htc doesn't like QoS NDP frames, use regular ones
 * hwsim: set up wmediumd for radios created later


Andrew Zaborowski (1):
  mac80211_hwsim: Set wmediumd for new radios

Ben Caradoc-Davies (1):
  mac80211: add ieee80211_hw flag for QoS NDP support

Johannes Berg (1):
  ath9k_htc: use non-QoS NDP for AP probing

 drivers/net/wireless/ath/ath9k/htc_drv_init.c | 1 +
 drivers/net/wireless/mac80211_hwsim.c | 1 +
 include/net/mac80211.h| 4 
 net/mac80211/debugfs.c| 1 +
 net/mac80211/mlme.c   | 3 ++-
 5 files changed, 9 insertions(+), 1 deletion(-)

pull-request: mac80211 2018-03-02

[Johannes Berg]

Hi Dave,

Just a few more patches, but I'll be travelling over the next
week and probably won't be able to send things to you then.

Please pull and let me know if there's any problem.

Thanks,
johannes



The following changes since commit 93c62c45ed5fad1b87e3a45835b251cd68de9c46:

  rxrpc: Fix send in rxrpc_send_data_packet() (2018-02-22 15:37:47 -0500)

are available in the Git repository at:

  git://git.kernel.org/pub/scm/linux/kernel/git/jberg/mac80211.git 
tags/mac80211-for-davem-2018-03-02

for you to fetch changes up to a78872363614367c3f37e3a5b4181c7a6b207b37:

  cfg80211: add missing dependency to CFG80211 suboptions (2018-02-27 10:54:12 
+0100)


Three more patches:
 * fix for a regression in 4-addr mode with fast-RX
 * fix for a Kconfig problem with the new regdb
 * fix for the long-standing TCP performance issue in
   wifi using the new sk_pacing_shift_update()


Felix Fietkau (1):
  mac80211: drop frames with unexpected DS bits from fast-rx to slow path

Romain Naour (1):
  cfg80211: add missing dependency to CFG80211 suboptions

Toke Høiland-Jørgensen (1):
  mac80211: Adjust TSQ pacing shift

 net/mac80211/rx.c|  2 +-
 net/mac80211/tx.c|  8 
 net/wireless/Kconfig | 13 +
 3 files changed, 14 insertions(+), 9 deletions(-)

pull-request: mac80211-next 2018-03-02

[Johannes Berg]

Hi Dave,

Like before... Thanks for pulling net into net-next, the Add-BA patch
below would otherwise not really be possible :-)

The only sort of interesting thing is the fast-RX improvements from
Felix, they help on routers where these things (A-MSDU and 4-addr mode)
are more important and where fast-RX really helps due to the reduced
CPU load.

Please pull and let me know if there's any problem.

Thanks,
johannes



The following changes since commit f74290fdb363665538743d14c4f00aeacdb68d87:

  Merge git://git.kernel.org/pub/scm/linux/kernel/git/davem/net (2018-02-24 
00:04:20 -0500)

are available in the Git repository at:

  git://git.kernel.org/pub/scm/linux/kernel/git/jberg/mac80211-next.git 
tags/mac80211-next-for-davem-2018-03-02

for you to fetch changes up to 2e75bb2f8b8928aa01d91219a90df1e6fbc7cdd4:

  net: Convert hwsim_net_ops (2018-03-02 10:01:25 +0100)


Only a few new things:
 * hwsim net namespace stuff from Kirill Tkhai
 * A-MSDU support in fast-RX
 * 4-addr mode support in fast-RX
 * support for a spec quirk in Add-BA negotiation


Felix Fietkau (4):
  mac80211: support AP 4-addr mode fast-rx
  mac80211: support fast-rx with incompatible PS capabilities when PS is 
disabled
  mac80211: support station 4-addr mode fast-rx
  mac80211: support A-MSDU in fast-rx

Ilan Peer (1):
  mac80211: agg-rx: Accept ADDBA request update if timeout did not change

Kirill Tkhai (2):
  mac80211_hwsim: Make hwsim_netgroup IDA
  net: Convert hwsim_net_ops

 drivers/net/wireless/mac80211_hwsim.c |  15 ++--
 include/net/cfg80211.h|   6 +-
 net/mac80211/agg-rx.c |  14 ++-
 net/mac80211/cfg.c|   1 +
 net/mac80211/rx.c | 159 +-
 net/wireless/util.c   |   5 +-
 6 files changed, 128 insertions(+), 72 deletions(-)

Re: KASAN: use-after-free Read in mac80211_hwsim_del_radio

[Johannes Berg]

On Thu, 2018-03-01 at 13:32 +0100, Benjamin Beichler wrote:
> 
> Could you give me a link to or forward the original email ? I googled
> "KASAN: use-after-free Read in mac80211_hwsim_del_radio", but only your
> answer (without the logs) appears. I try to have a look then in the next
> few days.
> 
Yeah oddly it didn't appear on the list (yet?)

I'll fwd to you.

johannes

Re: KASAN: use-after-free Read in mac80211_hwsim_del_radio

[Johannes Berg]

Hi,

> syzbot hit the following crash on upstream commit
> f3afe530d644488a074291da04a69a296ab63046 (Tue Feb 27 22:02:39 2018 +)
> Merge branch 'fixes-v4.16-rc4' of  
> git://git.kernel.org/pub/scm/linux/kernel/git/jmorris/linux-security
> 
> So far this crash happened 4 times on upstream.
> Unfortunately, I don't have any reproducer for this crash yet.
> Raw console output is attached.

That's ... a pretty complex scenario.

Looks like we have a race between destroying a network namespace, which
moves everything back into the init_ns and may have to rename objects
asynchronously (cleanup_net), with destroying the radio in hwsim that's
also asynchronous (destroy_radio).

Benjamin, would you be able to take a look at this? I'm preparing for a
trip and will leave Saturday for a week so I don't think I'll be able
to really dig into this before mid-March.

johannes

Re: pull-request: mac80211-next 2018-02-22

[Johannes Berg]

On Thu, 2018-02-22 at 15:19 -0500, David Miller wrote:
> 
> Pulled, thank you!

Thanks. I just realized that I have a patch pending for -next that
depends another commit in net/mac80211 (or would otherwise conflict
badly while applying, and again while merging later), could I ask you
to pull net into net-next at some point now?

Thanks,
johannes

Re: pull-request: mac80211-next 2018-02-22

[Johannes Berg]

On Thu, 2018-02-22 at 15:19 -0500, David Miller wrote:
> From: Johannes Berg <johan...@sipsolutions.net>
> Date: Thu, 22 Feb 2018 21:16:18 +0100
> 
> > Wireless is slow ... but we're preparing for HE (802.11ax),
> > so I guess soon we'll have a big chunk of work coming :-)
> 
> I wondered where you guys have been hiding :-)

Yeah, I don't like this development model much, but the spec isn't
finished yet and every time I look at an area I end up changing it
*again* which isn't fun to do upstream :-)

(Was just doing the HE sniffer stuff in radiotap these days ... uh,
yeah, I've more or less rewritten it twice already - hopefully no more)

johannes

pull-request: mac80211-next 2018-02-22

[Johannes Berg]

Hi Dave,

Wireless is slow ... but we're preparing for HE (802.11ax),
so I guess soon we'll have a big chunk of work coming :-)

Please pull and let me know if there's any problem.

Thanks,
johannes



The following changes since commit 91e6dd8284256ef62b43b78da6e7684e4f06ac2f:

  ipmr: Fix ptrdiff_t print formatting (2018-01-30 09:20:25 -0500)

are available in the Git repository at:

  git://git.kernel.org/pub/scm/linux/kernel/git/jberg/mac80211-next.git 
tags/mac80211-next-for-davem-2018-02-22

for you to fetch changes up to 94ba92713f8329c96e0a8e2880b3c1a785d1c95c:

  mac80211: Call mgd_prep_tx before transmitting deauthentication (2018-02-22 
21:13:04 +0100)


Various updates across wireless.

One thing to note: I've included a new ethertype
that wireless uses (ETH_P_PREAUTH) in if_ether.h.


Ben Greear (1):
  mac80211: Add txq flags to debugfs

Benjamin Beichler (3):
  mac80211_hwsim: add permanent mac address option for new radios
  mac80211_hwsim: add nl_err_msg in hwsim_new_radio in netlink case
  mac80211_hwsim: add generation count for netlink dump operation

Colin Ian King (1):
  mac80211: remove redundant initialization to pointer 'hdr'

Denis Kenzior (1):
  uapi: Add 802.11 Preauthentication to if_ether

Ilan Peer (1):
  mac80211: Call mgd_prep_tx before transmitting deauthentication

Johannes Berg (2):
  nl80211: remove unnecessary genlmsg_cancel() calls
  mac80211: support reporting A-MPDU EOF bit value/known

Sara Sharon (1):
  mac80211: add get TID helper

Srinivas Dasari (4):
  cfg80211/nl80211: Optional authentication offload to userspace
  nl80211: Allow SAE Authentication for NL80211_CMD_CONNECT
  nl80211: Fix external_auth check for offloaded authentication
  ieee80211: Increase PMK maximum length to 64 bytes

Sunil Dutt (1):
  nl80211: Introduce scan flags to emphasize requested scan behavior

Venkateswara Naralasetty (2):
  cfg80211: send ack_signal to user in probe client response
  mac80211: Add tx ack signal support in sta info

tami...@codeaurora.org (2):
  cfg80211: Add support to notify station's opmode change to userspace
  mac80211: Add support to notify ht/vht opmode modification.

 drivers/net/wireless/ath/wil6210/cfg80211.c |   3 +-
 drivers/net/wireless/mac80211_hwsim.c   |  81 ---
 drivers/net/wireless/mac80211_hwsim.h   |   9 +-
 include/linux/ieee80211.h   |  14 +-
 include/net/cfg80211.h  | 104 +-
 include/net/ieee80211_radiotap.h|   2 +
 include/net/mac80211.h  |  19 +++
 include/uapi/linux/if_ether.h   |   1 +
 include/uapi/linux/nl80211.h|  90 +++-
 net/mac80211/debugfs.c  |   1 +
 net/mac80211/debugfs_sta.c  |  10 +-
 net/mac80211/iface.c|   3 +-
 net/mac80211/michael.c  |   2 +-
 net/mac80211/mlme.c |  18 ++-
 net/mac80211/rc80211_minstrel_ht.c  |   2 +-
 net/mac80211/rx.c   |  24 +++-
 net/mac80211/sta_info.c |   6 +
 net/mac80211/sta_info.h |   2 +
 net/mac80211/status.c   |  11 +-
 net/mac80211/tx.c   |  11 +-
 net/mac80211/vht.c  |   9 ++
 net/mac80211/wpa.c  |   8 +-
 net/wireless/nl80211.c  | 203 +++-
 net/wireless/rdev-ops.h |  15 ++
 net/wireless/trace.h|  23 
 25 files changed, 584 insertions(+), 87 deletions(-)

pull-request: mac80211 2018-02-22

[Johannes Berg]

Hi Dave,

A bunch of fixes, including the nla_put_string() issue
just in from Kees. Otherwise nothing really super urgent
or interesting.

Please pull and let me know if there's any problem.

Thanks,
johannes



The following changes since commit ba804bb4b72e57374b5f567b783aa0298fba0ce6:

  Merge git://git.kernel.org/pub/scm/linux/kernel/git/davem/net (2018-01-26 
09:03:16 -0800)

are available in the Git repository at:

  git://git.kernel.org/pub/scm/linux/kernel/git/jberg/mac80211.git 
tags/mac80211-for-davem-2018-02-22

for you to fetch changes up to 657308f73e674e86b60509a430a46e569bf02846:

  regulatory: add NUL to request alpha2 (2018-02-22 20:57:48 +0100)


Various fixes across the tree, the shortlog basically says it all:

  cfg80211: fix cfg80211_beacon_dup
  -> old bug in this code

  cfg80211: clear wep keys after disconnection
  -> certain ways of disconnecting left the keys

  mac80211: round IEEE80211_TX_STATUS_HEADROOM up to multiple of 4
  -> alignment issues with using 14 bytes

  mac80211: Do not disconnect on invalid operating class
  -> if the AP has a bogus operating class, let it be

  mac80211: Fix sending ADDBA response for an ongoing session
  -> don't send the same frame twice

  cfg80211: use only 1Mbps for basic rates in mesh
  -> interop issue with old versions of our code

  mac80211_hwsim: don't use WQ_MEM_RECLAIM
  -> it causes splats because it flushes work on a non-reclaim WQ

  regulatory: add NUL to request alpha2
  -> nla_put_string() issue from Kees

  mac80211: mesh: fix wrong mesh TTL offset calculation
  -> protocol issue

  mac80211: fix a possible leak of station stats
  -> error path might leak memory

  mac80211: fix calling sleeping function in atomic context
  -> percpu allocations need to be made with gfp flags


Arnd Bergmann (1):
  cfg80211: fix cfg80211_beacon_dup

Avraham Stern (1):
  cfg80211: clear wep keys after disconnection

Felix Fietkau (1):
  mac80211: round IEEE80211_TX_STATUS_HEADROOM up to multiple of 4

Ilan Peer (2):
  mac80211: Do not disconnect on invalid operating class
  mac80211: Fix sending ADDBA response for an ongoing session

Johannes Berg (3):
  cfg80211: use only 1Mbps for basic rates in mesh
  mac80211_hwsim: don't use WQ_MEM_RECLAIM
  regulatory: add NUL to request alpha2

Peter Oh (1):
  mac80211: mesh: fix wrong mesh TTL offset calculation

Sara Sharon (2):
  mac80211: fix a possible leak of station stats
  mac80211: fix calling sleeping function in atomic context

 drivers/net/wireless/mac80211_hwsim.c |  2 +-
 include/net/mac80211.h|  2 +-
 include/net/regulatory.h  |  2 +-
 net/mac80211/agg-rx.c |  4 +---
 net/mac80211/cfg.c|  2 +-
 net/mac80211/ieee80211_i.h|  2 +-
 net/mac80211/mesh.c   | 17 ++---
 net/mac80211/spectmgmt.c  |  7 +++
 net/mac80211/sta_info.c   |  3 ++-
 net/wireless/mesh.c   | 25 ++---
 net/wireless/sme.c|  2 ++
 11 files changed, 41 insertions(+), 27 deletions(-)

Re: nla_put_string() vs NLA_STRING

[Johannes Berg]

On Tue, 2018-02-20 at 22:00 -0800, Kees Cook wrote:

> It seems that in at least one case[1], nla_put_string() is being used
> on an NLA_STRING, which lacks a NULL terminator, which leads to
> silliness when nla_put_string() uses strlen() to figure out the size:

Fun! I'm not a big fan of the whole NLA_STRING thing with or without
NUL terminator anyway, it's a bit confusing at times :-)

> This is a problem at least here:
> 
> struct regulatory_request {
> ...
> char alpha2[2];
> ...
> 
> static const struct nla_policy nl80211_policy[NUM_NL80211_ATTR] = {
> ...
> [NL80211_ATTR_REG_ALPHA2] = { .type = NLA_STRING, .len = 2 },
> ...

Yeah, this is clearly stupid. We already fixed one of these, see commit
a5fe8e7695dc ("regulatory: add NUL to alpha2"). I'll fix up the second
one too.

> So, this specific problem needs fixing (in at least two places calling
> nla_put_string(msg, NL80211_ATTR_REG_ALPHA2, ...)). While I suspect
> it's only ever written an extra byte from the following variable in
> the structure which is an enum nl80211_dfs_regions, 

Only one, since the other has alpha2[3] already :-)

And in that case, yes, on little endian and only if the dfs region is
non-zero, though the dfs region was added later so dunno what else
there was - but certainly this struct would have always contained some
enum value that had zero-bytes.

> I worry there
> might be a lot more of these (though I'd hope unterminated strings are
> uncommon for internal representation).

Generally they are, I'd argue.

> And more generally, it seems
> like only the NLA _input_ functions actually check nla_policy details.
> It seems that the output functions should do the same too, yes?

It doesn't really work that way - there's no real guarantee that the
policy is symmetric on input/output.

johannes

Re: WARNING in check_flush_dependency

[Johannes Berg]

On Mon, 2018-02-19 at 19:58 +0100, Dmitry Vyukov wrote:
> 
> > Yeah, we clearly shouldn't have WQ_RECLAIM set on this workqueue...
> 
> Hi Johannes,
> 
> Do you mind to send a patch to fix this?

Yeah, sorry, I've been behind on patches a bit. I just applied it
today, and once I've let it sit for a day for build bot etc. I'll send
it up.

Let me double-check I've added the syzbot annotation to it though ...

johannes

Re: WARNING in check_flush_dependency

[Johannes Berg]

On Mon, 2018-01-22 at 23:39 -0800, syzbot wrote:
> Hello,
> 
> syzbot hit the following crash on upstream commit
> 0d665e7b109d512b7cae3ccef6e8654714887844 (Fri Jan 19 12:49:24 2018 +)
> mm, page_vma_mapped: Drop faulty pointer arithmetics in check_pte()
> 
> So far this crash happened 23 times on net-next, upstream.
> C reproducer is attached.
> syzkaller reproducer is attached.
> Raw console output is attached.
> compiler: gcc (GCC) 7.1.1 20170620
> .config is attached.
> user-space arch: i386
> 
> IMPORTANT: if you fix the bug, please add the following tag to the commit:
> Reported-by: syzbot+41cdaf4232c50e658...@syzkaller.appspotmail.com
> It will help syzbot understand when the bug is fixed. See footer for  
> details.
> If you forward the report, please keep this part and the footer.
> 
> [ cut here ]
> workqueue: WQ_MEM_RECLAIM hwsim_wq:destroy_radio is  
> flushing !WQ_MEM_RECLAIM events_highpri:flush_backlog
> WARNING: CPU: 0 PID: 3706 at kernel/workqueue.c:2439  
> check_flush_dependency+0x239/0x380 kernel/workqueue.c:2435
> Kernel panic - not syncing: panic_on_warn set ...

Yeah, we clearly shouldn't have WQ_RECLAIM set on this workqueue...

johannes

Re: pull-request: mac80211-next 2018-01-22

[Johannes Berg]

On Mon, 2018-01-22 at 10:17 -0500, David Miller wrote:
> From: Johannes Berg <johan...@sipsolutions.net>
> Date: Mon, 22 Jan 2018 14:15:00 +0100
> 
> > A few more (only four, really) changes have come in, so I figured
> > since the merge window hasn't opened yesterday, I'd still send them
> > to you.
> > 
> > Please pull and let me know if there's any problem.
> 
> I had to resolve a conflict in mac80211_hwsim.c, please check my
> resolution and send me any fixes which are necessary.

Sorry, my bad, I apologize. I knew about this and didn't think of it
when writing the pull request.

Resolution looks good - I was worried about the destruction order for a
second there, but it doesn't actually matter.

Thanks!

johannes

pull-request: mac80211-next 2018-01-22

[Johannes Berg]

Hi Dave,

A few more (only four, really) changes have come in, so I figured
since the merge window hasn't opened yesterday, I'd still send them
to you.

Please pull and let me know if there's any problem.

Thanks,
johannes



The following changes since commit 564737f981fb4b4b3266901508bb9b90d9d43de8:

  Merge branch '10GbE' of 
git://git.kernel.org/pub/scm/linux/kernel/git/jkirsher/next-queue (2018-01-14 
12:25:04 -0500)

are available in the Git repository at:

  git://git.kernel.org/pub/scm/linux/kernel/git/jberg/mac80211-next.git 
tags/mac80211-next-for-davem-2018-01-22

for you to fetch changes up to 0ddcff49b672239dda94d70d0fcf50317a9f4b51:

  mac80211_hwsim: fix possible memory leak in hwsim_new_radio_nl() (2018-01-22 
14:03:29 +0100)


Less than a handful of changes:
 * possible memory leak fix in hwsim
 * speed up hwsim
 * add hwsim userspace rate control API
 * code cleanups


Benjamin Beichler (2):
  mac80211_hwsim: add hashtable with mac address keys for faster lookup
  mac80211_hwsim: add hwsim_tx_rate_flags to netlink attributes

Christopher Díaz Riveros (1):
  debugfs_sta: Remove unneeded semicolons

weiyongjun (A) (1):
  mac80211_hwsim: fix possible memory leak in hwsim_new_radio_nl()

 drivers/net/wireless/mac80211_hwsim.c | 95 ---
 drivers/net/wireless/mac80211_hwsim.h | 68 -
 net/mac80211/debugfs_sta.c|  4 +-
 3 files changed, 146 insertions(+), 21 deletions(-)

[PATCH] cfg80211: fix station info handling bugs

[Johannes Berg]

From: Johannes Berg <johannes.b...@intel.com>

Fix two places where the structure isn't initialized to zero,
and thus can't be filled properly by the driver.

Fixes: 4a4b8169501b ("cfg80211: Accept multiple RSSI thresholds for CQM")
Fixes: 9930380f0bd8 ("cfg80211: implement IWRATE")
Signed-off-by: Johannes Berg <johannes.b...@intel.com>
---
Dave, can you apply this as an exception? I'm not really expecting
any other patches to show up now, and seems easier to have a single
patch than a whole pull request (especially now that patchwork seems
to be swallowing mine ...)
---
 net/wireless/nl80211.c | 2 +-
 net/wireless/wext-compat.c | 3 +--
 2 files changed, 2 insertions(+), 3 deletions(-)

diff --git a/net/wireless/nl80211.c b/net/wireless/nl80211.c
index c084dd2205ac..91e55bb85416 100644
--- a/net/wireless/nl80211.c
+++ b/net/wireless/nl80211.c
@@ -9832,7 +9832,7 @@ static int cfg80211_cqm_rssi_update(struct 
cfg80211_registered_device *rdev,
 */
if (!wdev->cqm_config->last_rssi_event_value && wdev->current_bss &&
rdev->ops->get_station) {
-   struct station_info sinfo;
+   struct station_info sinfo = {};
u8 *mac_addr;
 
mac_addr = wdev->current_bss->pub.bssid;
diff --git a/net/wireless/wext-compat.c b/net/wireless/wext-compat.c
index 7ca04a7de85a..05186a47878f 100644
--- a/net/wireless/wext-compat.c
+++ b/net/wireless/wext-compat.c
@@ -1254,8 +1254,7 @@ static int cfg80211_wext_giwrate(struct net_device *dev,
 {
struct wireless_dev *wdev = dev->ieee80211_ptr;
struct cfg80211_registered_device *rdev = wiphy_to_rdev(wdev->wiphy);
-   /* we are under RTNL - globally locked - so can use a static struct */
-   static struct station_info sinfo;
+   struct station_info sinfo = {};
u8 addr[ETH_ALEN];
int err;
 
-- 
2.15.1

Re: WARNING in rfkill_alloc

[Johannes Berg]

On Mon, 2018-01-15 at 10:12 +0100, Dmitry Vyukov wrote:

> However, there can be some surprising things, for example, executing
> one ioctl/setsockopt with data meant for another one, or these
> 0x are actually mean 0 (for involved reasons), 

I think those fff was actually what was throwing me off.

> or we
> can simply have bugs in these descriptions so they don't match C
> structures and then all data is messed/shifted.

No, I think this part was OK.

> If this representation does not make sense to you right away, your
> best bet is looking at/running the C reproducer where you can see true
> data layout:
> 
> 
[...]
Yeah, good point, I should've just done that.

> > Ah, then again, now I see the fault injection - I guess dev_set_name()
> > just failed and we didn't check the return value, will fix that.
> 
> Yes, it's highly likely the root cause. The raw.log file shows there
> there was an immediately preceding fault in kmalloc in the same
> process, in a close stack.

Yep, I submitted the fix now (with the correct reported-by).

Also for the other one, the wiphy_register() warning.

johannes

[PATCH] netlink: extack: avoid parenthesized string constant warning

[Johannes Berg]

From: Johannes Berg <johannes.b...@intel.com>

NL_SET_ERR_MSG() and NL_SET_ERR_MSG_ATTR() lead to the following warning
in newer versions of gcc:
  warning: array initialized from parenthesized string constant

Just remove the parentheses, they're not needed in this context since
anyway since there can be no operator precendence issues or similar.

Signed-off-by: Johannes Berg <johannes.b...@intel.com>
---
 include/linux/netlink.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/include/linux/netlink.h b/include/linux/netlink.h
index 49b4257ce1ea..f3075d6c7e82 100644
--- a/include/linux/netlink.h
+++ b/include/linux/netlink.h
@@ -85,7 +85,7 @@ struct netlink_ext_ack {
  * to the lack of an output buffer.)
  */
 #define NL_SET_ERR_MSG(extack, msg) do {   \
-   static const char __msg[] = (msg);  \
+   static const char __msg[] = msg;\
struct netlink_ext_ack *__extack = (extack);\
\
if (__extack)   \
@@ -101,7 +101,7 @@ struct netlink_ext_ack {
 } while (0)
 
 #define NL_SET_ERR_MSG_ATTR(extack, attr, msg) do {\
-   static const char __msg[] = (msg);  \
+   static const char __msg[] = msg;\
struct netlink_ext_ack *__extack = (extack);\
\
if (__extack) { \
-- 
2.15.1

pull-request: mac80211 2018-01-15

[Johannes Berg]

Hi Dave,

I know this comes last minute, so if it doesn't make it then
I guess we can live with that, but I got the earliest of the
patches here on Wednesday last week, and that was the most
uninteresting one - the others didn't come in until Saturday.

Please pull and let me know if there's any problem.

Thanks,
johannes



The following changes since commit 8155aedf512edd3f88ef19f7cacf476ace7d1322:

  Merge git://git.kernel.org/pub/scm/linux/kernel/git/bpf/bpf (2018-01-14 
11:01:33 -0500)

are available in the Git repository at:

  git://git.kernel.org/pub/scm/linux/kernel/git/jberg/mac80211.git 
tags/mac80211-for-davem-2018-01-15

for you to fetch changes up to 59b179b48ce2a6076448a44531242ac2b3f6cef2:

  cfg80211: check dev_set_name() return value (2018-01-15 11:35:06 +0100)


More fixes:
 * hwsim:
- properly flush deletion works at module unload
- validate # of channels passed from userspace
 * cfg80211:
- fix RCU locking regression
- initialize on-stack channel data for nl80211 event
- check dev_set_name() return value


Benjamin Beichler (1):
  mac80211_hwsim: add workqueue to wait for deferred radio deletion on mod 
unload

Dominik Brodowski (1):
  nl80211: take RCU read lock when calling ieee80211_bss_get_ie()

Johannes Berg (3):
  cfg80211: fully initialize old channel for event
  mac80211_hwsim: validate number of different channels
  cfg80211: check dev_set_name() return value

 drivers/net/wireless/mac80211_hwsim.c | 17 +++--
 include/net/cfg80211.h|  2 ++
 net/wireless/core.c   |  8 +++-
 net/wireless/core.h   |  2 --
 net/wireless/nl80211.c| 11 +++
 net/wireless/reg.c|  3 +--
 6 files changed, 32 insertions(+), 11 deletions(-)

Re: WARNING in rfkill_alloc

[Johannes Berg]

Hi,

> RIP: 0010:rfkill_alloc+0x2c0/0x380 net/rfkill/core.c:930

This seems pretty obvious - there's no name given.

>   wiphy_new_nm+0x159c/0x21d0 net/wireless/core.c:487
>   ieee80211_alloc_hw_nm+0x4b4/0x2140 net/mac80211/main.c:531

which is strange, because we try to validate the name here.

Can you help me read this?

sendmsg$nl_generic(r1, &(0x7fb3e000-0x38)={&(0x7fd4a000-
0xc)={0x10, 0x0, 0x0, 0x0}, 0xc,
&(0x7f007000)={&(0x7f1ca000)={0x14, 0x1c, 0x109,
0x, 0x, {0x4, 0x0, 0x0}, []}, 0x14},
0x1, 0x0, 0x0, 0x0}, 0x0)

I've reformatted it as

sendmsg$nl_generic(
r1,
&(0x7fb3e000-0x38)={
addr=   &(0x7fd4a000-0xc)={
0x10, 0x0, 0x0, 0x0
},
addrlen=0xc,
vec=&(0x7f007000)={
ptr=&(0x7f1ca000)={
0x14, 0x1c, 0x109, 0x,
0x, {0x4, 0x0, 0x0}, []
},
len=0x14
},
vlen=   0x1,
ctrl=   0x0,
ctrllen=0x0,
f=  0x0
},
0x0
)

but am still getting lost - what exactly is the *byte* sequence inside
the (full) message (including headers)?


Ah, then again, now I see the fault injection - I guess dev_set_name()
just failed and we didn't check the return value, will fix that.

johannes

Re: WARNING in wiphy_register

[Johannes Berg]

Hi syzbot maintainers,

Thanks for the report.

>   hwsim_new_radio_nl+0x5b7/0x7c0 drivers/net/wireless/mac80211_hwsim.c:3152
>   genl_family_rcv_msg+0x7b7/0xfb0 net/netlink/genetlink.c:599
>   genl_rcv_msg+0xb2/0x140 net/netlink/genetlink.c:624

You're getting into the kernel via generic netlink receive, so just as
an FYI - the generic netlink numbers aren't stable across systems, so
your reproducer has a quite good chance of not working without your
kernel .config and (virt) hardware environment.

I'll take a look at this and the rfkill one, I assume that there are
some sanity checks missing in hwsim generic netlink when it builds a
radio struct.

However, I can't really promise that I'll be able to validate the
changes against your reproducer.

johannes

Re: [PATCH v3] nl80211: take RCU read lock when calling ieee80211_bss_get_ie()

[Johannes Berg]

On Mon, 2018-01-15 at 08:12 +0100, Dominik Brodowski wrote:
> As ieee80211_bss_get_ie() derefences an RCU to return ssid_ie, both
> the call to this function and any operation on this variable need
> protection by the RCU read lock.
> 
> Fixes: 44905265bc15 ("nl80211: don't expose wdev->ssid for most interfaces")
> Signed-off-by: Dominik Brodowski 
> ---
> 
> > but after, perhaps it's easier to just do
> > 
> > if (ssid_ie &&
> > nla_put(...)
> > goto nla_put_failure_rcu_locked;
> > 
> > and avoid the extra label (but yeah, it's getting late)
> 
> OK, done that (and updated the commit message), and testet it.
> 

Applied, thanks!

johannes

Re: [PATCH v2] nl80211: take RCU read lock when calling ieee80211_bss_get_ie()

[Johannes Berg]

On Sun, 2018-01-14 at 23:22 +0100, Dominik Brodowski wrote:
> 
> + rcu_read_lock();
>   ssid_ie = ieee80211_bss_get_ie(>current_bss->pub,
>  WLAN_EID_SSID);
>   if (!ssid_ie)
> - break;

nit-picking now: that "break" here may have been easier before these
changes

> + goto nla_rcu_unlock;
>   if (nla_put(msg, NL80211_ATTR_SSID, ssid_ie[1], ssid_ie + 2))
> - goto nla_put_failure_locked;
> + goto nla_put_failure_rcu_locked;
> + nla_rcu_unlock:
> + rcu_read_unlock();
>   break;

but after, perhaps it's easier to just do

if (ssid_ie &&
nla_put(...)
goto nla_put_failure_rcu_locked;

and avoid the extra label (but yeah, it's getting late)

johannes

Re: [PATCH] nl80211: take RCU read lock when calling ieee80211_bss_get_ie()

[Johannes Berg]

Hi,

> Fixes: 44905265bc15 ("nl80211: don't expose wdev->ssid for most interfaces")
> Signed-off-by: Dominik Brodowski 
> ---
> 
> This patch fixes the regression I reported in the last couple of weeks for
> various v4.15-rcX revisions to netdev, where a "suspicious RCU usage"
> showed up in net/wireless/util.c:778.

Huh. You should added linux-wireless to those reports, I simply didn't
see them!

> diff --git a/net/wireless/nl80211.c b/net/wireless/nl80211.c
> index 2b3dbcd40e46..1eecc249fb5e 100644
> --- a/net/wireless/nl80211.c
> +++ b/net/wireless/nl80211.c
> @@ -2618,8 +2618,10 @@ static int nl80211_send_iface(struct sk_buff *msg, u32 
> portid, u32 seq, int flag
>   const u8 *ssid_ie;
>   if (!wdev->current_bss)
>   break;
> + rcu_read_lock();
>   ssid_ie = ieee80211_bss_get_ie(>current_bss->pub,
>  WLAN_EID_SSID);
> + rcu_read_unlock();
>   if (!ssid_ie)
>   break;
>   if (nla_put(msg, NL80211_ATTR_SSID, ssid_ie[1], ssid_ie + 2))

This uses the ssid_ie, so that doesn't really seem right? The
protection should extend beyond the usage.

johannes

Re: [PATCH] nl80211: Check for the required netlink attribute presence

[Johannes Berg]

On Wed, 2018-01-03 at 11:00 +0800, Hao Chen wrote:
> nl80211_nan_add_func() does not check if the required attribute
> NL80211_NAN_FUNC_FOLLOW_UP_DEST is present when processing
> NL80211_CMD_ADD_NAN_FUNCTION request. This request can be issued
> by users with CAP_NET_ADMIN privilege and may result in NULL dereference
> and a system crash. Add a check for the required attribute presence.

Applied, thanks.

johannes

pull-request: mac80211-next 2018-01-04

[Johannes Berg]

Hi Dave,

And here's a set for -next. It's not really big, and things
are all over the place. I'm thinking that Luca hasn't flushed
his queue out, but perhaps that'll then wait for the next
release, unless there are fixes that he still has.

Please pull and let me know if there's any problem.

Thanks,
johannes



The following changes since commit 51e18a453f5f59a40c721d4aeab082b4e2e9fac6:

  Merge git://git.kernel.org/pub/scm/linux/kernel/git/davem/net (2017-12-09 
22:09:55 -0500)

are available in the Git repository at:

  git://git.kernel.org/pub/scm/linux/kernel/git/jberg/mac80211-next.git 
tags/mac80211-next-for-davem-2018-01-04

for you to fetch changes up to 3a3713ec360138f806c6fc368d1de570f692b347:

  mac80211: Fix setting TX power on monitor interfaces (2018-01-04 15:27:48 
+0100)


We have things all over the place, no point listing them.

One thing is notable: I applied two patches and later
reverted them - we'll get back to that once all the driver
situation is sorted out.


Adiel Aloni (1):
  mac80211_hwsim: enforce PS_MANUAL_POLL to be set after PS_ENABLED

David Spinadel (2):
  mac80211: Add MIC space only for TX key option
  nl80211: send deauth reason if locally generated

Emmanuel Grumbach (1):
  mac80211: always update the PM state of a peer on MGMT / DATA frames

Gustavo A. R. Silva (1):
  mac80211: mark expected switch fall-throughs

Johannes Berg (6):
  mac80211: avoid looking up tid_tx/tid_rx from timers
  mac80211: make __ieee80211_start_rx_ba_session static
  nl80211: add a few extended error strings to key parsing
  mac80211: don't warn on AID field without top two MSBs set
  Revert "mac80211: Add airtime account and scheduling to TXQs"
  Revert "mac80211: Add TXQ scheduling API"

Luca Coelho (1):
  mac80211: remove BUG() when interface type is invalid

Peter Große (1):
  mac80211: Fix setting TX power on monitor interfaces

Sara Sharon (1):
  mac80211: call synchronize_net once in the restart flow

Sergey Matyukevich (1):
  cfg80211: cleanup signal strength units notation

Sunil Dutt (1):
  cfg80211: Scan results to also report the per chain signal strength

Toke Høiland-Jørgensen (2):
  mac80211: Add TXQ scheduling API
  mac80211: Add airtime account and scheduling to TXQs

Tova Mussai (1):
  cfg80211: IBSS: Add support for static WEP in driver for IBSS

Yingying Tang (1):
  mac80211: enable TDLS peer buffer STA feature

 drivers/net/wireless/mac80211_hwsim.c | 17 +
 include/net/cfg80211.h| 17 +++--
 include/net/mac80211.h| 10 +-
 include/uapi/linux/nl80211.h  |  4 +++
 net/mac80211/agg-rx.c | 26 +-
 net/mac80211/agg-tx.c | 34 ++
 net/mac80211/cfg.c| 31 +++-
 net/mac80211/debugfs.c|  1 +
 net/mac80211/driver-ops.h |  3 +-
 net/mac80211/ht.c |  1 +
 net/mac80211/ieee80211_i.h|  4 ---
 net/mac80211/iface.c  |  4 +--
 net/mac80211/key.c| 12 +--
 net/mac80211/main.c   |  3 ++
 net/mac80211/mesh.c   |  2 ++
 net/mac80211/mesh_hwmp.c  |  1 +
 net/mac80211/mesh_plink.c |  2 +-
 net/mac80211/mlme.c   | 10 +++---
 net/mac80211/offchannel.c |  4 +--
 net/mac80211/rx.c | 17 +++--
 net/mac80211/tdls.c   |  6 +++-
 net/mac80211/tx.c |  4 ++-
 net/mac80211/util.c   | 19 +-
 net/mac80211/wme.c|  1 +
 net/mac80211/wpa.c| 16 ++---
 net/wireless/ibss.c   |  5 +++
 net/wireless/mlme.c   |  6 ++--
 net/wireless/nl80211.c| 68 ---
 net/wireless/scan.c   |  5 +++
 net/wireless/trace.h  | 12 +++
 30 files changed, 219 insertions(+), 126 deletions(-)

pull-request: mac80211 2018-01-04

[Johannes Berg]

Hi Dave,

It's probably getting quite late for the current cycle, but
these fixes seemed important enough to send them to you
separately anyway.

Please pull and let me know if there's any problem.

Thanks,
johannes



The following changes since commit 820d1d5eba5e0db88432f4b19149d87523ee193c:

  Merge branch '40GbE' of 
git://git.kernel.org/pub/scm/linux/kernel/git/jkirsher/net-queue (2018-01-03 
13:49:24 -0500)

are available in the Git repository at:

  git://git.kernel.org/pub/scm/linux/kernel/git/jberg/mac80211.git 
tags/mac80211-for-davem-2018-01-04

for you to fetch changes up to 736a80bbfda709fb3631f5f62056f250a38e5804:

  mac80211: mesh: drop frames appearing to be from us (2018-01-04 15:51:53 
+0100)


Two fixes:
 * drop mesh frames appearing to be from ourselves
 * check another netlink attribute for existence


Hao Chen (1):
  nl80211: Check for the required netlink attribute presence

Johannes Berg (1):
  mac80211: mesh: drop frames appearing to be from us

 net/mac80211/rx.c  | 2 ++
 net/wireless/nl80211.c | 3 ++-
 2 files changed, 4 insertions(+), 1 deletion(-)

Re: [net] Revert "net: core: maybe return -EEXIST in __dev_alloc_name"

[Johannes Berg]

On Tue, 2018-01-02 at 11:50 -0500, David Miller wrote:
> From: Michael Ellerman <m...@ellerman.id.au>
> Date: Fri, 22 Dec 2017 15:22:22 +1100
> 
> >> On Tue, Dec 19 2017, Michael Ellerman <mich...@concordia.ellerman.id.au> 
> >> wrote:
> >>> This revert seems to have broken networking on one of my powerpc
> >>> machines, according to git bisect.
> >>>
> >>> The symptom is DHCP fails and I don't get a link, I didn't dig any
> >>> further than that. I can if it's helpful.
> >>>
> >>> I think the problem is that 87c320e51519 ("net: core: dev_get_valid_name
> >>> is now the same as dev_alloc_name_ns") only makes sense while
> >>> d6f295e9def0 remains in the tree.
> >>
> >> I'm sorry about all of this, I really didn't think there would be such
> >> consequences of changing an errno return. Indeed, d6f29 was preparation
> >> for unifying the two functions that do the exact same thing (and how we
> >> ever got into that situation is somewhat unclear), except for
> >> their behaviour in the case the requested name already exists. So one of
> >> the two interfaces had to change its return value, and as I wrote, I
> >> thought EEXIST was the saner choice when an explicit name (no %d) had
> >> been requested.
> > 
> > No worries.
> > 
> >>> ie. before the entire series, dev_get_valid_name() would return EEXIST,
> >>> and that was retained when 87c320e51519 was merged, but now that
> >>> d6f295e9def0 has been reverted dev_get_valid_name() is returning ENFILE.
> >>>
> >>> I can get the network up again if I also revert 87c320e51519 ("net:
> >>> core: dev_get_valid_name is now the same as dev_alloc_name_ns"), or with
> >>> the gross patch below.
> >>
> >> I don't think changing -ENFILE to -EEXIST would be right either, since
> >> dev_get_valid_name() used to be able to return both (-EEXIST in the case
> >> where there's no %d, -ENFILE in the case where we end up calling
> >> dev_alloc_name_ns()). If anything, we could do the check for the old
> >> -EEXIST condition first, and then call dev_alloc_name_ns(). But I'm also
> >> fine with reverting.
> > 
> > Yeah I think a revert would be best, given it's nearly rc5.
> > 
> > My userspace is not exotic AFAIK, just debian something, so presumably
> > this will affect other people too.
> 
> I've just queued up the following revert, thanks!
> 
> 
> From 5047543928139184f060c8f3bccb788b3df4c1ea Mon Sep 17 00:00:00 2001
> From: "David S. Miller" <da...@davemloft.net>
> Date: Tue, 2 Jan 2018 11:45:07 -0500
> Subject: [PATCH] Revert "net: core: dev_get_valid_name is now the same as
>  dev_alloc_name_ns"
> 
> This reverts commit 87c320e51519a83c496ab7bfb4e96c8f9c001e89.
> 
> Changing the error return code in some situations turns out to
> be harmful in practice.  In particular Michael Ellerman reports
> that DHCP fails on his powerpc machines, and this revert gets
> things working again.
> 
> Johannes Berg agrees that this revert is the best course of
> action for now.

I'm not sure my voice matters much, I merely did the first revert of
these two patches ... :)

But I agree with Michael that you can't really salvage this without the
other patch, and that one caused problems in wifi ...

Thanks :)

johannes 

Re: [net] Revert "net: core: maybe return -EEXIST in __dev_alloc_name"

[Johannes Berg]

Hi,

> This revert seems to have broken networking on one of my powerpc
> machines, according to git bisect.

Fun!

TBH, I only looked at the immediate problem we ran into, and reverted
what was causing it. I don't think we saw the follow-up problem you're
seeing.

> The symptom is DHCP fails and I don't get a link, I didn't dig any
> further than that. I can if it's helpful.
> 
> I think the problem is that 87c320e51519 ("net: core: dev_get_valid_name
> is now the same as dev_alloc_name_ns") only makes sense while
> d6f295e9def0 remains in the tree.
> 
> ie. before the entire series, dev_get_valid_name() would return EEXIST,
> and that was retained when 87c320e51519 was merged, but now that
> d6f295e9def0 has been reverted dev_get_valid_name() is returning ENFILE.
> 
> I can get the network up again if I also revert 87c320e51519 ("net:
> core: dev_get_valid_name is now the same as dev_alloc_name_ns"), or with
> the gross patch below.

Makes sense. I guess that should be reverted too then, or even your
"gross" patch applied.

johannes

pull-request: mac80211 2017-12-19

[Johannes Berg]

Hi Dave,

Other work has been hectic, and I got caught by rc4. We still
have a few more fixes though - and more build issues were
reported.

Please pull and let me know if there's any problem.

Thanks,
johannes



The following changes since commit 0afe9d4ab9d40c281bdcdd118661fe8e4bdcef18:

  mac80211: fix locking in ieee80211_sta_tear_down_BA_sessions (2017-12-11 
10:50:00 +0100)

are available in the git repository at:

  git://git.kernel.org/pub/scm/linux/kernel/git/jberg/mac80211.git 
tags/mac80211-for-davem-2017-12-19

for you to fetch changes up to 04a7279ff12fc47b657f70731d401c0064f5838a:

  cfg80211: ship certificates as hex files (2017-12-19 09:28:01 +0100)


A few more fixes:
 * hwsim:
   - set To-DS bit in some frames missing it
   - fix sleeping in atomic
 * nl80211:
   - doc cleanup
   - fix locking in an error path
 * build:
   - don't append to created certs C files
   - ship certificate pre-hexdumped


Adiel Aloni (1):
  mac80211_hwsim: enable TODS BIT in null data frame

Jia-Ju Bai (1):
  mac80211_hwsim: Fix a possible sleep-in-atomic bug in hwsim_get_radio_nl

Johannes Berg (2):
  nl80211: fix nl80211_send_iface() error paths
  cfg80211: ship certificates as hex files

Jonathan Corbet (1):
  nl80211: Remove obsolete kerneldoc line

Thierry Reding (1):
  cfg80211: always rewrite generated files from scratch

 drivers/net/wireless/mac80211_hwsim.c |   3 +-
 include/net/cfg80211.h|   1 -
 net/wireless/Makefile |  31 
 net/wireless/certs/sforshee.hex   |  86 ++
 net/wireless/certs/sforshee.x509  | Bin 680 -> 0 bytes
 net/wireless/nl80211.c|   6 ++-
 6 files changed, 102 insertions(+), 25 deletions(-)
 create mode 100644 net/wireless/certs/sforshee.hex
 delete mode 100644 net/wireless/certs/sforshee.x509

Re: [PATCH] wireless: Always rewrite generated files from scratch

[Johannes Berg]

On Thu, 2017-12-14 at 14:33 +0100, Thierry Reding wrote:
> From: Thierry Reding 
> 
> Currently the certs C code generation appends to the generated files,
> which is most likely a leftover from commit 715a12334764 ("wireless:
> don't write C files on failures"). This causes duplicate code in the
> generated files if the certificates have their timestamps modified
> between builds and thereby trigger the generation rules.

Yes, d'oh. Applied, thanks.

johannes

Re: linux-next: build failure after merge of the mac80211-next tree

[Johannes Berg]

Hi Stephen,

Thanks!

Felix made me aware of this yesterday evening and said he's going to
work out the required changes to mt76.

Kalle and I will make sure to submit the trees to Dave one by one so he
doesn't have to deal with it :)

Unfortunately, this might take a few days to resolve.

> -void mt76_wake_tx_queue(struct ieee80211_hw *hw, struct ieee80211_txq *txq)
> +void mt76_wake_tx_queue(struct ieee80211_hw *hw)
>  {
> + struct ieee80211_txq *txq;
>   struct mt76_dev *dev = hw->priv;
> - struct mt76_txq *mtxq = (struct mt76_txq *) txq->drv_priv;
> - struct mt76_queue *hwq = mtxq->hwq;
> + struct mt76_txq *mtxq;
> + struct mt76_queue *hwq;
>  
> + txq = ieee80211_next_txq(hw);
> + mtxq = (struct mt76_txq *) txq->drv_priv;
> + hwq = mtxq->hwq;


Looks pretty much right to me - perhaps for safety there should be a
NULL check on txq, but OTOH when you get here it should be non-NULL.

Sorry for the inconvenience, I hadn't realized mt76 went in now.

johannes

Re: 76f43b4 fix for stable

[Johannes Berg]

On Mon, 2017-12-11 at 14:51 +0100, Richard Schütz wrote:
> Hello,
> 
> as per netdev-FAQ.txt I'm requesting the submission of commit 
> 57629915d568c522ac1422df7bba4bee5b5c7a7c ("mac80211: Fix addition of 
> mesh configuration element") to stable. Because of automatic selection 
> commit 76f43b4c0a9337af22827d78de4f2b8fd5328489 ("mac80211: Remove 
> invalid flag operations in mesh TSF synchronization") was introduced 
> into stable recently without this accompanying fix.

It seems that due to the Fixes: tag Greg should pick it up anyway, but
in any case I don't really deal with stable myself (in that sense, I
guess wireless isn't really part of netdev...)

johannes

pull-request: mac80211 2017-12-11

[Johannes Berg]

Hi Dave,

Three fixes, two related to build issues with the new regdb stuff,
and one for some patch overlap problem that caused locking to be
missing which in turn caused lots of warnings.

Please pull and let me know if there's any problem.

Thanks,
johannes



The following changes since commit ccab371f746abca05a599d074cb3b95a549ef590:

  Merge branch 'sfp-phylink-fixes' (2017-12-01 15:18:42 -0500)

are available in the git repository at:

  git://git.kernel.org/pub/scm/linux/kernel/git/jberg/mac80211.git 
tags/mac80211-for-davem-2017-12-11

for you to fetch changes up to 0afe9d4ab9d40c281bdcdd118661fe8e4bdcef18:

  mac80211: fix locking in ieee80211_sta_tear_down_BA_sessions (2017-12-11 
10:50:00 +0100)


Three fixes:
 * for certificate C file generation, don't use hexdump as it's
   not always installed by default, use pure posix instead (od/sed)
 * for certificate C file generation, don't write the file if
   anything fails, so the build abort will not cause a bad build
   upon a second attempt
 * fix locking in ieee80211_sta_tear_down_BA_sessions() which had
   been causing lots of locking warnings


Johannes Berg (3):
  wireless: replace usage of hexdump with od/sed
  wireless: don't write C files on failures
  mac80211: fix locking in ieee80211_sta_tear_down_BA_sessions

 net/mac80211/ht.c |  5 ++---
 net/wireless/Makefile | 48 ++--
 2 files changed, 40 insertions(+), 13 deletions(-)

Re: Wireless regressions in v4.15-rc1

[Johannes Berg]

On Sat, 2017-12-02 at 14:59 +0200, Kalle Valo wrote:
> 
> net: netlink: Update attr validation to require exact length for some types
> https://git.kernel.org/linus/28033ae4e0f5

This was reverted, more or less, to print only a warning:

https://git.kernel.org/pub/scm/linux/kernel/git/davem/net.git/commit/?id=6e237d099fac1f73a7b6d7287bb9191f29585a4e

> Jouni fixed this already in hostapd but we also need a fix for kernel so
> that old hostapd versions continue to work:
> 
> https://w1.fi/cgit/hostap/commit/?id=a2426829ce426de82d2fa47071ca41ea81c43307
> 
> Jouni also found a similar problem with mesh:
> 
> https://w1.fi/cgit/hostap/commit/?id=963d3149abfcbab5b83f9023bc50321f777360d1

See above for the kernel revert to let old hostapd versions continue to
work (properly only on little endian platforms) when they don't have
these fixes.

> And Johannes already submitted a revert related to wpa_supplicant:
> 
> [net] Revert "net: core: maybe return -EEXIST in __dev_alloc_name" 
> diffmboxseries
> https://patchwork.ozlabs.org/patch/843863/

That was applied.

> And with ath10k I'm now seeing this:
> 
> [  133.175508] WARNING: CPU: 2 PID: 1743 at net/mac80211/agg-tx.c:315 
> ___ieee80211_stop_tx_ba_session+0x1ab/0x280 [mac80211]

Just sent a fix for this, kinda a merge/patch failure.

> johannes

Re: [net] netlink: Relax attr validation for fixed length types

[Johannes Berg]

On Tue, 2017-12-05 at 12:55 -0700, David Ahern wrote:
> @@ -70,10 +78,9 @@ static int validate_nla(const struct nlattr *nla, int 
> maxtype,
>   BUG_ON(pt->type > NLA_TYPE_MAX);
>  
>   /* for data types NLA_U* and NLA_S* require exact length */

You should update the comment now :-)

And the comment on nla_attr_len as well.

With the comments fixed, this looks fine to me.

Reviewed-by: Johannes Berg <johan...@sipsolutions.net>


Since we already have two tables now and may want to add attribute
types with exact enforcement in the future (like the BITFIELD32 one),
I'd actually do things a bit more data driven, but I haven't tested it
right now, and it's better done in net-next after this fix.


diff --git a/lib/nlattr.c b/lib/nlattr.c
index 8bf78b4b78f0..e65eb5400a1a 100644
--- a/lib/nlattr.c
+++ b/lib/nlattr.c
@@ -15,21 +15,67 @@
 #include 
 #include 
 
-/* for these data types attribute length must be exactly given size */
-static const u8 nla_attr_len[NLA_TYPE_MAX+1] = {
-   [NLA_U8]= sizeof(u8),
-   [NLA_U16]   = sizeof(u16),
-   [NLA_U32]   = sizeof(u32),
-   [NLA_U64]   = sizeof(u64),
-   [NLA_S8]= sizeof(s8),
-   [NLA_S16]   = sizeof(s16),
-   [NLA_S32]   = sizeof(s32),
-   [NLA_S64]   = sizeof(s64),
-};
-
-static const u8 nla_attr_minlen[NLA_TYPE_MAX+1] = {
-   [NLA_MSECS] = sizeof(u64),
-   [NLA_NESTED]= NLA_HDRLEN,
+/* netlink validation flags */
+#define NLVF_WARN_WRONG_LENBIT(0)
+#define NLVF_REJECT_WRONG_LEN  BIT(1)
+
+static const struct {
+   u8 len, flags;
+} nla_attr_val[NLA_TYPE_MAX + 1] = {
+   [NLA_FLAG] = {
+   .len = 0,
+   .flags = NLVF_REJECT_WRONG_LEN,
+   },
+   [NLA_BITFIELD32] = {
+   /* further checks below */
+   .len = sizeof(struct nla_bitfield32),
+   .flags = NLVF_REJECT_WRONG_LEN,
+   },
+   [NLA_U8] = {
+   .len = sizeof(u8),
+   .flags = NLVF_WARN_WRONG_LEN,
+   },
+   [NLA_U16] = {
+   .len = sizeof(u16),
+   .flags = NLVF_WARN_WRONG_LEN,
+   },
+   [NLA_U32] = {
+   .len = sizeof(u32),
+   .flags = NLVF_WARN_WRONG_LEN,
+   },
+   [NLA_U64] = {
+   .len = sizeof(u64),
+   .flags = NLVF_WARN_WRONG_LEN,
+   },
+   [NLA_S8] = {
+   .len = sizeof(s8),
+   .flags = NLVF_WARN_WRONG_LEN,
+   },
+   [NLA_S16] = {
+   .len = sizeof(s16),
+   .flags = NLVF_WARN_WRONG_LEN,
+   },
+   [NLA_S32] = {
+   .len = sizeof(s32),
+   .flags = NLVF_WARN_WRONG_LEN,
+   },
+   [NLA_S64] = {
+   .len = sizeof(s64),
+   .flags = NLVF_WARN_WRONG_LEN,
+   },
+   [NLA_MSECS] = {
+   .len = sizeof(s64),
+   .flags = NLVF_WARN_WRONG_LEN,
+   },
+   [NLA_NESTED] = {
+   /* minimum length */
+   .len = NLA_HDRLEN,
+   },
+   [NLA_STRING] = {
+   /* minimum length, further checks below */
+   .len = 1,
+   },
+   /* others have .len = 0 and no flags */
 };
 
 static int validate_nla_bitfield32(const struct nlattr *nla,
@@ -60,7 +106,7 @@ static int validate_nla(const struct nlattr *nla, int 
maxtype,
const struct nla_policy *policy)
 {
const struct nla_policy *pt;
-   int minlen = 0, attrlen = nla_len(nla), type = nla_type(nla);
+   int minlen, attrlen = nla_len(nla), type = nla_type(nla);
 
if (type <= 0 || type > maxtype)
return 0;
@@ -69,23 +115,51 @@ static int validate_nla(const struct nlattr *nla, int 
maxtype,
 
BUG_ON(pt->type > NLA_TYPE_MAX);
 
-   /* for data types NLA_U* and NLA_S* require exact length */
-   if (nla_attr_len[pt->type]) {
-   if (attrlen != nla_attr_len[pt->type])
+   switch (pt->type) {
+   case NLA_BINARY:
+   if (pt->len && attrlen > pt->len)
return -ERANGE;
-   return 0;
-   }
+   break;
 
-   switch (pt->type) {
-   case NLA_FLAG:
-   if (attrlen > 0)
+   case NLA_NESTED_COMPAT:
+   if (attrlen < pt->len)
+   return -ERANGE;
+   if (attrlen < NLA_ALIGN(pt->len))
+   break;
+   if (attrlen < NLA_ALIGN(pt->len) + NLA_HDRLEN)
+   return -ERANGE;
+   nla = nla_data(nla) + NLA_ALIGN(pt->len);
+   if (attrlen < NLA_ALIGN(pt->len) + NLA_HDRLEN + nla_len(nla))
return -ERANGE;
break;
 
-   case NLA_BITFIELD32:
-   if (attrlen != sizeof(struct nla_bitfield32))
+   case NLA_NESTED:
+   

Re: [PATCH net 1/2] netlink: add NLA_U8_BUGGY attribute type

[Johannes Berg]

On Tue, 2017-12-05 at 11:41 -0500, David Miller wrote:
> 
> There is no reasonable interpretation for what that application is
> doing, so I think we can safely call that case as buggy.
> 
> We are only trying to handle the situation where a U8 attribute
> is presented as a bonafide U32 or a correct U8.
> 
> Does this make sense?

Well the application is buggy, but we don't really know in what way?
Perhaps somebody even did the equivalent of
nla_put_u32(ATTR, cpu_to_le32(x))
when they noticed it was broken on BE, and end up with a similar case
as I had above.

I don't think there's a good solution to this, applications must be
fixed anyhow. I'm just saying that I'd save the extra code and stay
compatible with applications as written today, even if they're now
broken on BE - and rely on the warning to fix it. Trying to fix it up
seems to have the potential to just break something else.

johannes

Re: [PATCH net 1/2] netlink: add NLA_U8_BUGGY attribute type

[Johannes Berg]

On Tue, 2017-12-05 at 11:31 -0500, David Miller wrote:
> 
> > We could try to fix up the big endian problem here, but we
> > don't know *how* userspace misbehaved - if using nla_put_u32
> > then we could, but we also found a debug tool (which we'll
> > ignore for the purposes of this regression) that was putting
> > the padding into the length.

> We're stuck with this thing forever... I'd like to consider other
> options.
> 
> I've seen this problem at least one time before, therefore I
> suggest when we see a U8 attribute with a U32's length:
> 
> 1) We access it as a u32, this takes care of all endianness
>issues.

Possible, but as I said above, I've seen at least one tool (a debug
only script) now that will actually emit a U8 followed by 3 bytes of
padding to make it netlink-aligned, but set the length to 4. That would
be broken by making this change.

I'm not saying this is bad - but there are different levels of
compatibility and I'd probably go for "bug compatibility" here rather
than "fix-it-up compatibility".

Your call, ultimately - I've already fixed the tool I had found :-)

> 2) We emit a warning so that the app gets fixes.

For sure.

johannes

[PATCH net 1/2] netlink: add NLA_U8_BUGGY attribute type

[Johannes Berg]

From: Johannes Berg <johannes.b...@intel.com>

This netlink type is used only for backwards compatibility
with broken userspace that used the wrong size for a given
u8 attribute, which is now rejected. It would've been wrong
before already, since on big endian the wrong value (always
zero) would be used by the kernel, but we can't break the
existing deployed userspace - hostapd for example now fails
to initialize entirely.

We could try to fix up the big endian problem here, but we
don't know *how* userspace misbehaved - if using nla_put_u32
then we could, but we also found a debug tool (which we'll
ignore for the purposes of this regression) that was putting
the padding into the length.

Fixes: 28033ae4e0f5 ("net: netlink: Update attr validation to require exact 
length for some types")
Signed-off-by: Johannes Berg <johannes.b...@intel.com>
---
 include/net/netlink.h | 1 +
 lib/nlattr.c  | 1 +
 2 files changed, 2 insertions(+)

diff --git a/include/net/netlink.h b/include/net/netlink.h
index 0c154f98e987..448a9b86c959 100644
--- a/include/net/netlink.h
+++ b/include/net/netlink.h
@@ -180,6 +180,7 @@ enum {
NLA_S32,
NLA_S64,
NLA_BITFIELD32,
+   NLA_U8_BUGGY, /* don't use this - only for bug-ward compatibility */
__NLA_TYPE_MAX,
 };
 
diff --git a/lib/nlattr.c b/lib/nlattr.c
index 8bf78b4b78f0..2b89d25d4745 100644
--- a/lib/nlattr.c
+++ b/lib/nlattr.c
@@ -28,6 +28,7 @@ static const u8 nla_attr_len[NLA_TYPE_MAX+1] = {
 };
 
 static const u8 nla_attr_minlen[NLA_TYPE_MAX+1] = {
+   [NLA_U8_BUGGY]  = sizeof(u8),
[NLA_MSECS] = sizeof(u64),
[NLA_NESTED]= NLA_HDRLEN,
 };
-- 
2.14.2

[PATCH net 2/2] nl80211: use NLA_U8_BUGGY for two attributes

[Johannes Berg]

From: Johannes Berg <johannes.b...@intel.com>

We discovered that these are set incorrectly by the
corresponding userspace code, so keep compatible with
their bugs even if they'd always set the value to 0.

Reported-by: Jouni Malinen <j...@w1.fi>
Fixes: 28033ae4e0f5 ("net: netlink: Update attr validation to require exact 
length for some types")
Signed-off-by: Johannes Berg <johannes.b...@intel.com>
---
 net/wireless/nl80211.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/net/wireless/nl80211.c b/net/wireless/nl80211.c
index b1ac23ca20c8..751b4efbf09a 100644
--- a/net/wireless/nl80211.c
+++ b/net/wireless/nl80211.c
@@ -384,7 +384,7 @@ static const struct nla_policy 
nl80211_policy[NUM_NL80211_ATTR] = {
[NL80211_ATTR_TSID] = { .type = NLA_U8 },
[NL80211_ATTR_USER_PRIO] = { .type = NLA_U8 },
[NL80211_ATTR_ADMITTED_TIME] = { .type = NLA_U16 },
-   [NL80211_ATTR_SMPS_MODE] = { .type = NLA_U8 },
+   [NL80211_ATTR_SMPS_MODE] = { .type = NLA_U8_BUGGY },
[NL80211_ATTR_MAC_MASK] = { .len = ETH_ALEN },
[NL80211_ATTR_WIPHY_SELF_MANAGED_REG] = { .type = NLA_FLAG },
[NL80211_ATTR_NETNS_FD] = { .type = NLA_U32 },
@@ -5830,7 +5830,7 @@ static const struct nla_policy 
nl80211_meshconf_params_policy[NL80211_MESHCONF_A
[NL80211_MESHCONF_MAX_RETRIES] = { .type = NLA_U8 },
[NL80211_MESHCONF_TTL] = { .type = NLA_U8 },
[NL80211_MESHCONF_ELEMENT_TTL] = { .type = NLA_U8 },
-   [NL80211_MESHCONF_AUTO_OPEN_PLINKS] = { .type = NLA_U8 },
+   [NL80211_MESHCONF_AUTO_OPEN_PLINKS] = { .type = NLA_U8_BUGGY },
[NL80211_MESHCONF_SYNC_OFFSET_MAX_NEIGHBOR] = { .type = NLA_U32 },
[NL80211_MESHCONF_HWMP_MAX_PREQ_RETRIES] = { .type = NLA_U8 },
[NL80211_MESHCONF_PATH_REFRESH_TIME] = { .type = NLA_U32 },
-- 
2.14.2

[PATCH net] Revert "net: core: maybe return -EEXIST in __dev_alloc_name"

[Johannes Berg]

From: Johannes Berg <johannes.b...@intel.com>

This reverts commit d6f295e9def0; some userspace (in the case
we noticed it's wpa_supplicant), is relying on the current
error code to determine that a fixed name interface already
exists.

Reported-by: Jouni Malinen <j...@w1.fi>
Signed-off-by: Johannes Berg <johannes.b...@intel.com>
---
 net/core/dev.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/core/dev.c b/net/core/dev.c
index 07ed21d64f92..f47e96b62308 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -1106,7 +1106,7 @@ static int __dev_alloc_name(struct net *net, const char 
*name, char *buf)
 * when the name is long and there isn't enough space left
 * for the digits, or if all bits are used.
 */
-   return p ? -ENFILE : -EEXIST;
+   return -ENFILE;
 }
 
 static int dev_alloc_name_ns(struct net *net,
-- 
2.14.2

pull-request: mac80211 2017-11-27

[Johannes Berg]

Hi Dave,

Here are a few more fixes, one of which fixes the crash Florian
reported which I think is the same that zero-day reported.

Please pull and let me know if there's any problem.

Thanks,
johannes



The following changes since commit a13e8d418f3cb9d0b4efa6e744b8b23c0e3cdfe8:

  Merge tag 'mac80211-for-davem-2017-11-20' of 
git://git.kernel.org/pub/scm/linux/kernel/git/jberg/mac80211 (2017-11-21 
20:30:57 +0900)

are available in the git repository at:

  git://git.kernel.org/pub/scm/linux/kernel/git/jberg/mac80211.git 
tags/mac80211-for-davem-2017-11-27

for you to fetch changes up to 72e2c3438ba3bd2ed640b6b5ea9e58993dd9ab7f:

  mac80211: tear down RX aggregations first (2017-11-27 11:24:59 +0100)


Four fixes:
 * CRYPTO_SHA256 is needed for regdb validation
 * mac80211: mesh path metric was wrong in some frames
 * mac80211: use QoS null-data packets on QoS connections
 * mac80211: tear down RX aggregation sessions first to
   drop fewer packets in HW restart scenarios


Chun-Yeow Yeoh (1):
  mac80211: fix the update of path metric for RANN frame

Johannes Berg (2):
  cfg80211: select CRYPTO_SHA256 if needed
  mac80211: use QoS NDP for AP probing

Sara Sharon (1):
  mac80211: tear down RX aggregations first

 drivers/net/wireless/ath/ath9k/channel.c |  2 +-
 drivers/net/wireless/st/cw1200/sta.c |  4 ++--
 drivers/net/wireless/ti/wl1251/main.c|  2 +-
 drivers/net/wireless/ti/wlcore/cmd.c |  5 +++--
 include/net/mac80211.h   |  8 +++-
 net/mac80211/ht.c|  4 +++-
 net/mac80211/mesh_hwmp.c | 15 +--
 net/mac80211/mlme.c  |  2 +-
 net/mac80211/tx.c| 29 +++--
 net/wireless/Kconfig |  7 +++
 10 files changed, 61 insertions(+), 17 deletions(-)

Re: kernel BUG at crypto/asymmetric_keys/public_key.c:80

[Johannes Berg]

On Thu, 2017-11-23 at 09:47 -0800, Florian Fainelli wrote:

> Absolutely, please find it enclosed.

Thanks.

This is a bit odd. I didn't think the most likely reason is that you
have

CONFIG_CRYPTO_SHA256=m

but everything else built-in. Thus, when loading the certificate,
there's no way to calculate the digest since that requires sha-256,
hence

BUG_ON(!sig->digest);

If you make CONFIG_CRYPTO_SHA256=y then it should go away.

I guess I'll do this:

diff --git a/net/wireless/Kconfig b/net/wireless/Kconfig
index da91bb547db3..1abcc4fc4df1 100644
--- a/net/wireless/Kconfig
+++ b/net/wireless/Kconfig
@@ -20,6 +20,10 @@ config CFG80211
tristate "cfg80211 - wireless configuration API"
depends on RFKILL || !RFKILL
select FW_LOADER
+   # may need to update this when certificates are changed and are
+   # using a different algorithm, though right now they shouldn't
+   # (this is here rather than below to allow it to be a module)
+   select CRYPTO_SHA256 if CFG80211_USE_KERNEL_REGDB_KEYS
---help---
  cfg80211 is the Linux wireless LAN (802.11) configuration API.
  Enable this if you have a wireless device.
@@ -113,6 +117,9 @@ config CFG80211_EXTRA_REGDB_KEYDIR
  certificates like in the kernel sources (net/wireless/certs/)
  that shall be accepted for a signed regulatory database.
 
+ Note that you need to also select the correct CRYPTO_ modules
+ for your certificates, and if cfg80211 is built-in they also must be.
+
 config CFG80211_REG_CELLULAR_HINTS
bool "cfg80211 regulatory support for cellular base station hints"
depends on CFG80211_CERTIFICATION_ONUS


Can you try if that fixes your config for you?

johannes

Re: kernel BUG at crypto/asymmetric_keys/public_key.c:80

[Johannes Berg]

On Wed, 2017-11-22 at 15:07 -0800, Florian Fainelli wrote:
> On 11/22/2017 10:42 AM, Johannes Berg wrote:
> > On Wed, 2017-11-22 at 19:29 +0100, Arend van Spriel wrote:
> > > + Johannes
> > > 
> > >  >>> BUG_ON(!sig->digest);
> > >  BUG_ON(!sig->s);
> > 
> > I *think* this is the same bug that was reported before, then this
> > should fix it:
> > 
> > https://git.kernel.org/pub/scm/linux/kernel/git/davem/net.git/commit/?id=d7be102f2945a626f55e0501e52bb31ba3e77b81
> > 
> > Can you try?
> 
> My baseline already has this commit actually, is there something else
> you would want me to check?

Hmm, different problem then - could you put the entire boot log
somewhere I can read it?

johannes

Re: kernel BUG at crypto/asymmetric_keys/public_key.c:80

[Johannes Berg]

On Wed, 2017-11-22 at 19:29 +0100, Arend van Spriel wrote:
> + Johannes
> 
>  >>> BUG_ON(!sig->digest);
>  BUG_ON(!sig->s);

I *think* this is the same bug that was reported before, then this
should fix it:

https://git.kernel.org/pub/scm/linux/kernel/git/davem/net.git/commit/?id=d7be102f2945a626f55e0501e52bb31ba3e77b81

Can you try?

johannes

Re: pull-request: mac80211 2017-11-20

[Johannes Berg]

On Tue, 2017-11-21 at 20:17 +0900, David Miller wrote:
> From: Johannes Berg <johan...@sipsolutions.net>
> Date: Mon, 20 Nov 2017 17:06:44 +0100
> 
> >   ssh://korg/pub/scm/linux/kernel/git/jberg/mac80211.git 
> > tags/mac80211-for-davem-2017-11-20
> 
> That's an awesome URL, but I don't think I'll be able to pull
> from it :-)

Huh. That's what I get for following Konstantin's advice to use
insteadOf in git configuration ...

Should be

git://git.kernel.org/pub/scm/linux/kernel/git/jberg/mac80211.git 
tags/mac80211-for-davem-2017-11-20

(or https if you prefer)

johannes

pull-request: mac80211 2017-11-20

[Johannes Berg]

Hi Dave,

Sorry this is coming now, I had a super hectic time after travel
since I had to catch up after two weeks of being away ...

That's not really an excuse, but I'm asking you anyway to pull
Kees's timer conversions with the fixes since he really wants
to make more changes on top and clean this up properly. I've had
those pending, but in the hectic week after coming back didn't
send the mac80211-next pull request that I should have done.

If you don't want that, let me know and I'll respin without it.

Otherwise, please pull and let me know if there's any problem.

Thanks,
johannes



The following changes since commit 32a72bbd5da2411eab591bf9bc2e39349106193a:

  net: vxge: Fix some indentation issues (2017-11-20 11:36:30 +0900)

are available in the git repository at:

  ssh://korg/pub/scm/linux/kernel/git/jberg/mac80211.git 
tags/mac80211-for-davem-2017-11-20

for you to fetch changes up to 33ddd81e2bd5d9970b9f01ab383ba45035fa41ee:

  mac80211: properly free requested-but-not-started TX agg sessions (2017-11-20 
17:01:31 +0100)


A few things:
 * straggler timer conversions from Kees
 * memory leak fix in hwsim
 * fix some fallout from regdb changes if wireless is built-in
 * also free aggregation sessions in startup state when station
   goes away, to avoid crashing the timer


Ben Hutchings (1):
  mac80211_hwsim: Fix memory leak in hwsim_new_radio_nl()

Johannes Berg (3):
  nl80211: don't expose wdev->ssid for most interfaces
  cfg80211: initialize regulatory keys/database later
  mac80211: properly free requested-but-not-started TX agg sessions

Kees Cook (2):
  mac80211: Convert timers to use timer_setup()
  mac80211: aggregation: Convert timers to use timer_setup()

 drivers/net/wireless/mac80211_hwsim.c |  5 +++-
 net/mac80211/agg-rx.c | 41 -
 net/mac80211/agg-tx.c | 49 ---
 net/mac80211/ibss.c   |  7 +++--
 net/mac80211/ieee80211_i.h|  3 ++-
 net/mac80211/led.c| 11 
 net/mac80211/main.c   |  3 +--
 net/mac80211/mesh.c   | 27 +--
 net/mac80211/mesh.h   |  2 +-
 net/mac80211/mesh_hwmp.c  |  4 +--
 net/mac80211/mesh_pathtbl.c   |  3 +--
 net/mac80211/mlme.c   | 32 ++-
 net/mac80211/ocb.c| 10 +++
 net/mac80211/sta_info.c   | 15 +++
 net/mac80211/sta_info.h   | 12 +++--
 net/wireless/nl80211.c| 26 +--
 net/wireless/reg.c| 42 +++---
 17 files changed, 155 insertions(+), 137 deletions(-)

Re: [PATCH net] genetlink: fix genlmsg_nlhdr()

[Johannes Berg]

On Wed, 2017-11-15 at 13:09 +0100, Michal Kubecek wrote:
> According to the description, first argument of genlmsg_nlhdr() points to
> what genlmsg_put() returns, i.e. beginning of user header. Therefore we
> should only subtract size of genetlink header and netlink message header,
> not user header.
> 
> This also means we don't need to pass the pointer to genetlink family and
> the same is true for genl_dump_check_consistent() which is the only caller
> of genlmsg_nlhdr(). (Note that at the moment, these functions are only
> used for families which do not have user header so that they are not
> affected.)
> 
> Fixes: 670dc2833d14 ("netlink: advertise incomplete dumps")
> Signed-off-by: Michal Kubecek <mkube...@suse.cz>

Looks sensible, though I don't think it's really needed in net since it
really has no effect - as you note, family->hdrsize is 0 for all the
families calling this right now.

Reviewed-by: Johannes Berg <johan...@sipsolutions.net>

johannes

[PATCH] netlink: remove unused NETLINK SKB flags

[Johannes Berg]

From: Johannes Berg <johannes.b...@intel.com>

These flags are unused, remove them to be less confusing.

Signed-off-by: Johannes Berg <johannes.b...@intel.com>
---
 include/linux/netlink.h | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/include/linux/netlink.h b/include/linux/netlink.h
index 8664fd26eb5d..7b57f01fd7ef 100644
--- a/include/linux/netlink.h
+++ b/include/linux/netlink.h
@@ -16,9 +16,6 @@ static inline struct nlmsghdr *nlmsg_hdr(const struct sk_buff 
*skb)
 }
 
 enum netlink_skb_flags {
-   NETLINK_SKB_MMAPED  = 0x1,  /* Packet data is mmaped */
-   NETLINK_SKB_TX  = 0x2,  /* Packet was sent by userspace */
-   NETLINK_SKB_DELIVERED   = 0x4,  /* Packet was delivered */
NETLINK_SKB_DST = 0x8,  /* Dst set in sendto or sendmsg */
 };
 
-- 
2.14.2

[PATCH] netlink: remove unnecessary forward declaration

[Johannes Berg]

From: Johannes Berg <johannes.b...@intel.com>

netlink_skb_destructor() is actually defined before the first usage
in the file, so remove the unnecessary forward declaration.

Signed-off-by: Johannes Berg <johannes.b...@intel.com>
---
 net/netlink/af_netlink.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/net/netlink/af_netlink.c b/net/netlink/af_netlink.c
index f34750691c5c..80b8a0ecd2b1 100644
--- a/net/netlink/af_netlink.c
+++ b/net/netlink/af_netlink.c
@@ -128,7 +128,6 @@ static const char *const nlk_cb_mutex_key_strings[MAX_LINKS 
+ 1] = {
 };
 
 static int netlink_dump(struct sock *sk);
-static void netlink_skb_destructor(struct sk_buff *skb);
 
 /* nl_table locking explained:
  * Lookup and traversal are protected with an RCU read-side lock. Insertion
-- 
2.14.2

Re: [net-next] tcp: allow drivers to tweak TSQ logic

[Johannes Berg]

On Sat, 2017-11-11 at 15:38 -0800, Eric Dumazet wrote:

> > Hm. I wish we wouldn't have to do this on every skb, but perhaps it
> > doesn't matter that much.
> 
> Yes, it does not matter, even at 40Gbit ;)

:)

> Same cache line already ;)

Hah. It seemed so far away, but actually looking at the layout helps :)

> > Unrelated to that, I think this is missing a documentation update since
> > the struct has kernel-doc comments.
> 
> Yeah, I believe these kernel-doc on gigantic struct sock are useless and
> we should remove them, they have zero useful info.

No argument from me, but while it's there ... anyway I see you resent
this, thanks for doing this.

I'll try to get people to run some tests for our driver.

johannes