Re: SCTP_SOCKOPT_PEELOFF is missing SOCK_CLOEXEC (and SOCK_NONBLOCK)

[Marcelo Ricardo Leitner <marcelo.leitner@gmail.com>]

On Tue, Jun 20, 2017 at 02:14:51PM -0400, Neil Horman wrote:
> On Tue, Jun 20, 2017 at 12:41:47PM -0300, Marcelo Ricardo Leitner wrote:
> > Hi,
> > 
> > On Tue, Jun 20, 2017 at 10:39:51AM -0400, Neil Horman wrote:
> > > On Tue, Jun 20, 2017 at 03:24:13PM +0200, Andreas Steinmetz wrote:
> > > > [please CC me, I'm not subscribed]
> > > > 
> > > > It seems that if one does a getsockopt(SCTP_SOCKOPT_PEELOFF) a.k.a.
> > > > sctp_peeloff(), even if the socket descriptor from which the
> > > > association is to be peeled off has SOCK_CLOEXEC/SOCK_NONBLOCK set,
> > > > the peeled off socket descriptor doesn't have so.
> > > > 
> > > > It would be advisable to either clone these flags or add an atomic
> > > > version of SCTP_SOCKOPT_PEELOFF (accept4() style).
> > > > 
> > > > The missing SOCK_CLOEXEC requires unintentional additional locking
> > > > which typically is prone to errors and can slow down processing.
> > > > -- 
> > > > Andreas Steinmetz                       SPAMmers use robotrap@domdv.de
> > > > --
> > > > To unsubscribe from this list: send the line "unsubscribe linux-sctp" in
> > > > the body of a message to majordomo@vger.kernel.org
> > > > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> > > > 
> > > 
> > > 
> > > Try this patch, and confirm that it fixes the problem please?
> > > 
> > > Thanks!
> > > Neil
> > > 
> > > 
> > > diff --git a/net/sctp/socket.c b/net/sctp/socket.c
> > > index f16c8d9..a95e3d6 100644
> > > --- a/net/sctp/socket.c
> > > +++ b/net/sctp/socket.c
> > > @@ -4912,7 +4912,7 @@ int sctp_do_peeloff(struct sock *sk, sctp_assoc_t id, struct socket **sockp)
> > >  		return -EINVAL;
> > >  
> > >  	/* Create a new socket.  */
> > > -	err = sock_create(sk->sk_family, SOCK_SEQPACKET, IPPROTO_SCTP, &sock);
> > > +	err = sock_create(sk->sk_family, sk->sk_type, IPPROTO_SCTP, &sock);
> > 
> > I don't think this will work. Syscall socket() filters out these
> > flags and handle them separetly after calling sock_create() with just
> > the type, so I don't think this information is preserved in there.
> > 
> Shoot, you're right, they get masked as assigned to the fdflags, and aren't
> preserved in the socket flags.  We would have to find another way to go.
> 
> > Interesting to note that according to man page dup() syscall also
> > doesn't duplicate these flags. One has to use dup3() instead in order to
> > be able to specify them.
> > 
> Yeah.  Actually in the case of dup3, it won't even let you specify O_CLOEXEC at
> all, it returns an error if its set in the flags field.

Wait. It was inverted:
                     v
        if ((flags & ~O_CLOEXEC) != 0)
                return -EINVAL;
One can't specify O_NONBLOCK but O_CLOEXEC should be fine.

> > Maybe by extending sctp_peeloff_arg_t to have a flags attribute in
> > there, we can allow the application to specify it and feed into
> > get_unused_fd_flags() call in sctp_getsockopt_peeloff() instead, or even
> > just overload the sd, which is currently an output-only value, to
> > contain flags as the patch below. (We probably should add some sanity
> > checking in there, though)
> > 
> 
> I wonder if theres something in the sock_map_fd function set that might let us
> clone an fd with all its flags to a new socket?

Checked again now, I don't see it. But I don't think we should always
clone the flags.

  Marcelo

> 
> Neil
> 
> > Thanks,
> > Marcelo
> > 
> > ---8<---
> > 
> > diff --git a/net/sctp/socket.c b/net/sctp/socket.c
> > index e1fabf67d4ad..50021a925b16 100644
> > --- a/net/sctp/socket.c
> > +++ b/net/sctp/socket.c
> > @@ -4950,8 +4950,10 @@ static int sctp_getsockopt_peeloff(struct sock *sk, int len, char __user *optval
> >  	if (retval < 0)
> >  		goto out;
> >  
> > -	/* Map the socket to an unused fd that can be returned to the user.  */
> > -	retval = get_unused_fd_flags(0);
> > +	/* Map the socket to an unused fd that can be returned to the user.
> > +	 * peeloff.sd[in] is used to transport flags
> > +	 */
> > +	retval = get_unused_fd_flags(peeloff.sd);
> >  	if (retval < 0) {
> >  		sock_release(newsock);
> >  		goto out;
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-sctp" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> > 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-sctp" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>