[MPTCP] Re: [PATCH net] mptcp: explicitly zeros msk fields on clone

[MPTCP] Re: [PATCH net] mptcp: explicitly zeros msk fields on clone

[Paolo Abeni]

[-- Attachment #1: Type: text/plain, Size: 833 bytes --]

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

[MPTCP] Re: [PATCH net] mptcp: explicitly zeros msk fields on clone

[Christoph Paasch]

[-- Attachment #1: Type: text/plain, Size: 1246 bytes --]

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

[MPTCP] Re: [PATCH net] mptcp: explicitly zeros msk fields on clone

[Paolo Abeni]

[-- Attachment #1: Type: text/plain, Size: 1118 bytes --]

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

[MPTCP] Re: [PATCH net] mptcp: explicitly zeros msk fields on clone

[Christoph Paasch]

[-- Attachment #1: Type: text/plain, Size: 1249 bytes --]

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(a)lists.01.org
> To unsubscribe send an email to mptcp-leave(a)lists.01.org