[PATCH] cfg80211: size various nl80211 messages correctly

linux-wireless.vger.kernel.org archive mirror
 help / color / mirror / Atom feed

* [PATCH] cfg80211: size various nl80211 messages correctly
@ 2017-01-09 10:10 Johannes Berg
  2017-01-09 11:39 ` kbuild test robot
  2017-01-09 12:36 ` Arend Van Spriel
  0 siblings, 2 replies; 6+ messages in thread
From: Johannes Berg @ 2017-01-09 10:10 UTC (permalink / raw)
  To: linux-wireless; +Cc: Johannes Berg

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

Ilan reported that sometimes nl80211 messages weren't working if
the frames being transported got very large, which was really a
problem for userspace-to-kernel messages, but prompted me to look
at the code.

Upon review, I found various places where variable-length data is
transported in an nl80211 message but the message isn't allocated
taking that into account. This shouldn't cause any problems since
the frames aren't really that long, apart in one place where two
(possibly very long frames) might not fit.

Fix all the places (that I found) that get variable length data
from the driver and put it into a message to take the length of
the variable data into account. The 100 there is just a safe
constant for the remaining message overhead (it's usually around
50 for most messages.)

Signed-off-by: Johannes Berg <johannes.berg@intel.com>
---
 net/wireless/nl80211.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/net/wireless/nl80211.c b/net/wireless/nl80211.c
index 23692658fe98..f55b251e4b0d 100644
--- a/net/wireless/nl80211.c
+++ b/net/wireless/nl80211.c
@@ -13249,7 +13249,7 @@ void nl80211_send_disconnected(struct cfg80211_registered_device *rdev,
 	struct sk_buff *msg;
 	void *hdr;
 
-	msg = nlmsg_new(NLMSG_DEFAULT_SIZE, GFP_KERNEL);
+	msg = nlmsg_new(100 + ie_len, GFP_KERNEL);
 	if (!msg)
 		return;
 
@@ -13325,7 +13325,7 @@ void cfg80211_notify_new_peer_candidate(struct net_device *dev, const u8 *addr,
 
 	trace_cfg80211_notify_new_peer_candidate(dev, addr);
 
-	msg = nlmsg_new(NLMSG_DEFAULT_SIZE, gfp);
+	msg = nlmsg_new(100 + ie_len, gfp);
 	if (!msg)
 		return;
 
@@ -13362,7 +13362,7 @@ void nl80211_michael_mic_failure(struct cfg80211_registered_device *rdev,
 	struct sk_buff *msg;
 	void *hdr;
 
-	msg = nlmsg_new(NLMSG_DEFAULT_SIZE, gfp);
+	msg = nlmsg_new(100 + req_ie_len + resp_ie_len, gfp);
 	if (!msg)
 		return;
 
@@ -13456,7 +13456,7 @@ static void nl80211_send_remain_on_chan_event(
 	struct sk_buff *msg;
 	void *hdr;
 
-	msg = nlmsg_new(NLMSG_DEFAULT_SIZE, gfp);
+	msg = nlmsg_new(100 + len, gfp);
 	if (!msg)
 		return;
 
@@ -13696,7 +13696,7 @@ int nl80211_send_mgmt(struct cfg80211_registered_device *rdev,
 	struct sk_buff *msg;
 	void *hdr;
 
-	msg = nlmsg_new(NLMSG_DEFAULT_SIZE, gfp);
+	msg = nlmsg_new(100 + len, gfp);
 	if (!msg)
 		return -ENOMEM;
 
@@ -13740,7 +13740,7 @@ void cfg80211_mgmt_tx_status(struct wireless_dev *wdev, u64 cookie,
 
 	trace_cfg80211_mgmt_tx_status(wdev, cookie, ack);
 
-	msg = nlmsg_new(NLMSG_DEFAULT_SIZE, gfp);
+	msg = nlmsg_new(100 + len, gfp);
 	if (!msg)
 		return;
 
@@ -14046,7 +14046,7 @@ static void nl80211_ch_switch_notify(struct cfg80211_registered_device *rdev,
 	struct sk_buff *msg;
 	void *hdr;
 
-	msg = nlmsg_new(NLMSG_DEFAULT_SIZE, gfp);
+	msg = nlmsg_new(100 + req_ie_len + resp_ie_len, gfp);
 	if (!msg)
 		return;
 
@@ -14551,7 +14551,7 @@ void cfg80211_ft_event(struct net_device *netdev,
 	if (!ft_event->target_ap)
 		return;
 
-	msg = nlmsg_new(NLMSG_DEFAULT_SIZE, GFP_KERNEL);
+	msg = nlmsg_new(100 + ft_event->ric_ies_len, GFP_KERNEL);
 	if (!msg)
 		return;
 
-- 
2.9.3

^ permalink raw reply related	[flat|nested] 6+ messages in thread

* Re: [PATCH] cfg80211: size various nl80211 messages correctly
  2017-01-09 10:10 [PATCH] cfg80211: size various nl80211 messages correctly Johannes Berg
@ 2017-01-09 11:39 ` kbuild test robot
  2017-01-09 12:36 ` Arend Van Spriel
  1 sibling, 0 replies; 6+ messages in thread
From: kbuild test robot @ 2017-01-09 11:39 UTC (permalink / raw)
  To: Johannes Berg; +Cc: kbuild-all, linux-wireless, Johannes Berg

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

Hi Johannes,

[auto build test ERROR on mac80211-next/master]
[also build test ERROR on v4.10-rc3 next-20170106]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Johannes-Berg/cfg80211-size-various-nl80211-messages-correctly/20170109-185808
base:   https://git.kernel.org/pub/scm/linux/kernel/git/jberg/mac80211-next.git master
config: x86_64-kexec (attached as .config)
compiler: gcc-6 (Debian 6.2.0-3) 6.2.0 20160901
reproduce:
        # save the attached .config to linux build tree
        make ARCH=x86_64 

All errors (new ones prefixed by >>):

   net/wireless/nl80211.c: In function 'nl80211_michael_mic_failure':
>> net/wireless/nl80211.c:13365:24: error: 'req_ie_len' undeclared (first use in this function)
     msg = nlmsg_new(100 + req_ie_len + resp_ie_len, gfp);
                           ^~~~~~~~~~
   net/wireless/nl80211.c:13365:24: note: each undeclared identifier is reported only once for each function it appears in
>> net/wireless/nl80211.c:13365:37: error: 'resp_ie_len' undeclared (first use in this function)
     msg = nlmsg_new(100 + req_ie_len + resp_ie_len, gfp);
                                        ^~~~~~~~~~~
   net/wireless/nl80211.c: In function 'nl80211_send_remain_on_chan_event':
>> net/wireless/nl80211.c:13459:24: error: 'len' undeclared (first use in this function)
     msg = nlmsg_new(100 + len, gfp);
                           ^~~
   net/wireless/nl80211.c: In function 'nl80211_ch_switch_notify':
   net/wireless/nl80211.c:14049:24: error: 'req_ie_len' undeclared (first use in this function)
     msg = nlmsg_new(100 + req_ie_len + resp_ie_len, gfp);
                           ^~~~~~~~~~
   net/wireless/nl80211.c:14049:37: error: 'resp_ie_len' undeclared (first use in this function)
     msg = nlmsg_new(100 + req_ie_len + resp_ie_len, gfp);
                                        ^~~~~~~~~~~

vim +/req_ie_len +13365 net/wireless/nl80211.c

 13359					 enum nl80211_key_type key_type, int key_id,
 13360					 const u8 *tsc, gfp_t gfp)
 13361	{
 13362		struct sk_buff *msg;
 13363		void *hdr;
 13364	
 13365		msg = nlmsg_new(100 + req_ie_len + resp_ie_len, gfp);
 13366		if (!msg)
 13367			return;
 13368	
 13369		hdr = nl80211hdr_put(msg, 0, 0, 0, NL80211_CMD_MICHAEL_MIC_FAILURE);
 13370		if (!hdr) {
 13371			nlmsg_free(msg);
 13372			return;
 13373		}
 13374	
 13375		if (nla_put_u32(msg, NL80211_ATTR_WIPHY, rdev->wiphy_idx) ||
 13376		    nla_put_u32(msg, NL80211_ATTR_IFINDEX, netdev->ifindex) ||
 13377		    (addr && nla_put(msg, NL80211_ATTR_MAC, ETH_ALEN, addr)) ||
 13378		    nla_put_u32(msg, NL80211_ATTR_KEY_TYPE, key_type) ||
 13379		    (key_id != -1 &&
 13380		     nla_put_u8(msg, NL80211_ATTR_KEY_IDX, key_id)) ||
 13381		    (tsc && nla_put(msg, NL80211_ATTR_KEY_SEQ, 6, tsc)))
 13382			goto nla_put_failure;
 13383	
 13384		genlmsg_end(msg, hdr);
 13385	
 13386		genlmsg_multicast_netns(&nl80211_fam, wiphy_net(&rdev->wiphy), msg, 0,
 13387					NL80211_MCGRP_MLME, gfp);
 13388		return;
 13389	
 13390	 nla_put_failure:
 13391		genlmsg_cancel(msg, hdr);
 13392		nlmsg_free(msg);
 13393	}
 13394	
 13395	void nl80211_send_beacon_hint_event(struct wiphy *wiphy,
 13396					    struct ieee80211_channel *channel_before,
 13397					    struct ieee80211_channel *channel_after)
 13398	{
 13399		struct sk_buff *msg;
 13400		void *hdr;
 13401		struct nlattr *nl_freq;
 13402	
 13403		msg = nlmsg_new(NLMSG_DEFAULT_SIZE, GFP_ATOMIC);
 13404		if (!msg)
 13405			return;
 13406	
 13407		hdr = nl80211hdr_put(msg, 0, 0, 0, NL80211_CMD_REG_BEACON_HINT);
 13408		if (!hdr) {
 13409			nlmsg_free(msg);
 13410			return;
 13411		}
 13412	
 13413		/*
 13414		 * Since we are applying the beacon hint to a wiphy we know its
 13415		 * wiphy_idx is valid
 13416		 */
 13417		if (nla_put_u32(msg, NL80211_ATTR_WIPHY, get_wiphy_idx(wiphy)))
 13418			goto nla_put_failure;
 13419	
 13420		/* Before */
 13421		nl_freq = nla_nest_start(msg, NL80211_ATTR_FREQ_BEFORE);
 13422		if (!nl_freq)
 13423			goto nla_put_failure;
 13424		if (nl80211_msg_put_channel(msg, channel_before, false))
 13425			goto nla_put_failure;
 13426		nla_nest_end(msg, nl_freq);
 13427	
 13428		/* After */
 13429		nl_freq = nla_nest_start(msg, NL80211_ATTR_FREQ_AFTER);
 13430		if (!nl_freq)
 13431			goto nla_put_failure;
 13432		if (nl80211_msg_put_channel(msg, channel_after, false))
 13433			goto nla_put_failure;
 13434		nla_nest_end(msg, nl_freq);
 13435	
 13436		genlmsg_end(msg, hdr);
 13437	
 13438		rcu_read_lock();
 13439		genlmsg_multicast_allns(&nl80211_fam, msg, 0,
 13440					NL80211_MCGRP_REGULATORY, GFP_ATOMIC);
 13441		rcu_read_unlock();
 13442	
 13443		return;
 13444	
 13445	nla_put_failure:
 13446		genlmsg_cancel(msg, hdr);
 13447		nlmsg_free(msg);
 13448	}
 13449	
 13450	static void nl80211_send_remain_on_chan_event(
 13451		int cmd, struct cfg80211_registered_device *rdev,
 13452		struct wireless_dev *wdev, u64 cookie,
 13453		struct ieee80211_channel *chan,
 13454		unsigned int duration, gfp_t gfp)
 13455	{
 13456		struct sk_buff *msg;
 13457		void *hdr;
 13458	
 13459		msg = nlmsg_new(100 + len, gfp);
 13460		if (!msg)
 13461			return;
 13462	

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 24305 bytes --]

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] cfg80211: size various nl80211 messages correctly
  2017-01-09 10:10 [PATCH] cfg80211: size various nl80211 messages correctly Johannes Berg
  2017-01-09 11:39 ` kbuild test robot
@ 2017-01-09 12:36 ` Arend Van Spriel
  2017-01-09 12:39   ` Johannes Berg
  1 sibling, 1 reply; 6+ messages in thread
From: Arend Van Spriel @ 2017-01-09 12:36 UTC (permalink / raw)
  To: Johannes Berg, linux-wireless; +Cc: Johannes Berg



On 9-1-2017 11:10, Johannes Berg wrote:
> From: Johannes Berg <johannes.berg@intel.com>
> 
> Ilan reported that sometimes nl80211 messages weren't working if
> the frames being transported got very large, which was really a
> problem for userspace-to-kernel messages, but prompted me to look
> at the code.
> 
> Upon review, I found various places where variable-length data is
> transported in an nl80211 message but the message isn't allocated
> taking that into account. This shouldn't cause any problems since
> the frames aren't really that long, apart in one place where two
> (possibly very long frames) might not fit.
> 
> Fix all the places (that I found) that get variable length data
> from the driver and put it into a message to take the length of
> the variable data into account. The 100 there is just a safe
> constant for the remaining message overhead (it's usually around
> 50 for most messages.)
> 
> Signed-off-by: Johannes Berg <johannes.berg@intel.com>
> ---
>  net/wireless/nl80211.c | 16 ++++++++--------
>  1 file changed, 8 insertions(+), 8 deletions(-)
> 
> diff --git a/net/wireless/nl80211.c b/net/wireless/nl80211.c
> index 23692658fe98..f55b251e4b0d 100644
> --- a/net/wireless/nl80211.c
> +++ b/net/wireless/nl80211.c
> @@ -13249,7 +13249,7 @@ void nl80211_send_disconnected(struct cfg80211_registered_device *rdev,
>  	struct sk_buff *msg;
>  	void *hdr;
>  
> -	msg = nlmsg_new(NLMSG_DEFAULT_SIZE, GFP_KERNEL);
> +	msg = nlmsg_new(100 + ie_len, GFP_KERNEL);

Don't you want the '100' to be a define?

Regards,
Arend

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] cfg80211: size various nl80211 messages correctly
  2017-01-09 12:36 ` Arend Van Spriel
@ 2017-01-09 12:39   ` Johannes Berg
  2017-01-09 14:28     ` IgorMitsyanko
  0 siblings, 1 reply; 6+ messages in thread
From: Johannes Berg @ 2017-01-09 12:39 UTC (permalink / raw)
  To: Arend Van Spriel, linux-wireless

> > -	msg = nlmsg_new(NLMSG_DEFAULT_SIZE, GFP_KERNEL);
> > +	msg = nlmsg_new(100 + ie_len, GFP_KERNEL);
> 
> Don't you want the '100' to be a define?

I didn't want to glorify it too much - some places may need more or
less over time. There's no significance to this number.

johannes

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] cfg80211: size various nl80211 messages correctly
  2017-01-09 12:39   ` Johannes Berg
@ 2017-01-09 14:28     ` IgorMitsyanko
  2017-01-09 14:39       ` IgorMitsyanko
  0 siblings, 1 reply; 6+ messages in thread
From: IgorMitsyanko @ 2017-01-09 14:28 UTC (permalink / raw)
  To: Johannes Berg; +Cc: Arend Van Spriel, linux-wireless

On 01/09/2017 03:39 PM, Johannes Berg wrote:
>>> -	msg = nlmsg_new(NLMSG_DEFAULT_SIZE, GFP_KERNEL);
>>> +	msg = nlmsg_new(100 + ie_len, GFP_KERNEL);
>>
>> Don't you want the '100' to be a define?
>
> I didn't want to glorify it too much - some places may need more or
> less over time. There's no significance to this number.
>
> johannes
>

Looking at what NLMSG_DEFAULT_SIZE is, wouldn't it be more descriptive 
to replace 100 with something like:

#define NLMSG_OVERHEAD (SKB_DATA_ALIGN(sizeof(struct skb_shared_info)) + 
NLMSG_HDRLEN)

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] cfg80211: size various nl80211 messages correctly
  2017-01-09 14:28     ` IgorMitsyanko
@ 2017-01-09 14:39       ` IgorMitsyanko
  0 siblings, 0 replies; 6+ messages in thread
From: IgorMitsyanko @ 2017-01-09 14:39 UTC (permalink / raw)
  To: Johannes Berg; +Cc: Arend Van Spriel, linux-wireless

On 01/09/2017 05:28 PM, IgorMitsyanko wrote:
> On 01/09/2017 03:39 PM, Johannes Berg wrote:
>>>> -    msg = nlmsg_new(NLMSG_DEFAULT_SIZE, GFP_KERNEL);
>>>> +    msg = nlmsg_new(100 + ie_len, GFP_KERNEL);
>>>
>>> Don't you want the '100' to be a define?
>>
>> I didn't want to glorify it too much - some places may need more or
>> less over time. There's no significance to this number.
>>
>> johannes
>>
>
> Looking at what NLMSG_DEFAULT_SIZE is, wouldn't it be more descriptive
> to replace 100 with something like:
>
> #define NLMSG_OVERHEAD (SKB_DATA_ALIGN(sizeof(struct skb_shared_info)) +
> NLMSG_HDRLEN)
>

Now I see, it's not for msg overhead, but for other fixed-width netlink 
attributes like WIPHY, IFINDEX etc

^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2017-01-09 14:40 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2017-01-09 10:10 [PATCH] cfg80211: size various nl80211 messages correctly Johannes Berg
2017-01-09 11:39 ` kbuild test robot
2017-01-09 12:36 ` Arend Van Spriel
2017-01-09 12:39   ` Johannes Berg
2017-01-09 14:28     ` IgorMitsyanko
2017-01-09 14:39       ` IgorMitsyanko

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).