| Message ID | 1421722023-4691-1-git-send-email-masashi.honma@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Johannes Berg |
| Headers | show |
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 -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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. -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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);
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(-)