| Message ID | tencent_28A52E260A59791DE3E6C44846805D812005@qq.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Geliang Tang |
| Headers | show |
| Series | [1/3] mptcp: fix invalid addr occupy 'add_addr_accepted' | expand |
Hi Gang, On 09/01/2025 14:32, Gang Yan wrote: > From: Gang Yan <yangang@kylinos.cn> > > This patch fixes an issue where an invalid address is announced > as a signal, the 'add_addr_accepted' is incorrectly added twice. > If reach the limits, even the invalid connection is removed from > conn_list by mptcp_worker, the variable is not updated, so the > available address can not be added. Detail: I don't know if this is a fix for an issue (== fix for stable), or something new. If it is a fix, it should be for stable, with a 'Fixes' tag. But I don't know if we want that in stable. From what I understand, it can help a client when interacting with a wrongly configured server. But it will not fix the server's behaviour. > When 'ADD_ADDR' adds an invalid address in the LAN, it will trigger > the 'tcp_done_with_error' at the TCP level due to 'icmp_unreach'. > At this point, 'RETRANS_ADDR' will increment 'add_addr_accepted' again. Do you mean that each time a SYN + MP_JOIN is retransmitted, and if it was created upon the reception of an ADD_ADDR, the msk->pm.add_addr_accepted counter is incremented? If yes, can you show me via which path this is done please? I'm not sure to understand how we can get this. > This patch is also helpful for issue#498. Can you give a bit more details about how the issue is being fixed here please? e.g. what's happening after tcp_done_with_error()? e.g. the subflow will be closed via (...). It is then possible to hook there to check (...) and decrement the counter only if (...). > > Closes: https://github.com/multipath-tcp/mptcp_net-next/issues/498 > Signed-off-by: Gang Yan <yangang@kylinos.cn> > --- > net/mptcp/pm_netlink.c | 25 +++++++++++++++++++++++++ > net/mptcp/protocol.c | 3 +++ > net/mptcp/protocol.h | 2 ++ > 3 files changed, 30 insertions(+) > > diff --git a/net/mptcp/pm_netlink.c b/net/mptcp/pm_netlink.c > index 618289aac0ab..63b5c1d6d87a 100644 > --- a/net/mptcp/pm_netlink.c > +++ b/net/mptcp/pm_netlink.c > @@ -958,6 +958,31 @@ void mptcp_pm_nl_work(struct mptcp_sock *msk) > spin_unlock_bh(&msk->pm.lock); > } > > +void mptcp_pm_subflow_closed_external(struct mptcp_sock *msk, > + struct mptcp_subflow_context *subflow) > +{ > + u8 remote_id = READ_ONCE(subflow->remote_id); > + s16 local_id = READ_ONCE(subflow->local_id); > + struct mptcp_subflow_context *iter; > + > + if (!subflow->request_join || !remote_id) It might be good to add a comment here above to clearly explain which cases are included and/or excluded here. At least mentioning that you are looking at the subflows that have been created upon the reception of an ADD_ADDR, which means a requested MPJ to a remote ID > 0. (also, please use 'remote_id > 0', that's clearer in this case, even if it is the same) > + return; > + > + mptcp_for_each_subflow(msk, iter) { > + u8 iter_rmtid = READ_ONCE(iter->remote_id); > + s16 iter_locid = READ_ONCE(iter->local_id); > + > + if (remote_id == iter_rmtid && iter->request_join && > + local_id != iter_locid) > + return; Then please add a comment here above as well: you are checking if there are other subflows connected to the same remote ID. Saying that, would it not be better to check that like that? mptcp_for_each_subflow(msk, iter) { if (subflow == iter) continue; /* other subflows created from the same announced ADD_ADDR? */ if (iter->request_join && remote_id == READ_ONCE(iter->remote_id)) return; } if (!WARN_ON_ONCE(msk->pm.add_addr_accepted == 0)) return; /* only decr for the last subflow attached to the accepted ADD_ADDR */ (...) > + } > + > + spin_lock_bh(&msk->pm.lock); > + if (--msk->pm.add_addr_accepted < mptcp_pm_get_add_addr_accept_max(msk)) > + WRITE_ONCE(msk->pm.accept_addr, true); > + spin_unlock_bh(&msk->pm.lock); > +} > + > static bool address_use_port(struct mptcp_pm_addr_entry *entry) > { > return (entry->flags & > diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c > index 21bc3586c33e..93c832d7feed 100644 > --- a/net/mptcp/protocol.c > +++ b/net/mptcp/protocol.c > @@ -2569,6 +2569,9 @@ static void __mptcp_close_subflow(struct sock *sk) > continue; > > mptcp_close_ssk(sk, ssk, subflow); > + > + if (mptcp_pm_is_kernel(msk)) > + mptcp_pm_subflow_closed_external(msk, subflow); Detail: usually, the core call generic PM functions (in pm.c), which calls the proper PM, e.g. protocol.c → pm.c → pm_netlink.c → or pm_userspace.c So: __mptcp_close_subflow() → mptcp_pm_subflow_closed_external() → if mptcp_pm_is_kernel(): mptcp_pm_nl_subflow_closed_external() > } > > } > diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h > index b4c72a73594f..2923a8cafd91 100644 > --- a/net/mptcp/protocol.h > +++ b/net/mptcp/protocol.h > @@ -1148,6 +1148,8 @@ unsigned int mptcp_pm_get_add_addr_signal_max(const struct mptcp_sock *msk); > unsigned int mptcp_pm_get_add_addr_accept_max(const struct mptcp_sock *msk); > unsigned int mptcp_pm_get_subflows_max(const struct mptcp_sock *msk); > unsigned int mptcp_pm_get_local_addr_max(const struct mptcp_sock *msk); > +void mptcp_pm_subflow_closed_external(struct mptcp_sock *msk, > + struct mptcp_subflow_context *closed_subflow); > > /* called under PM lock */ > static inline void __mptcp_pm_close_subflow(struct mptcp_sock *msk) Cheers, Matt
diff --git a/net/mptcp/pm_netlink.c b/net/mptcp/pm_netlink.c index 618289aac0ab..63b5c1d6d87a 100644 --- a/net/mptcp/pm_netlink.c +++ b/net/mptcp/pm_netlink.c @@ -958,6 +958,31 @@ void mptcp_pm_nl_work(struct mptcp_sock *msk) spin_unlock_bh(&msk->pm.lock); } +void mptcp_pm_subflow_closed_external(struct mptcp_sock *msk, + struct mptcp_subflow_context *subflow) +{ + u8 remote_id = READ_ONCE(subflow->remote_id); + s16 local_id = READ_ONCE(subflow->local_id); + struct mptcp_subflow_context *iter; + + if (!subflow->request_join || !remote_id) + return; + + mptcp_for_each_subflow(msk, iter) { + u8 iter_rmtid = READ_ONCE(iter->remote_id); + s16 iter_locid = READ_ONCE(iter->local_id); + + if (remote_id == iter_rmtid && iter->request_join && + local_id != iter_locid) + return; + } + + spin_lock_bh(&msk->pm.lock); + if (--msk->pm.add_addr_accepted < mptcp_pm_get_add_addr_accept_max(msk)) + WRITE_ONCE(msk->pm.accept_addr, true); + spin_unlock_bh(&msk->pm.lock); +} + static bool address_use_port(struct mptcp_pm_addr_entry *entry) { return (entry->flags & diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c index 21bc3586c33e..93c832d7feed 100644 --- a/net/mptcp/protocol.c +++ b/net/mptcp/protocol.c @@ -2569,6 +2569,9 @@ static void __mptcp_close_subflow(struct sock *sk) continue; mptcp_close_ssk(sk, ssk, subflow); + + if (mptcp_pm_is_kernel(msk)) + mptcp_pm_subflow_closed_external(msk, subflow); } } diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h index b4c72a73594f..2923a8cafd91 100644 --- a/net/mptcp/protocol.h +++ b/net/mptcp/protocol.h @@ -1148,6 +1148,8 @@ unsigned int mptcp_pm_get_add_addr_signal_max(const struct mptcp_sock *msk); unsigned int mptcp_pm_get_add_addr_accept_max(const struct mptcp_sock *msk); unsigned int mptcp_pm_get_subflows_max(const struct mptcp_sock *msk); unsigned int mptcp_pm_get_local_addr_max(const struct mptcp_sock *msk); +void mptcp_pm_subflow_closed_external(struct mptcp_sock *msk, + struct mptcp_subflow_context *closed_subflow); /* called under PM lock */ static inline void __mptcp_pm_close_subflow(struct mptcp_sock *msk)