* [PATCH net] mptcp: validate 'id' when stopping the ADD_ADDR retransmit timer
@ 2021-05-11 17:46 Davide Caratti
2021-05-11 22:39 ` Mat Martineau
0 siblings, 1 reply; 7+ messages in thread
From: Davide Caratti @ 2021-05-11 17:46 UTC (permalink / raw)
To: mptcp, Geliang Tang, Matthieu Baerts
when Linux receives an echo-ed ADD_ADDR, it checks the IP address against
the list of "announced" addresses. In case of a positive match, the timer
that handles retransmissions is stopped regardless of the 'Address Id' in
the received packet: this behaviour does not comply with RFC8684 §3.4.1.
Fix it by validating the 'Address Id' in received echo-ed ADD_ADDRs.
Tested using packetdrill, with the following captured output:
unpatched kernel:
Out <...> Flags [.], ack 1, win 256, options [mptcp add-addr v1 id 1 198.51.100.2 hmac 0xfd2e62517888fe29,mptcp dss ack 3007449509], length 0
In <...> Flags [.], ack 1, win 257, options [mptcp add-addr v1-echo id 1 1.2.3.4,mptcp dss ack 3013740213], length 0
Out <...> Flags [.], ack 1, win 256, options [mptcp add-addr v1 id 1 198.51.100.2 hmac 0xfd2e62517888fe29,mptcp dss ack 3007449509], length 0
In <...> Flags [.], ack 1, win 257, options [mptcp add-addr v1-echo id 90 198.51.100.2,mptcp dss ack 3013740213], length 0
^^^ retransmission is stopped here, but 'Address Id' is 90
patched kernel:
Out <...> Flags [.], ack 1, win 256, options [mptcp add-addr v1 id 1 198.51.100.2 hmac 0x1cf372d59e05f4b8,mptcp dss ack 3007449509], length 0
In <...> Flags [.], ack 1, win 257, options [mptcp add-addr v1-echo id 1 1.2.3.4,mptcp dss ack 1672384568], length 0
Out <...> Flags [.], ack 1, win 256, options [mptcp add-addr v1 id 1 198.51.100.2 hmac 0x1cf372d59e05f4b8,mptcp dss ack 3007449509], length 0
In <...> Flags [.], ack 1, win 257, options [mptcp add-addr v1-echo id 90 198.51.100.2,mptcp dss ack 1672384568], length 0
Out <...> Flags [.], ack 1, win 256, options [mptcp add-addr v1 id 1 198.51.100.2 hmac 0x1cf372d59e05f4b8,mptcp dss ack 3007449509], length 0
In <...> Flags [.], ack 1, win 257, options [mptcp add-addr v1-echo id 1 198.51.100.2,mptcp dss ack 1672384568], length 0
^^^ retransmission is stopped here, only when both 'Address Id' and 'IP Address' match
Fixes: 00cfd77b9063 ("mptcp: retransmit ADD_ADDR when timeout")
Signed-off-by: Davide Caratti <dcaratti@redhat.com>
---
net/mptcp/pm_netlink.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/net/mptcp/pm_netlink.c b/net/mptcp/pm_netlink.c
index d094588afad8..4318d4f5902e 100644
--- a/net/mptcp/pm_netlink.c
+++ b/net/mptcp/pm_netlink.c
@@ -353,11 +353,11 @@ mptcp_pm_del_add_timer(struct mptcp_sock *msk,
spin_lock_bh(&msk->pm.lock);
entry = mptcp_lookup_anno_list_by_saddr(msk, addr);
- if (entry)
+ if (entry && entry->addr.id == addr->id)
entry->retrans_times = ADD_ADDR_RETRANS_MAX;
spin_unlock_bh(&msk->pm.lock);
- if (entry)
+ if (entry && entry->addr.id == addr->id)
sk_stop_timer_sync(sk, &entry->add_timer);
return entry;
--
2.31.1
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH net] mptcp: validate 'id' when stopping the ADD_ADDR retransmit timer
2021-05-11 17:46 [PATCH net] mptcp: validate 'id' when stopping the ADD_ADDR retransmit timer Davide Caratti
@ 2021-05-11 22:39 ` Mat Martineau
2021-05-14 9:13 ` Davide Caratti
0 siblings, 1 reply; 7+ messages in thread
From: Mat Martineau @ 2021-05-11 22:39 UTC (permalink / raw)
To: Davide Caratti; +Cc: mptcp, Geliang Tang, Matthieu Baerts
[-- Attachment #1: Type: text/plain, Size: 3528 bytes --]
On Tue, 11 May 2021, Davide Caratti wrote:
> when Linux receives an echo-ed ADD_ADDR, it checks the IP address against
> the list of "announced" addresses. In case of a positive match, the timer
> that handles retransmissions is stopped regardless of the 'Address Id' in
> the received packet: this behaviour does not comply with RFC8684 §3.4.1.
>
> Fix it by validating the 'Address Id' in received echo-ed ADD_ADDRs.
> Tested using packetdrill, with the following captured output:
>
> unpatched kernel:
>
> Out <...> Flags [.], ack 1, win 256, options [mptcp add-addr v1 id 1 198.51.100.2 hmac 0xfd2e62517888fe29,mptcp dss ack 3007449509], length 0
> In <...> Flags [.], ack 1, win 257, options [mptcp add-addr v1-echo id 1 1.2.3.4,mptcp dss ack 3013740213], length 0
> Out <...> Flags [.], ack 1, win 256, options [mptcp add-addr v1 id 1 198.51.100.2 hmac 0xfd2e62517888fe29,mptcp dss ack 3007449509], length 0
> In <...> Flags [.], ack 1, win 257, options [mptcp add-addr v1-echo id 90 198.51.100.2,mptcp dss ack 3013740213], length 0
> ^^^ retransmission is stopped here, but 'Address Id' is 90
>
> patched kernel:
>
> Out <...> Flags [.], ack 1, win 256, options [mptcp add-addr v1 id 1 198.51.100.2 hmac 0x1cf372d59e05f4b8,mptcp dss ack 3007449509], length 0
> In <...> Flags [.], ack 1, win 257, options [mptcp add-addr v1-echo id 1 1.2.3.4,mptcp dss ack 1672384568], length 0
> Out <...> Flags [.], ack 1, win 256, options [mptcp add-addr v1 id 1 198.51.100.2 hmac 0x1cf372d59e05f4b8,mptcp dss ack 3007449509], length 0
> In <...> Flags [.], ack 1, win 257, options [mptcp add-addr v1-echo id 90 198.51.100.2,mptcp dss ack 1672384568], length 0
> Out <...> Flags [.], ack 1, win 256, options [mptcp add-addr v1 id 1 198.51.100.2 hmac 0x1cf372d59e05f4b8,mptcp dss ack 3007449509], length 0
> In <...> Flags [.], ack 1, win 257, options [mptcp add-addr v1-echo id 1 198.51.100.2,mptcp dss ack 1672384568], length 0
> ^^^ retransmission is stopped here, only when both 'Address Id' and 'IP Address' match
>
> Fixes: 00cfd77b9063 ("mptcp: retransmit ADD_ADDR when timeout")
> Signed-off-by: Davide Caratti <dcaratti@redhat.com>
> ---
> net/mptcp/pm_netlink.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/net/mptcp/pm_netlink.c b/net/mptcp/pm_netlink.c
> index d094588afad8..4318d4f5902e 100644
> --- a/net/mptcp/pm_netlink.c
> +++ b/net/mptcp/pm_netlink.c
> @@ -353,11 +353,11 @@ mptcp_pm_del_add_timer(struct mptcp_sock *msk,
>
> spin_lock_bh(&msk->pm.lock);
> entry = mptcp_lookup_anno_list_by_saddr(msk, addr);
> - if (entry)
> + if (entry && entry->addr.id == addr->id)
> entry->retrans_times = ADD_ADDR_RETRANS_MAX;
> spin_unlock_bh(&msk->pm.lock);
>
> - if (entry)
> + if (entry && entry->addr.id == addr->id)
> sk_stop_timer_sync(sk, &entry->add_timer);
>
> return entry;
Hi Davide -
The code path for ADD_ADDR echo ignores the return value of
mptcp_pm_del_add_timer(), so there's no chance there of leaving
entry->add_timer running if the addr.id doesn't match.
The function is also called by remove_anno_list_by_saddr(), which will
free 'entry' if a non-NULL value is returned by mptcp_pm_del_add_timer().
There's more of a chance here that the sk_stop_timer_sync() would be
skipped and entry would be freed anyway. Looking at the various paths to
this in the path manager it seems like it *should* not happen, but I'm not
convinced it's impossible. Did you look at the effect of this change on
these other code paths?
--
Mat Martineau
Intel
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH net] mptcp: validate 'id' when stopping the ADD_ADDR retransmit timer
2021-05-11 22:39 ` Mat Martineau
@ 2021-05-14 9:13 ` Davide Caratti
2021-05-14 18:41 ` Mat Martineau
0 siblings, 1 reply; 7+ messages in thread
From: Davide Caratti @ 2021-05-14 9:13 UTC (permalink / raw)
To: Mat Martineau; +Cc: mptcp, Geliang Tang, Matthieu Baerts
hello Mat, thanks for reviewing!
On Tue, May 11, 2021 at 03:39:14PM -0700, Mat Martineau wrote:
> On Tue, 11 May 2021, Davide Caratti wrote:
>
> > when Linux receives an echo-ed ADD_ADDR, it checks the IP address against
> > the list of "announced" addresses. In case of a positive match, the timer
> > that handles retransmissions is stopped regardless of the 'Address Id' in
> > the received packet: this behaviour does not comply with RFC8684 §3.4.1.
> >
> > Fix it by validating the 'Address Id' in received echo-ed ADD_ADDRs.
> > Tested using packetdrill, with the following captured output:
> >
>
> Hi Davide -
>
> The code path for ADD_ADDR echo ignores the return value of
> mptcp_pm_del_add_timer(), so there's no chance there of leaving
> entry->add_timer running if the addr.id doesn't match.
true, and that's the purpose of this patch. When addr.id doesn't match,
it leaves the timer running to allow another attempt of ADD_ADDR + HMAC.
> The function is also called by remove_anno_list_by_saddr(), which will free
> 'entry' if a non-NULL value is returned by mptcp_pm_del_add_timer(). There's
> more of a chance here that the sk_stop_timer_sync() would be skipped and
> entry would be freed anyway. Looking at the various paths to this in the
> path manager it seems like it *should* not happen, but I'm not convinced
> it's impossible. Did you look at the effect of this change on these other
> code paths?
good point, I only tried packetdrill and the kselftests. I see
mptcp_pm_remove_anno_addr()
remove_anno_list_by_saddr()
and its callers: in this case,either 'addr.id' is always read from the return
value of
__lookup_addr_by_id(pernet, addr.addr.id);
or by iterating over
&pernet->local_addr_list
, so those should all be 'signal' endpoints previously added by netlink:
the value of addr.id should always match and the timer should always
be stopped.
However, I also don't feel 100% safe - because 'entry' is freed by
remove_anno_list_by_saddr() and the value of 'address.id' is never checked
in addresses_equal(). Maybe it's safer to do soemething like:
347 struct mptcp_pm_add_entry *
348 mptcp_pm_del_add_timer(struct mptcp_sock *msk,
349 struct mptcp_addr_info *addr, bool strict)
350 {
351 struct mptcp_pm_add_entry *entry;
352 struct sock *sk = (struct sock *)msk;
353
354 spin_lock_bh(&msk->pm.lock);
355 entry = mptcp_lookup_anno_list_by_saddr(msk, addr);
356 if (entry && strict && entry->addr.id == addr->id)
357 entry->retrans_times = ADD_ADDR_RETRANS_MAX;
358 spin_unlock_bh(&msk->pm.lock);
359
360 if (entry && strict && entry->addr.id == addr->id)
361 sk_stop_timer_sync(sk, &entry->add_timer);
362
363 return entry;
364 }
so that mptcp_pm_del_add_timer(..., true) is called only in the receive
path of echo-ed ADD_ADDR. WDYT?
thanks!
--
davide
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH net] mptcp: validate 'id' when stopping the ADD_ADDR retransmit timer
2021-05-14 9:13 ` Davide Caratti
@ 2021-05-14 18:41 ` Mat Martineau
0 siblings, 0 replies; 7+ messages in thread
From: Mat Martineau @ 2021-05-14 18:41 UTC (permalink / raw)
To: Davide Caratti; +Cc: mptcp, Geliang Tang, Matthieu Baerts
[-- Attachment #1: Type: text/plain, Size: 3235 bytes --]
On Fri, 14 May 2021, Davide Caratti wrote:
> hello Mat, thanks for reviewing!
>
> On Tue, May 11, 2021 at 03:39:14PM -0700, Mat Martineau wrote:
>> On Tue, 11 May 2021, Davide Caratti wrote:
>>
>>> when Linux receives an echo-ed ADD_ADDR, it checks the IP address against
>>> the list of "announced" addresses. In case of a positive match, the timer
>>> that handles retransmissions is stopped regardless of the 'Address Id' in
>>> the received packet: this behaviour does not comply with RFC8684 §3.4.1.
>>>
>>> Fix it by validating the 'Address Id' in received echo-ed ADD_ADDRs.
>>> Tested using packetdrill, with the following captured output:
>>>
>>
>> Hi Davide -
>>
>> The code path for ADD_ADDR echo ignores the return value of
>> mptcp_pm_del_add_timer(), so there's no chance there of leaving
>> entry->add_timer running if the addr.id doesn't match.
>
> true, and that's the purpose of this patch. When addr.id doesn't match,
> it leaves the timer running to allow another attempt of ADD_ADDR + HMAC.
>
>> The function is also called by remove_anno_list_by_saddr(), which will free
>> 'entry' if a non-NULL value is returned by mptcp_pm_del_add_timer(). There's
>> more of a chance here that the sk_stop_timer_sync() would be skipped and
>> entry would be freed anyway. Looking at the various paths to this in the
>> path manager it seems like it *should* not happen, but I'm not convinced
>> it's impossible. Did you look at the effect of this change on these other
>> code paths?
>
> good point, I only tried packetdrill and the kselftests. I see
>
> mptcp_pm_remove_anno_addr()
> remove_anno_list_by_saddr()
>
> and its callers: in this case,either 'addr.id' is always read from the return
> value of
>
> __lookup_addr_by_id(pernet, addr.addr.id);
>
> or by iterating over
>
> &pernet->local_addr_list
>
> , so those should all be 'signal' endpoints previously added by netlink:
> the value of addr.id should always match and the timer should always
> be stopped.
>
> However, I also don't feel 100% safe - because 'entry' is freed by
> remove_anno_list_by_saddr() and the value of 'address.id' is never checked
> in addresses_equal(). Maybe it's safer to do soemething like:
>
> 347 struct mptcp_pm_add_entry *
> 348 mptcp_pm_del_add_timer(struct mptcp_sock *msk,
> 349 struct mptcp_addr_info *addr, bool strict)
> 350 {
> 351 struct mptcp_pm_add_entry *entry;
> 352 struct sock *sk = (struct sock *)msk;
> 353
> 354 spin_lock_bh(&msk->pm.lock);
> 355 entry = mptcp_lookup_anno_list_by_saddr(msk, addr);
> 356 if (entry && strict && entry->addr.id == addr->id)
> 357 entry->retrans_times = ADD_ADDR_RETRANS_MAX;
> 358 spin_unlock_bh(&msk->pm.lock);
> 359
> 360 if (entry && strict && entry->addr.id == addr->id)
> 361 sk_stop_timer_sync(sk, &entry->add_timer);
> 362
> 363 return entry;
> 364 }
>
>
> so that mptcp_pm_del_add_timer(..., true) is called only in the receive
> path of echo-ed ADD_ADDR. WDYT?
>
I think that's the right general direction, but (entry && (!strict ||
entry->addr.id == addr->id)) is the logic needed for the !strict callers
to work like before.
--
Mat Martineau
Intel
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH net] mptcp: validate 'id' when stopping the ADD_ADDR retransmit timer
2021-05-18 17:36 Davide Caratti
2021-05-18 23:06 ` Mat Martineau
@ 2021-05-20 18:39 ` Matthieu Baerts
1 sibling, 0 replies; 7+ messages in thread
From: Matthieu Baerts @ 2021-05-20 18:39 UTC (permalink / raw)
To: Davide Caratti, mptcp; +Cc: Geliang Tang, Mat Martineau
Hi Davide, Mat,
On 18/05/2021 19:36, Davide Caratti wrote:
> when Linux receives an echo-ed ADD_ADDR, it checks the IP address against
> the list of "announced" addresses. In case of a positive match, the timer
> that handles retransmissions is stopped regardless of the 'Address Id' in
> the received packet: this behaviour does not comply with RFC8684 §3.4.1.
>
> Fix it by validating the 'Address Id' in received echo-ed ADD_ADDRs.
Thank you for the patch and the review!
- 64705a88e6f0: mptcp: validate 'id' when stopping the ADD_ADDR
retransmit timer
- Results: bffd86183cb5..02925519e50e
Builds and tests are now in progress:
https://cirrus-ci.com/github/multipath-tcp/mptcp_net-next/export/20210520T183902
https://github.com/multipath-tcp/mptcp_net-next/actions/workflows/build-validation.yml?query=branch:export/20210520T183902
Cheers,
Matt
--
Tessares | Belgium | Hybrid Access Solutions
www.tessares.net
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH net] mptcp: validate 'id' when stopping the ADD_ADDR retransmit timer
2021-05-18 17:36 Davide Caratti
@ 2021-05-18 23:06 ` Mat Martineau
2021-05-20 18:39 ` Matthieu Baerts
1 sibling, 0 replies; 7+ messages in thread
From: Mat Martineau @ 2021-05-18 23:06 UTC (permalink / raw)
To: Davide Caratti; +Cc: mptcp, Geliang Tang
[-- Attachment #1: Type: text/plain, Size: 4803 bytes --]
On Tue, 18 May 2021, Davide Caratti wrote:
> when Linux receives an echo-ed ADD_ADDR, it checks the IP address against
> the list of "announced" addresses. In case of a positive match, the timer
> that handles retransmissions is stopped regardless of the 'Address Id' in
> the received packet: this behaviour does not comply with RFC8684 §3.4.1.
>
> Fix it by validating the 'Address Id' in received echo-ed ADD_ADDRs.
> Tested using packetdrill, with the following captured output:
>
> unpatched kernel:
>
> Out <...> Flags [.], ack 1, win 256, options [mptcp add-addr v1 id 1 198.51.100.2 hmac 0xfd2e62517888fe29,mptcp dss ack 3007449509], length 0
> In <...> Flags [.], ack 1, win 257, options [mptcp add-addr v1-echo id 1 1.2.3.4,mptcp dss ack 3013740213], length 0
> Out <...> Flags [.], ack 1, win 256, options [mptcp add-addr v1 id 1 198.51.100.2 hmac 0xfd2e62517888fe29,mptcp dss ack 3007449509], length 0
> In <...> Flags [.], ack 1, win 257, options [mptcp add-addr v1-echo id 90 198.51.100.2,mptcp dss ack 3013740213], length 0
> ^^^ retransmission is stopped here, but 'Address Id' is 90
>
> patched kernel:
>
> Out <...> Flags [.], ack 1, win 256, options [mptcp add-addr v1 id 1 198.51.100.2 hmac 0x1cf372d59e05f4b8,mptcp dss ack 3007449509], length 0
> In <...> Flags [.], ack 1, win 257, options [mptcp add-addr v1-echo id 1 1.2.3.4,mptcp dss ack 1672384568], length 0
> Out <...> Flags [.], ack 1, win 256, options [mptcp add-addr v1 id 1 198.51.100.2 hmac 0x1cf372d59e05f4b8,mptcp dss ack 3007449509], length 0
> In <...> Flags [.], ack 1, win 257, options [mptcp add-addr v1-echo id 90 198.51.100.2,mptcp dss ack 1672384568], length 0
> Out <...> Flags [.], ack 1, win 256, options [mptcp add-addr v1 id 1 198.51.100.2 hmac 0x1cf372d59e05f4b8,mptcp dss ack 3007449509], length 0
> In <...> Flags [.], ack 1, win 257, options [mptcp add-addr v1-echo id 1 198.51.100.2,mptcp dss ack 1672384568], length 0
> ^^^ retransmission is stopped here, only when both 'Address Id' and 'IP Address' match
>
> Fixes: 00cfd77b9063 ("mptcp: retransmit ADD_ADDR when timeout")
> Signed-off-by: Davide Caratti <dcaratti@redhat.com>
> ---
> net/mptcp/options.c | 2 +-
> net/mptcp/pm_netlink.c | 8 ++++----
> net/mptcp/protocol.h | 2 +-
> 3 files changed, 6 insertions(+), 6 deletions(-)
Thanks Davide - looks good to me.
Reviewed-by: Mat Martineau <mathew.j.martineau@linux.intel.com>
>
> diff --git a/net/mptcp/options.c b/net/mptcp/options.c
> index 3428c163299b..55732b8d7e08 100644
> --- a/net/mptcp/options.c
> +++ b/net/mptcp/options.c
> @@ -1067,7 +1067,7 @@ void mptcp_incoming_options(struct sock *sk, struct sk_buff *skb)
> MPTCP_INC_STATS(sock_net(sk), MPTCP_MIB_ADDADDR);
> } else {
> mptcp_pm_add_addr_echoed(msk, &mp_opt.addr);
> - mptcp_pm_del_add_timer(msk, &mp_opt.addr);
> + mptcp_pm_del_add_timer(msk, &mp_opt.addr, true);
> MPTCP_INC_STATS(sock_net(sk), MPTCP_MIB_ECHOADD);
> }
>
> diff --git a/net/mptcp/pm_netlink.c b/net/mptcp/pm_netlink.c
> index d094588afad8..09722598994d 100644
> --- a/net/mptcp/pm_netlink.c
> +++ b/net/mptcp/pm_netlink.c
> @@ -346,18 +346,18 @@ static void mptcp_pm_add_timer(struct timer_list *timer)
>
> struct mptcp_pm_add_entry *
> mptcp_pm_del_add_timer(struct mptcp_sock *msk,
> - struct mptcp_addr_info *addr)
> + struct mptcp_addr_info *addr, bool check_id)
> {
> struct mptcp_pm_add_entry *entry;
> struct sock *sk = (struct sock *)msk;
>
> spin_lock_bh(&msk->pm.lock);
> entry = mptcp_lookup_anno_list_by_saddr(msk, addr);
> - if (entry)
> + if (entry && (!check_id || entry->addr.id == addr->id))
> entry->retrans_times = ADD_ADDR_RETRANS_MAX;
> spin_unlock_bh(&msk->pm.lock);
>
> - if (entry)
> + if (entry && (!check_id || entry->addr.id == addr->id))
> sk_stop_timer_sync(sk, &entry->add_timer);
>
> return entry;
> @@ -1070,7 +1070,7 @@ static bool remove_anno_list_by_saddr(struct mptcp_sock *msk,
> {
> struct mptcp_pm_add_entry *entry;
>
> - entry = mptcp_pm_del_add_timer(msk, addr);
> + entry = mptcp_pm_del_add_timer(msk, addr, false);
> if (entry) {
> list_del(&entry->list);
> kfree(entry);
> diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
> index 868e878af526..16e50caf200e 100644
> --- a/net/mptcp/protocol.h
> +++ b/net/mptcp/protocol.h
> @@ -693,7 +693,7 @@ void mptcp_pm_free_anno_list(struct mptcp_sock *msk);
> bool mptcp_pm_sport_in_anno_list(struct mptcp_sock *msk, const struct sock *sk);
> struct mptcp_pm_add_entry *
> mptcp_pm_del_add_timer(struct mptcp_sock *msk,
> - struct mptcp_addr_info *addr);
> + struct mptcp_addr_info *addr, bool check_id);
> struct mptcp_pm_add_entry *
> mptcp_lookup_anno_list_by_saddr(struct mptcp_sock *msk,
> struct mptcp_addr_info *addr);
> --
> 2.31.1
>
>
--
Mat Martineau
Intel
^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH net] mptcp: validate 'id' when stopping the ADD_ADDR retransmit timer
@ 2021-05-18 17:36 Davide Caratti
2021-05-18 23:06 ` Mat Martineau
2021-05-20 18:39 ` Matthieu Baerts
0 siblings, 2 replies; 7+ messages in thread
From: Davide Caratti @ 2021-05-18 17:36 UTC (permalink / raw)
To: mptcp; +Cc: Geliang Tang, Mat Martineau
when Linux receives an echo-ed ADD_ADDR, it checks the IP address against
the list of "announced" addresses. In case of a positive match, the timer
that handles retransmissions is stopped regardless of the 'Address Id' in
the received packet: this behaviour does not comply with RFC8684 §3.4.1.
Fix it by validating the 'Address Id' in received echo-ed ADD_ADDRs.
Tested using packetdrill, with the following captured output:
unpatched kernel:
Out <...> Flags [.], ack 1, win 256, options [mptcp add-addr v1 id 1 198.51.100.2 hmac 0xfd2e62517888fe29,mptcp dss ack 3007449509], length 0
In <...> Flags [.], ack 1, win 257, options [mptcp add-addr v1-echo id 1 1.2.3.4,mptcp dss ack 3013740213], length 0
Out <...> Flags [.], ack 1, win 256, options [mptcp add-addr v1 id 1 198.51.100.2 hmac 0xfd2e62517888fe29,mptcp dss ack 3007449509], length 0
In <...> Flags [.], ack 1, win 257, options [mptcp add-addr v1-echo id 90 198.51.100.2,mptcp dss ack 3013740213], length 0
^^^ retransmission is stopped here, but 'Address Id' is 90
patched kernel:
Out <...> Flags [.], ack 1, win 256, options [mptcp add-addr v1 id 1 198.51.100.2 hmac 0x1cf372d59e05f4b8,mptcp dss ack 3007449509], length 0
In <...> Flags [.], ack 1, win 257, options [mptcp add-addr v1-echo id 1 1.2.3.4,mptcp dss ack 1672384568], length 0
Out <...> Flags [.], ack 1, win 256, options [mptcp add-addr v1 id 1 198.51.100.2 hmac 0x1cf372d59e05f4b8,mptcp dss ack 3007449509], length 0
In <...> Flags [.], ack 1, win 257, options [mptcp add-addr v1-echo id 90 198.51.100.2,mptcp dss ack 1672384568], length 0
Out <...> Flags [.], ack 1, win 256, options [mptcp add-addr v1 id 1 198.51.100.2 hmac 0x1cf372d59e05f4b8,mptcp dss ack 3007449509], length 0
In <...> Flags [.], ack 1, win 257, options [mptcp add-addr v1-echo id 1 198.51.100.2,mptcp dss ack 1672384568], length 0
^^^ retransmission is stopped here, only when both 'Address Id' and 'IP Address' match
Fixes: 00cfd77b9063 ("mptcp: retransmit ADD_ADDR when timeout")
Signed-off-by: Davide Caratti <dcaratti@redhat.com>
---
net/mptcp/options.c | 2 +-
net/mptcp/pm_netlink.c | 8 ++++----
net/mptcp/protocol.h | 2 +-
3 files changed, 6 insertions(+), 6 deletions(-)
diff --git a/net/mptcp/options.c b/net/mptcp/options.c
index 3428c163299b..55732b8d7e08 100644
--- a/net/mptcp/options.c
+++ b/net/mptcp/options.c
@@ -1067,7 +1067,7 @@ void mptcp_incoming_options(struct sock *sk, struct sk_buff *skb)
MPTCP_INC_STATS(sock_net(sk), MPTCP_MIB_ADDADDR);
} else {
mptcp_pm_add_addr_echoed(msk, &mp_opt.addr);
- mptcp_pm_del_add_timer(msk, &mp_opt.addr);
+ mptcp_pm_del_add_timer(msk, &mp_opt.addr, true);
MPTCP_INC_STATS(sock_net(sk), MPTCP_MIB_ECHOADD);
}
diff --git a/net/mptcp/pm_netlink.c b/net/mptcp/pm_netlink.c
index d094588afad8..09722598994d 100644
--- a/net/mptcp/pm_netlink.c
+++ b/net/mptcp/pm_netlink.c
@@ -346,18 +346,18 @@ static void mptcp_pm_add_timer(struct timer_list *timer)
struct mptcp_pm_add_entry *
mptcp_pm_del_add_timer(struct mptcp_sock *msk,
- struct mptcp_addr_info *addr)
+ struct mptcp_addr_info *addr, bool check_id)
{
struct mptcp_pm_add_entry *entry;
struct sock *sk = (struct sock *)msk;
spin_lock_bh(&msk->pm.lock);
entry = mptcp_lookup_anno_list_by_saddr(msk, addr);
- if (entry)
+ if (entry && (!check_id || entry->addr.id == addr->id))
entry->retrans_times = ADD_ADDR_RETRANS_MAX;
spin_unlock_bh(&msk->pm.lock);
- if (entry)
+ if (entry && (!check_id || entry->addr.id == addr->id))
sk_stop_timer_sync(sk, &entry->add_timer);
return entry;
@@ -1070,7 +1070,7 @@ static bool remove_anno_list_by_saddr(struct mptcp_sock *msk,
{
struct mptcp_pm_add_entry *entry;
- entry = mptcp_pm_del_add_timer(msk, addr);
+ entry = mptcp_pm_del_add_timer(msk, addr, false);
if (entry) {
list_del(&entry->list);
kfree(entry);
diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
index 868e878af526..16e50caf200e 100644
--- a/net/mptcp/protocol.h
+++ b/net/mptcp/protocol.h
@@ -693,7 +693,7 @@ void mptcp_pm_free_anno_list(struct mptcp_sock *msk);
bool mptcp_pm_sport_in_anno_list(struct mptcp_sock *msk, const struct sock *sk);
struct mptcp_pm_add_entry *
mptcp_pm_del_add_timer(struct mptcp_sock *msk,
- struct mptcp_addr_info *addr);
+ struct mptcp_addr_info *addr, bool check_id);
struct mptcp_pm_add_entry *
mptcp_lookup_anno_list_by_saddr(struct mptcp_sock *msk,
struct mptcp_addr_info *addr);
--
2.31.1
^ permalink raw reply related [flat|nested] 7+ messages in thread
end of thread, other threads:[~2021-05-20 18:39 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-05-11 17:46 [PATCH net] mptcp: validate 'id' when stopping the ADD_ADDR retransmit timer Davide Caratti
2021-05-11 22:39 ` Mat Martineau
2021-05-14 9:13 ` Davide Caratti
2021-05-14 18:41 ` Mat Martineau
2021-05-18 17:36 Davide Caratti
2021-05-18 23:06 ` Mat Martineau
2021-05-20 18:39 ` Matthieu Baerts
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.