| Message ID | 47b5459f6b9b07c5a669d5a5233b35cf6ec601ac.1708532911.git.dcaratti@redhat.com (mailing list archive) |
|---|---|
| State | Accepted, archived |
| Commit | 6d6a1d5ea72ebf4796a8187bf46620deebf2770e |
| Delegated to: | Matthieu Baerts |
| Headers | show |
| Series | [mptcp-net] mptcp: fix double-free on socket dismantle | expand |
| Context | Check | Description |
|---|---|---|
| matttbe/build | success | Build and static analysis OK |
| matttbe/checkpatch | warning | total: 0 errors, 1 warnings, 0 checks, 63 lines checked |
| matttbe/KVM_Validation__normal | success | Success! ✅ |
Hi Davide,
Thank you for your modifications, that's great!
Our CI (GitHub Action) did some validations and here is its report:
- KVM Validation: normal:
- Success! ✅:
- Task: https://github.com/multipath-tcp/mptcp_net-next/actions/runs/7992617007
Initiator: Patchew Applier
Commits: https://github.com/multipath-tcp/mptcp_net-next/commits/dc4d4d869ec0
If there are some issues, you can reproduce them using the same environment as
the one used by the CI thanks to a docker image, e.g.:
$ cd [kernel source code]
$ docker run -v "${PWD}:${PWD}:rw" -w "${PWD}" --privileged --rm -it \
--pull always mptcp/mptcp-upstream-virtme-docker:latest \
auto-normal
For more details:
https://github.com/multipath-tcp/mptcp-upstream-virtme-docker
Please note that despite all the efforts that have been already done to have a
stable tests suite when executed on a public CI like here, it is possible some
reported issues are not due to your modifications. Still, do not hesitate to
help us improve that ;-)
Cheers,
MPTCP GH Action bot
Bot operated by Matthieu Baerts (NGI0 Core)
On Wed, 21 Feb 2024, Davide Caratti wrote: > when MPTCP server accepts an incoming connection, it clones its listener > socket. However, the pointer to 'inet_opt' for the new socket has the same > value as the original one: as a consequence, on program exit it's possible > to observe the following splat: > ... > > Something similar (a refcount underflow) happens with CALIPSO/IPv6. Fix > this by duplicating IP / IPv6 options after clone, so that > ip{,6}_sock_destruct() doesn't end up freeing the same memory area twice. > > Signed-off-by: Davide Caratti <dcaratti@redhat.com> > --- > net/mptcp/protocol.c | 49 ++++++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 49 insertions(+) Thanks Davide. Looks good to me, and the appropriate "fixes" tag goes all the way back to the beginning: Fixes: cf7da0d66cc1 ("mptcp: Create SUBFLOW socket for incoming connections") Reviewed-by: Mat Martineau <martineau@kernel.org> > > diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c > index 50dcba41b6ef..352334bda0e3 100644 > --- a/net/mptcp/protocol.c > +++ b/net/mptcp/protocol.c > @@ -3202,8 +3202,50 @@ static struct ipv6_pinfo *mptcp_inet6_sk(const struct sock *sk) > > return (struct ipv6_pinfo *)(((u8 *)sk) + offset); > } > + > +static void mptcp_copy_ip6_options(struct sock *newsk, const struct sock *sk) > +{ > + const struct ipv6_pinfo *np = inet6_sk(sk); > + struct ipv6_txoptions *opt; > + struct ipv6_pinfo *newnp; > + > + newnp = inet6_sk(newsk); > + > + rcu_read_lock(); > + opt = rcu_dereference(np->opt); > + if (opt) { > + opt = ipv6_dup_options(newsk, opt); > + if (!opt) > + net_warn_ratelimited("%s: Failed to copy ip6 options\n", __func__); > + } > + RCU_INIT_POINTER(newnp->opt, opt); > + rcu_read_unlock(); > +} > #endif > > +static void mptcp_copy_ip_options(struct sock *newsk, const struct sock *sk) > +{ > + struct ip_options_rcu *inet_opt, *newopt = NULL; > + const struct inet_sock *inet = inet_sk(sk); > + struct inet_sock *newinet; > + > + newinet = inet_sk(newsk); > + > + rcu_read_lock(); > + inet_opt = rcu_dereference(inet->inet_opt); > + if (inet_opt) { > + newopt = sock_kmalloc(newsk, sizeof(*inet_opt) + > + inet_opt->opt.optlen, GFP_ATOMIC); > + if (newopt) > + memcpy(newopt, inet_opt, sizeof(*inet_opt) + > + inet_opt->opt.optlen); > + else > + net_warn_ratelimited("%s: Failed to copy ip options\n", __func__); > + } > + RCU_INIT_POINTER(newinet->inet_opt, newopt); > + rcu_read_unlock(); > +} > + > struct sock *mptcp_sk_clone_init(const struct sock *sk, > const struct mptcp_options_received *mp_opt, > struct sock *ssk, > @@ -3224,6 +3266,13 @@ struct sock *mptcp_sk_clone_init(const struct sock *sk, > > __mptcp_init_sock(nsk); > > +#if IS_ENABLED(CONFIG_MPTCP_IPV6) > + if (nsk->sk_family == AF_INET6) > + mptcp_copy_ip6_options(nsk, sk); > + else > +#endif > + mptcp_copy_ip_options(nsk, sk); > + > msk = mptcp_sk(nsk); > WRITE_ONCE(msk->local_key, subflow_req->local_key); > WRITE_ONCE(msk->token, subflow_req->token); > -- > 2.43.0 > > >
Hi Davide,
Thank you for your modifications, that's great!
Our CI (GitHub Action) did some validations and here is its report:
- KVM Validation: normal:
- Success! ✅:
- Task: https://github.com/multipath-tcp/mptcp_net-next/actions/runs/7998762098
Initiator: Patchew Applier
Commits: https://github.com/multipath-tcp/mptcp_net-next/commits/57dfa41f7285
If there are some issues, you can reproduce them using the same environment as
the one used by the CI thanks to a docker image, e.g.:
$ cd [kernel source code]
$ docker run -v "${PWD}:${PWD}:rw" -w "${PWD}" --privileged --rm -it \
--pull always mptcp/mptcp-upstream-virtme-docker:latest \
auto-normal
For more details:
https://github.com/multipath-tcp/mptcp-upstream-virtme-docker
Please note that despite all the efforts that have been already done to have a
stable tests suite when executed on a public CI like here, it is possible some
reported issues are not due to your modifications. Still, do not hesitate to
help us improve that ;-)
Cheers,
MPTCP GH Action bot
Bot operated by Matthieu Baerts (NGI0 Core)
Hi Davide, Mat, On 21/02/2024 5:30 pm, Davide Caratti wrote: > when MPTCP server accepts an incoming connection, it clones its listener > socket. However, the pointer to 'inet_opt' for the new socket has the same > value as the original one: as a consequence, on program exit it's possible > to observe the following splat: (...) > Something similar (a refcount underflow) happens with CALIPSO/IPv6. Fix > this by duplicating IP / IPv6 options after clone, so that > ip{,6}_sock_destruct() doesn't end up freeing the same memory area twice. Thank you for the patch and the review! Now in our tree (fix for -net), with Mat's RvB and Fixes tags: New patches for t/upstream-net and t/upstream: - 6d6a1d5ea72e: mptcp: fix double-free on socket dismantle - Results: 74677c9f0a8c..b35846da7660 (export-net) - Results: 55a829ff16aa..df010bcea0cb (export) Cheers, Matt
diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c index 50dcba41b6ef..352334bda0e3 100644 --- a/net/mptcp/protocol.c +++ b/net/mptcp/protocol.c @@ -3202,8 +3202,50 @@ static struct ipv6_pinfo *mptcp_inet6_sk(const struct sock *sk) return (struct ipv6_pinfo *)(((u8 *)sk) + offset); } + +static void mptcp_copy_ip6_options(struct sock *newsk, const struct sock *sk) +{ + const struct ipv6_pinfo *np = inet6_sk(sk); + struct ipv6_txoptions *opt; + struct ipv6_pinfo *newnp; + + newnp = inet6_sk(newsk); + + rcu_read_lock(); + opt = rcu_dereference(np->opt); + if (opt) { + opt = ipv6_dup_options(newsk, opt); + if (!opt) + net_warn_ratelimited("%s: Failed to copy ip6 options\n", __func__); + } + RCU_INIT_POINTER(newnp->opt, opt); + rcu_read_unlock(); +} #endif +static void mptcp_copy_ip_options(struct sock *newsk, const struct sock *sk) +{ + struct ip_options_rcu *inet_opt, *newopt = NULL; + const struct inet_sock *inet = inet_sk(sk); + struct inet_sock *newinet; + + newinet = inet_sk(newsk); + + rcu_read_lock(); + inet_opt = rcu_dereference(inet->inet_opt); + if (inet_opt) { + newopt = sock_kmalloc(newsk, sizeof(*inet_opt) + + inet_opt->opt.optlen, GFP_ATOMIC); + if (newopt) + memcpy(newopt, inet_opt, sizeof(*inet_opt) + + inet_opt->opt.optlen); + else + net_warn_ratelimited("%s: Failed to copy ip options\n", __func__); + } + RCU_INIT_POINTER(newinet->inet_opt, newopt); + rcu_read_unlock(); +} + struct sock *mptcp_sk_clone_init(const struct sock *sk, const struct mptcp_options_received *mp_opt, struct sock *ssk, @@ -3224,6 +3266,13 @@ struct sock *mptcp_sk_clone_init(const struct sock *sk, __mptcp_init_sock(nsk); +#if IS_ENABLED(CONFIG_MPTCP_IPV6) + if (nsk->sk_family == AF_INET6) + mptcp_copy_ip6_options(nsk, sk); + else +#endif + mptcp_copy_ip_options(nsk, sk); + msk = mptcp_sk(nsk); WRITE_ONCE(msk->local_key, subflow_req->local_key); WRITE_ONCE(msk->token, subflow_req->token);