Re: [PATCH 1/3] cfg80211: Add option to report the bss entry in connect result

2016-04-14 Thread Johannes Berg 
On Mon, 2016-04-11 at 15:16 +0530, Kanchanapally, Vidyullatha wrote:
> From: "Kanchanapally, Vidyullatha" 
> 
> Since cfg80211 maintains separate BSS table entries for APs if the
> same
> BSSID, SSID pair is seen on multiple channels, it is possible that it
> can map the current_bss to a BSS entry on the wrong channel. This
> current_bss will not get flushed unless disconnected and cfg80211
> reports a wrong channel as the associated channel.
> 
> Fix this by introducing a new cfg80211_connect_bss() function which
> is
> similar to cfg80211_connect_result(), but it includes an additional
> parameter: the bss the STA is connected to. This allows drivers to
> provide the exact bss entry that matches the BSS to which the
> connection
> was completed.

Applied.

I'm surprised you're so worried about this test scenario (we've
discussed this multiple times before), but I can't really argue with
passing the BSS pointer either :)

johannes
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH 1/3] cfg80211: Add option to report the bss entry in connect result

2016-04-12 Thread Kanchanapally, Vidyullatha 
Hi Arend,

The bss table entry in kernel will have the channel indicated in the 
beacon/probe response itself.

But here we are addressing a case where the AP is changing its channel (not 
through CSA but is disconnecting clients on old channel and is coming up on a 
new channel).
In such a scenario, because of the kernel's scan timeout logic, there is a 
small amount of time where the bss table entry is present for the AP on both 
the old and new channel.

If a connect happens with in this time, the kernel might map the 
"wdev->current_bss" to a wrong bss entry, which is obtained in
" __cfg80211_connect_result" through "cfg80211_get_bss(wdev->wiphy, NULL, 
...)". The channel pointer passed here is NULL.
The "wdev->current_bss" is held by the kernel through the connection and the 
bss entry shall not get flushed until a disconnect, although the AP is not seen 
any more in scan results.
The kernel for such a connection will report a wrong channel as associated 
channel.

The current cfg80211 connect indication "cfg80211_connect_result" does not have 
a provision to indicate either the channel or the bss entry.
Hence we are introducing a new cfg80211 api called "cfg80211_connect_bss" which 
includes a bss entry.

Thanks
Vidyullatha

-Original Message-
From: Arend van Spriel [mailto:arend.vanspr...@broadcom.com] 
Sent: Tuesday, April 12, 2016 2:22 PM
To: Kanchanapally, Vidyullatha ; 
johan...@sipsolutions.net
Cc: linux-wireless@vger.kernel.org; Malinen, Jouni ; 
Undekari, Sunil Dutt ; Hullur Subramanyam, Amarnath 

Subject: Re: [PATCH 1/3] cfg80211: Add option to report the bss entry in 
connect result



On 11-04-16 11:46, Kanchanapally, Vidyullatha wrote:
> From: "Kanchanapally, Vidyullatha" 
> 
> Since cfg80211 maintains separate BSS table entries for APs if the 
> same BSSID, SSID pair is seen on multiple channels, it is possible 
> that it can map the current_bss to a BSS entry on the wrong channel. 
> This current_bss will not get flushed unless disconnected and cfg80211 
> reports a wrong channel as the associated channel.
> 
> Fix this by introducing a new cfg80211_connect_bss() function which is 
> similar to cfg80211_connect_result(), but it includes an additional
> parameter: the bss the STA is connected to. This allows drivers to 
> provide the exact bss entry that matches the BSS to which the 
> connection was completed.

How does this work? Is the bss table entry filed with channel it sees the bss 
during scan or does it use the channel indicated in the beacon/probe reponse. I 
would expect the latter. Is the AP beaconing on multiple channels using the 
same BSSID?

It would help understand this change with a driver patch.

Regards,
Arend

> Reviewed-by: Jouni Malinen 
> Signed-off-by: Vidyullatha Kanchanapally 
> Signed-off-by: Sunil Dutt 
> ---
>  include/net/cfg80211.h | 39 +++
>  net/wireless/core.h|  1 +
>  net/wireless/sme.c | 28 ++--
>  net/wireless/util.c|  2 +-
>  4 files changed, 59 insertions(+), 11 deletions(-)
> 
> diff --git a/include/net/cfg80211.h b/include/net/cfg80211.h index 
> 858a97ec..51f177c 100644
> --- a/include/net/cfg80211.h
> +++ b/include/net/cfg80211.h
> @@ -4648,6 +4648,32 @@ static inline void 
> cfg80211_testmode_event(struct sk_buff *skb, gfp_t gfp)  #endif
>  
>  /**
> + * cfg80211_connect_bss - notify cfg80211 of connection result
> + *
> + * @dev: network device
> + * @bssid: the BSSID of the AP
> + * @bss: entry of bss to which STA got connected to, can be obtained
> + *   through cfg80211_get_bss (may be %NULL)
> + * @req_ie: association request IEs (maybe be %NULL)
> + * @req_ie_len: association request IEs length
> + * @resp_ie: association response IEs (may be %NULL)
> + * @resp_ie_len: assoc response IEs length
> + * @status: status code, 0 for successful connection, use
> + *  %WLAN_STATUS_UNSPECIFIED_FAILURE if your device cannot give you
> + *  the real status code for failures.
> + * @gfp: allocation flags
> + *
> + * It should be called by the underlying driver whenever connect() 
> +has
> + * succeeded. This is similar to cfg80211_connect_result(), but with 
> +the
> + * option of identifying the exact bss entry for the connection. Only 
> +one of
> + * these functions should be called.
> + */
> +void cfg80211_connect_bss(struct net_device *dev, const u8 *bssid,
> +   struct cfg80211_bss *bss, const u8 *req_ie,
> +   size_t req_ie_len, const u8 *resp_ie,
> +   size_t resp_ie_len, u16 status, gfp_t gfp);
> +
> +/**
>   * cfg80211_connect_result - notify cfg80211 of connection result
>   *
>   * @dev: network device
> @@ -4664,10 +4690,15 @@ static inline void cfg80211_testmode_event(struct 
> sk_buff

Re: [PATCH 1/3] cfg80211: Add option to report the bss entry in connect result

2016-04-12 Thread Arend van Spriel 


On 11-04-16 11:46, Kanchanapally, Vidyullatha wrote:
> From: "Kanchanapally, Vidyullatha" 
> 
> Since cfg80211 maintains separate BSS table entries for APs if the same
> BSSID, SSID pair is seen on multiple channels, it is possible that it
> can map the current_bss to a BSS entry on the wrong channel. This
> current_bss will not get flushed unless disconnected and cfg80211
> reports a wrong channel as the associated channel.
> 
> Fix this by introducing a new cfg80211_connect_bss() function which is
> similar to cfg80211_connect_result(), but it includes an additional
> parameter: the bss the STA is connected to. This allows drivers to
> provide the exact bss entry that matches the BSS to which the connection
> was completed.

How does this work? Is the bss table entry filed with channel it sees
the bss during scan or does it use the channel indicated in the
beacon/probe reponse. I would expect the latter. Is the AP beaconing on
multiple channels using the same BSSID?

It would help understand this change with a driver patch.

Regards,
Arend

> Reviewed-by: Jouni Malinen 
> Signed-off-by: Vidyullatha Kanchanapally 
> Signed-off-by: Sunil Dutt 
> ---
>  include/net/cfg80211.h | 39 +++
>  net/wireless/core.h|  1 +
>  net/wireless/sme.c | 28 ++--
>  net/wireless/util.c|  2 +-
>  4 files changed, 59 insertions(+), 11 deletions(-)
> 
> diff --git a/include/net/cfg80211.h b/include/net/cfg80211.h
> index 858a97ec..51f177c 100644
> --- a/include/net/cfg80211.h
> +++ b/include/net/cfg80211.h
> @@ -4648,6 +4648,32 @@ static inline void cfg80211_testmode_event(struct 
> sk_buff *skb, gfp_t gfp)
>  #endif
>  
>  /**
> + * cfg80211_connect_bss - notify cfg80211 of connection result
> + *
> + * @dev: network device
> + * @bssid: the BSSID of the AP
> + * @bss: entry of bss to which STA got connected to, can be obtained
> + *   through cfg80211_get_bss (may be %NULL)
> + * @req_ie: association request IEs (maybe be %NULL)
> + * @req_ie_len: association request IEs length
> + * @resp_ie: association response IEs (may be %NULL)
> + * @resp_ie_len: assoc response IEs length
> + * @status: status code, 0 for successful connection, use
> + *  %WLAN_STATUS_UNSPECIFIED_FAILURE if your device cannot give you
> + *  the real status code for failures.
> + * @gfp: allocation flags
> + *
> + * It should be called by the underlying driver whenever connect() has
> + * succeeded. This is similar to cfg80211_connect_result(), but with the
> + * option of identifying the exact bss entry for the connection. Only one of
> + * these functions should be called.
> + */
> +void cfg80211_connect_bss(struct net_device *dev, const u8 *bssid,
> +   struct cfg80211_bss *bss, const u8 *req_ie,
> +   size_t req_ie_len, const u8 *resp_ie,
> +   size_t resp_ie_len, u16 status, gfp_t gfp);
> +
> +/**
>   * cfg80211_connect_result - notify cfg80211 of connection result
>   *
>   * @dev: network device
> @@ -4664,10 +4690,15 @@ static inline void cfg80211_testmode_event(struct 
> sk_buff *skb, gfp_t gfp)
>   * It should be called by the underlying driver whenever connect() has
>   * succeeded.
>   */
> -void cfg80211_connect_result(struct net_device *dev, const u8 *bssid,
> -  const u8 *req_ie, size_t req_ie_len,
> -  const u8 *resp_ie, size_t resp_ie_len,
> -  u16 status, gfp_t gfp);
> +static inline void
> +cfg80211_connect_result(struct net_device *dev, const u8 *bssid,
> + const u8 *req_ie, size_t req_ie_len,
> + const u8 *resp_ie, size_t resp_ie_len,
> + u16 status, gfp_t gfp)
> +{
> + cfg80211_connect_bss(dev, bssid, NULL, req_ie, req_ie_len, resp_ie,
> +  resp_ie_len, status, gfp);
> +}
>  
>  /**
>   * cfg80211_roamed - notify cfg80211 of roaming
> diff --git a/net/wireless/core.h b/net/wireless/core.h
> index 64c70e4..f3b0a07 100644
> --- a/net/wireless/core.h
> +++ b/net/wireless/core.h
> @@ -214,6 +214,7 @@ struct cfg80211_event {
>   const u8 *resp_ie;
>   size_t req_ie_len;
>   size_t resp_ie_len;
> + struct cfg80211_bss *bss;
>   u16 status;
>   } cr;
>   struct {
> diff --git a/net/wireless/sme.c b/net/wireless/sme.c
> index 1fba416..da97424d 100644
> --- a/net/wireless/sme.c
> +++ b/net/wireless/sme.c
> @@ -753,19 +753,32 @@ void __cfg80211_connect_result(struct net_device *dev, 
> const u8 *bssid,
>   kfree(country_ie);
>  }
>  
> -void cfg80211_connect_result(struct net_device *dev, const u8 *bssid,
> -  const u8 *req_ie, size_t req_ie_len,
> -  const u8 *resp_ie,

[PATCH 1/3] cfg80211: Add option to report the bss entry in connect result

2016-04-11 Thread Kanchanapally, Vidyullatha 
From: "Kanchanapally, Vidyullatha" 

Since cfg80211 maintains separate BSS table entries for APs if the same
BSSID, SSID pair is seen on multiple channels, it is possible that it
can map the current_bss to a BSS entry on the wrong channel. This
current_bss will not get flushed unless disconnected and cfg80211
reports a wrong channel as the associated channel.

Fix this by introducing a new cfg80211_connect_bss() function which is
similar to cfg80211_connect_result(), but it includes an additional
parameter: the bss the STA is connected to. This allows drivers to
provide the exact bss entry that matches the BSS to which the connection
was completed.

Reviewed-by: Jouni Malinen 
Signed-off-by: Vidyullatha Kanchanapally 
Signed-off-by: Sunil Dutt 
---
 include/net/cfg80211.h | 39 +++
 net/wireless/core.h|  1 +
 net/wireless/sme.c | 28 ++--
 net/wireless/util.c|  2 +-
 4 files changed, 59 insertions(+), 11 deletions(-)

diff --git a/include/net/cfg80211.h b/include/net/cfg80211.h
index 858a97ec..51f177c 100644
--- a/include/net/cfg80211.h
+++ b/include/net/cfg80211.h
@@ -4648,6 +4648,32 @@ static inline void cfg80211_testmode_event(struct 
sk_buff *skb, gfp_t gfp)
 #endif
 
 /**
+ * cfg80211_connect_bss - notify cfg80211 of connection result
+ *
+ * @dev: network device
+ * @bssid: the BSSID of the AP
+ * @bss: entry of bss to which STA got connected to, can be obtained
+ * through cfg80211_get_bss (may be %NULL)
+ * @req_ie: association request IEs (maybe be %NULL)
+ * @req_ie_len: association request IEs length
+ * @resp_ie: association response IEs (may be %NULL)
+ * @resp_ie_len: assoc response IEs length
+ * @status: status code, 0 for successful connection, use
+ *  %WLAN_STATUS_UNSPECIFIED_FAILURE if your device cannot give you
+ *  the real status code for failures.
+ * @gfp: allocation flags
+ *
+ * It should be called by the underlying driver whenever connect() has
+ * succeeded. This is similar to cfg80211_connect_result(), but with the
+ * option of identifying the exact bss entry for the connection. Only one of
+ * these functions should be called.
+ */
+void cfg80211_connect_bss(struct net_device *dev, const u8 *bssid,
+ struct cfg80211_bss *bss, const u8 *req_ie,
+ size_t req_ie_len, const u8 *resp_ie,
+ size_t resp_ie_len, u16 status, gfp_t gfp);
+
+/**
  * cfg80211_connect_result - notify cfg80211 of connection result
  *
  * @dev: network device
@@ -4664,10 +4690,15 @@ static inline void cfg80211_testmode_event(struct 
sk_buff *skb, gfp_t gfp)
  * It should be called by the underlying driver whenever connect() has
  * succeeded.
  */
-void cfg80211_connect_result(struct net_device *dev, const u8 *bssid,
-const u8 *req_ie, size_t req_ie_len,
-const u8 *resp_ie, size_t resp_ie_len,
-u16 status, gfp_t gfp);
+static inline void
+cfg80211_connect_result(struct net_device *dev, const u8 *bssid,
+   const u8 *req_ie, size_t req_ie_len,
+   const u8 *resp_ie, size_t resp_ie_len,
+   u16 status, gfp_t gfp)
+{
+   cfg80211_connect_bss(dev, bssid, NULL, req_ie, req_ie_len, resp_ie,
+resp_ie_len, status, gfp);
+}
 
 /**
  * cfg80211_roamed - notify cfg80211 of roaming
diff --git a/net/wireless/core.h b/net/wireless/core.h
index 64c70e4..f3b0a07 100644
--- a/net/wireless/core.h
+++ b/net/wireless/core.h
@@ -214,6 +214,7 @@ struct cfg80211_event {
const u8 *resp_ie;
size_t req_ie_len;
size_t resp_ie_len;
+   struct cfg80211_bss *bss;
u16 status;
} cr;
struct {
diff --git a/net/wireless/sme.c b/net/wireless/sme.c
index 1fba416..da97424d 100644
--- a/net/wireless/sme.c
+++ b/net/wireless/sme.c
@@ -753,19 +753,32 @@ void __cfg80211_connect_result(struct net_device *dev, 
const u8 *bssid,
kfree(country_ie);
 }
 
-void cfg80211_connect_result(struct net_device *dev, const u8 *bssid,
-const u8 *req_ie, size_t req_ie_len,
-const u8 *resp_ie, size_t resp_ie_len,
-u16 status, gfp_t gfp)
+/* Consumes bss object one way or another */
+void cfg80211_connect_bss(struct net_device *dev, const u8 *bssid,
+ struct cfg80211_bss *bss, const u8 *req_ie,
+ size_t req_ie_len, const u8 *resp_ie,
+ size_t resp_ie_len, u16 status, gfp_t gfp)
 {
struct wireless_dev *wdev = dev->ieee80211_ptr;
struct cfg80211_registered_device *rdev = wiphy_to_rdev(wdev->wiphy);
struct