Skip to content

Commit

Permalink
mptcp: corner case locking for rx path fields initialization
Browse files Browse the repository at this point in the history 
Most MPTCP-level related fields are under the mptcp data lock
protection, but are written one-off without such lock at MPC
complete time, both for the client and the server

Leverage the mptcp_propagate_state() infrastructure to move such
initialization under the proper lock client-wise.

The server side critical init steps are done by
mptcp_subflow_fully_established(): ensure the caller properly held the
relevant lock, and avoid acquiring the same lock in the nested scopes.

There are no real potential races, as write access to such fields
is implicitly serialized by the MPTCP state machine; the primary
goal is consistency.

Fixes: d22f498 ("mptcp: process MP_CAPABLE data option")
Signed-off-by: Paolo Abeni <[email protected]>
  • Loading branch information
@intel-lab-lkp
Paolo Abeni authored and intel-lab-lkp committed Jan 15, 2024
1 parent 779338d commit f34baa0
Show file tree
Hide file tree
Showing 5 changed files with 50 additions and 39 deletions.
6 changes: 2 additions & 4 deletions net/mptcp/fastopen.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -59,13 +59,12 @@ void mptcp_fastopen_subflow_synack_set_params(struct mptcp_subflow_context *subf
mptcp_data_unlock(sk);
}

void mptcp_fastopen_gen_msk_ackseq(struct mptcp_sock *msk, struct mptcp_subflow_context *subflow,
const struct mptcp_options_received *mp_opt)
void __mptcp_fastopen_gen_msk_ackseq(struct mptcp_sock *msk, struct mptcp_subflow_context *subflow,
const struct mptcp_options_received *mp_opt)
{
struct sock *sk = (struct sock *)msk;
struct sk_buff *skb;

mptcp_data_lock(sk);
skb = skb_peek_tail(&sk->sk_receive_queue);
if (skb) {
WARN_ON_ONCE(MPTCP_SKB_CB(skb)->end_seq);
Expand All @@ -77,5 +76,4 @@ void mptcp_fastopen_gen_msk_ackseq(struct mptcp_sock *msk, struct mptcp_subflow_
}

pr_debug("msk=%p ack_seq=%llx", msk, msk->ack_seq);
mptcp_data_unlock(sk);
}
9 changes: 5 additions & 4 deletions net/mptcp/options.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -962,9 +962,7 @@ static bool check_fully_established(struct mptcp_sock *msk, struct sock *ssk,
/* subflows are fully established as soon as we get any
* additional ack, including ADD_ADDR.
*/
subflow->fully_established = 1;
WRITE_ONCE(msk->fully_established, true);
goto check_notify;
goto set_fully_established;
}

/* If the first established packet does not contain MP_CAPABLE + data
Expand All @@ -986,7 +984,10 @@ static bool check_fully_established(struct mptcp_sock *msk, struct sock *ssk,
set_fully_established:
if (unlikely(!READ_ONCE(msk->pm.server_side)))
pr_warn_once("bogus mpc option on established client sk");
mptcp_subflow_fully_established(subflow, mp_opt);

mptcp_data_lock((struct sock *)msk);
__mptcp_subflow_fully_established(msk, subflow, mp_opt);
mptcp_data_unlock((struct sock *)msk);

check_notify:
/* if the subflow is not already linked into the conn_list, we can't
Expand Down
9 changes: 6 additions & 3 deletions net/mptcp/protocol.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3189,6 +3189,7 @@ struct sock *mptcp_sk_clone_init(const struct sock *sk,
{
struct mptcp_subflow_request_sock *subflow_req = mptcp_subflow_rsk(req);
struct sock *nsk = sk_clone_lock(sk, GFP_ATOMIC);
struct mptcp_subflow_context *subflow;
struct mptcp_sock *msk;

if (!nsk)
Expand Down Expand Up @@ -3229,7 +3230,8 @@ struct sock *mptcp_sk_clone_init(const struct sock *sk,

/* The msk maintain a ref to each subflow in the connections list */
WRITE_ONCE(msk->first, ssk);
list_add(&mptcp_subflow_ctx(ssk)->node, &msk->conn_list);
subflow = mptcp_subflow_ctx(ssk);
list_add(&subflow->node, &msk->conn_list);
sock_hold(ssk);

/* new mpc subflow takes ownership of the newly
Expand All @@ -3244,6 +3246,9 @@ struct sock *mptcp_sk_clone_init(const struct sock *sk,
__mptcp_propagate_sndbuf(nsk, ssk);

mptcp_rcv_space_init(msk, ssk);

if (mp_opt->suboptions & OPTION_MPTCP_MPC_ACK)
__mptcp_subflow_fully_established(msk, subflow, mp_opt);
bh_unlock_sock(nsk);

/* note: the newly allocated socket refcount is 2 now */
Expand Down Expand Up @@ -3481,8 +3486,6 @@ void mptcp_finish_connect(struct sock *ssk)
* accessing the field below
*/
WRITE_ONCE(msk->local_key, subflow->local_key);
WRITE_ONCE(msk->snd_una, subflow->idsn + 1);
WRITE_ONCE(msk->wnd_end, subflow->idsn + 1 + tcp_sk(ssk)->snd_wnd);

mptcp_pm_new_connection(msk, ssk, 0);
}
Expand Down
9 changes: 5 additions & 4 deletions net/mptcp/protocol.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -622,8 +622,9 @@ unsigned int mptcp_stale_loss_cnt(const struct net *net);
unsigned int mptcp_close_timeout(const struct sock *sk);
int mptcp_get_pm_type(const struct net *net);
const char *mptcp_get_scheduler(const struct net *net);
void mptcp_subflow_fully_established(struct mptcp_subflow_context *subflow,
const struct mptcp_options_received *mp_opt);
void __mptcp_subflow_fully_established(struct mptcp_sock *msk,
struct mptcp_subflow_context *subflow,
const struct mptcp_options_received *mp_opt);
bool __mptcp_retransmit_pending_data(struct sock *sk);
void mptcp_check_and_set_pending(struct sock *sk);
void __mptcp_push_pending(struct sock *sk, unsigned int flags);
Expand Down Expand Up @@ -957,8 +958,8 @@ void mptcp_event_pm_listener(const struct sock *ssk,
enum mptcp_event_type event);
bool mptcp_userspace_pm_active(const struct mptcp_sock *msk);

void mptcp_fastopen_gen_msk_ackseq(struct mptcp_sock *msk, struct mptcp_subflow_context *subflow,
const struct mptcp_options_received *mp_opt);
void __mptcp_fastopen_gen_msk_ackseq(struct mptcp_sock *msk, struct mptcp_subflow_context *subflow,
const struct mptcp_options_received *mp_opt);
void mptcp_fastopen_subflow_synack_set_params(struct mptcp_subflow_context *subflow,
struct request_sock *req);

Expand Down
56 changes: 32 additions & 24 deletions net/mptcp/subflow.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -441,20 +441,6 @@ void __mptcp_sync_state(struct sock *sk, int state)
}
}

static void mptcp_propagate_state(struct sock *sk, struct sock *ssk)
{
struct mptcp_sock *msk = mptcp_sk(sk);

mptcp_data_lock(sk);
if (!sock_owned_by_user(sk)) {
__mptcp_sync_state(sk, ssk->sk_state);
} else {
msk->pending_state = ssk->sk_state;
__set_bit(MPTCP_SYNC_STATE, &msk->cb_flags);
}
mptcp_data_unlock(sk);
}

static void subflow_set_remote_key(struct mptcp_sock *msk,
struct mptcp_subflow_context *subflow,
const struct mptcp_options_received *mp_opt)
Expand All @@ -476,6 +462,31 @@ static void subflow_set_remote_key(struct mptcp_sock *msk,
atomic64_set(&msk->rcv_wnd_sent, subflow->iasn);
}

static void mptcp_propagate_state(struct sock *sk, struct sock *ssk,
struct mptcp_subflow_context *subflow,
const struct mptcp_options_received *mp_opt)
{
struct mptcp_sock *msk = mptcp_sk(sk);

mptcp_data_lock(sk);
if (mp_opt) {
/* Options are available only in the non fallback cases
* avoid updating rx path fields otherwise
*/
WRITE_ONCE(msk->snd_una, subflow->idsn + 1);
WRITE_ONCE(msk->wnd_end, subflow->idsn + 1 + tcp_sk(ssk)->snd_wnd);
subflow_set_remote_key(msk, subflow, mp_opt);
}

if (!sock_owned_by_user(sk)) {
__mptcp_sync_state(sk, ssk->sk_state);
} else {
msk->pending_state = ssk->sk_state;
__set_bit(MPTCP_SYNC_STATE, &msk->cb_flags);
}
mptcp_data_unlock(sk);
}

static void subflow_finish_connect(struct sock *sk, const struct sk_buff *skb)
{
struct mptcp_subflow_context *subflow = mptcp_subflow_ctx(sk);
Expand Down Expand Up @@ -510,10 +521,9 @@ static void subflow_finish_connect(struct sock *sk, const struct sk_buff *skb)
if (mp_opt.deny_join_id0)
WRITE_ONCE(msk->pm.remote_deny_join_id0, true);
subflow->mp_capable = 1;
subflow_set_remote_key(msk, subflow, &mp_opt);
MPTCP_INC_STATS(sock_net(sk), MPTCP_MIB_MPCAPABLEACTIVEACK);
mptcp_finish_connect(sk);
mptcp_propagate_state(parent, sk);
mptcp_propagate_state(parent, sk, subflow, &mp_opt);
} else if (subflow->request_join) {
u8 hmac[SHA256_DIGEST_SIZE];

Expand Down Expand Up @@ -556,7 +566,7 @@ static void subflow_finish_connect(struct sock *sk, const struct sk_buff *skb)
}
} else if (mptcp_check_fallback(sk)) {
fallback:
mptcp_propagate_state(parent, sk);
mptcp_propagate_state(parent, sk, subflow, NULL);
}
return;

Expand Down Expand Up @@ -741,17 +751,16 @@ void mptcp_subflow_drop_ctx(struct sock *ssk)
kfree_rcu(ctx, rcu);
}

void mptcp_subflow_fully_established(struct mptcp_subflow_context *subflow,
const struct mptcp_options_received *mp_opt)
void __mptcp_subflow_fully_established(struct mptcp_sock *msk,
struct mptcp_subflow_context *subflow,
const struct mptcp_options_received *mp_opt)
{
struct mptcp_sock *msk = mptcp_sk(subflow->conn);

subflow_set_remote_key(msk, subflow, mp_opt);
subflow->fully_established = 1;
WRITE_ONCE(msk->fully_established, true);

if (subflow->is_mptfo)
mptcp_fastopen_gen_msk_ackseq(msk, subflow, mp_opt);
__mptcp_fastopen_gen_msk_ackseq(msk, subflow, mp_opt);
}

static struct sock *subflow_syn_recv_sock(const struct sock *sk,
Expand Down Expand Up @@ -843,7 +852,6 @@ static struct sock *subflow_syn_recv_sock(const struct sock *sk,
* mpc option
*/
if (mp_opt.suboptions & OPTION_MPTCP_MPC_ACK) {
mptcp_subflow_fully_established(ctx, &mp_opt);
mptcp_pm_fully_established(owner, child);
ctx->pm_notified = 1;
}
Expand Down Expand Up @@ -1755,7 +1763,7 @@ static void subflow_state_change(struct sock *sk)
mptcp_do_fallback(sk);
pr_fallback(msk);
subflow->conn_finished = 1;
mptcp_propagate_state(parent, sk);
mptcp_propagate_state(parent, sk, subflow, NULL);
}

/* as recvmsg() does not acquire the subflow socket for ssk selection
Expand Down

0 comments on commit f34baa0

Please sign in to comment.