Re: [PATCH 1/2] mac80211: mesh_plink: handle confirm frames with new plid

Re: [PATCH 1/2] mac80211: mesh_plink: handle confirm frames with new plid

[Bob Copeland]

[oops, +linux-wireless]
On Sat, Jun 28, 2014 at 04:35:25PM -0400, Bob Copeland wrote:
> The 802.11 standard says when processing a plink confirm
> frame:
> 
> "If the peerLinkID in the mesh peering instance has not been
> set, the Local Link ID field of the Mesh Peering Confirm
> request shall be copied into the peerLinkID in the mesh
> peering instance."
> 
> We were only doing this when receiving an open peering frame,
> but it could happen that the open frame gets lost and so we
> should handle this case rather than rejecting the confirm and
> failing the whole peering process.
> 
> Reported-by: Yu Niiro <yu.niiro@gmail.com>
> Signed-off-by: Bob Copeland <me@bobcopeland.com>
> ---
>  net/mac80211/mesh_plink.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/net/mac80211/mesh_plink.c b/net/mac80211/mesh_plink.c
> index 63b8741..c47194d 100644
> --- a/net/mac80211/mesh_plink.c
> +++ b/net/mac80211/mesh_plink.c
> @@ -959,7 +959,8 @@ mesh_plink_get_event(struct ieee80211_sub_if_data *sdata,
>  		if (!matches_local)
>  			event = CNF_RJCT;
>  		if (!mesh_plink_free_count(sdata) ||
> -		    (sta->llid != llid || sta->plid != plid))
> +		    sta->llid != llid ||
> +		    (sta->plid && sta->plid != plid))
>  			event = CNF_IGNR;
>  		else
>  			event = CNF_ACPT;
> @@ -1080,6 +1081,10 @@ mesh_process_plink_frame(struct ieee80211_sub_if_data *sdata,
>  		goto unlock_rcu;
>  	}
>  
> +	/* 802.11-2012 13.3.7.2 - update plid on CNF if not set */
> +	if (!sta->plid && event == CNF_ACPT)
> +		sta->plid = plid;
> +
>  	changed |= mesh_plink_fsm(sdata, sta, event);
>  
>  unlock_rcu:
> -- 
> 2.0.0.rc2
> 

-- 
Bob Copeland %% www.bobcopeland.com

Re: [PATCH 1/2] mac80211: mesh_plink: handle confirm frames with new plid

[Yeoh Chun-Yeow]

Hi, Bob

What is the consequence if we don't handle this case? Is the peer
going to do the re-auth again?

Regards,
Chun-Yeow

On Sun, Jun 29, 2014 at 4:54 AM, Bob Copeland <me@bobcopeland.com> wrote:
> [oops, +linux-wireless]
> On Sat, Jun 28, 2014 at 04:35:25PM -0400, Bob Copeland wrote:
>> The 802.11 standard says when processing a plink confirm
>> frame:
>>
>> "If the peerLinkID in the mesh peering instance has not been
>> set, the Local Link ID field of the Mesh Peering Confirm
>> request shall be copied into the peerLinkID in the mesh
>> peering instance."
>>
>> We were only doing this when receiving an open peering frame,
>> but it could happen that the open frame gets lost and so we
>> should handle this case rather than rejecting the confirm and
>> failing the whole peering process.
>>
>> Reported-by: Yu Niiro <yu.niiro@gmail.com>
>> Signed-off-by: Bob Copeland <me@bobcopeland.com>
>> ---
>>  net/mac80211/mesh_plink.c | 7 ++++++-
>>  1 file changed, 6 insertions(+), 1 deletion(-)
>>
>> diff --git a/net/mac80211/mesh_plink.c b/net/mac80211/mesh_plink.c
>> index 63b8741..c47194d 100644
>> --- a/net/mac80211/mesh_plink.c
>> +++ b/net/mac80211/mesh_plink.c
>> @@ -959,7 +959,8 @@ mesh_plink_get_event(struct ieee80211_sub_if_data *sdata,
>>               if (!matches_local)
>>                       event = CNF_RJCT;
>>               if (!mesh_plink_free_count(sdata) ||
>> -                 (sta->llid != llid || sta->plid != plid))
>> +                 sta->llid != llid ||
>> +                 (sta->plid && sta->plid != plid))
>>                       event = CNF_IGNR;
>>               else
>>                       event = CNF_ACPT;
>> @@ -1080,6 +1081,10 @@ mesh_process_plink_frame(struct ieee80211_sub_if_data *sdata,
>>               goto unlock_rcu;
>>       }
>>
>> +     /* 802.11-2012 13.3.7.2 - update plid on CNF if not set */
>> +     if (!sta->plid && event == CNF_ACPT)
>> +             sta->plid = plid;
>> +
>>       changed |= mesh_plink_fsm(sdata, sta, event);
>>
>>  unlock_rcu:
>> --
>> 2.0.0.rc2
>>
>
> --
> Bob Copeland %% www.bobcopeland.com
> _______________________________________________
> Devel mailing list
> Devel@lists.open80211s.org
> http://lists.open80211s.org/cgi-bin/mailman/listinfo/devel

Re: [PATCH 1/2] mac80211: mesh_plink: handle confirm frames with new plid

[Bob Copeland]

[reordered top-posting]

On Thu, Jul 03, 2014 at 08:36:28PM +0800, Chun-Yeow Yeoh wrote:
> >> "If the peerLinkID in the mesh peering instance has not been
> >> set, the Local Link ID field of the Mesh Peering Confirm
> >> request shall be copied into the peerLinkID in the mesh
> >> peering instance."
> >>
> Hi, Bob
> 
> What is the consequence if we don't handle this case? Is the peer
> going to do the re-auth again?
> 
> Regards,
> Chun-Yeow

It shouldn't be a big problem -- looking at 802.11-2012 figure 13-2:

Let's say station A is in OPN_SNT and station B is in OPN_RCVD,
but station A failed to get the Open frame from B.

When A gets a Confirm frame from B, it would ignore it (due to
missing plid), then it would resend Open on dot11MeshRetryTimeout.
Unfortunately, Confirm responses from B to that Open frame would
be ignored too.

However, B should also retry Open on dot11MeshRetryTimeout.
If any are successful, A moves to OPN_RCVD, then both have plids
and everything should be ok from then on.

In the worst case, plink timer on either station fires
dot11MeshMaxRetries times and peering instance closes.  Then peering
can start over after leaving holding state and either station gets a
beacon.

So in sum, it just adds a bit of resiliency and means we don't have
to wait for a dot11MeshRetryTimeout in the case of one lost open
frame.

-- 
Bob Copeland %% www.bobcopeland.com

Re: [PATCH 1/2] mac80211: mesh_plink: handle confirm frames with new plid

[Bob Copeland]

On Thu, Jul 03, 2014 at 10:28:37AM -0400, Bob Copeland wrote:
> > What is the consequence if we don't handle this case? Is the peer
> > going to do the re-auth again?
> > 
> > Regards,
> > Chun-Yeow
> 
> It shouldn't be a big problem -- looking at 802.11-2012 figure 13-2:

Actually, thinking about it more, a worse case is if B reaches
ESTAB and then A reboots.  Now, A sends an Open, gets back
a Confirm that A will ignore (due to no plid).  A will still
timeout and send a Close frame to restart everything, but we
would have to wait quite a while to re-establish the peering.

-- 
Bob Copeland %% www.bobcopeland.com

Re: [PATCH 1/2] mac80211: mesh_plink: handle confirm frames with new plid

[Yeoh Chun-Yeow]

Hi, Bob

Thanks for your explanation.

I am also thinking whether it is easy to replicate the mentioned
problem. It seems that the re-auth is working fine.

Regards,
Chun-Yeow

On Thu, Jul 3, 2014 at 11:10 PM, Bob Copeland <me@bobcopeland.com> wrote:
> On Thu, Jul 03, 2014 at 10:28:37AM -0400, Bob Copeland wrote:
>> > What is the consequence if we don't handle this case? Is the peer
>> > going to do the re-auth again?
>> >
>> > Regards,
>> > Chun-Yeow
>>
>> It shouldn't be a big problem -- looking at 802.11-2012 figure 13-2:
>
> Actually, thinking about it more, a worse case is if B reaches
> ESTAB and then A reboots.  Now, A sends an Open, gets back
> a Confirm that A will ignore (due to no plid).  A will still
> timeout and send a Close frame to restart everything, but we
> would have to wait quite a while to re-establish the peering.
>
> --
> Bob Copeland %% www.bobcopeland.com

Re: [PATCH 1/2] mac80211: mesh_plink: handle confirm frames with new plid

[Johannes Berg]

On Sat, 2014-06-28 at 16:54 -0400, Bob Copeland wrote:
> [oops, +linux-wireless]
> On Sat, Jun 28, 2014 at 04:35:25PM -0400, Bob Copeland wrote:
> > The 802.11 standard says when processing a plink confirm
> > frame:
> > 
> > "If the peerLinkID in the mesh peering instance has not been
> > set, the Local Link ID field of the Mesh Peering Confirm
> > request shall be copied into the peerLinkID in the mesh
> > peering instance."
> > 
> > We were only doing this when receiving an open peering frame,
> > but it could happen that the open frame gets lost and so we
> > should handle this case rather than rejecting the confirm and
> > failing the whole peering process.

Applied.

johannes

Re: [PATCH 2/2] mac80211: mesh_plink: use get_unaligned_le16 instead of memcpy

[Bob Copeland]

[+CC linux-wireless]
On Sat, Jun 28, 2014 at 04:35:26PM -0400, Bob Copeland wrote:
> Use get_unaligned_le16 to access llid/plid.
> 
> Signed-off-by: Bob Copeland <me@bobcopeland.com>
> ---
>  net/mac80211/mesh_plink.c | 10 +++-------
>  1 file changed, 3 insertions(+), 7 deletions(-)
> 
> diff --git a/net/mac80211/mesh_plink.c b/net/mac80211/mesh_plink.c
> index c47194d..421ec06 100644
> --- a/net/mac80211/mesh_plink.c
> +++ b/net/mac80211/mesh_plink.c
> @@ -1004,7 +1004,6 @@ mesh_process_plink_frame(struct ieee80211_sub_if_data *sdata,
>  	enum ieee80211_self_protected_actioncode ftype;
>  	u32 changed = 0;
>  	u8 ie_len = elems->peering_len;
> -	__le16 _plid, _llid;
>  	u16 plid, llid = 0;
>  
>  	if (!elems->peering) {
> @@ -1039,13 +1038,10 @@ mesh_process_plink_frame(struct ieee80211_sub_if_data *sdata,
>  	/* Note the lines below are correct, the llid in the frame is the plid
>  	 * from the point of view of this host.
>  	 */
> -	memcpy(&_plid, PLINK_GET_LLID(elems->peering), sizeof(__le16));
> -	plid = le16_to_cpu(_plid);
> +	plid = get_unaligned_le16(PLINK_GET_LLID(elems->peering));
>  	if (ftype == WLAN_SP_MESH_PEERING_CONFIRM ||
> -	    (ftype == WLAN_SP_MESH_PEERING_CLOSE && ie_len == 8)) {
> -		memcpy(&_llid, PLINK_GET_PLID(elems->peering), sizeof(__le16));
> -		llid = le16_to_cpu(_llid);
> -	}
> +	    (ftype == WLAN_SP_MESH_PEERING_CLOSE && ie_len == 8))
> +		llid = get_unaligned_le16(PLINK_GET_PLID(elems->peering));
>  
>  	/* WARNING: Only for sta pointer, is dropped & re-acquired */
>  	rcu_read_lock();
> -- 
> 2.0.0.rc2
> 

-- 
Bob Copeland %% www.bobcopeland.com

Re: [PATCH 2/2] mac80211: mesh_plink: use get_unaligned_le16 instead of memcpy

[Johannes Berg]

On Sat, 2014-06-28 at 16:54 -0400, Bob Copeland wrote:
> [+CC linux-wireless]
> On Sat, Jun 28, 2014 at 04:35:26PM -0400, Bob Copeland wrote:
> > Use get_unaligned_le16 to access llid/plid.

Applied.

johannes