| Message ID | 20210915121144.9891-1-fw@strlen.de (mailing list archive) |
|---|---|
| State | Accepted, archived |
| Commit | 711ea7c216d755e271ba90d940c077ce8c566d07 |
| Delegated to: | Matthieu Baerts |
| Headers | show |
| Series | mptcp: don't return sockets in foreign netns | expand |
On Wed, 15 Sep 2021, Florian Westphal wrote: > mptcp_token_get_sock() may return a mptcp socket that is in > a different net namespace than the socket that received the token value. > > The mptcp syncookie code path had an explicit check for this, > this moves the test intp mptcp_token_get_sock() function. > > Eventually token.c should be converted to pernet storage, but > such change is not suitable for net tree. > > Signed-off-by: Florian Westphal <fw@strlen.de> For the net tree, would need a Fixes tag. How about: Fixes: 2c5ebd001d4f0 ("mptcp: refactor token container") mptcp_token_get_sock() was originally introduced in f296234c98a8f, but the changes here would only apply to Paolo's refactor and KUNIT tests. Looks good and the kunit tests build/run fine on my system, thanks Florian: Reviewed-by: Mat Martineau <mathew.j.martineau@linux.intel.com> - Mat > --- > net/mptcp/mptcp_diag.c | 2 +- > net/mptcp/protocol.h | 2 +- > net/mptcp/subflow.c | 2 +- > net/mptcp/syncookies.c | 13 +------------ > net/mptcp/token.c | 10 +++++++--- > net/mptcp/token_test.c | 14 ++++++++------ > 6 files changed, 19 insertions(+), 24 deletions(-) > > diff --git a/net/mptcp/mptcp_diag.c b/net/mptcp/mptcp_diag.c > index fdf0c1f15a65..f44125dd6697 100644 > --- a/net/mptcp/mptcp_diag.c > +++ b/net/mptcp/mptcp_diag.c > @@ -36,7 +36,7 @@ static int mptcp_diag_dump_one(struct netlink_callback *cb, > struct sock *sk; > > net = sock_net(in_skb->sk); > - msk = mptcp_token_get_sock(req->id.idiag_cookie[0]); > + msk = mptcp_token_get_sock(net, req->id.idiag_cookie[0]); > if (!msk) > goto out_nosk; > > diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h > index d3e6fd1615f1..dc984676c5eb 100644 > --- a/net/mptcp/protocol.h > +++ b/net/mptcp/protocol.h > @@ -709,7 +709,7 @@ int mptcp_token_new_connect(struct sock *sk); > void mptcp_token_accept(struct mptcp_subflow_request_sock *r, > struct mptcp_sock *msk); > bool mptcp_token_exists(u32 token); > -struct mptcp_sock *mptcp_token_get_sock(u32 token); > +struct mptcp_sock *mptcp_token_get_sock(struct net *net, u32 token); > struct mptcp_sock *mptcp_token_iter_next(const struct net *net, long *s_slot, > long *s_num); > void mptcp_token_destroy(struct mptcp_sock *msk); > diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c > index 1de7ce883c37..6172f380dfb7 100644 > --- a/net/mptcp/subflow.c > +++ b/net/mptcp/subflow.c > @@ -86,7 +86,7 @@ static struct mptcp_sock *subflow_token_join_request(struct request_sock *req) > struct mptcp_sock *msk; > int local_id; > > - msk = mptcp_token_get_sock(subflow_req->token); > + msk = mptcp_token_get_sock(sock_net(req_to_sk(req)), subflow_req->token); > if (!msk) { > SUBFLOW_REQ_INC_STATS(req, MPTCP_MIB_JOINNOTOKEN); > return NULL; > diff --git a/net/mptcp/syncookies.c b/net/mptcp/syncookies.c > index 37127781aee9..7f22526346a7 100644 > --- a/net/mptcp/syncookies.c > +++ b/net/mptcp/syncookies.c > @@ -108,18 +108,12 @@ bool mptcp_token_join_cookie_init_state(struct mptcp_subflow_request_sock *subfl > > e->valid = 0; > > - msk = mptcp_token_get_sock(e->token); > + msk = mptcp_token_get_sock(net, e->token); > if (!msk) { > spin_unlock_bh(&join_entry_locks[i]); > return false; > } > > - /* If this fails, the token got re-used in the mean time by another > - * mptcp socket in a different netns, i.e. entry is outdated. > - */ > - if (!net_eq(sock_net((struct sock *)msk), net)) > - goto err_put; > - > subflow_req->remote_nonce = e->remote_nonce; > subflow_req->local_nonce = e->local_nonce; > subflow_req->backup = e->backup; > @@ -128,11 +122,6 @@ bool mptcp_token_join_cookie_init_state(struct mptcp_subflow_request_sock *subfl > subflow_req->msk = msk; > spin_unlock_bh(&join_entry_locks[i]); > return true; > - > -err_put: > - spin_unlock_bh(&join_entry_locks[i]); > - sock_put((struct sock *)msk); > - return false; > } > > void __init mptcp_join_cookie_init(void) > diff --git a/net/mptcp/token.c b/net/mptcp/token.c > index a98e554b034f..313a3e793e61 100644 > --- a/net/mptcp/token.c > +++ b/net/mptcp/token.c > @@ -238,7 +238,7 @@ bool mptcp_token_exists(u32 token) > * > * returns NULL if no connection with the given token value exists. > */ > -struct mptcp_sock *mptcp_token_get_sock(u32 token) > +struct mptcp_sock *mptcp_token_get_sock(struct net *net, u32 token) > { > struct hlist_nulls_node *pos; > struct token_bucket *bucket; > @@ -251,11 +251,15 @@ struct mptcp_sock *mptcp_token_get_sock(u32 token) > again: > sk_nulls_for_each_rcu(sk, pos, &bucket->msk_chain) { > msk = mptcp_sk(sk); > - if (READ_ONCE(msk->token) != token) > + if (READ_ONCE(msk->token) != token || > + !net_eq(sock_net(sk), net)) > continue; > + > if (!refcount_inc_not_zero(&sk->sk_refcnt)) > goto not_found; > - if (READ_ONCE(msk->token) != token) { > + > + if (READ_ONCE(msk->token) != token || > + !net_eq(sock_net(sk), net)) { > sock_put(sk); > goto again; > } > diff --git a/net/mptcp/token_test.c b/net/mptcp/token_test.c > index e1bd6f0a0676..5d984bec1cd8 100644 > --- a/net/mptcp/token_test.c > +++ b/net/mptcp/token_test.c > @@ -11,6 +11,7 @@ static struct mptcp_subflow_request_sock *build_req_sock(struct kunit *test) > GFP_USER); > KUNIT_EXPECT_NOT_ERR_OR_NULL(test, req); > mptcp_token_init_request((struct request_sock *)req); > + sock_net_set((struct sock *)req, &init_net); > return req; > } > > @@ -22,7 +23,7 @@ static void mptcp_token_test_req_basic(struct kunit *test) > KUNIT_ASSERT_EQ(test, 0, > mptcp_token_new_request((struct request_sock *)req)); > KUNIT_EXPECT_NE(test, 0, (int)req->token); > - KUNIT_EXPECT_PTR_EQ(test, null_msk, mptcp_token_get_sock(req->token)); > + KUNIT_EXPECT_PTR_EQ(test, null_msk, mptcp_token_get_sock(&init_net, req->token)); > > /* cleanup */ > mptcp_token_destroy_request((struct request_sock *)req); > @@ -55,6 +56,7 @@ static struct mptcp_sock *build_msk(struct kunit *test) > msk = kunit_kzalloc(test, sizeof(struct mptcp_sock), GFP_USER); > KUNIT_EXPECT_NOT_ERR_OR_NULL(test, msk); > refcount_set(&((struct sock *)msk)->sk_refcnt, 1); > + sock_net_set((struct sock *)msk, &init_net); > return msk; > } > > @@ -74,11 +76,11 @@ static void mptcp_token_test_msk_basic(struct kunit *test) > mptcp_token_new_connect((struct sock *)icsk)); > KUNIT_EXPECT_NE(test, 0, (int)ctx->token); > KUNIT_EXPECT_EQ(test, ctx->token, msk->token); > - KUNIT_EXPECT_PTR_EQ(test, msk, mptcp_token_get_sock(ctx->token)); > + KUNIT_EXPECT_PTR_EQ(test, msk, mptcp_token_get_sock(&init_net, ctx->token)); > KUNIT_EXPECT_EQ(test, 2, (int)refcount_read(&sk->sk_refcnt)); > > mptcp_token_destroy(msk); > - KUNIT_EXPECT_PTR_EQ(test, null_msk, mptcp_token_get_sock(ctx->token)); > + KUNIT_EXPECT_PTR_EQ(test, null_msk, mptcp_token_get_sock(&init_net, ctx->token)); > } > > static void mptcp_token_test_accept(struct kunit *test) > @@ -90,11 +92,11 @@ static void mptcp_token_test_accept(struct kunit *test) > mptcp_token_new_request((struct request_sock *)req)); > msk->token = req->token; > mptcp_token_accept(req, msk); > - KUNIT_EXPECT_PTR_EQ(test, msk, mptcp_token_get_sock(msk->token)); > + KUNIT_EXPECT_PTR_EQ(test, msk, mptcp_token_get_sock(&init_net, msk->token)); > > /* this is now a no-op */ > mptcp_token_destroy_request((struct request_sock *)req); > - KUNIT_EXPECT_PTR_EQ(test, msk, mptcp_token_get_sock(msk->token)); > + KUNIT_EXPECT_PTR_EQ(test, msk, mptcp_token_get_sock(&init_net, msk->token)); > > /* cleanup */ > mptcp_token_destroy(msk); > @@ -116,7 +118,7 @@ static void mptcp_token_test_destroyed(struct kunit *test) > > /* simulate race on removal */ > refcount_set(&sk->sk_refcnt, 0); > - KUNIT_EXPECT_PTR_EQ(test, null_msk, mptcp_token_get_sock(msk->token)); > + KUNIT_EXPECT_PTR_EQ(test, null_msk, mptcp_token_get_sock(&init_net, msk->token)); > > /* cleanup */ > mptcp_token_destroy(msk); > -- > 2.32.0 > > > -- Mat Martineau Intel
Mat Martineau <mathew.j.martineau@linux.intel.com> wrote: > > On Wed, 15 Sep 2021, Florian Westphal wrote: > > > mptcp_token_get_sock() may return a mptcp socket that is in > > a different net namespace than the socket that received the token value. > > > > The mptcp syncookie code path had an explicit check for this, > > this moves the test intp mptcp_token_get_sock() function. > > > > Eventually token.c should be converted to pernet storage, but > > such change is not suitable for net tree. > > > > Signed-off-by: Florian Westphal <fw@strlen.de> > > For the net tree, would need a Fixes tag. How about: > > Fixes: 2c5ebd001d4f0 ("mptcp: refactor token container") > > mptcp_token_get_sock() was originally introduced in f296234c98a8f, but the > changes here would only apply to Paolo's refactor and KUNIT tests. Yep, f296234c98a8f is 'more correct' (day 0), but I'm fine with using 2c5ebd001d4f0 instead. Thanks for reviewing.
Hi Florian, Mat, On 15/09/2021 14:11, Florian Westphal wrote: > mptcp_token_get_sock() may return a mptcp socket that is in > a different net namespace than the socket that received the token value. > > The mptcp syncookie code path had an explicit check for this, > this moves the test intp mptcp_token_get_sock() function. > > Eventually token.c should be converted to pernet storage, but > such change is not suitable for net tree. Thank you for the patch and the review! Now applied in our tree (fix for -net) with Mat's RvB tag, a Fixes tag and without a small typo (s/intp/into). - 711ea7c216d7: mptcp: don't return sockets in foreign netns - Results: c76567ae64fc..f05d3f3e1d3f Builds and tests are now in progress: https://cirrus-ci.com/github/multipath-tcp/mptcp_net-next/export/20210916T162130 https://github.com/multipath-tcp/mptcp_net-next/actions/workflows/build-validation.yml?query=branch:export/20210916T162130 Cheers, Matt
diff --git a/net/mptcp/mptcp_diag.c b/net/mptcp/mptcp_diag.c index fdf0c1f15a65..f44125dd6697 100644 --- a/net/mptcp/mptcp_diag.c +++ b/net/mptcp/mptcp_diag.c @@ -36,7 +36,7 @@ static int mptcp_diag_dump_one(struct netlink_callback *cb, struct sock *sk; net = sock_net(in_skb->sk); - msk = mptcp_token_get_sock(req->id.idiag_cookie[0]); + msk = mptcp_token_get_sock(net, req->id.idiag_cookie[0]); if (!msk) goto out_nosk; diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h index d3e6fd1615f1..dc984676c5eb 100644 --- a/net/mptcp/protocol.h +++ b/net/mptcp/protocol.h @@ -709,7 +709,7 @@ int mptcp_token_new_connect(struct sock *sk); void mptcp_token_accept(struct mptcp_subflow_request_sock *r, struct mptcp_sock *msk); bool mptcp_token_exists(u32 token); -struct mptcp_sock *mptcp_token_get_sock(u32 token); +struct mptcp_sock *mptcp_token_get_sock(struct net *net, u32 token); struct mptcp_sock *mptcp_token_iter_next(const struct net *net, long *s_slot, long *s_num); void mptcp_token_destroy(struct mptcp_sock *msk); diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c index 1de7ce883c37..6172f380dfb7 100644 --- a/net/mptcp/subflow.c +++ b/net/mptcp/subflow.c @@ -86,7 +86,7 @@ static struct mptcp_sock *subflow_token_join_request(struct request_sock *req) struct mptcp_sock *msk; int local_id; - msk = mptcp_token_get_sock(subflow_req->token); + msk = mptcp_token_get_sock(sock_net(req_to_sk(req)), subflow_req->token); if (!msk) { SUBFLOW_REQ_INC_STATS(req, MPTCP_MIB_JOINNOTOKEN); return NULL; diff --git a/net/mptcp/syncookies.c b/net/mptcp/syncookies.c index 37127781aee9..7f22526346a7 100644 --- a/net/mptcp/syncookies.c +++ b/net/mptcp/syncookies.c @@ -108,18 +108,12 @@ bool mptcp_token_join_cookie_init_state(struct mptcp_subflow_request_sock *subfl e->valid = 0; - msk = mptcp_token_get_sock(e->token); + msk = mptcp_token_get_sock(net, e->token); if (!msk) { spin_unlock_bh(&join_entry_locks[i]); return false; } - /* If this fails, the token got re-used in the mean time by another - * mptcp socket in a different netns, i.e. entry is outdated. - */ - if (!net_eq(sock_net((struct sock *)msk), net)) - goto err_put; - subflow_req->remote_nonce = e->remote_nonce; subflow_req->local_nonce = e->local_nonce; subflow_req->backup = e->backup; @@ -128,11 +122,6 @@ bool mptcp_token_join_cookie_init_state(struct mptcp_subflow_request_sock *subfl subflow_req->msk = msk; spin_unlock_bh(&join_entry_locks[i]); return true; - -err_put: - spin_unlock_bh(&join_entry_locks[i]); - sock_put((struct sock *)msk); - return false; } void __init mptcp_join_cookie_init(void) diff --git a/net/mptcp/token.c b/net/mptcp/token.c index a98e554b034f..313a3e793e61 100644 --- a/net/mptcp/token.c +++ b/net/mptcp/token.c @@ -238,7 +238,7 @@ bool mptcp_token_exists(u32 token) * * returns NULL if no connection with the given token value exists. */ -struct mptcp_sock *mptcp_token_get_sock(u32 token) +struct mptcp_sock *mptcp_token_get_sock(struct net *net, u32 token) { struct hlist_nulls_node *pos; struct token_bucket *bucket; @@ -251,11 +251,15 @@ struct mptcp_sock *mptcp_token_get_sock(u32 token) again: sk_nulls_for_each_rcu(sk, pos, &bucket->msk_chain) { msk = mptcp_sk(sk); - if (READ_ONCE(msk->token) != token) + if (READ_ONCE(msk->token) != token || + !net_eq(sock_net(sk), net)) continue; + if (!refcount_inc_not_zero(&sk->sk_refcnt)) goto not_found; - if (READ_ONCE(msk->token) != token) { + + if (READ_ONCE(msk->token) != token || + !net_eq(sock_net(sk), net)) { sock_put(sk); goto again; } diff --git a/net/mptcp/token_test.c b/net/mptcp/token_test.c index e1bd6f0a0676..5d984bec1cd8 100644 --- a/net/mptcp/token_test.c +++ b/net/mptcp/token_test.c @@ -11,6 +11,7 @@ static struct mptcp_subflow_request_sock *build_req_sock(struct kunit *test) GFP_USER); KUNIT_EXPECT_NOT_ERR_OR_NULL(test, req); mptcp_token_init_request((struct request_sock *)req); + sock_net_set((struct sock *)req, &init_net); return req; } @@ -22,7 +23,7 @@ static void mptcp_token_test_req_basic(struct kunit *test) KUNIT_ASSERT_EQ(test, 0, mptcp_token_new_request((struct request_sock *)req)); KUNIT_EXPECT_NE(test, 0, (int)req->token); - KUNIT_EXPECT_PTR_EQ(test, null_msk, mptcp_token_get_sock(req->token)); + KUNIT_EXPECT_PTR_EQ(test, null_msk, mptcp_token_get_sock(&init_net, req->token)); /* cleanup */ mptcp_token_destroy_request((struct request_sock *)req); @@ -55,6 +56,7 @@ static struct mptcp_sock *build_msk(struct kunit *test) msk = kunit_kzalloc(test, sizeof(struct mptcp_sock), GFP_USER); KUNIT_EXPECT_NOT_ERR_OR_NULL(test, msk); refcount_set(&((struct sock *)msk)->sk_refcnt, 1); + sock_net_set((struct sock *)msk, &init_net); return msk; } @@ -74,11 +76,11 @@ static void mptcp_token_test_msk_basic(struct kunit *test) mptcp_token_new_connect((struct sock *)icsk)); KUNIT_EXPECT_NE(test, 0, (int)ctx->token); KUNIT_EXPECT_EQ(test, ctx->token, msk->token); - KUNIT_EXPECT_PTR_EQ(test, msk, mptcp_token_get_sock(ctx->token)); + KUNIT_EXPECT_PTR_EQ(test, msk, mptcp_token_get_sock(&init_net, ctx->token)); KUNIT_EXPECT_EQ(test, 2, (int)refcount_read(&sk->sk_refcnt)); mptcp_token_destroy(msk); - KUNIT_EXPECT_PTR_EQ(test, null_msk, mptcp_token_get_sock(ctx->token)); + KUNIT_EXPECT_PTR_EQ(test, null_msk, mptcp_token_get_sock(&init_net, ctx->token)); } static void mptcp_token_test_accept(struct kunit *test) @@ -90,11 +92,11 @@ static void mptcp_token_test_accept(struct kunit *test) mptcp_token_new_request((struct request_sock *)req)); msk->token = req->token; mptcp_token_accept(req, msk); - KUNIT_EXPECT_PTR_EQ(test, msk, mptcp_token_get_sock(msk->token)); + KUNIT_EXPECT_PTR_EQ(test, msk, mptcp_token_get_sock(&init_net, msk->token)); /* this is now a no-op */ mptcp_token_destroy_request((struct request_sock *)req); - KUNIT_EXPECT_PTR_EQ(test, msk, mptcp_token_get_sock(msk->token)); + KUNIT_EXPECT_PTR_EQ(test, msk, mptcp_token_get_sock(&init_net, msk->token)); /* cleanup */ mptcp_token_destroy(msk); @@ -116,7 +118,7 @@ static void mptcp_token_test_destroyed(struct kunit *test) /* simulate race on removal */ refcount_set(&sk->sk_refcnt, 0); - KUNIT_EXPECT_PTR_EQ(test, null_msk, mptcp_token_get_sock(msk->token)); + KUNIT_EXPECT_PTR_EQ(test, null_msk, mptcp_token_get_sock(&init_net, msk->token)); /* cleanup */ mptcp_token_destroy(msk);
mptcp_token_get_sock() may return a mptcp socket that is in a different net namespace than the socket that received the token value. The mptcp syncookie code path had an explicit check for this, this moves the test intp mptcp_token_get_sock() function. Eventually token.c should be converted to pernet storage, but such change is not suitable for net tree. Signed-off-by: Florian Westphal <fw@strlen.de> --- net/mptcp/mptcp_diag.c | 2 +- net/mptcp/protocol.h | 2 +- net/mptcp/subflow.c | 2 +- net/mptcp/syncookies.c | 13 +------------ net/mptcp/token.c | 10 +++++++--- net/mptcp/token_test.c | 14 ++++++++------ 6 files changed, 19 insertions(+), 24 deletions(-)