| Message ID | 876dfafb9ec7b090a1da4c2af777be20973649ec.1621359292.git.dcaratti@redhat.com |
|---|---|
| State | Accepted, archived |
| Headers | show |
| Series | [net] mptcp: validate 'id' when stopping the ADD_ADDR retransmit timer | expand |
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
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
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);
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(-)