| Message ID | 539a0cd36a6ff12d3b8d702a2cb314925a32cd9f.1594996220.git.pabeni@redhat.com |
|---|---|
| State | Rejected, archived |
| Delegated to: | Paolo Abeni |
| Headers | show |
| Series | [net] mptcp: explicitly zeros msk fields on clone | expand |
On Fri, 2020-07-17 at 17:12 +0200, Paolo Abeni wrote: > sk_clone_lock() does not set the __GFP_ZERO flag on the > internal socket allocation, so we must explicitly zero > all the relevant msk fields, or we could see inconsitent > socket status leading to unexpected fallback or data > corruption. I noticed only after sending this that sk_clone_lock() calls sock_copy() which in turn copies everything up to prot->obj_size - that is sizeof(struct mptcp_sock) or sizeof(struct mptcp6_sock) in our case). Since the listening socket should always have the relevant fields cleared this patch is not needed. There is a caveat: a socket could go through: connect(); shutdown(); listen(); but I think the correct way to handle the above is clearing properly the status in listen(), will look at that later /P
On 07/17/20 - 17:30, Paolo Abeni wrote: > On Fri, 2020-07-17 at 17:12 +0200, Paolo Abeni wrote: > > sk_clone_lock() does not set the __GFP_ZERO flag on the > > internal socket allocation, so we must explicitly zero > > all the relevant msk fields, or we could see inconsitent > > socket status leading to unexpected fallback or data > > corruption. > > I noticed only after sending this that sk_clone_lock() calls > sock_copy() which in turn copies everything up to prot->obj_size - that > is sizeof(struct mptcp_sock) or sizeof(struct mptcp6_sock) in our > case). > > Since the listening socket should always have the relevant fields > cleared this patch is not needed. > > There is a caveat: a socket could go through: > > connect(); shutdown(); listen(); Wouldn't that then go through mptcp_disconnect()? Thus, in the same style as for TCP (tcp_disconnect()), the zeroing can happen in mptcp_disconnect(). Christoph > > but I think the correct way to handle the above is clearing properly > the status in listen(), will look at that later > > /P > _______________________________________________ > mptcp mailing list -- mptcp@lists.01.org > To unsubscribe send an email to mptcp-leave@lists.01.org
On Fri, 2020-07-17 at 09:11 -0700, Christoph Paasch wrote: > On 07/17/20 - 17:30, Paolo Abeni wrote: > > On Fri, 2020-07-17 at 17:12 +0200, Paolo Abeni wrote: > > > sk_clone_lock() does not set the __GFP_ZERO flag on the > > > internal socket allocation, so we must explicitly zero > > > all the relevant msk fields, or we could see inconsitent > > > socket status leading to unexpected fallback or data > > > corruption. > > > > I noticed only after sending this that sk_clone_lock() calls > > sock_copy() which in turn copies everything up to prot->obj_size - that > > is sizeof(struct mptcp_sock) or sizeof(struct mptcp6_sock) in our > > case). > > > > Since the listening socket should always have the relevant fields > > cleared this patch is not needed. > > > > There is a caveat: a socket could go through: > > > > connect(); shutdown(); listen(); > > Wouldn't that then go through mptcp_disconnect()? Thus, in the same style as > for TCP (tcp_disconnect()), the zeroing can happen in mptcp_disconnect(). mptcp_disconnect() is never invoked, see comments in protocol.c ;) /P
On 07/17/20 - 18:19, Paolo Abeni wrote: > On Fri, 2020-07-17 at 09:11 -0700, Christoph Paasch wrote: > > On 07/17/20 - 17:30, Paolo Abeni wrote: > > > On Fri, 2020-07-17 at 17:12 +0200, Paolo Abeni wrote: > > > > sk_clone_lock() does not set the __GFP_ZERO flag on the > > > > internal socket allocation, so we must explicitly zero > > > > all the relevant msk fields, or we could see inconsitent > > > > socket status leading to unexpected fallback or data > > > > corruption. > > > > > > I noticed only after sending this that sk_clone_lock() calls > > > sock_copy() which in turn copies everything up to prot->obj_size - that > > > is sizeof(struct mptcp_sock) or sizeof(struct mptcp6_sock) in our > > > case). > > > > > > Since the listening socket should always have the relevant fields > > > cleared this patch is not needed. > > > > > > There is a caveat: a socket could go through: > > > > > > connect(); shutdown(); listen(); > > > > Wouldn't that then go through mptcp_disconnect()? Thus, in the same style as > > for TCP (tcp_disconnect()), the zeroing can happen in mptcp_disconnect(). > > mptcp_disconnect() is never invoked, see comments in protocol.c ;) I see! Sounds good then. Christoph
diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c index dbe43e0cd734..5449fee82239 100644 --- a/net/mptcp/protocol.c +++ b/net/mptcp/protocol.c @@ -1511,12 +1511,15 @@ struct sock *mptcp_sk_clone(const struct sock *sk, inet_sk(nsk)->pinet6 = mptcp_inet6_sk(nsk); #endif - __mptcp_init_sock(nsk); - msk = mptcp_sk(nsk); msk->local_key = subflow_req->local_key; msk->token = subflow_req->token; msk->subflow = NULL; + msk->cached_ext = NULL; + msk->flags = 0; + msk->can_ack = false; + + __mptcp_init_sock(nsk); msk->write_seq = subflow_req->idsn + 1; atomic64_set(&msk->snd_una, msk->write_seq);
sk_clone_lock() does not set the __GFP_ZERO flag on the internal socket allocation, so we must explicitly zero all the relevant msk fields, or we could see inconsitent socket status leading to unexpected fallback or data corruption. Fixes: d22f4988ffec ("mptcp: process MP_CAPABLE data option") Signed-off-by: Paolo Abeni <pabeni@redhat.com> --- The goal is send this patch upstream asap, so that is hopefully included soon in net-next. In the export branch should be located on top of current net-next HEAD, that is, before all pending export branch patches --- net/mptcp/protocol.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-)