* [PATCH] cfg80211: update information elements in cached BSS struct
@ 2010-12-09 10:48 Sven Neumann
2010-12-09 12:47 ` Johannes Berg
0 siblings, 1 reply; 6+ messages in thread
From: Sven Neumann @ 2010-12-09 10:48 UTC (permalink / raw)
To: linux-wireless; +Cc: Sven Neumann
When a cached BSS struct is updated because a new beacon was received,
the code replaces the cached information elements by the IEs from the
new beacon. However it did not update the pub.information_elements
and pub.len_information_elements fields leaving them either pointing
to the old beacon IEs or in an inconsistent state where the data is
replaced by the new beacon IEs but len_information_elements still has
its value from the first beacon.
Fix this by updating the information elements fields if they are
pointing to beacon IEs.
Signed-off-by: Sven Neumann <s.neumann@raumfeld.com>
---
net/wireless/scan.c | 8 ++++++++
1 files changed, 8 insertions(+), 0 deletions(-)
diff --git a/net/wireless/scan.c b/net/wireless/scan.c
index 503ebb8..629bfaa 100644
--- a/net/wireless/scan.c
+++ b/net/wireless/scan.c
@@ -465,6 +465,9 @@ cfg80211_bss_update(struct cfg80211_registered_device *dev,
size_t used = dev->wiphy.bss_priv_size + sizeof(*res);
size_t ielen = res->pub.len_beacon_ies;
+ int information_elements_is_beacon_ies =
+ (found->pub.information_elements == found->pub.beacon_ies);
+
if (found->pub.beacon_ies &&
!found->beacon_ies_allocated &&
ksize(found) >= used + ielen) {
@@ -487,6 +490,11 @@ cfg80211_bss_update(struct cfg80211_registered_device *dev,
found->pub.len_beacon_ies = ielen;
}
}
+
+ if (information_elements_is_beacon_ies) {
+ found->pub.information_elements = found->pub.beacon_ies;
+ found->pub.len_information_elements = found->pub.len_beacon_ies;
+ }
}
kref_put(&res->ref, bss_release);
--
1.7.1
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] cfg80211: update information elements in cached BSS struct
2010-12-09 10:48 [PATCH] cfg80211: update information elements in cached BSS struct Sven Neumann
@ 2010-12-09 12:47 ` Johannes Berg
2010-12-09 13:17 ` Sven Neumann
0 siblings, 1 reply; 6+ messages in thread
From: Johannes Berg @ 2010-12-09 12:47 UTC (permalink / raw)
To: Sven Neumann; +Cc: linux-wireless
On Thu, 2010-12-09 at 11:48 +0100, Sven Neumann wrote:
> When a cached BSS struct is updated because a new beacon was received,
> the code replaces the cached information elements by the IEs from the
> new beacon. However it did not update the pub.information_elements
> and pub.len_information_elements fields leaving them either pointing
> to the old beacon IEs or in an inconsistent state where the data is
> replaced by the new beacon IEs but len_information_elements still has
> its value from the first beacon.
>
> Fix this by updating the information elements fields if they are
> pointing to beacon IEs.
Err, no, this is intentional -- we want data from probe responses to
"win" and be displayed preferentially. If you look at the code that
dumps them out, that's what will happen.
If you need the most up-to-date beacon IEs, use (the equivalent of) "iw
wlan0 scan dump -b".
johannes
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] cfg80211: update information elements in cached BSS struct
2010-12-09 12:47 ` Johannes Berg
@ 2010-12-09 13:17 ` Sven Neumann
2010-12-09 13:53 ` Johannes Berg
0 siblings, 1 reply; 6+ messages in thread
From: Sven Neumann @ 2010-12-09 13:17 UTC (permalink / raw)
To: Johannes Berg; +Cc: linux-wireless
Hi,
On Thu, 2010-12-09 at 13:47 +0100, Johannes Berg wrote:
> On Thu, 2010-12-09 at 11:48 +0100, Sven Neumann wrote:
> > When a cached BSS struct is updated because a new beacon was received,
> > the code replaces the cached information elements by the IEs from the
> > new beacon. However it did not update the pub.information_elements
> > and pub.len_information_elements fields leaving them either pointing
> > to the old beacon IEs or in an inconsistent state where the data is
> > replaced by the new beacon IEs but len_information_elements still has
> > its value from the first beacon.
> >
> > Fix this by updating the information elements fields if they are
> > pointing to beacon IEs.
>
> Err, no, this is intentional -- we want data from probe responses to
> "win" and be displayed preferentially. If you look at the code that
> dumps them out, that's what will happen.
As far as I can see my patch does not break this behavior. The
information elements fields are only updated if they are currently
pointing to beacon IEs. If there's ever a probe response frame, then it
will win and the information elements fields won't be changed for
beacons arriving later.
> If you need the most up-to-date beacon IEs, use (the equivalent of) "iw
> wlan0 scan dump -b".
What my patch is fixing are the following situations (and yes, this
happened in real life, it is reproducable and it is reproducably fixed
by the patch that I've submitted):
AP sends a beacon without RSN IE. This is the first beacon received
and the station with IEs is added to the BSS cache.
pub.information_elements now points to pub.beacon_ies and
pub.len_information_elements has the value of pub.len_beacon_ies.
AP sends another beacon, this time it includes RSN IE. As there's
not enough place for the larger IEs, memory is allocated for it
and the pub.beacon_ies pointer is changed to point to this newly
allocated memory. Now pub.information_elements still points to
the old value.
User-space asks for scan results and will receive the IEs from the
first beacon because even though the data from subsequent beacons
has been added to the cache, pub.information_elements still points
to the old data.
As a result user-space looking for a WPA enabled access-point will
skip the AP and never associate with it. This situation will only
ever fix itself if you are lucky enough that so few beacons are
arriving that the cached RSS expires at some point.
And there are even worse scenarios:
If the second beacon is smaller than the first or only slightly larger
(so that it fits into the allocated memory which has some padding),
then no new memory will be allocated for the IEs. Instead the data
in pub.beacon_ies will be changed and pub.len_beacon_ies will be
updated. Now pub.information_elements points to the new data
but pub.len_information_elements still holds the old value. Data
and length have become inconsistent. As a result invalid IEs are
returned to user-space when it asks for scan results.
As you can see this is all without any probe response frames. Our driver
(libertas) doesn't do any active scanning and thus there are no probe
response frames received. I still tried to write the patch in a way that
should not break things if probe response frames are received.
Please review this patch again. I am pretty sure that there's something
broken. Perhaps my fix is not totally correct, but it fixes a
reproducable association problem for us.
Regards,
Sven
--
Sven Neumann
Head of Software Development
RAUMFELD GmbH | Reichenberger Str. 124 | 10999 Berlin | Germany
Tel: +49.30.340.60.98.0 | Fax: +49.30.340.60.98.99 | s.neumann@raumfeld.com
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] cfg80211: update information elements in cached BSS struct
2010-12-09 13:17 ` Sven Neumann
@ 2010-12-09 13:53 ` Johannes Berg
2010-12-09 14:05 ` Sven Neumann
0 siblings, 1 reply; 6+ messages in thread
From: Johannes Berg @ 2010-12-09 13:53 UTC (permalink / raw)
To: Sven Neumann; +Cc: linux-wireless
On Thu, 2010-12-09 at 14:17 +0100, Sven Neumann wrote:
[snip
> Please review this patch again. I am pretty sure that there's something
> broken. Perhaps my fix is not totally correct, but it fixes a
> reproducable association problem for us.
You're right, the if catches that.
Can you please make the variable a bool, remove the blank line before
it? And your line length was way above 80. A comment would also be good
about why this is done, maybe a one-liner like the probe resp one, just
saying "Override IEs if they were from a beacon before".
Thanks!
johannes
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH] cfg80211: update information elements in cached BSS struct
2010-12-09 13:53 ` Johannes Berg
@ 2010-12-09 14:05 ` Sven Neumann
2010-12-09 14:07 ` Johannes Berg
0 siblings, 1 reply; 6+ messages in thread
From: Sven Neumann @ 2010-12-09 14:05 UTC (permalink / raw)
To: linux-wireless; +Cc: Johannes Berg, Sven Neumann
When a cached BSS struct is updated because a new beacon was received,
the code replaces the cached information elements by the IEs from the
new beacon. However it did not update the pub.information_elements
and pub.len_information_elements fields leaving them either pointing
to the old beacon IEs or in an inconsistent state where the data is
replaced by the new beacon IEs but len_information_elements still has
its value from the first beacon.
Fix this by updating the information elements fields if they are
pointing to beacon IEs.
Signed-off-by: Sven Neumann <s.neumann@raumfeld.com>
---
net/wireless/scan.c | 11 +++++++++++
1 files changed, 11 insertions(+), 0 deletions(-)
diff --git a/net/wireless/scan.c b/net/wireless/scan.c
index 503ebb8..ea427f4 100644
--- a/net/wireless/scan.c
+++ b/net/wireless/scan.c
@@ -464,6 +464,9 @@ cfg80211_bss_update(struct cfg80211_registered_device *dev,
if (res->pub.beacon_ies) {
size_t used = dev->wiphy.bss_priv_size + sizeof(*res);
size_t ielen = res->pub.len_beacon_ies;
+ bool information_elements_is_beacon_ies =
+ (found->pub.information_elements ==
+ found->pub.beacon_ies);
if (found->pub.beacon_ies &&
!found->beacon_ies_allocated &&
@@ -487,6 +490,14 @@ cfg80211_bss_update(struct cfg80211_registered_device *dev,
found->pub.len_beacon_ies = ielen;
}
}
+
+ /* Override IEs if they were from a beacon before */
+ if (information_elements_is_beacon_ies) {
+ found->pub.information_elements =
+ found->pub.beacon_ies;
+ found->pub.len_information_elements =
+ found->pub.len_beacon_ies;
+ }
}
kref_put(&res->ref, bss_release);
--
1.7.1
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] cfg80211: update information elements in cached BSS struct
2010-12-09 14:05 ` Sven Neumann
@ 2010-12-09 14:07 ` Johannes Berg
0 siblings, 0 replies; 6+ messages in thread
From: Johannes Berg @ 2010-12-09 14:07 UTC (permalink / raw)
To: Sven Neumann; +Cc: linux-wireless
On Thu, 2010-12-09 at 15:05 +0100, Sven Neumann wrote:
> When a cached BSS struct is updated because a new beacon was received,
> the code replaces the cached information elements by the IEs from the
> new beacon. However it did not update the pub.information_elements
> and pub.len_information_elements fields leaving them either pointing
> to the old beacon IEs or in an inconsistent state where the data is
> replaced by the new beacon IEs but len_information_elements still has
> its value from the first beacon.
>
> Fix this by updating the information elements fields if they are
> pointing to beacon IEs.
>
> Signed-off-by: Sven Neumann <s.neumann@raumfeld.com>
Reviewed-by: Johannes Berg <johannes@sipsolutions.net>
> ---
> net/wireless/scan.c | 11 +++++++++++
> 1 files changed, 11 insertions(+), 0 deletions(-)
>
> diff --git a/net/wireless/scan.c b/net/wireless/scan.c
> index 503ebb8..ea427f4 100644
> --- a/net/wireless/scan.c
> +++ b/net/wireless/scan.c
> @@ -464,6 +464,9 @@ cfg80211_bss_update(struct cfg80211_registered_device *dev,
> if (res->pub.beacon_ies) {
> size_t used = dev->wiphy.bss_priv_size + sizeof(*res);
> size_t ielen = res->pub.len_beacon_ies;
> + bool information_elements_is_beacon_ies =
> + (found->pub.information_elements ==
> + found->pub.beacon_ies);
>
> if (found->pub.beacon_ies &&
> !found->beacon_ies_allocated &&
> @@ -487,6 +490,14 @@ cfg80211_bss_update(struct cfg80211_registered_device *dev,
> found->pub.len_beacon_ies = ielen;
> }
> }
> +
> + /* Override IEs if they were from a beacon before */
> + if (information_elements_is_beacon_ies) {
> + found->pub.information_elements =
> + found->pub.beacon_ies;
> + found->pub.len_information_elements =
> + found->pub.len_beacon_ies;
> + }
> }
>
> kref_put(&res->ref, bss_release);
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2010-12-09 14:07 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2010-12-09 10:48 [PATCH] cfg80211: update information elements in cached BSS struct Sven Neumann
2010-12-09 12:47 ` Johannes Berg
2010-12-09 13:17 ` Sven Neumann
2010-12-09 13:53 ` Johannes Berg
2010-12-09 14:05 ` Sven Neumann
2010-12-09 14:07 ` Johannes Berg
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).