[PATCH] mesh: Avoid STA expiration timer truncation to u32

[PATCH] mesh: Avoid STA expiration timer truncation to u32

[Masashi Honma]

On some combination of plink_timeout and HZ, the STA expiration timer will be
unexpectedly truncated to u32. Maybe there is a question "Who sets such a large
number to plink_timeout ?". At least wpa_supplicant will set 0xffffffff to
plink_timeout to disable this timer because wpa_supplicant has it's own
expiration mechanism.

Signed-off-by: Masashi Honma <masashi.honma@gmail.com>
---
 net/mac80211/mesh.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/net/mac80211/mesh.c b/net/mac80211/mesh.c
index 0c8b2a7..bb721a0 100644
--- a/net/mac80211/mesh.c
+++ b/net/mac80211/mesh.c
@@ -574,7 +574,8 @@ static void ieee80211_mesh_housekeeping(struct ieee80211_sub_if_data *sdata)
 	struct ieee80211_if_mesh *ifmsh = &sdata->u.mesh;
 	u32 changed;
 
-	ieee80211_sta_expire(sdata, ifmsh->mshcfg.plink_timeout * HZ);
+	ieee80211_sta_expire(sdata, ifmsh->mshcfg.plink_timeout *
+			     (unsigned long)HZ);
 	mesh_path_expire(sdata);
 
 	changed = mesh_accept_plinks_update(sdata);
-- 
2.1.0

Re: [PATCH] mesh: Avoid STA expiration timer truncation to u32

[Kalle Valo]

Masashi Honma <masashi.honma@gmail.com> writes:

> On some combination of plink_timeout and HZ, the STA expiration timer will be
> unexpectedly truncated to u32. Maybe there is a question "Who sets such a large
> number to plink_timeout ?". At least wpa_supplicant will set 0xffffffff to
> plink_timeout to disable this timer because wpa_supplicant has it's own
> expiration mechanism.
>
> Signed-off-by: Masashi Honma <masashi.honma@gmail.com>

For mac80211 patches please use prefix "mac80211: ".

-- 
Kalle Valo

Re: [PATCH] mesh: Avoid STA expiration timer truncation to u32

[Masashi Honma]

2015-01-16 14:49 GMT+09:00 Kalle Valo <kvalo@codeaurora.org>:
> For mac80211 patches please use prefix "mac80211: ".

Thanks. I will fix it.

[PATCH v2] mac80211: Avoid STA expiration timer truncation to u32

[Masashi Honma]

On some combination of plink_timeout and HZ, the STA expiration timer will be
unexpectedly truncated to u32. Maybe there is a question "Who sets such a large
number to plink_timeout ?". At least wpa_supplicant will set 0xffffffff to
plink_timeout to disable this timer because wpa_supplicant has it's own
expiration mechanism.

Signed-off-by: Masashi Honma <masashi.honma@gmail.com>
---
 net/mac80211/mesh.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/net/mac80211/mesh.c b/net/mac80211/mesh.c
index 0c8b2a7..bb721a0 100644
--- a/net/mac80211/mesh.c
+++ b/net/mac80211/mesh.c
@@ -574,7 +574,8 @@ static void ieee80211_mesh_housekeeping(struct ieee80211_sub_if_data *sdata)
 	struct ieee80211_if_mesh *ifmsh = &sdata->u.mesh;
 	u32 changed;
 
-	ieee80211_sta_expire(sdata, ifmsh->mshcfg.plink_timeout * HZ);
+	ieee80211_sta_expire(sdata, ifmsh->mshcfg.plink_timeout *
+			     (unsigned long)HZ);
 	mesh_path_expire(sdata);
 
 	changed = mesh_accept_plinks_update(sdata);
-- 
2.1.0

Re: [PATCH v2] mac80211: Avoid STA expiration timer truncation to u32

[Bob Copeland]

On Fri, Jan 16, 2015 at 03:18:41PM +0900, Masashi Honma wrote:
> On some combination of plink_timeout and HZ, the STA expiration timer will be
> unexpectedly truncated to u32. Maybe there is a question "Who sets such a large

Nice catch!

> -	ieee80211_sta_expire(sdata, ifmsh->mshcfg.plink_timeout * HZ);
> +	ieee80211_sta_expire(sdata, ifmsh->mshcfg.plink_timeout *
> +			     (unsigned long)HZ);

There's a remaining problem on 32-bit platforms: there, unsigned long
is 32 bits so 0xffffffff * HZ will still truncate to 32 bits.

For normal values of HZ though, result will still be 'a rather large
number,' so maybe it's not worth caring about that.

-- 
Bob Copeland %% http://bobcopeland.com/

Re: [PATCH v2] mac80211: Avoid STA expiration timer truncation to u32

[Masashi Honma]

2015-01-16 22:31 GMT+09:00 Bob Copeland <me@bobcopeland.com>:
> Nice catch!
> There's a remaining problem on 32-bit platforms: there, unsigned long
> is 32 bits so 0xffffffff * HZ will still truncate to 32 bits.
>
> For normal values of HZ though, result will still be 'a rather large
> number,' so maybe it's not worth caring about that.

Thank you for your review.

I modified my patch.

[PATCH v3] mac80211: Avoid STA expiration timer truncation to u32

[Masashi Honma]

On some combination of plink_timeout and HZ, the STA expiration timer will be
unexpectedly truncated to u32. Maybe there is a question "Who sets such a large
number to plink_timeout ?". At least wpa_supplicant will set 0xffffffff to
plink_timeout to disable this timer because wpa_supplicant has it's own
expiration mechanism.

Signed-off-by: Masashi Honma <masashi.honma@gmail.com>
---
 net/mac80211/mesh.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/net/mac80211/mesh.c b/net/mac80211/mesh.c
index 0c8b2a7..3c40894 100644
--- a/net/mac80211/mesh.c
+++ b/net/mac80211/mesh.c
@@ -573,8 +573,11 @@ static void ieee80211_mesh_housekeeping(struct ieee80211_sub_if_data *sdata)
 {
 	struct ieee80211_if_mesh *ifmsh = &sdata->u.mesh;
 	u32 changed;
+	u64 exp_time;
 
-	ieee80211_sta_expire(sdata, ifmsh->mshcfg.plink_timeout * HZ);
+	exp_time = ifmsh->mshcfg.plink_timeout * (u64)HZ;
+	if (exp_time < 0x100000000)
+		ieee80211_sta_expire(sdata, exp_time);
 	mesh_path_expire(sdata);
 
 	changed = mesh_accept_plinks_update(sdata);
-- 
2.1.0

Re: [PATCH v3] mac80211: Avoid STA expiration timer truncation to u32

[Johannes Berg]

On Tue, 2015-01-20 at 11:47 +0900, Masashi Honma wrote:
> On some combination of plink_timeout and HZ, the STA expiration timer will be
> unexpectedly truncated to u32. Maybe there is a question "Who sets such a large
> number to plink_timeout ?". At least wpa_supplicant will set 0xffffffff to
> plink_timeout to disable this timer because wpa_supplicant has it's own
> expiration mechanism.

Ok - but that doesn't really disable the timer? Perhaps we should have a
new userspace API to explicitly disable it? OTOH, worst case I guess
that means it's like >100 years in the future, so I guess it doesn't
matter. However, though, you can hardly rely on this fix being present
in the kernel, so you can't really set such a large value
unconditionally anyway, no? Otherwise a newer wpa_supplicant running on
an older kernel would suddenly behave incorrectly. That doesn't seem
right.

Having an explicit feature to disable plink timeout would perhaps be
better?

> --- a/net/mac80211/mesh.c
> +++ b/net/mac80211/mesh.c
> @@ -573,8 +573,11 @@ static void ieee80211_mesh_housekeeping(struct ieee80211_sub_if_data *sdata)
>  {
>  	struct ieee80211_if_mesh *ifmsh = &sdata->u.mesh;
>  	u32 changed;
> +	u64 exp_time;
>  
> -	ieee80211_sta_expire(sdata, ifmsh->mshcfg.plink_timeout * HZ);
> +	exp_time = ifmsh->mshcfg.plink_timeout * (u64)HZ;
> +	if (exp_time < 0x100000000)
> +		ieee80211_sta_expire(sdata, exp_time);

I'm not convinced this is right. For one, I believe on 32-bit machines
you'll need to write "0x100000000ULL" instead of the plain constant.
Perhaps preferably, you'd use use ">= MAX_UINT".

However, the argument to ieee80211_sta_expire() is an unsigned long (as
is jiffies), so on 64-bit machines you could even still use the value
and the conditional isn't needed.

Given these complications, I would prefer having a feature attribute to
treat e.g. 0 as disabling the timer entirely, and if this feature isn't
present then have wpa_supplicant instead use a safe value that doesn't
trigger the kernel bug - e.g. 0xffffffff/1000 [which is the max possible
HZ].

johannes

Re: [PATCH v3] mac80211: Avoid STA expiration timer truncation to u32

[Masashi Honma]

2015-01-23 18:42 GMT+09:00 Johannes Berg <johannes@sipsolutions.net>:
> Ok - but that doesn't really disable the timer? Perhaps we should have a
> new userspace API to explicitly disable it? OTOH, worst case I guess
> that means it's like >100 years in the future, so I guess it doesn't
> matter. However, though, you can hardly rely on this fix being present
> in the kernel, so you can't really set such a large value
> unconditionally anyway, no? Otherwise a newer wpa_supplicant running on
> an older kernel would suddenly behave incorrectly. That doesn't seem
> right.
>
> Having an explicit feature to disable plink timeout would perhaps be
> better?

Thank you for your review.

On my environment, HZ macro is 250. So jiffies counts up 250 per seconds.
So jiffies overflows in 199 days.
It is a large value still. But on my arm64 environment, jiffies could over the
u32 max value. Because it looks starts with about 0xffff0000.
So I need this patch. On the i386 environment, it does not occur.

> I'm not convinced this is right. For one, I believe on 32-bit machines
> you'll need to write "0x100000000ULL" instead of the plain constant.
> Perhaps preferably, you'd use use ">= MAX_UINT".

I have written such a code "0x100000000ULL" few years ago. But now,
"0x100000000" works. I re-tested on 64bit.
Anyway I think using "MAX_UINT" is better.

> However, the argument to ieee80211_sta_expire() is an unsigned long (as
> is jiffies), so on 64-bit machines you could even still use the value
> and the conditional isn't needed.

Yes, this is a code for 32bit machine.

> Given these complications, I would prefer having a feature attribute to
> treat e.g. 0 as disabling the timer entirely, and if this feature isn't
> present then have wpa_supplicant instead use a safe value that doesn't
> trigger the kernel bug - e.g. 0xffffffff/1000 [which is the max possible
> HZ].

Looks fine. I will modify this patch.

[PATCH v4] mac80211: Allow 0 for NL80211_MESHCONF_PLINK_TIMEOUT to disable STA expiration

[Masashi Honma]

Both wpa_supplicant and mac80211 has inactivity timer. By default
wpa_supplicant will be timed out in 5 minutes and mac80211's it is 30 minutes.
If wpa_supplicant uses more long timer than mac80211, wpa_supplicant will get
unexpected disconnection by mac80211. This patch adds functionality of disabling
mac80211 inactivity timer to avoid to prevent wpa_supplicant inactivity timer.

I have thought setting 0xffffffff to NL80211_MESHCONF_PLINK_TIMEOUT will solve
this problem without this patch. But the approach does not work on 32 bit
system. To explain the reason, I will show STA expiration rule in kernel. This
is the expression.

(current jiffies) > (frame Rx jiffies + NL80211_MESHCONF_PLINK_TIMEOUT * 250)

On 32bit system, right side could be over flow and be unexpected small value if
NL80211_MESHCONF_PLINK_TIMEOUT is sufficiently large. STA expiration occurs by
this reason. So I made this patch.

Signed-off-by: Masashi Honma <masashi.honma@gmail.com>
---
 net/mac80211/mesh.c    | 3 ++-
 net/wireless/nl80211.c | 2 +-
 2 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/net/mac80211/mesh.c b/net/mac80211/mesh.c
index 0c8b2a7..acf441f 100644
--- a/net/mac80211/mesh.c
+++ b/net/mac80211/mesh.c
@@ -574,7 +574,8 @@ static void ieee80211_mesh_housekeeping(struct ieee80211_sub_if_data *sdata)
 	struct ieee80211_if_mesh *ifmsh = &sdata->u.mesh;
 	u32 changed;
 
-	ieee80211_sta_expire(sdata, ifmsh->mshcfg.plink_timeout * HZ);
+	if (ifmsh->mshcfg.plink_timeout > 0)
+		ieee80211_sta_expire(sdata, ifmsh->mshcfg.plink_timeout * HZ);
 	mesh_path_expire(sdata);
 
 	changed = mesh_accept_plinks_update(sdata);
diff --git a/net/wireless/nl80211.c b/net/wireless/nl80211.c
index e9ad9d9..bef52af 100644
--- a/net/wireless/nl80211.c
+++ b/net/wireless/nl80211.c
@@ -5261,7 +5261,7 @@ do {									    \
 	FILL_IN_MESH_PARAM_IF_SET(tb, cfg, dot11MeshAwakeWindowDuration,
 				  0, 65535, mask,
 				  NL80211_MESHCONF_AWAKE_WINDOW, nla_get_u16);
-	FILL_IN_MESH_PARAM_IF_SET(tb, cfg, plink_timeout, 1, 0xffffffff,
+	FILL_IN_MESH_PARAM_IF_SET(tb, cfg, plink_timeout, 0, 0xffffffff,
 				  mask, NL80211_MESHCONF_PLINK_TIMEOUT,
 				  nla_get_u32);
 	if (mask_out)
-- 
2.1.0

Re: [PATCH v4] mac80211: Allow 0 for NL80211_MESHCONF_PLINK_TIMEOUT to disable STA expiration

[Johannes Berg]

Hi,

Sorry about the late reply! I'm getting back to merging now and taking a
closer look at this issue.

> +++ b/net/wireless/nl80211.c
> @@ -5261,7 +5261,7 @@ do {									    \
>  	FILL_IN_MESH_PARAM_IF_SET(tb, cfg, dot11MeshAwakeWindowDuration,
>  				  0, 65535, mask,
>  				  NL80211_MESHCONF_AWAKE_WINDOW, nla_get_u16);
> -	FILL_IN_MESH_PARAM_IF_SET(tb, cfg, plink_timeout, 1, 0xffffffff,
> +	FILL_IN_MESH_PARAM_IF_SET(tb, cfg, plink_timeout, 0, 0xffffffff,
>  				  mask, NL80211_MESHCONF_PLINK_TIMEOUT,

I think you should document this new behaviour also in nl80211.h.

Additionally - what's the plan on how to discover this? Should userspace
just try to set with 0 value and then drop back to some big number if it
gets an error?

johannes

Fwd: [PATCH v4] mac80211: Allow 0 for NL80211_MESHCONF_PLINK_TIMEOUT to disable STA expiration

[Masashi Honma]

I will re-send this message to ML because of delivery error.

---------- Forwarded message ----------
From: Masashi Honma <masashi.honma@gmail.com>
Date: 2015-02-24 19:00 GMT+09:00
Subject: Re: [PATCH v4] mac80211: Allow 0 for
NL80211_MESHCONF_PLINK_TIMEOUT to disable STA expiration
To: Johannes Berg <johannes@sipsolutions.net>
Cc: "linux-wireless@vger.kernel.org" <linux-wireless@vger.kernel.org>,
Bob Copeland <me@bobcopeland.com>


2015-02-24 18:46 GMT+09:00 Johannes Berg <johannes@sipsolutions.net>:
>
> Hi,
>
> Sorry about the late reply! I'm getting back to merging now and taking a
> closer look at this issue.



Thank you for your review.

>
>
> > +++ b/net/wireless/nl80211.c
> > @@ -5261,7 +5261,7 @@ do {                                                                        \
> >       FILL_IN_MESH_PARAM_IF_SET(tb, cfg, dot11MeshAwakeWindowDuration,
> >                                 0, 65535, mask,
> >                                 NL80211_MESHCONF_AWAKE_WINDOW, nla_get_u16);
> > -     FILL_IN_MESH_PARAM_IF_SET(tb, cfg, plink_timeout, 1, 0xffffffff,
> > +     FILL_IN_MESH_PARAM_IF_SET(tb, cfg, plink_timeout, 0, 0xffffffff,
> >                                 mask, NL80211_MESHCONF_PLINK_TIMEOUT,
>
> I think you should document this new behaviour also in nl80211.h.


Yes. I will modify and re-send this patch.

>
>
> Additionally - what's the plan on how to discover this? Should userspace
> just try to set with 0 value and then drop back to some big number if it
> gets an error?


I already modified wpa_supplicant by commit
0cb5f8d94536e097af7a11273f79239001a602d6.
The way is same as you. It tries to set with 0, if failed it re-try
with 60sec future of wpa_supplicant timeout value.

[PATCH v5] mac80211: Allow 0 for NL80211_MESHCONF_PLINK_TIMEOUT to disable STA expiration

[Masashi Honma]

Both wpa_supplicant and mac80211 has inactivity timer. By default
wpa_supplicant will be timed out in 5 minutes and mac80211's it is 30 minutes.
If wpa_supplicant uses more long timer than mac80211, wpa_supplicant will get
unexpected disconnection by mac80211. This patch adds functionality of disabling
mac80211 inactivity timer to avoid to prevent wpa_supplicant inactivity timer.

I have thought setting 0xffffffff to NL80211_MESHCONF_PLINK_TIMEOUT will solve
this problem without this patch. But the approach does not work on 32 bit
system. To explain the reason, I will show STA expiration rule in kernel. This
is the expression.

(current jiffies) > (frame Rx jiffies + NL80211_MESHCONF_PLINK_TIMEOUT * 250)

On 32bit system, right side could be over flow and be unexpected small value if
NL80211_MESHCONF_PLINK_TIMEOUT is sufficiently large. STA expiration occurs by
this reason. So I made this patch.

Signed-off-by: Masashi Honma <masashi.honma@gmail.com>
---
 include/uapi/linux/nl80211.h | 3 ++-
 net/mac80211/mesh.c          | 3 ++-
 net/wireless/nl80211.c       | 2 +-
 3 files changed, 5 insertions(+), 3 deletions(-)

diff --git a/include/uapi/linux/nl80211.h b/include/uapi/linux/nl80211.h
index 1cbc3aa..0c71180 100644
--- a/include/uapi/linux/nl80211.h
+++ b/include/uapi/linux/nl80211.h
@@ -3092,7 +3092,8 @@ enum nl80211_mesh_power_mode {
  *
  * @NL80211_MESHCONF_PLINK_TIMEOUT: If no tx activity is seen from a STA we've
  *	established peering with for longer than this time (in seconds), then
- *	remove it from the STA's list of peers.  Default is 30 minutes.
+ *	remove it from the STA's list of peers. You may set this to 0 to disable
+ *	the removal of the STA. Default is 30 minutes.
  *
  * @__NL80211_MESHCONF_ATTR_AFTER_LAST: internal use
  */
diff --git a/net/mac80211/mesh.c b/net/mac80211/mesh.c
index 0c8b2a7..acf441f 100644
--- a/net/mac80211/mesh.c
+++ b/net/mac80211/mesh.c
@@ -574,7 +574,8 @@ static void ieee80211_mesh_housekeeping(struct ieee80211_sub_if_data *sdata)
 	struct ieee80211_if_mesh *ifmsh = &sdata->u.mesh;
 	u32 changed;
 
-	ieee80211_sta_expire(sdata, ifmsh->mshcfg.plink_timeout * HZ);
+	if (ifmsh->mshcfg.plink_timeout > 0)
+		ieee80211_sta_expire(sdata, ifmsh->mshcfg.plink_timeout * HZ);
 	mesh_path_expire(sdata);
 
 	changed = mesh_accept_plinks_update(sdata);
diff --git a/net/wireless/nl80211.c b/net/wireless/nl80211.c
index e9ad9d9..bef52af 100644
--- a/net/wireless/nl80211.c
+++ b/net/wireless/nl80211.c
@@ -5261,7 +5261,7 @@ do {									    \
 	FILL_IN_MESH_PARAM_IF_SET(tb, cfg, dot11MeshAwakeWindowDuration,
 				  0, 65535, mask,
 				  NL80211_MESHCONF_AWAKE_WINDOW, nla_get_u16);
-	FILL_IN_MESH_PARAM_IF_SET(tb, cfg, plink_timeout, 1, 0xffffffff,
+	FILL_IN_MESH_PARAM_IF_SET(tb, cfg, plink_timeout, 0, 0xffffffff,
 				  mask, NL80211_MESHCONF_PLINK_TIMEOUT,
 				  nla_get_u32);
 	if (mask_out)
-- 
2.1.0

Re: [PATCH v5] mac80211: Allow 0 for NL80211_MESHCONF_PLINK_TIMEOUT to disable STA expiration

[Johannes Berg]

On Tue, 2015-02-24 at 22:42 +0900, Masashi Honma wrote:
> Both wpa_supplicant and mac80211 has inactivity timer. By default
> wpa_supplicant will be timed out in 5 minutes and mac80211's it is 30 minutes.
> If wpa_supplicant uses more long timer than mac80211, wpa_supplicant will get
> unexpected disconnection by mac80211. This patch adds functionality of disabling
> mac80211 inactivity timer to avoid to prevent wpa_supplicant inactivity timer.
> 
> I have thought setting 0xffffffff to NL80211_MESHCONF_PLINK_TIMEOUT will solve
> this problem without this patch. But the approach does not work on 32 bit
> system. To explain the reason, I will show STA expiration rule in kernel. This
> is the expression.
> 
> (current jiffies) > (frame Rx jiffies + NL80211_MESHCONF_PLINK_TIMEOUT * 250)
> 
> On 32bit system, right side could be over flow and be unexpected small value if
> NL80211_MESHCONF_PLINK_TIMEOUT is sufficiently large. STA expiration occurs by
> this reason. So I made this patch.

Applied, I've reworded and rewrapped the commit log - in the future
please send commit logs with at most 72 characters per line.

johannes

Re: [PATCH v5] mac80211: Allow 0 for NL80211_MESHCONF_PLINK_TIMEOUT to disable STA expiration

[Masashi Honma]

2015-02-25 5:08 GMT+09:00 Johannes Berg <johannes@sipsolutions.net>:
> Applied, I've reworded and rewrapped the commit log - in the future
> please send commit logs with at most 72 characters per line.

Thanks. I will remember 72 characters rule.