* [MPTCP] Re: [PATCH net-next] mptcp: add REMOVE_ADDR support v1
@ 2020-07-22 9:02 Geliang Tang
0 siblings, 0 replies; 2+ messages in thread
From: Geliang Tang @ 2020-07-22 9:02 UTC (permalink / raw)
To: mptcp
[-- Attachment #1: Type: text/plain, Size: 18358 bytes --]
On Thu, Jul 16, 2020 at 04:41:52PM +0200, Paolo Abeni wrote:
> Hi,
>
> On Thu, 2020-07-16 at 16:45 +0800, Geliang Tang wrote:
> > Add REMOVE_ADDR support.
> >
> > Signed-off-by: Geliang Tang <geliangtang(a)gmail.com>
> > ---
> > This is the first version of REMOVE_ADDR support. It's not finished yet but
> > it works. I'll add selftest case and patch commit description later. Please
> > give me some advice on how to improve it. Thanks.
>
> I think this code could be split in a few patches:
>
> * one renaming the existing add_addr related functions
> * one doing the rm_addr option parsing/writing
> * one implementing the rm addr logic
>
> This latter part is the least clear to me. If I read the code
> correctly, the idea is having each msk checking in the data path if any
> address has been removed via the PM netlink APIs. Is the above correct?
>
> I think it would be better triggering the remove addr in the reverse
> way: when the PM netlink removes an address it should traverse all the
> existing msk sockets - using the recently introduced
> mptcp_token_iter_next() helper - and set 'rm_addr_signal' on the
> relevant sockets - the ones that already announced it.
>
> This latter condition is possibly a bit hard to track. As far as I read
> the RFC, we could use a simpler one: send the RM_ADDR on the msk with a
> subflows using the relevant addr.
>
> The idea behind the above is that servers should not usually send
> RM_ADDR, while clients should have a limited number of open MPTCP
> connections, so traversing all the token table should not be a
> problem.
>
> Not sure if the above is somewhat readable - we can discuss it in the
> mtg soon!
>
> Please see also:
>
> https://github.com/multipath-tcp/mptcp_net-next/issues/19
>
> which is somewhat related.
>
> Cheers,
>
> Paolo
Hi Paolo,
Thanks for your suggestions. I have updated my patches, and sent out patchset v2 to you.
-Geliang
>
> > ---
> > net/mptcp/options.c | 49 +++++++++++++++++++++++++----
> > net/mptcp/pm.c | 71 ++++++++++++++++++++++++++++++++++++++----
> > net/mptcp/pm_netlink.c | 47 +++++++++++++++++++++++++++-
> > net/mptcp/protocol.c | 12 +++++--
> > net/mptcp/protocol.h | 26 +++++++++++++---
> > net/mptcp/subflow.c | 1 +
> > 6 files changed, 187 insertions(+), 19 deletions(-)
> >
> > diff --git a/net/mptcp/options.c b/net/mptcp/options.c
> > index 19707c07efc1..0d4d334fbc08 100644
> > --- a/net/mptcp/options.c
> > +++ b/net/mptcp/options.c
> > @@ -572,10 +572,10 @@ static u64 add_addr6_generate_hmac(u64 key1, u64 key2, u8 addr_id,
> > }
> > #endif
> >
> > -static bool mptcp_established_options_addr(struct sock *sk,
> > - unsigned int *size,
> > - unsigned int remaining,
> > - struct mptcp_out_options *opts)
> > +static bool mptcp_established_options_add_addr(struct sock *sk,
> > + unsigned int *size,
> > + unsigned int remaining,
> > + struct mptcp_out_options *opts)
> > {
> > struct mptcp_subflow_context *subflow = mptcp_subflow_ctx(sk);
> > struct mptcp_sock *msk = mptcp_sk(subflow->conn);
> > @@ -583,7 +583,7 @@ static bool mptcp_established_options_addr(struct sock *sk,
> > int len;
> >
> > if (!mptcp_pm_should_signal(msk) ||
> > - !(mptcp_pm_addr_signal(msk, remaining, &saddr)))
> > + !(mptcp_pm_add_addr_signal(msk, remaining, &saddr)))
> > return false;
> >
> > len = mptcp_add_addr_len(saddr.family);
> > @@ -615,6 +615,31 @@ static bool mptcp_established_options_addr(struct sock *sk,
> > return true;
> > }
> >
> > +static bool mptcp_established_options_rm_addr(struct sock *sk,
> > + unsigned int *size,
> > + unsigned int remaining,
> > + struct mptcp_out_options *opts)
> > +{
> > + struct mptcp_subflow_context *subflow = mptcp_subflow_ctx(sk);
> > + struct mptcp_sock *msk = mptcp_sk(subflow->conn);
> > + u8 rm_id;
> > +
> > + if (!mptcp_pm_should_rm_signal(msk) ||
> > + !(mptcp_pm_rm_addr_signal(msk, remaining, &rm_id)))
> > + return false;
> > +
> > + if (remaining < TCPOLEN_MPTCP_RM_ADDR_BASE)
> > + return false;
> > +
> > + *size = TCPOLEN_MPTCP_RM_ADDR_BASE;
> > + opts->suboptions |= OPTION_MPTCP_RM_ADDR;
> > + opts->rm_id = rm_id;
> > +
> > + pr_debug("rm_id=%d", opts->rm_id);
> > +
> > + return true;
> > +}
> > +
> > bool mptcp_established_options(struct sock *sk, struct sk_buff *skb,
> > unsigned int *size, unsigned int remaining,
> > struct mptcp_out_options *opts)
> > @@ -641,7 +666,13 @@ bool mptcp_established_options(struct sock *sk, struct sk_buff *skb,
> >
> > *size += opt_size;
> > remaining -= opt_size;
> > - if (mptcp_established_options_addr(sk, &opt_size, remaining, opts)) {
> > + if (mptcp_established_options_add_addr(sk, &opt_size, remaining, opts)) {
> > + *size += opt_size;
> > + remaining -= opt_size;
> > + ret = true;
> > + }
> > +
> > + if (mptcp_established_options_rm_addr(sk, &opt_size, remaining, opts)) {
> > *size += opt_size;
> > remaining -= opt_size;
> > ret = true;
> > @@ -729,6 +760,7 @@ static bool check_fully_established(struct mptcp_sock *msk, struct sock *sk,
> > subflow->can_ack = 1;
> >
> > fully_established:
> > + mptcp_pm_addr_update(msk);
> > if (likely(subflow->pm_notified))
> > return true;
> >
> > @@ -845,6 +877,11 @@ void mptcp_incoming_options(struct sock *sk, struct sk_buff *skb,
> > mp_opt.add_addr = 0;
> > }
> >
> > + if (mp_opt.rm_addr) {
> > + mptcp_pm_rm_addr_received(msk, mp_opt.rm_id);
> > + mp_opt.rm_addr = 0;
> > + }
> > +
> > if (!mp_opt.dss)
> > return;
> >
> > diff --git a/net/mptcp/pm.c b/net/mptcp/pm.c
> > index a8ad20559aaa..c811559ca78f 100644
> > --- a/net/mptcp/pm.c
> > +++ b/net/mptcp/pm.c
> > @@ -18,13 +18,17 @@ int mptcp_pm_announce_addr(struct mptcp_sock *msk,
> > pr_debug("msk=%p, local_id=%d", msk, addr->id);
> >
> > msk->pm.local = *addr;
> > - WRITE_ONCE(msk->pm.addr_signal, true);
> > + WRITE_ONCE(msk->pm.add_addr_signal, true);
> > return 0;
> > }
> >
> > int mptcp_pm_remove_addr(struct mptcp_sock *msk, u8 local_id)
> > {
> > - return -ENOTSUPP;
> > + pr_debug("msk=%p, local_id=%d", msk, local_id);
> > +
> > + msk->pm.rm_id = local_id;
> > + WRITE_ONCE(msk->pm.rm_addr_signal, true);
> > + return 0;
> > }
> >
> > int mptcp_pm_remove_subflow(struct mptcp_sock *msk, u8 remote_id)
> > @@ -81,6 +85,24 @@ static bool mptcp_pm_schedule_work(struct mptcp_sock *msk,
> > return true;
> > }
> >
> > +void mptcp_pm_addr_update(struct mptcp_sock *msk)
> > +{
> > + struct mptcp_pm_data *pm = &msk->pm;
> > +
> > + pr_debug("msk=%p", msk);
> > +
> > + /* try to avoid acquiring the lock below */
> > + if (!READ_ONCE(pm->work_pending))
> > + return;
> > +
> > + spin_lock_bh(&pm->lock);
> > +
> > + if (READ_ONCE(pm->work_pending))
> > + mptcp_pm_schedule_work(msk, MPTCP_PM_ADDR_UPDATE);
> > +
> > + spin_unlock_bh(&pm->lock);
> > +}
> > +
> > void mptcp_pm_fully_established(struct mptcp_sock *msk)
> > {
> > struct mptcp_pm_data *pm = &msk->pm;
> > @@ -151,8 +173,8 @@ void mptcp_pm_add_addr_received(struct mptcp_sock *msk,
> >
> > /* path manager helpers */
> >
> > -bool mptcp_pm_addr_signal(struct mptcp_sock *msk, unsigned int remaining,
> > - struct mptcp_addr_info *saddr)
> > +bool mptcp_pm_add_addr_signal(struct mptcp_sock *msk, unsigned int remaining,
> > + struct mptcp_addr_info *saddr)
> > {
> > int ret = false;
> >
> > @@ -166,7 +188,42 @@ bool mptcp_pm_addr_signal(struct mptcp_sock *msk, unsigned int remaining,
> > goto out_unlock;
> >
> > *saddr = msk->pm.local;
> > - WRITE_ONCE(msk->pm.addr_signal, false);
> > + WRITE_ONCE(msk->pm.add_addr_signal, false);
> > + ret = true;
> > +
> > +out_unlock:
> > + spin_unlock_bh(&msk->pm.lock);
> > + return ret;
> > +}
> > +
> > +void mptcp_pm_rm_addr_received(struct mptcp_sock *msk, u8 rm_id)
> > +{
> > + struct mptcp_pm_data *pm = &msk->pm;
> > +
> > + pr_debug("msk=%p remote_id=%d", msk, rm_id);
> > +
> > + spin_lock_bh(&pm->lock);
> > + mptcp_pm_schedule_work(msk, MPTCP_PM_RM_ADDR_RECEIVED);
> > + pm->rm_id = rm_id;
> > + spin_unlock_bh(&pm->lock);
> > +}
> > +
> > +bool mptcp_pm_rm_addr_signal(struct mptcp_sock *msk, unsigned int remaining,
> > + u8 *rm_id)
> > +{
> > + int ret = false;
> > +
> > + spin_lock_bh(&msk->pm.lock);
> > +
> > + /* double check after the lock is acquired */
> > + if (!mptcp_pm_should_rm_signal(msk))
> > + goto out_unlock;
> > +
> > + if (remaining < TCPOLEN_MPTCP_RM_ADDR_BASE)
> > + goto out_unlock;
> > +
> > + *rm_id = msk->pm.rm_id;
> > + WRITE_ONCE(msk->pm.rm_addr_signal, false);
> > ret = true;
> >
> > out_unlock:
> > @@ -186,9 +243,11 @@ void mptcp_pm_data_init(struct mptcp_sock *msk)
> > msk->pm.local_addr_used = 0;
> > msk->pm.subflows = 0;
> > WRITE_ONCE(msk->pm.work_pending, false);
> > - WRITE_ONCE(msk->pm.addr_signal, false);
> > + WRITE_ONCE(msk->pm.add_addr_signal, false);
> > + WRITE_ONCE(msk->pm.rm_addr_signal, false);
> > WRITE_ONCE(msk->pm.accept_addr, false);
> > WRITE_ONCE(msk->pm.accept_subflow, false);
> > + WRITE_ONCE(msk->pm.addr_updated, false);
> > msk->pm.status = 0;
> >
> > spin_lock_init(&msk->pm.lock);
> > diff --git a/net/mptcp/pm_netlink.c b/net/mptcp/pm_netlink.c
> > index c8820c4156e6..c9933387be09 100644
> > --- a/net/mptcp/pm_netlink.c
> > +++ b/net/mptcp/pm_netlink.c
> > @@ -39,6 +39,7 @@ struct pm_nl_pernet {
> > unsigned int local_addr_max;
> > unsigned int subflows_max;
> > unsigned int next_id;
> > + unsigned int rm_id;
> > };
> >
> > #define MPTCP_PM_ADDR_MAX 8
> > @@ -165,7 +166,7 @@ static void check_work_pending(struct mptcp_sock *msk)
> > {
> > if (msk->pm.add_addr_signaled == msk->pm.add_addr_signal_max &&
> > (msk->pm.local_addr_used == msk->pm.local_addr_max ||
> > - msk->pm.subflows == msk->pm.subflows_max))
> > + msk->pm.subflows == msk->pm.subflows_max) && msk->pm.addr_updated)
> > WRITE_ONCE(msk->pm.work_pending, false);
> > }
> >
> > @@ -196,6 +197,11 @@ static void mptcp_pm_create_subflow_or_signal_addr(struct mptcp_sock *msk)
> > msk->pm.local_addr_used = msk->pm.add_addr_signal_max;
> > }
> >
> > + check_work_pending(msk);
> > + } else if (msk->pm.add_addr_signaled > msk->pm.add_addr_signal_max) {
> > + msk->pm.add_addr_signaled--;
> > + mptcp_pm_remove_addr(msk, pernet->rm_id);
> > +
> > check_work_pending(msk);
> > }
> >
> > @@ -261,6 +267,26 @@ void mptcp_pm_nl_add_addr_received(struct mptcp_sock *msk)
> > spin_lock_bh(&msk->pm.lock);
> > }
> >
> > +void mptcp_pm_nl_rm_addr_received(struct mptcp_sock *msk)
> > +{
> > + struct mptcp_subflow_context *subflow, *tmp;
> > +
> > + pr_debug("remote_id %d", msk->pm.rm_id);
> > +
> > + msk->pm.add_addr_accepted--;
> > + msk->pm.subflows--;
> > + WRITE_ONCE(msk->pm.accept_addr, true);
> > +
> > + list_for_each_entry_safe(subflow, tmp, &msk->conn_list, node) {
> > + struct sock *tcp_sk = mptcp_subflow_tcp_sock(subflow);
> > +
> > + if (msk->pm.rm_id == subflow->remote_id) {
> > + mptcp_subflow_shutdown(tcp_sk, 1, 1, msk->write_seq);
> > + list_del(&subflow->node);
> > + }
> > + }
> > +}
> > +
> > static bool address_use_port(struct mptcp_pm_addr_entry *entry)
> > {
> > return (entry->flags &
> > @@ -354,6 +380,24 @@ int mptcp_pm_nl_get_local_id(struct mptcp_sock *msk, struct sock_common *skc)
> > return ret;
> > }
> >
> > +void mptcp_pm_nl_addr_update(struct mptcp_sock *msk)
> > +{
> > + struct mptcp_pm_data *pm = &msk->pm;
> > + struct pm_nl_pernet *pernet;
> > +
> > + pernet = net_generic(sock_net((struct sock *)msk), pm_nl_pernet_id);
> > +
> > + if (pm->add_addr_signal_max != pernet->add_addr_signal_max) {
> > + pm->add_addr_signal_max = READ_ONCE(pernet->add_addr_signal_max);
> > + pm->add_addr_accept_max = READ_ONCE(pernet->add_addr_accept_max);
> > +
> > + mptcp_pm_create_subflow_or_signal_addr(msk);
> > + WRITE_ONCE(pm->addr_updated, true);
> > + } else {
> > + WRITE_ONCE(pm->addr_updated, false);
> > + }
> > +}
> > +
> > void mptcp_pm_nl_data_init(struct mptcp_sock *msk)
> > {
> > struct mptcp_pm_data *pm = &msk->pm;
> > @@ -541,6 +585,7 @@ static int mptcp_nl_cmd_del_addr(struct sk_buff *skb, struct genl_info *info)
> > pernet->local_addr_max--;
> >
> > pernet->addrs--;
> > + pernet->rm_id = addr.addr.id;
> > list_del_rcu(&entry->list);
> > kfree_rcu(entry, rcu);
> > out:
> > diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
> > index dbe43e0cd734..31b836ed0786 100644
> > --- a/net/mptcp/protocol.c
> > +++ b/net/mptcp/protocol.c
> > @@ -1225,6 +1225,14 @@ static void pm_work(struct mptcp_sock *msk)
> > pm->status &= ~BIT(MPTCP_PM_ADD_ADDR_RECEIVED);
> > mptcp_pm_nl_add_addr_received(msk);
> > }
> > + if (pm->status & BIT(MPTCP_PM_RM_ADDR_RECEIVED)) {
> > + pm->status &= ~BIT(MPTCP_PM_RM_ADDR_RECEIVED);
> > + mptcp_pm_nl_rm_addr_received(msk);
> > + }
> > + if (pm->status & BIT(MPTCP_PM_ADDR_UPDATE)) {
> > + pm->status &= ~BIT(MPTCP_PM_ADDR_UPDATE);
> > + mptcp_pm_nl_addr_update(msk);
> > + }
> > if (pm->status & BIT(MPTCP_PM_ESTABLISHED)) {
> > pm->status &= ~BIT(MPTCP_PM_ESTABLISHED);
> > mptcp_pm_nl_fully_established(msk);
> > @@ -1381,8 +1389,8 @@ static void mptcp_cancel_work(struct sock *sk)
> > sock_put(sk);
> > }
> >
> > -static void mptcp_subflow_shutdown(struct sock *ssk, int how,
> > - bool data_fin_tx_enable, u64 data_fin_tx_seq)
> > +void mptcp_subflow_shutdown(struct sock *ssk, int how,
> > + bool data_fin_tx_enable, u64 data_fin_tx_seq)
> > {
> > lock_sock(ssk);
> >
> > diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
> > index e5baaef5ec89..5587613f5b03 100644
> > --- a/net/mptcp/protocol.h
> > +++ b/net/mptcp/protocol.h
> > @@ -150,6 +150,8 @@ struct mptcp_addr_info {
> >
> > enum mptcp_pm_status {
> > MPTCP_PM_ADD_ADDR_RECEIVED,
> > + MPTCP_PM_RM_ADDR_RECEIVED,
> > + MPTCP_PM_ADDR_UPDATE,
> > MPTCP_PM_ESTABLISHED,
> > MPTCP_PM_SUBFLOW_ESTABLISHED,
> > };
> > @@ -160,11 +162,13 @@ struct mptcp_pm_data {
> >
> > spinlock_t lock; /*protects the whole PM data */
> >
> > - bool addr_signal;
> > + bool add_addr_signal;
> > + bool rm_addr_signal;
> > bool server_side;
> > bool work_pending;
> > bool accept_addr;
> > bool accept_subflow;
> > + bool addr_updated;
> > u8 add_addr_signaled;
> > u8 add_addr_accepted;
> > u8 local_addr_used;
> > @@ -174,6 +178,7 @@ struct mptcp_pm_data {
> > u8 local_addr_max;
> > u8 subflows_max;
> > u8 status;
> > + u8 rm_id;
> > };
> >
> > struct mptcp_data_frag {
> > @@ -344,6 +349,8 @@ mptcp_subflow_get_mapped_dsn(const struct mptcp_subflow_context *subflow)
> > int mptcp_is_enabled(struct net *net);
> > bool mptcp_subflow_data_available(struct sock *sk);
> > void __init mptcp_subflow_init(void);
> > +void mptcp_subflow_shutdown(struct sock *ssk, int how,
> > + bool data_fin_tx_enable, u64 data_fin_tx_seq);
> >
> > /* called with sk socket lock held */
> > int __mptcp_subflow_connect(struct sock *sk, int ifindex,
> > @@ -419,8 +426,10 @@ void mptcp_pm_connection_closed(struct mptcp_sock *msk);
> > void mptcp_pm_subflow_established(struct mptcp_sock *msk,
> > struct mptcp_subflow_context *subflow);
> > void mptcp_pm_subflow_closed(struct mptcp_sock *msk, u8 id);
> > +void mptcp_pm_addr_update(struct mptcp_sock *msk);
> > void mptcp_pm_add_addr_received(struct mptcp_sock *msk,
> > const struct mptcp_addr_info *addr);
> > +void mptcp_pm_rm_addr_received(struct mptcp_sock *msk, u8 rm_id);
> >
> > int mptcp_pm_announce_addr(struct mptcp_sock *msk,
> > const struct mptcp_addr_info *addr);
> > @@ -429,7 +438,12 @@ int mptcp_pm_remove_subflow(struct mptcp_sock *msk, u8 remote_id);
> >
> > static inline bool mptcp_pm_should_signal(struct mptcp_sock *msk)
> > {
> > - return READ_ONCE(msk->pm.addr_signal);
> > + return READ_ONCE(msk->pm.add_addr_signal);
> > +}
> > +
> > +static inline bool mptcp_pm_should_rm_signal(struct mptcp_sock *msk)
> > +{
> > + return READ_ONCE(msk->pm.rm_addr_signal);
> > }
> >
> > static inline unsigned int mptcp_add_addr_len(int family)
> > @@ -439,15 +453,19 @@ static inline unsigned int mptcp_add_addr_len(int family)
> > return TCPOLEN_MPTCP_ADD_ADDR6;
> > }
> >
> > -bool mptcp_pm_addr_signal(struct mptcp_sock *msk, unsigned int remaining,
> > - struct mptcp_addr_info *saddr);
> > +bool mptcp_pm_add_addr_signal(struct mptcp_sock *msk, unsigned int remaining,
> > + struct mptcp_addr_info *saddr);
> > +bool mptcp_pm_rm_addr_signal(struct mptcp_sock *msk, unsigned int remaining,
> > + u8 *rm_id);
> > int mptcp_pm_get_local_id(struct mptcp_sock *msk, struct sock_common *skc);
> >
> > void __init mptcp_pm_nl_init(void);
> > void mptcp_pm_nl_data_init(struct mptcp_sock *msk);
> > +void mptcp_pm_nl_addr_update(struct mptcp_sock *msk);
> > void mptcp_pm_nl_fully_established(struct mptcp_sock *msk);
> > void mptcp_pm_nl_subflow_established(struct mptcp_sock *msk);
> > void mptcp_pm_nl_add_addr_received(struct mptcp_sock *msk);
> > +void mptcp_pm_nl_rm_addr_received(struct mptcp_sock *msk);
> > int mptcp_pm_nl_get_local_id(struct mptcp_sock *msk, struct sock_common *skc);
> >
> > static inline struct mptcp_ext *mptcp_get_ext(struct sk_buff *skb)
> > diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c
> > index 9f7f3772c13c..326c2df256b7 100644
> > --- a/net/mptcp/subflow.c
> > +++ b/net/mptcp/subflow.c
> > @@ -989,6 +989,7 @@ int __mptcp_subflow_connect(struct sock *sk, int ifindex,
> > subflow->remote_key = msk->remote_key;
> > subflow->local_key = msk->local_key;
> > subflow->token = msk->token;
> > + subflow->remote_id = remote->id;
> > mptcp_info2sockaddr(loc, &addr);
> >
> > addrlen = sizeof(struct sockaddr_in);
>
^ permalink raw reply [flat|nested] 2+ messages in thread* [MPTCP] Re: [PATCH net-next] mptcp: add REMOVE_ADDR support v1
@ 2020-07-16 14:41 Paolo Abeni
0 siblings, 0 replies; 2+ messages in thread
From: Paolo Abeni @ 2020-07-16 14:41 UTC (permalink / raw)
To: mptcp
[-- Attachment #1: Type: text/plain, Size: 17183 bytes --]
Hi,
On Thu, 2020-07-16 at 16:45 +0800, Geliang Tang wrote:
> Add REMOVE_ADDR support.
>
> Signed-off-by: Geliang Tang <geliangtang(a)gmail.com>
> ---
> This is the first version of REMOVE_ADDR support. It's not finished yet but
> it works. I'll add selftest case and patch commit description later. Please
> give me some advice on how to improve it. Thanks.
I think this code could be split in a few patches:
* one renaming the existing add_addr related functions
* one doing the rm_addr option parsing/writing
* one implementing the rm addr logic
This latter part is the least clear to me. If I read the code
correctly, the idea is having each msk checking in the data path if any
address has been removed via the PM netlink APIs. Is the above correct?
I think it would be better triggering the remove addr in the reverse
way: when the PM netlink removes an address it should traverse all the
existing msk sockets - using the recently introduced
mptcp_token_iter_next() helper - and set 'rm_addr_signal' on the
relevant sockets - the ones that already announced it.
This latter condition is possibly a bit hard to track. As far as I read
the RFC, we could use a simpler one: send the RM_ADDR on the msk with a
subflows using the relevant addr.
The idea behind the above is that servers should not usually send
RM_ADDR, while clients should have a limited number of open MPTCP
connections, so traversing all the token table should not be a
problem.
Not sure if the above is somewhat readable - we can discuss it in the
mtg soon!
Please see also:
https://github.com/multipath-tcp/mptcp_net-next/issues/19
which is somewhat related.
Cheers,
Paolo
> ---
> net/mptcp/options.c | 49 +++++++++++++++++++++++++----
> net/mptcp/pm.c | 71 ++++++++++++++++++++++++++++++++++++++----
> net/mptcp/pm_netlink.c | 47 +++++++++++++++++++++++++++-
> net/mptcp/protocol.c | 12 +++++--
> net/mptcp/protocol.h | 26 +++++++++++++---
> net/mptcp/subflow.c | 1 +
> 6 files changed, 187 insertions(+), 19 deletions(-)
>
> diff --git a/net/mptcp/options.c b/net/mptcp/options.c
> index 19707c07efc1..0d4d334fbc08 100644
> --- a/net/mptcp/options.c
> +++ b/net/mptcp/options.c
> @@ -572,10 +572,10 @@ static u64 add_addr6_generate_hmac(u64 key1, u64 key2, u8 addr_id,
> }
> #endif
>
> -static bool mptcp_established_options_addr(struct sock *sk,
> - unsigned int *size,
> - unsigned int remaining,
> - struct mptcp_out_options *opts)
> +static bool mptcp_established_options_add_addr(struct sock *sk,
> + unsigned int *size,
> + unsigned int remaining,
> + struct mptcp_out_options *opts)
> {
> struct mptcp_subflow_context *subflow = mptcp_subflow_ctx(sk);
> struct mptcp_sock *msk = mptcp_sk(subflow->conn);
> @@ -583,7 +583,7 @@ static bool mptcp_established_options_addr(struct sock *sk,
> int len;
>
> if (!mptcp_pm_should_signal(msk) ||
> - !(mptcp_pm_addr_signal(msk, remaining, &saddr)))
> + !(mptcp_pm_add_addr_signal(msk, remaining, &saddr)))
> return false;
>
> len = mptcp_add_addr_len(saddr.family);
> @@ -615,6 +615,31 @@ static bool mptcp_established_options_addr(struct sock *sk,
> return true;
> }
>
> +static bool mptcp_established_options_rm_addr(struct sock *sk,
> + unsigned int *size,
> + unsigned int remaining,
> + struct mptcp_out_options *opts)
> +{
> + struct mptcp_subflow_context *subflow = mptcp_subflow_ctx(sk);
> + struct mptcp_sock *msk = mptcp_sk(subflow->conn);
> + u8 rm_id;
> +
> + if (!mptcp_pm_should_rm_signal(msk) ||
> + !(mptcp_pm_rm_addr_signal(msk, remaining, &rm_id)))
> + return false;
> +
> + if (remaining < TCPOLEN_MPTCP_RM_ADDR_BASE)
> + return false;
> +
> + *size = TCPOLEN_MPTCP_RM_ADDR_BASE;
> + opts->suboptions |= OPTION_MPTCP_RM_ADDR;
> + opts->rm_id = rm_id;
> +
> + pr_debug("rm_id=%d", opts->rm_id);
> +
> + return true;
> +}
> +
> bool mptcp_established_options(struct sock *sk, struct sk_buff *skb,
> unsigned int *size, unsigned int remaining,
> struct mptcp_out_options *opts)
> @@ -641,7 +666,13 @@ bool mptcp_established_options(struct sock *sk, struct sk_buff *skb,
>
> *size += opt_size;
> remaining -= opt_size;
> - if (mptcp_established_options_addr(sk, &opt_size, remaining, opts)) {
> + if (mptcp_established_options_add_addr(sk, &opt_size, remaining, opts)) {
> + *size += opt_size;
> + remaining -= opt_size;
> + ret = true;
> + }
> +
> + if (mptcp_established_options_rm_addr(sk, &opt_size, remaining, opts)) {
> *size += opt_size;
> remaining -= opt_size;
> ret = true;
> @@ -729,6 +760,7 @@ static bool check_fully_established(struct mptcp_sock *msk, struct sock *sk,
> subflow->can_ack = 1;
>
> fully_established:
> + mptcp_pm_addr_update(msk);
> if (likely(subflow->pm_notified))
> return true;
>
> @@ -845,6 +877,11 @@ void mptcp_incoming_options(struct sock *sk, struct sk_buff *skb,
> mp_opt.add_addr = 0;
> }
>
> + if (mp_opt.rm_addr) {
> + mptcp_pm_rm_addr_received(msk, mp_opt.rm_id);
> + mp_opt.rm_addr = 0;
> + }
> +
> if (!mp_opt.dss)
> return;
>
> diff --git a/net/mptcp/pm.c b/net/mptcp/pm.c
> index a8ad20559aaa..c811559ca78f 100644
> --- a/net/mptcp/pm.c
> +++ b/net/mptcp/pm.c
> @@ -18,13 +18,17 @@ int mptcp_pm_announce_addr(struct mptcp_sock *msk,
> pr_debug("msk=%p, local_id=%d", msk, addr->id);
>
> msk->pm.local = *addr;
> - WRITE_ONCE(msk->pm.addr_signal, true);
> + WRITE_ONCE(msk->pm.add_addr_signal, true);
> return 0;
> }
>
> int mptcp_pm_remove_addr(struct mptcp_sock *msk, u8 local_id)
> {
> - return -ENOTSUPP;
> + pr_debug("msk=%p, local_id=%d", msk, local_id);
> +
> + msk->pm.rm_id = local_id;
> + WRITE_ONCE(msk->pm.rm_addr_signal, true);
> + return 0;
> }
>
> int mptcp_pm_remove_subflow(struct mptcp_sock *msk, u8 remote_id)
> @@ -81,6 +85,24 @@ static bool mptcp_pm_schedule_work(struct mptcp_sock *msk,
> return true;
> }
>
> +void mptcp_pm_addr_update(struct mptcp_sock *msk)
> +{
> + struct mptcp_pm_data *pm = &msk->pm;
> +
> + pr_debug("msk=%p", msk);
> +
> + /* try to avoid acquiring the lock below */
> + if (!READ_ONCE(pm->work_pending))
> + return;
> +
> + spin_lock_bh(&pm->lock);
> +
> + if (READ_ONCE(pm->work_pending))
> + mptcp_pm_schedule_work(msk, MPTCP_PM_ADDR_UPDATE);
> +
> + spin_unlock_bh(&pm->lock);
> +}
> +
> void mptcp_pm_fully_established(struct mptcp_sock *msk)
> {
> struct mptcp_pm_data *pm = &msk->pm;
> @@ -151,8 +173,8 @@ void mptcp_pm_add_addr_received(struct mptcp_sock *msk,
>
> /* path manager helpers */
>
> -bool mptcp_pm_addr_signal(struct mptcp_sock *msk, unsigned int remaining,
> - struct mptcp_addr_info *saddr)
> +bool mptcp_pm_add_addr_signal(struct mptcp_sock *msk, unsigned int remaining,
> + struct mptcp_addr_info *saddr)
> {
> int ret = false;
>
> @@ -166,7 +188,42 @@ bool mptcp_pm_addr_signal(struct mptcp_sock *msk, unsigned int remaining,
> goto out_unlock;
>
> *saddr = msk->pm.local;
> - WRITE_ONCE(msk->pm.addr_signal, false);
> + WRITE_ONCE(msk->pm.add_addr_signal, false);
> + ret = true;
> +
> +out_unlock:
> + spin_unlock_bh(&msk->pm.lock);
> + return ret;
> +}
> +
> +void mptcp_pm_rm_addr_received(struct mptcp_sock *msk, u8 rm_id)
> +{
> + struct mptcp_pm_data *pm = &msk->pm;
> +
> + pr_debug("msk=%p remote_id=%d", msk, rm_id);
> +
> + spin_lock_bh(&pm->lock);
> + mptcp_pm_schedule_work(msk, MPTCP_PM_RM_ADDR_RECEIVED);
> + pm->rm_id = rm_id;
> + spin_unlock_bh(&pm->lock);
> +}
> +
> +bool mptcp_pm_rm_addr_signal(struct mptcp_sock *msk, unsigned int remaining,
> + u8 *rm_id)
> +{
> + int ret = false;
> +
> + spin_lock_bh(&msk->pm.lock);
> +
> + /* double check after the lock is acquired */
> + if (!mptcp_pm_should_rm_signal(msk))
> + goto out_unlock;
> +
> + if (remaining < TCPOLEN_MPTCP_RM_ADDR_BASE)
> + goto out_unlock;
> +
> + *rm_id = msk->pm.rm_id;
> + WRITE_ONCE(msk->pm.rm_addr_signal, false);
> ret = true;
>
> out_unlock:
> @@ -186,9 +243,11 @@ void mptcp_pm_data_init(struct mptcp_sock *msk)
> msk->pm.local_addr_used = 0;
> msk->pm.subflows = 0;
> WRITE_ONCE(msk->pm.work_pending, false);
> - WRITE_ONCE(msk->pm.addr_signal, false);
> + WRITE_ONCE(msk->pm.add_addr_signal, false);
> + WRITE_ONCE(msk->pm.rm_addr_signal, false);
> WRITE_ONCE(msk->pm.accept_addr, false);
> WRITE_ONCE(msk->pm.accept_subflow, false);
> + WRITE_ONCE(msk->pm.addr_updated, false);
> msk->pm.status = 0;
>
> spin_lock_init(&msk->pm.lock);
> diff --git a/net/mptcp/pm_netlink.c b/net/mptcp/pm_netlink.c
> index c8820c4156e6..c9933387be09 100644
> --- a/net/mptcp/pm_netlink.c
> +++ b/net/mptcp/pm_netlink.c
> @@ -39,6 +39,7 @@ struct pm_nl_pernet {
> unsigned int local_addr_max;
> unsigned int subflows_max;
> unsigned int next_id;
> + unsigned int rm_id;
> };
>
> #define MPTCP_PM_ADDR_MAX 8
> @@ -165,7 +166,7 @@ static void check_work_pending(struct mptcp_sock *msk)
> {
> if (msk->pm.add_addr_signaled == msk->pm.add_addr_signal_max &&
> (msk->pm.local_addr_used == msk->pm.local_addr_max ||
> - msk->pm.subflows == msk->pm.subflows_max))
> + msk->pm.subflows == msk->pm.subflows_max) && msk->pm.addr_updated)
> WRITE_ONCE(msk->pm.work_pending, false);
> }
>
> @@ -196,6 +197,11 @@ static void mptcp_pm_create_subflow_or_signal_addr(struct mptcp_sock *msk)
> msk->pm.local_addr_used = msk->pm.add_addr_signal_max;
> }
>
> + check_work_pending(msk);
> + } else if (msk->pm.add_addr_signaled > msk->pm.add_addr_signal_max) {
> + msk->pm.add_addr_signaled--;
> + mptcp_pm_remove_addr(msk, pernet->rm_id);
> +
> check_work_pending(msk);
> }
>
> @@ -261,6 +267,26 @@ void mptcp_pm_nl_add_addr_received(struct mptcp_sock *msk)
> spin_lock_bh(&msk->pm.lock);
> }
>
> +void mptcp_pm_nl_rm_addr_received(struct mptcp_sock *msk)
> +{
> + struct mptcp_subflow_context *subflow, *tmp;
> +
> + pr_debug("remote_id %d", msk->pm.rm_id);
> +
> + msk->pm.add_addr_accepted--;
> + msk->pm.subflows--;
> + WRITE_ONCE(msk->pm.accept_addr, true);
> +
> + list_for_each_entry_safe(subflow, tmp, &msk->conn_list, node) {
> + struct sock *tcp_sk = mptcp_subflow_tcp_sock(subflow);
> +
> + if (msk->pm.rm_id == subflow->remote_id) {
> + mptcp_subflow_shutdown(tcp_sk, 1, 1, msk->write_seq);
> + list_del(&subflow->node);
> + }
> + }
> +}
> +
> static bool address_use_port(struct mptcp_pm_addr_entry *entry)
> {
> return (entry->flags &
> @@ -354,6 +380,24 @@ int mptcp_pm_nl_get_local_id(struct mptcp_sock *msk, struct sock_common *skc)
> return ret;
> }
>
> +void mptcp_pm_nl_addr_update(struct mptcp_sock *msk)
> +{
> + struct mptcp_pm_data *pm = &msk->pm;
> + struct pm_nl_pernet *pernet;
> +
> + pernet = net_generic(sock_net((struct sock *)msk), pm_nl_pernet_id);
> +
> + if (pm->add_addr_signal_max != pernet->add_addr_signal_max) {
> + pm->add_addr_signal_max = READ_ONCE(pernet->add_addr_signal_max);
> + pm->add_addr_accept_max = READ_ONCE(pernet->add_addr_accept_max);
> +
> + mptcp_pm_create_subflow_or_signal_addr(msk);
> + WRITE_ONCE(pm->addr_updated, true);
> + } else {
> + WRITE_ONCE(pm->addr_updated, false);
> + }
> +}
> +
> void mptcp_pm_nl_data_init(struct mptcp_sock *msk)
> {
> struct mptcp_pm_data *pm = &msk->pm;
> @@ -541,6 +585,7 @@ static int mptcp_nl_cmd_del_addr(struct sk_buff *skb, struct genl_info *info)
> pernet->local_addr_max--;
>
> pernet->addrs--;
> + pernet->rm_id = addr.addr.id;
> list_del_rcu(&entry->list);
> kfree_rcu(entry, rcu);
> out:
> diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
> index dbe43e0cd734..31b836ed0786 100644
> --- a/net/mptcp/protocol.c
> +++ b/net/mptcp/protocol.c
> @@ -1225,6 +1225,14 @@ static void pm_work(struct mptcp_sock *msk)
> pm->status &= ~BIT(MPTCP_PM_ADD_ADDR_RECEIVED);
> mptcp_pm_nl_add_addr_received(msk);
> }
> + if (pm->status & BIT(MPTCP_PM_RM_ADDR_RECEIVED)) {
> + pm->status &= ~BIT(MPTCP_PM_RM_ADDR_RECEIVED);
> + mptcp_pm_nl_rm_addr_received(msk);
> + }
> + if (pm->status & BIT(MPTCP_PM_ADDR_UPDATE)) {
> + pm->status &= ~BIT(MPTCP_PM_ADDR_UPDATE);
> + mptcp_pm_nl_addr_update(msk);
> + }
> if (pm->status & BIT(MPTCP_PM_ESTABLISHED)) {
> pm->status &= ~BIT(MPTCP_PM_ESTABLISHED);
> mptcp_pm_nl_fully_established(msk);
> @@ -1381,8 +1389,8 @@ static void mptcp_cancel_work(struct sock *sk)
> sock_put(sk);
> }
>
> -static void mptcp_subflow_shutdown(struct sock *ssk, int how,
> - bool data_fin_tx_enable, u64 data_fin_tx_seq)
> +void mptcp_subflow_shutdown(struct sock *ssk, int how,
> + bool data_fin_tx_enable, u64 data_fin_tx_seq)
> {
> lock_sock(ssk);
>
> diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
> index e5baaef5ec89..5587613f5b03 100644
> --- a/net/mptcp/protocol.h
> +++ b/net/mptcp/protocol.h
> @@ -150,6 +150,8 @@ struct mptcp_addr_info {
>
> enum mptcp_pm_status {
> MPTCP_PM_ADD_ADDR_RECEIVED,
> + MPTCP_PM_RM_ADDR_RECEIVED,
> + MPTCP_PM_ADDR_UPDATE,
> MPTCP_PM_ESTABLISHED,
> MPTCP_PM_SUBFLOW_ESTABLISHED,
> };
> @@ -160,11 +162,13 @@ struct mptcp_pm_data {
>
> spinlock_t lock; /*protects the whole PM data */
>
> - bool addr_signal;
> + bool add_addr_signal;
> + bool rm_addr_signal;
> bool server_side;
> bool work_pending;
> bool accept_addr;
> bool accept_subflow;
> + bool addr_updated;
> u8 add_addr_signaled;
> u8 add_addr_accepted;
> u8 local_addr_used;
> @@ -174,6 +178,7 @@ struct mptcp_pm_data {
> u8 local_addr_max;
> u8 subflows_max;
> u8 status;
> + u8 rm_id;
> };
>
> struct mptcp_data_frag {
> @@ -344,6 +349,8 @@ mptcp_subflow_get_mapped_dsn(const struct mptcp_subflow_context *subflow)
> int mptcp_is_enabled(struct net *net);
> bool mptcp_subflow_data_available(struct sock *sk);
> void __init mptcp_subflow_init(void);
> +void mptcp_subflow_shutdown(struct sock *ssk, int how,
> + bool data_fin_tx_enable, u64 data_fin_tx_seq);
>
> /* called with sk socket lock held */
> int __mptcp_subflow_connect(struct sock *sk, int ifindex,
> @@ -419,8 +426,10 @@ void mptcp_pm_connection_closed(struct mptcp_sock *msk);
> void mptcp_pm_subflow_established(struct mptcp_sock *msk,
> struct mptcp_subflow_context *subflow);
> void mptcp_pm_subflow_closed(struct mptcp_sock *msk, u8 id);
> +void mptcp_pm_addr_update(struct mptcp_sock *msk);
> void mptcp_pm_add_addr_received(struct mptcp_sock *msk,
> const struct mptcp_addr_info *addr);
> +void mptcp_pm_rm_addr_received(struct mptcp_sock *msk, u8 rm_id);
>
> int mptcp_pm_announce_addr(struct mptcp_sock *msk,
> const struct mptcp_addr_info *addr);
> @@ -429,7 +438,12 @@ int mptcp_pm_remove_subflow(struct mptcp_sock *msk, u8 remote_id);
>
> static inline bool mptcp_pm_should_signal(struct mptcp_sock *msk)
> {
> - return READ_ONCE(msk->pm.addr_signal);
> + return READ_ONCE(msk->pm.add_addr_signal);
> +}
> +
> +static inline bool mptcp_pm_should_rm_signal(struct mptcp_sock *msk)
> +{
> + return READ_ONCE(msk->pm.rm_addr_signal);
> }
>
> static inline unsigned int mptcp_add_addr_len(int family)
> @@ -439,15 +453,19 @@ static inline unsigned int mptcp_add_addr_len(int family)
> return TCPOLEN_MPTCP_ADD_ADDR6;
> }
>
> -bool mptcp_pm_addr_signal(struct mptcp_sock *msk, unsigned int remaining,
> - struct mptcp_addr_info *saddr);
> +bool mptcp_pm_add_addr_signal(struct mptcp_sock *msk, unsigned int remaining,
> + struct mptcp_addr_info *saddr);
> +bool mptcp_pm_rm_addr_signal(struct mptcp_sock *msk, unsigned int remaining,
> + u8 *rm_id);
> int mptcp_pm_get_local_id(struct mptcp_sock *msk, struct sock_common *skc);
>
> void __init mptcp_pm_nl_init(void);
> void mptcp_pm_nl_data_init(struct mptcp_sock *msk);
> +void mptcp_pm_nl_addr_update(struct mptcp_sock *msk);
> void mptcp_pm_nl_fully_established(struct mptcp_sock *msk);
> void mptcp_pm_nl_subflow_established(struct mptcp_sock *msk);
> void mptcp_pm_nl_add_addr_received(struct mptcp_sock *msk);
> +void mptcp_pm_nl_rm_addr_received(struct mptcp_sock *msk);
> int mptcp_pm_nl_get_local_id(struct mptcp_sock *msk, struct sock_common *skc);
>
> static inline struct mptcp_ext *mptcp_get_ext(struct sk_buff *skb)
> diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c
> index 9f7f3772c13c..326c2df256b7 100644
> --- a/net/mptcp/subflow.c
> +++ b/net/mptcp/subflow.c
> @@ -989,6 +989,7 @@ int __mptcp_subflow_connect(struct sock *sk, int ifindex,
> subflow->remote_key = msk->remote_key;
> subflow->local_key = msk->local_key;
> subflow->token = msk->token;
> + subflow->remote_id = remote->id;
> mptcp_info2sockaddr(loc, &addr);
>
> addrlen = sizeof(struct sockaddr_in);
^ permalink raw reply [flat|nested] 2+ messages in thread
end of thread, other threads:[~2020-07-22 9:02 UTC | newest]
Thread overview: 2+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2020-07-22 9:02 [MPTCP] Re: [PATCH net-next] mptcp: add REMOVE_ADDR support v1 Geliang Tang
-- strict thread matches above, loose matches on Subject: below --
2020-07-16 14:41 Paolo Abeni
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.