SCTP_SOCKOPT_PEELOFF is missing SOCK_CLOEXEC (and SOCK_NONBLOCK)

SCTP_SOCKOPT_PEELOFF is missing SOCK_CLOEXEC (and SOCK_NONBLOCK)

[Andreas Steinmetz]

[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

Re: SCTP_SOCKOPT_PEELOFF is missing SOCK_CLOEXEC (and SOCK_NONBLOCK)

[Neil Horman]

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);
 	if (err < 0)
 		return err;
 

Re: SCTP_SOCKOPT_PEELOFF is missing SOCK_CLOEXEC (and SOCK_NONBLOCK)

[Marcelo Ricardo Leitner]

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.

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.

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)

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;

Re: SCTP_SOCKOPT_PEELOFF is missing SOCK_CLOEXEC (and SOCK_NONBLOCK)

[Neil Horman]

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.
> 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?

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
> 

Re: SCTP_SOCKOPT_PEELOFF is missing SOCK_CLOEXEC (and SOCK_NONBLOCK)

[Marcelo Ricardo Leitner]

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
> 

Re: SCTP_SOCKOPT_PEELOFF is missing SOCK_CLOEXEC (and SOCK_NONBLOCK)

[Neil Horman]

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.
> 
> 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.
> 
> 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)
> 
Thinking about this some more, I'm a bit hesitant to change the
sctp_peeloff_arg_t, since thats exposed to user space.  Instead, what if we use
the sd value in the peeloff arg to fetch the close_on_exec flag in the new fd?
Something like this (untested) patch:


diff --git a/net/sctp/socket.c b/net/sctp/socket.c
index f16c8d9..6386ac4 100644
--- a/net/sctp/socket.c
+++ b/net/sctp/socket.c
@@ -4939,6 +4939,8 @@ static int sctp_getsockopt_peeloff(struct sock *sk, int len, char __user *optval
 	sctp_peeloff_arg_t peeloff;
 	struct socket *newsock;
 	struct file *newfile;
+	struct file *oldfile
+	unsigned flags = 0;
 	int retval = 0;
 
 	if (len < sizeof(sctp_peeloff_arg_t))
@@ -4951,8 +4953,17 @@ static int sctp_getsockopt_peeloff(struct sock *sk, int len, char __user *optval
 	if (retval < 0)
 		goto out;
 
+	if (get_close_on_exec(peeloff.sd))
+		flags |= O_CLOEXEC;
+
+	oldfile = fget(peelof.sd);
+	if (oldfile) {
+		flags |= oldfile->f_flags & O_NONBLOCK;
+		fput(oldfile);
+	}
+
 	/* Map the socket to an unused fd that can be returned to the user.  */
-	retval = get_unused_fd_flags(0);
+	retval = get_unused_fd_flags(flags);
 	if (retval < 0) {
 		sock_release(newsock);
 		goto out;

Re: SCTP_SOCKOPT_PEELOFF is missing SOCK_CLOEXEC (and SOCK_NONBLOCK)

[Marcelo Ricardo Leitner]

On Tue, Jun 20, 2017 at 03:00:46PM -0400, Neil Horman wrote:
> On Tue, Jun 20, 2017 at 12:41:47PM -0300, Marcelo Ricardo Leitner wrote:
...
> > 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)
> > 
> Thinking about this some more, I'm a bit hesitant to change the
> sctp_peeloff_arg_t, since thats exposed to user space.  Instead, what if we use

+1

> the sd value in the peeloff arg to fetch the close_on_exec flag in the new fd?
> Something like this (untested) patch:

Yes. :-) That's similar to what I proposed, though you used peeloff.sd
to find the old fd and copy the flag from it and I used it as a pure
'flags' field instead.

I'm still not comfortable on hardwiring this copy. What if the
application doesn't want to inherit the flag?
accept() calls accept4(... , flags=0)
dup2() calls dup3(... , flags=0)
I don't see this direct inheritance anywhere else.

  Marcelo

> 
> 
> diff --git a/net/sctp/socket.c b/net/sctp/socket.c
> index f16c8d9..6386ac4 100644
> --- a/net/sctp/socket.c
> +++ b/net/sctp/socket.c
> @@ -4939,6 +4939,8 @@ static int sctp_getsockopt_peeloff(struct sock *sk, int len, char __user *optval
>  	sctp_peeloff_arg_t peeloff;
>  	struct socket *newsock;
>  	struct file *newfile;
> +	struct file *oldfile
> +	unsigned flags = 0;
>  	int retval = 0;
>  
>  	if (len < sizeof(sctp_peeloff_arg_t))
> @@ -4951,8 +4953,17 @@ static int sctp_getsockopt_peeloff(struct sock *sk, int len, char __user *optval
>  	if (retval < 0)
>  		goto out;
>  
> +	if (get_close_on_exec(peeloff.sd))
> +		flags |= O_CLOEXEC;
> +
> +	oldfile = fget(peelof.sd);
> +	if (oldfile) {
> +		flags |= oldfile->f_flags & O_NONBLOCK;
> +		fput(oldfile);
> +	}
> +
>  	/* Map the socket to an unused fd that can be returned to the user.  */
> -	retval = get_unused_fd_flags(0);
> +	retval = get_unused_fd_flags(flags);
>  	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
> 

Re: SCTP_SOCKOPT_PEELOFF is missing SOCK_CLOEXEC (and SOCK_NONBLOCK)

[Andreas Steinmetz]

Just as an idea:

Keep sctp_peeloff_arg_t as is.

Introduce sctp_peeloff2_arg_t with a flags field.

Decide in kernel space according to structure size if the "classic"
version or the "improved" version is used.

In case of the "classic" version keep the behaviour as is.

In case of the "improved" version set CLOEXEC/NONBLOCK according to the
flags set in userspace.

This way older applications that rely on current behavior are not
affected while new applications may use the improved model.
The userspace library should then present the new model with a
separate call, e.g. sctp_peeloff2.

Am Tuesday, den 20.06.2017, 16:21 -0300 schrieb Marcelo Ricardo
Leitner:
> On Tue, Jun 20, 2017 at 03:00:46PM -0400, Neil Horman wrote:
> > On Tue, Jun 20, 2017 at 12:41:47PM -0300, Marcelo Ricardo Leitner
> > wrote:
> 
> ...
> > > 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)
> > > 
> > 
> > Thinking about this some more, I'm a bit hesitant to change the
> > sctp_peeloff_arg_t, since thats exposed to user space.  Instead,
> > what if we use
> 
> +1
> 
> > the sd value in the peeloff arg to fetch the close_on_exec flag in
> > the new fd?
> > Something like this (untested) patch:
> 
> Yes. :-) That's similar to what I proposed, though you used
> peeloff.sd
> to find the old fd and copy the flag from it and I used it as a pure
> 'flags' field instead.
> 
> I'm still not comfortable on hardwiring this copy. What if the
> application doesn't want to inherit the flag?
> accept() calls accept4(... , flags=0)
> dup2() calls dup3(... , flags=0)
> I don't see this direct inheritance anywhere else.
> 
>   Marcelo
> 
> > 
> > 
> > diff --git a/net/sctp/socket.c b/net/sctp/socket.c
> > index f16c8d9..6386ac4 100644
> > --- a/net/sctp/socket.c
> > +++ b/net/sctp/socket.c
> > @@ -4939,6 +4939,8 @@ static int sctp_getsockopt_peeloff(struct
> > sock *sk, int len, char __user *optval
> >  	sctp_peeloff_arg_t peeloff;
> >  	struct socket *newsock;
> >  	struct file *newfile;
> > +	struct file *oldfile
> > +	unsigned flags = 0;
> >  	int retval = 0;
> >  
> >  	if (len < sizeof(sctp_peeloff_arg_t))
> > @@ -4951,8 +4953,17 @@ static int sctp_getsockopt_peeloff(struct
> > sock *sk, int len, char __user *optval
> >  	if (retval < 0)
> >  		goto out;
> >  
> > +	if (get_close_on_exec(peeloff.sd))
> > +		flags |= O_CLOEXEC;
> > +
> > +	oldfile = fget(peelof.sd);
> > +	if (oldfile) {
> > +		flags |= oldfile->f_flags & O_NONBLOCK;
> > +		fput(oldfile);
> > +	}
> > +
> >  	/* Map the socket to an unused fd that can be returned to
> > the user.  */
> > -	retval = get_unused_fd_flags(0);
> > +	retval = get_unused_fd_flags(flags);
> >  	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
> > 
-- 
Andreas Steinmetz                       SPAMmers use robotrap@domdv.de

Re: SCTP_SOCKOPT_PEELOFF is missing SOCK_CLOEXEC (and SOCK_NONBLOCK)

[Neil Horman]

On Tue, Jun 20, 2017 at 04:21:45PM -0300, Marcelo Ricardo Leitner wrote:
> On Tue, Jun 20, 2017 at 03:00:46PM -0400, Neil Horman wrote:
> > On Tue, Jun 20, 2017 at 12:41:47PM -0300, Marcelo Ricardo Leitner wrote:
> ...
> > > 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)
> > > 
> > Thinking about this some more, I'm a bit hesitant to change the
> > sctp_peeloff_arg_t, since thats exposed to user space.  Instead, what if we use
> 
> +1
> 
> > the sd value in the peeloff arg to fetch the close_on_exec flag in the new fd?
> > Something like this (untested) patch:
> 
> Yes. :-) That's similar to what I proposed, though you used peeloff.sd
> to find the old fd and copy the flag from it and I used it as a pure
> 'flags' field instead.
> 
> I'm still not comfortable on hardwiring this copy. What if the
> application doesn't want to inherit the flag?
> accept() calls accept4(... , flags=0)
> dup2() calls dup3(... , flags=0)
> I don't see this direct inheritance anywhere else.
> 
I agree, but this strikes me as something of a unique situation.  In alternate
cases of creating a new file descriptor within the same process as a clone of an
existing fd, we have dup/dup2 and dup3, with the former having defined behavior
of not copying the cloexec and nonblock flags, and the latter allowing them to
be explicitly specified for the new fd.

	In SCTP, we're creating a new fd, but have no express mechanism for
defining the new flags.  We could, as you say, add a flags field to the
peeloff_param_arg_t to provide that, but that has userspace ABI ramifications,
and makes programs less portable.

	Perhaps a new socket option SCTP_SOCKOPT_PEELOFF_FLAGS, and
corresponding lksctp-tools library function sctp_peeloff_flags, which accepts
the new fd's cloexec and nonblock flags as an argument?  That way at least, we
could define the origional peeloff behavior as not copying the flags, and allow
people to opt into the non-standard functions if they need it.  That would be in
keeping with how dup/dup2/dup3 were developed.

Thoughts?
Neil

>   Marcelo
> 
> > 
> > 
> > diff --git a/net/sctp/socket.c b/net/sctp/socket.c
> > index f16c8d9..6386ac4 100644
> > --- a/net/sctp/socket.c
> > +++ b/net/sctp/socket.c
> > @@ -4939,6 +4939,8 @@ static int sctp_getsockopt_peeloff(struct sock *sk, int len, char __user *optval
> >  	sctp_peeloff_arg_t peeloff;
> >  	struct socket *newsock;
> >  	struct file *newfile;
> > +	struct file *oldfile
> > +	unsigned flags = 0;
> >  	int retval = 0;
> >  
> >  	if (len < sizeof(sctp_peeloff_arg_t))
> > @@ -4951,8 +4953,17 @@ static int sctp_getsockopt_peeloff(struct sock *sk, int len, char __user *optval
> >  	if (retval < 0)
> >  		goto out;
> >  
> > +	if (get_close_on_exec(peeloff.sd))
> > +		flags |= O_CLOEXEC;
> > +
> > +	oldfile = fget(peelof.sd);
> > +	if (oldfile) {
> > +		flags |= oldfile->f_flags & O_NONBLOCK;
> > +		fput(oldfile);
> > +	}
> > +
> >  	/* Map the socket to an unused fd that can be returned to the user.  */
> > -	retval = get_unused_fd_flags(0);
> > +	retval = get_unused_fd_flags(flags);
> >  	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
> > 
> 

Re: SCTP_SOCKOPT_PEELOFF is missing SOCK_CLOEXEC (and SOCK_NONBLOCK)

[Marcelo Ricardo Leitner]

On Wed, Jun 21, 2017 at 08:27:14AM -0400, Neil Horman wrote:
> On Tue, Jun 20, 2017 at 04:21:45PM -0300, Marcelo Ricardo Leitner wrote:
> > On Tue, Jun 20, 2017 at 03:00:46PM -0400, Neil Horman wrote:
> > > On Tue, Jun 20, 2017 at 12:41:47PM -0300, Marcelo Ricardo Leitner wrote:
> > ...
> > > > 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)
> > > > 
> > > Thinking about this some more, I'm a bit hesitant to change the
> > > sctp_peeloff_arg_t, since thats exposed to user space.  Instead, what if we use
> > 
> > +1
> > 
> > > the sd value in the peeloff arg to fetch the close_on_exec flag in the new fd?
> > > Something like this (untested) patch:
> > 
> > Yes. :-) That's similar to what I proposed, though you used peeloff.sd
> > to find the old fd and copy the flag from it and I used it as a pure
> > 'flags' field instead.
> > 
> > I'm still not comfortable on hardwiring this copy. What if the
> > application doesn't want to inherit the flag?
> > accept() calls accept4(... , flags=0)
> > dup2() calls dup3(... , flags=0)
> > I don't see this direct inheritance anywhere else.
> > 
> I agree, but this strikes me as something of a unique situation.  In alternate
> cases of creating a new file descriptor within the same process as a clone of an
> existing fd, we have dup/dup2 and dup3, with the former having defined behavior
> of not copying the cloexec and nonblock flags, and the latter allowing them to
> be explicitly specified for the new fd.
> 
> 	In SCTP, we're creating a new fd, but have no express mechanism for
> defining the new flags.  We could, as you say, add a flags field to the
> peeloff_param_arg_t to provide that, but that has userspace ABI ramifications,
> and makes programs less portable.
> 
> 	Perhaps a new socket option SCTP_SOCKOPT_PEELOFF_FLAGS, and
> corresponding lksctp-tools library function sctp_peeloff_flags, which accepts
> the new fd's cloexec and nonblock flags as an argument?  That way at least, we
> could define the origional peeloff behavior as not copying the flags, and allow
> people to opt into the non-standard functions if they need it.  That would be in
> keeping with how dup/dup2/dup3 were developed.
> 
> Thoughts?
> Neil

That works for me.

We can't rely on using peeloff.sd to carry the flags because the
application may not have initialized it. It may be a variable in the
stack on which application simply did peeloff.asoc = X and we would be
working on unitialized data, so it's not safe.

On Andreas' idea to have a sctp_peeloff2_arg_t, it's also complicated
because the application is allowed to use a bigger-than-needed buffer
and in such cases it would lead us to the same situation as above.

So yes, I also think that the new SCTP_SOCKOPT_PEELOFF_FLAGS is the best
way out here.

Cheers,
Marcelo

Re: SCTP_SOCKOPT_PEELOFF is missing SOCK_CLOEXEC (and SOCK_NONBLOCK)

[Neil Horman]

On Wed, Jun 21, 2017 at 03:53:13PM -0300, Marcelo Ricardo Leitner wrote:
> On Wed, Jun 21, 2017 at 08:27:14AM -0400, Neil Horman wrote:
> > On Tue, Jun 20, 2017 at 04:21:45PM -0300, Marcelo Ricardo Leitner wrote:
> > > On Tue, Jun 20, 2017 at 03:00:46PM -0400, Neil Horman wrote:
> > > > On Tue, Jun 20, 2017 at 12:41:47PM -0300, Marcelo Ricardo Leitner wrote:
> > > ...
> > > > > 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)
> > > > > 
> > > > Thinking about this some more, I'm a bit hesitant to change the
> > > > sctp_peeloff_arg_t, since thats exposed to user space.  Instead, what if we use
> > > 
> > > +1
> > > 
> > > > the sd value in the peeloff arg to fetch the close_on_exec flag in the new fd?
> > > > Something like this (untested) patch:
> > > 
> > > Yes. :-) That's similar to what I proposed, though you used peeloff.sd
> > > to find the old fd and copy the flag from it and I used it as a pure
> > > 'flags' field instead.
> > > 
> > > I'm still not comfortable on hardwiring this copy. What if the
> > > application doesn't want to inherit the flag?
> > > accept() calls accept4(... , flags=0)
> > > dup2() calls dup3(... , flags=0)
> > > I don't see this direct inheritance anywhere else.
> > > 
> > I agree, but this strikes me as something of a unique situation.  In alternate
> > cases of creating a new file descriptor within the same process as a clone of an
> > existing fd, we have dup/dup2 and dup3, with the former having defined behavior
> > of not copying the cloexec and nonblock flags, and the latter allowing them to
> > be explicitly specified for the new fd.
> > 
> > 	In SCTP, we're creating a new fd, but have no express mechanism for
> > defining the new flags.  We could, as you say, add a flags field to the
> > peeloff_param_arg_t to provide that, but that has userspace ABI ramifications,
> > and makes programs less portable.
> > 
> > 	Perhaps a new socket option SCTP_SOCKOPT_PEELOFF_FLAGS, and
> > corresponding lksctp-tools library function sctp_peeloff_flags, which accepts
> > the new fd's cloexec and nonblock flags as an argument?  That way at least, we
> > could define the origional peeloff behavior as not copying the flags, and allow
> > people to opt into the non-standard functions if they need it.  That would be in
> > keeping with how dup/dup2/dup3 were developed.
> > 
> > Thoughts?
> > Neil
> 
> That works for me.
> 
> We can't rely on using peeloff.sd to carry the flags because the
> application may not have initialized it. It may be a variable in the
> stack on which application simply did peeloff.asoc = X and we would be
> working on unitialized data, so it's not safe.
> 
> On Andreas' idea to have a sctp_peeloff2_arg_t, it's also complicated
> because the application is allowed to use a bigger-than-needed buffer
> and in such cases it would lead us to the same situation as above.
> 
> So yes, I also think that the new SCTP_SOCKOPT_PEELOFF_FLAGS is the best
> way out here.
> 
> Cheers,
> Marcelo
> 
> 
I'll work up the patch tomorrow
Neil

Re: SCTP_SOCKOPT_PEELOFF is missing SOCK_CLOEXEC (and SOCK_NONBLOCK)

[Neil Horman]

On Wed, Jun 21, 2017 at 09:53:44PM -0400, Neil Horman wrote:
> On Wed, Jun 21, 2017 at 03:53:13PM -0300, Marcelo Ricardo Leitner wrote:
> > On Wed, Jun 21, 2017 at 08:27:14AM -0400, Neil Horman wrote:
> > > On Tue, Jun 20, 2017 at 04:21:45PM -0300, Marcelo Ricardo Leitner wrote:
> > > > On Tue, Jun 20, 2017 at 03:00:46PM -0400, Neil Horman wrote:
> > > > > On Tue, Jun 20, 2017 at 12:41:47PM -0300, Marcelo Ricardo Leitner wrote:
> > > > ...
> > > > > > 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)
> > > > > > 
> > > > > Thinking about this some more, I'm a bit hesitant to change the
> > > > > sctp_peeloff_arg_t, since thats exposed to user space.  Instead, what if we use
> > > > 
> > > > +1
> > > > 
> > > > > the sd value in the peeloff arg to fetch the close_on_exec flag in the new fd?
> > > > > Something like this (untested) patch:
> > > > 
> > > > Yes. :-) That's similar to what I proposed, though you used peeloff.sd
> > > > to find the old fd and copy the flag from it and I used it as a pure
> > > > 'flags' field instead.
> > > > 
> > > > I'm still not comfortable on hardwiring this copy. What if the
> > > > application doesn't want to inherit the flag?
> > > > accept() calls accept4(... , flags=0)
> > > > dup2() calls dup3(... , flags=0)
> > > > I don't see this direct inheritance anywhere else.
> > > > 
> > > I agree, but this strikes me as something of a unique situation.  In alternate
> > > cases of creating a new file descriptor within the same process as a clone of an
> > > existing fd, we have dup/dup2 and dup3, with the former having defined behavior
> > > of not copying the cloexec and nonblock flags, and the latter allowing them to
> > > be explicitly specified for the new fd.
> > > 
> > > 	In SCTP, we're creating a new fd, but have no express mechanism for
> > > defining the new flags.  We could, as you say, add a flags field to the
> > > peeloff_param_arg_t to provide that, but that has userspace ABI ramifications,
> > > and makes programs less portable.
> > > 
> > > 	Perhaps a new socket option SCTP_SOCKOPT_PEELOFF_FLAGS, and
> > > corresponding lksctp-tools library function sctp_peeloff_flags, which accepts
> > > the new fd's cloexec and nonblock flags as an argument?  That way at least, we
> > > could define the origional peeloff behavior as not copying the flags, and allow
> > > people to opt into the non-standard functions if they need it.  That would be in
> > > keeping with how dup/dup2/dup3 were developed.
> > > 
> > > Thoughts?
> > > Neil
> > 
> > That works for me.
> > 
> > We can't rely on using peeloff.sd to carry the flags because the
> > application may not have initialized it. It may be a variable in the
> > stack on which application simply did peeloff.asoc = X and we would be
> > working on unitialized data, so it's not safe.
> > 
> > On Andreas' idea to have a sctp_peeloff2_arg_t, it's also complicated
> > because the application is allowed to use a bigger-than-needed buffer
> > and in such cases it would lead us to the same situation as above.
> > 
> > So yes, I also think that the new SCTP_SOCKOPT_PEELOFF_FLAGS is the best
> > way out here.
> > 
> > Cheers,
> > Marcelo
> > 
> > 
> I'll work up the patch tomorrow
> Neil
> 
> --
> 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
> 

Ok, sorry for the delay.  I tested this patch out with the lksctp test suite to
validate that the traditional peeloff path works, but I don't have the userspace
code written to test the peeloff_flags path.  Andreas, if you could validate
that, I would appreciate it.  I'm not real thrilled with the export of the
set_close_on_exec function, but I'm not sure I like using the internal function
any better.  Either way, Andreas, if it works for you, I'll propose it
officially and we can go from there.

Neil


diff --git a/fs/file.c b/fs/file.c
index 1c2972e..a4521da 100644
--- a/fs/file.c
+++ b/fs/file.c
@@ -807,6 +807,7 @@ void set_close_on_exec(unsigned int fd, int flag)
 		__clear_close_on_exec(fd, fdt);
 	spin_unlock(&files->file_lock);
 }
+EXPORT_SYMBOL(set_close_on_exec);
 
 bool get_close_on_exec(unsigned int fd)
 {
diff --git a/include/uapi/linux/sctp.h b/include/uapi/linux/sctp.h
index ced9d8b..6217ff8 100644
--- a/include/uapi/linux/sctp.h
+++ b/include/uapi/linux/sctp.h
@@ -121,6 +121,7 @@ typedef __s32 sctp_assoc_t;
 #define SCTP_RESET_STREAMS	119
 #define SCTP_RESET_ASSOC	120
 #define SCTP_ADD_STREAMS	121
+#define SCTP_SOCKOPT_PEELOFF_FLAGS 122
 
 /* PR-SCTP policies */
 #define SCTP_PR_SCTP_NONE	0x0000
@@ -978,6 +979,11 @@ typedef struct {
 	int sd;
 } sctp_peeloff_arg_t;
 
+typedef struct {
+	sctp_peeloff_arg_t p_arg;
+	unsigned flags;
+} sctp_peeloff_flags_arg_t;
+
 /*
  *  Peer Address Thresholds socket option
  */
diff --git a/net/sctp/socket.c b/net/sctp/socket.c
index 7b6e20e..f210394 100644
--- a/net/sctp/socket.c
+++ b/net/sctp/socket.c
@@ -4933,20 +4933,12 @@ int sctp_do_peeloff(struct sock *sk, sctp_assoc_t id, struct socket **sockp)
 }
 EXPORT_SYMBOL(sctp_do_peeloff);
 
-static int sctp_getsockopt_peeloff(struct sock *sk, int len, char __user *optval, int __user *optlen)
+static int sctp_getsockopt_peeloff_common(struct sock *sk, sctp_peeloff_arg_t *peeloff, struct file **newfile)
 {
-	sctp_peeloff_arg_t peeloff;
 	struct socket *newsock;
-	struct file *newfile;
 	int retval = 0;
 
-	if (len < sizeof(sctp_peeloff_arg_t))
-		return -EINVAL;
-	len = sizeof(sctp_peeloff_arg_t);
-	if (copy_from_user(&peeloff, optval, len))
-		return -EFAULT;
-
-	retval = sctp_do_peeloff(sk, peeloff.associd, &newsock);
+	retval = sctp_do_peeloff(sk, peeloff->associd, &newsock);
 	if (retval < 0)
 		goto out;
 
@@ -4957,25 +4949,90 @@ static int sctp_getsockopt_peeloff(struct sock *sk, int len, char __user *optval
 		goto out;
 	}
 
-	newfile = sock_alloc_file(newsock, 0, NULL);
-	if (IS_ERR(newfile)) {
+	*newfile = sock_alloc_file(newsock, 0, NULL);
+	if (IS_ERR(*newfile)) {
 		put_unused_fd(retval);
 		sock_release(newsock);
-		return PTR_ERR(newfile);
+		retval = PTR_ERR(*newfile);
+		*newfile = NULL;
+		return retval;
 	}
 
 	pr_debug("%s: sk:%p, newsk:%p, sd:%d\n", __func__, sk, newsock->sk,
 		 retval);
+	
+	peeloff->sd = retval;
+out:
+	return retval;
+}
+
+static int sctp_getsockopt_peeloff(struct sock *sk, int len, char __user *optval, int __user *optlen)
+{
+	sctp_peeloff_arg_t peeloff;
+	struct file *newfile = NULL;
+	int retval = 0;
+
+	if (len < sizeof(sctp_peeloff_arg_t))
+		return -EINVAL;
+	len = sizeof(sctp_peeloff_arg_t);
+	if (copy_from_user(&peeloff, optval, len))
+		return -EFAULT;
+
+	retval = sctp_getsockopt_peeloff_common(sk, &peeloff, &newfile);
+	if (retval < 0)
+		goto out;
 
 	/* Return the fd mapped to the new socket.  */
 	if (put_user(len, optlen)) {
-		fput(newfile);
+		if (newfile)
+			fput(newfile);
 		put_unused_fd(retval);
 		return -EFAULT;
 	}
-	peeloff.sd = retval;
+
 	if (copy_to_user(optval, &peeloff, len)) {
-		fput(newfile);
+		if (newfile)
+			fput(newfile);
+		put_unused_fd(retval);
+		return -EFAULT;
+	}
+	fd_install(retval, newfile);
+out:
+	return retval;
+}
+
+static int sctp_getsockopt_peeloff_flags(struct sock *sk, int len, char __user *optval, int __user *optlen)
+{
+	sctp_peeloff_flags_arg_t peeloff;
+	struct file *newfile = NULL;
+	int retval = 0;
+
+	if (len < sizeof(sctp_peeloff_flags_arg_t))
+		return -EINVAL;
+	len = sizeof(sctp_peeloff_flags_arg_t);
+	if (copy_from_user(&peeloff, optval, len))
+		return -EFAULT;
+
+	retval = sctp_getsockopt_peeloff_common(sk, &peeloff.p_arg, &newfile);
+	if (retval < 0)
+		goto out;
+
+	set_close_on_exec(peeloff.p_arg.sd, !!(peeloff.flags & SOCK_CLOEXEC));
+
+	if (peeloff.flags & SOCK_NONBLOCK)
+		newfile->f_flags |= O_NONBLOCK;
+
+	/* Return the fd mapped to the new socket.  */
+	if (put_user(len, optlen)) {
+		if (newfile)
+			fput(newfile);
+		put_unused_fd(retval);
+		return -EFAULT;
+	}
+
+	if (copy_to_user(optval, &peeloff, len)) {
+		if (newfile)
+			fput(newfile);
 		put_unused_fd(retval);
 		return -EFAULT;
 	}
@@ -6758,6 +6815,9 @@ static int sctp_getsockopt(struct sock *sk, int level, int optname,
 	case SCTP_SOCKOPT_PEELOFF:
 		retval = sctp_getsockopt_peeloff(sk, len, optval, optlen);
 		break;
+	case SCTP_SOCKOPT_PEELOFF_FLAGS:
+		retval = sctp_getsockopt_peeloff_flags(sk, len, optval, optlen);
+		break;
 	case SCTP_PEER_ADDR_PARAMS:
 		retval = sctp_getsockopt_peer_addr_params(sk, len, optval,
 							  optlen);

Re: SCTP_SOCKOPT_PEELOFF is missing SOCK_CLOEXEC (and SOCK_NONBLOCK)

[Marcelo Ricardo Leitner]

On Fri, Jun 23, 2017 at 03:14:33PM -0400, Neil Horman wrote:
> On Wed, Jun 21, 2017 at 09:53:44PM -0400, Neil Horman wrote:
> > On Wed, Jun 21, 2017 at 03:53:13PM -0300, Marcelo Ricardo Leitner wrote:
> > > On Wed, Jun 21, 2017 at 08:27:14AM -0400, Neil Horman wrote:
> > > > On Tue, Jun 20, 2017 at 04:21:45PM -0300, Marcelo Ricardo Leitner wrote:
> > > > > On Tue, Jun 20, 2017 at 03:00:46PM -0400, Neil Horman wrote:
> > > > > > On Tue, Jun 20, 2017 at 12:41:47PM -0300, Marcelo Ricardo Leitner wrote:
> > > > > ...
> > > > > > > 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)
> > > > > > > 
> > > > > > Thinking about this some more, I'm a bit hesitant to change the
> > > > > > sctp_peeloff_arg_t, since thats exposed to user space.  Instead, what if we use
> > > > > 
> > > > > +1
> > > > > 
> > > > > > the sd value in the peeloff arg to fetch the close_on_exec flag in the new fd?
> > > > > > Something like this (untested) patch:
> > > > > 
> > > > > Yes. :-) That's similar to what I proposed, though you used peeloff.sd
> > > > > to find the old fd and copy the flag from it and I used it as a pure
> > > > > 'flags' field instead.
> > > > > 
> > > > > I'm still not comfortable on hardwiring this copy. What if the
> > > > > application doesn't want to inherit the flag?
> > > > > accept() calls accept4(... , flags=0)
> > > > > dup2() calls dup3(... , flags=0)
> > > > > I don't see this direct inheritance anywhere else.
> > > > > 
> > > > I agree, but this strikes me as something of a unique situation.  In alternate
> > > > cases of creating a new file descriptor within the same process as a clone of an
> > > > existing fd, we have dup/dup2 and dup3, with the former having defined behavior
> > > > of not copying the cloexec and nonblock flags, and the latter allowing them to
> > > > be explicitly specified for the new fd.
> > > > 
> > > > 	In SCTP, we're creating a new fd, but have no express mechanism for
> > > > defining the new flags.  We could, as you say, add a flags field to the
> > > > peeloff_param_arg_t to provide that, but that has userspace ABI ramifications,
> > > > and makes programs less portable.
> > > > 
> > > > 	Perhaps a new socket option SCTP_SOCKOPT_PEELOFF_FLAGS, and
> > > > corresponding lksctp-tools library function sctp_peeloff_flags, which accepts
> > > > the new fd's cloexec and nonblock flags as an argument?  That way at least, we
> > > > could define the origional peeloff behavior as not copying the flags, and allow
> > > > people to opt into the non-standard functions if they need it.  That would be in
> > > > keeping with how dup/dup2/dup3 were developed.
> > > > 
> > > > Thoughts?
> > > > Neil
> > > 
> > > That works for me.
> > > 
> > > We can't rely on using peeloff.sd to carry the flags because the
> > > application may not have initialized it. It may be a variable in the
> > > stack on which application simply did peeloff.asoc = X and we would be
> > > working on unitialized data, so it's not safe.
> > > 
> > > On Andreas' idea to have a sctp_peeloff2_arg_t, it's also complicated
> > > because the application is allowed to use a bigger-than-needed buffer
> > > and in such cases it would lead us to the same situation as above.
> > > 
> > > So yes, I also think that the new SCTP_SOCKOPT_PEELOFF_FLAGS is the best
> > > way out here.
> > > 
> > > Cheers,
> > > Marcelo
> > > 
> > > 
> > I'll work up the patch tomorrow
> > Neil
> > 
> > --
> > 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
> > 
> 
> Ok, sorry for the delay.  I tested this patch out with the lksctp test suite to
> validate that the traditional peeloff path works, but I don't have the userspace
> code written to test the peeloff_flags path.  Andreas, if you could validate
> that, I would appreciate it.  I'm not real thrilled with the export of the
> set_close_on_exec function, but I'm not sure I like using the internal function
> any better.  Either way, Andreas, if it works for you, I'll propose it
> officially and we can go from there.

What about exposing a flags arg on sctp_getsockopt_peeloff_common()?
Then I guess you don't need such post-processing as you can pass it to
get_unused_fd_flags() directly.

  Marcelo

> 
> Neil
> 
> 
> diff --git a/fs/file.c b/fs/file.c
> index 1c2972e..a4521da 100644
> --- a/fs/file.c
> +++ b/fs/file.c
> @@ -807,6 +807,7 @@ void set_close_on_exec(unsigned int fd, int flag)
>  		__clear_close_on_exec(fd, fdt);
>  	spin_unlock(&files->file_lock);
>  }
> +EXPORT_SYMBOL(set_close_on_exec);
>  
>  bool get_close_on_exec(unsigned int fd)
>  {
> diff --git a/include/uapi/linux/sctp.h b/include/uapi/linux/sctp.h
> index ced9d8b..6217ff8 100644
> --- a/include/uapi/linux/sctp.h
> +++ b/include/uapi/linux/sctp.h
> @@ -121,6 +121,7 @@ typedef __s32 sctp_assoc_t;
>  #define SCTP_RESET_STREAMS	119
>  #define SCTP_RESET_ASSOC	120
>  #define SCTP_ADD_STREAMS	121
> +#define SCTP_SOCKOPT_PEELOFF_FLAGS 122
>  
>  /* PR-SCTP policies */
>  #define SCTP_PR_SCTP_NONE	0x0000
> @@ -978,6 +979,11 @@ typedef struct {
>  	int sd;
>  } sctp_peeloff_arg_t;
>  
> +typedef struct {
> +	sctp_peeloff_arg_t p_arg;
> +	unsigned flags;
> +} sctp_peeloff_flags_arg_t;
> +
>  /*
>   *  Peer Address Thresholds socket option
>   */
> diff --git a/net/sctp/socket.c b/net/sctp/socket.c
> index 7b6e20e..f210394 100644
> --- a/net/sctp/socket.c
> +++ b/net/sctp/socket.c
> @@ -4933,20 +4933,12 @@ int sctp_do_peeloff(struct sock *sk, sctp_assoc_t id, struct socket **sockp)
>  }
>  EXPORT_SYMBOL(sctp_do_peeloff);
>  
> -static int sctp_getsockopt_peeloff(struct sock *sk, int len, char __user *optval, int __user *optlen)
> +static int sctp_getsockopt_peeloff_common(struct sock *sk, sctp_peeloff_arg_t *peeloff, struct file **newfile)
>  {
> -	sctp_peeloff_arg_t peeloff;
>  	struct socket *newsock;
> -	struct file *newfile;
>  	int retval = 0;
>  
> -	if (len < sizeof(sctp_peeloff_arg_t))
> -		return -EINVAL;
> -	len = sizeof(sctp_peeloff_arg_t);
> -	if (copy_from_user(&peeloff, optval, len))
> -		return -EFAULT;
> -
> -	retval = sctp_do_peeloff(sk, peeloff.associd, &newsock);
> +	retval = sctp_do_peeloff(sk, peeloff->associd, &newsock);
>  	if (retval < 0)
>  		goto out;
>  
> @@ -4957,25 +4949,90 @@ static int sctp_getsockopt_peeloff(struct sock *sk, int len, char __user *optval
>  		goto out;
>  	}
>  
> -	newfile = sock_alloc_file(newsock, 0, NULL);
> -	if (IS_ERR(newfile)) {
> +	*newfile = sock_alloc_file(newsock, 0, NULL);
> +	if (IS_ERR(*newfile)) {
>  		put_unused_fd(retval);
>  		sock_release(newsock);
> -		return PTR_ERR(newfile);
> +		retval = PTR_ERR(*newfile);
> +		*newfile = NULL;
> +		return retval;
>  	}
>  
>  	pr_debug("%s: sk:%p, newsk:%p, sd:%d\n", __func__, sk, newsock->sk,
>  		 retval);
> +	
> +	peeloff->sd = retval;
> +out:
> +	return retval;
> +}
> +
> +static int sctp_getsockopt_peeloff(struct sock *sk, int len, char __user *optval, int __user *optlen)
> +{
> +	sctp_peeloff_arg_t peeloff;
> +	struct file *newfile = NULL;
> +	int retval = 0;
> +
> +	if (len < sizeof(sctp_peeloff_arg_t))
> +		return -EINVAL;
> +	len = sizeof(sctp_peeloff_arg_t);
> +	if (copy_from_user(&peeloff, optval, len))
> +		return -EFAULT;
> +
> +	retval = sctp_getsockopt_peeloff_common(sk, &peeloff, &newfile);
> +	if (retval < 0)
> +		goto out;
>  
>  	/* Return the fd mapped to the new socket.  */
>  	if (put_user(len, optlen)) {
> -		fput(newfile);
> +		if (newfile)
> +			fput(newfile);
>  		put_unused_fd(retval);
>  		return -EFAULT;
>  	}
> -	peeloff.sd = retval;
> +
>  	if (copy_to_user(optval, &peeloff, len)) {
> -		fput(newfile);
> +		if (newfile)
> +			fput(newfile);
> +		put_unused_fd(retval);
> +		return -EFAULT;
> +	}
> +	fd_install(retval, newfile);
> +out:
> +	return retval;
> +}
> +
> +static int sctp_getsockopt_peeloff_flags(struct sock *sk, int len, char __user *optval, int __user *optlen)
> +{
> +	sctp_peeloff_flags_arg_t peeloff;
> +	struct file *newfile = NULL;
> +	int retval = 0;
> +
> +	if (len < sizeof(sctp_peeloff_flags_arg_t))
> +		return -EINVAL;
> +	len = sizeof(sctp_peeloff_flags_arg_t);
> +	if (copy_from_user(&peeloff, optval, len))
> +		return -EFAULT;
> +
> +	retval = sctp_getsockopt_peeloff_common(sk, &peeloff.p_arg, &newfile);
> +	if (retval < 0)
> +		goto out;
> +
> +	set_close_on_exec(peeloff.p_arg.sd, !!(peeloff.flags & SOCK_CLOEXEC));
> +
> +	if (peeloff.flags & SOCK_NONBLOCK)
> +		newfile->f_flags |= O_NONBLOCK;
> +
> +	/* Return the fd mapped to the new socket.  */
> +	if (put_user(len, optlen)) {
> +		if (newfile)
> +			fput(newfile);
> +		put_unused_fd(retval);
> +		return -EFAULT;
> +	}
> +
> +	if (copy_to_user(optval, &peeloff, len)) {
> +		if (newfile)
> +			fput(newfile);
>  		put_unused_fd(retval);
>  		return -EFAULT;
>  	}
> @@ -6758,6 +6815,9 @@ static int sctp_getsockopt(struct sock *sk, int level, int optname,
>  	case SCTP_SOCKOPT_PEELOFF:
>  		retval = sctp_getsockopt_peeloff(sk, len, optval, optlen);
>  		break;
> +	case SCTP_SOCKOPT_PEELOFF_FLAGS:
> +		retval = sctp_getsockopt_peeloff_flags(sk, len, optval, optlen);
> +		break;
>  	case SCTP_PEER_ADDR_PARAMS:
>  		retval = sctp_getsockopt_peer_addr_params(sk, len, optval,
>  							  optlen);
> --
> 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
> 

Re: SCTP_SOCKOPT_PEELOFF is missing SOCK_CLOEXEC (and SOCK_NONBLOCK)

[Neil Horman]

On Fri, Jun 23, 2017 at 04:33:15PM -0300, Marcelo Ricardo Leitner wrote:
> On Fri, Jun 23, 2017 at 03:14:33PM -0400, Neil Horman wrote:
> > On Wed, Jun 21, 2017 at 09:53:44PM -0400, Neil Horman wrote:
> > > On Wed, Jun 21, 2017 at 03:53:13PM -0300, Marcelo Ricardo Leitner wrote:
> > > > On Wed, Jun 21, 2017 at 08:27:14AM -0400, Neil Horman wrote:
> > > > > On Tue, Jun 20, 2017 at 04:21:45PM -0300, Marcelo Ricardo Leitner wrote:
> > > > > > On Tue, Jun 20, 2017 at 03:00:46PM -0400, Neil Horman wrote:
> > > > > > > On Tue, Jun 20, 2017 at 12:41:47PM -0300, Marcelo Ricardo Leitner wrote:
> > > > > > ...
> > > > > > > > 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)
> > > > > > > > 
> > > > > > > Thinking about this some more, I'm a bit hesitant to change the
> > > > > > > sctp_peeloff_arg_t, since thats exposed to user space.  Instead, what if we use
> > > > > > 
> > > > > > +1
> > > > > > 
> > > > > > > the sd value in the peeloff arg to fetch the close_on_exec flag in the new fd?
> > > > > > > Something like this (untested) patch:
> > > > > > 
> > > > > > Yes. :-) That's similar to what I proposed, though you used peeloff.sd
> > > > > > to find the old fd and copy the flag from it and I used it as a pure
> > > > > > 'flags' field instead.
> > > > > > 
> > > > > > I'm still not comfortable on hardwiring this copy. What if the
> > > > > > application doesn't want to inherit the flag?
> > > > > > accept() calls accept4(... , flags=0)
> > > > > > dup2() calls dup3(... , flags=0)
> > > > > > I don't see this direct inheritance anywhere else.
> > > > > > 
> > > > > I agree, but this strikes me as something of a unique situation.  In alternate
> > > > > cases of creating a new file descriptor within the same process as a clone of an
> > > > > existing fd, we have dup/dup2 and dup3, with the former having defined behavior
> > > > > of not copying the cloexec and nonblock flags, and the latter allowing them to
> > > > > be explicitly specified for the new fd.
> > > > > 
> > > > > 	In SCTP, we're creating a new fd, but have no express mechanism for
> > > > > defining the new flags.  We could, as you say, add a flags field to the
> > > > > peeloff_param_arg_t to provide that, but that has userspace ABI ramifications,
> > > > > and makes programs less portable.
> > > > > 
> > > > > 	Perhaps a new socket option SCTP_SOCKOPT_PEELOFF_FLAGS, and
> > > > > corresponding lksctp-tools library function sctp_peeloff_flags, which accepts
> > > > > the new fd's cloexec and nonblock flags as an argument?  That way at least, we
> > > > > could define the origional peeloff behavior as not copying the flags, and allow
> > > > > people to opt into the non-standard functions if they need it.  That would be in
> > > > > keeping with how dup/dup2/dup3 were developed.
> > > > > 
> > > > > Thoughts?
> > > > > Neil
> > > > 
> > > > That works for me.
> > > > 
> > > > We can't rely on using peeloff.sd to carry the flags because the
> > > > application may not have initialized it. It may be a variable in the
> > > > stack on which application simply did peeloff.asoc = X and we would be
> > > > working on unitialized data, so it's not safe.
> > > > 
> > > > On Andreas' idea to have a sctp_peeloff2_arg_t, it's also complicated
> > > > because the application is allowed to use a bigger-than-needed buffer
> > > > and in such cases it would lead us to the same situation as above.
> > > > 
> > > > So yes, I also think that the new SCTP_SOCKOPT_PEELOFF_FLAGS is the best
> > > > way out here.
> > > > 
> > > > Cheers,
> > > > Marcelo
> > > > 
> > > > 
> > > I'll work up the patch tomorrow
> > > Neil
> > > 
> > > --
> > > 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
> > > 
> > 
> > Ok, sorry for the delay.  I tested this patch out with the lksctp test suite to
> > validate that the traditional peeloff path works, but I don't have the userspace
> > code written to test the peeloff_flags path.  Andreas, if you could validate
> > that, I would appreciate it.  I'm not real thrilled with the export of the
> > set_close_on_exec function, but I'm not sure I like using the internal function
> > any better.  Either way, Andreas, if it works for you, I'll propose it
> > officially and we can go from there.
> 
> What about exposing a flags arg on sctp_getsockopt_peeloff_common()?
> Then I guess you don't need such post-processing as you can pass it to
> get_unused_fd_flags() directly.
> 
>   Marcelo
> 
Yeah, that makes sense, and then default the flags to 0 on
sctp_getsockopt_peelof.  Andreas, can you please test the patch as-is right now,
just to make sure the path with flags set works properly, and I'll clean this up
in parallel while you do that ahead of official submission?

Thanks
Neil

> > 
> > Neil
> > 
> > 
> > diff --git a/fs/file.c b/fs/file.c
> > index 1c2972e..a4521da 100644
> > --- a/fs/file.c
> > +++ b/fs/file.c
> > @@ -807,6 +807,7 @@ void set_close_on_exec(unsigned int fd, int flag)
> >  		__clear_close_on_exec(fd, fdt);
> >  	spin_unlock(&files->file_lock);
> >  }
> > +EXPORT_SYMBOL(set_close_on_exec);
> >  
> >  bool get_close_on_exec(unsigned int fd)
> >  {
> > diff --git a/include/uapi/linux/sctp.h b/include/uapi/linux/sctp.h
> > index ced9d8b..6217ff8 100644
> > --- a/include/uapi/linux/sctp.h
> > +++ b/include/uapi/linux/sctp.h
> > @@ -121,6 +121,7 @@ typedef __s32 sctp_assoc_t;
> >  #define SCTP_RESET_STREAMS	119
> >  #define SCTP_RESET_ASSOC	120
> >  #define SCTP_ADD_STREAMS	121
> > +#define SCTP_SOCKOPT_PEELOFF_FLAGS 122
> >  
> >  /* PR-SCTP policies */
> >  #define SCTP_PR_SCTP_NONE	0x0000
> > @@ -978,6 +979,11 @@ typedef struct {
> >  	int sd;
> >  } sctp_peeloff_arg_t;
> >  
> > +typedef struct {
> > +	sctp_peeloff_arg_t p_arg;
> > +	unsigned flags;
> > +} sctp_peeloff_flags_arg_t;
> > +
> >  /*
> >   *  Peer Address Thresholds socket option
> >   */
> > diff --git a/net/sctp/socket.c b/net/sctp/socket.c
> > index 7b6e20e..f210394 100644
> > --- a/net/sctp/socket.c
> > +++ b/net/sctp/socket.c
> > @@ -4933,20 +4933,12 @@ int sctp_do_peeloff(struct sock *sk, sctp_assoc_t id, struct socket **sockp)
> >  }
> >  EXPORT_SYMBOL(sctp_do_peeloff);
> >  
> > -static int sctp_getsockopt_peeloff(struct sock *sk, int len, char __user *optval, int __user *optlen)
> > +static int sctp_getsockopt_peeloff_common(struct sock *sk, sctp_peeloff_arg_t *peeloff, struct file **newfile)
> >  {
> > -	sctp_peeloff_arg_t peeloff;
> >  	struct socket *newsock;
> > -	struct file *newfile;
> >  	int retval = 0;
> >  
> > -	if (len < sizeof(sctp_peeloff_arg_t))
> > -		return -EINVAL;
> > -	len = sizeof(sctp_peeloff_arg_t);
> > -	if (copy_from_user(&peeloff, optval, len))
> > -		return -EFAULT;
> > -
> > -	retval = sctp_do_peeloff(sk, peeloff.associd, &newsock);
> > +	retval = sctp_do_peeloff(sk, peeloff->associd, &newsock);
> >  	if (retval < 0)
> >  		goto out;
> >  
> > @@ -4957,25 +4949,90 @@ static int sctp_getsockopt_peeloff(struct sock *sk, int len, char __user *optval
> >  		goto out;
> >  	}
> >  
> > -	newfile = sock_alloc_file(newsock, 0, NULL);
> > -	if (IS_ERR(newfile)) {
> > +	*newfile = sock_alloc_file(newsock, 0, NULL);
> > +	if (IS_ERR(*newfile)) {
> >  		put_unused_fd(retval);
> >  		sock_release(newsock);
> > -		return PTR_ERR(newfile);
> > +		retval = PTR_ERR(*newfile);
> > +		*newfile = NULL;
> > +		return retval;
> >  	}
> >  
> >  	pr_debug("%s: sk:%p, newsk:%p, sd:%d\n", __func__, sk, newsock->sk,
> >  		 retval);
> > +	
> > +	peeloff->sd = retval;
> > +out:
> > +	return retval;
> > +}
> > +
> > +static int sctp_getsockopt_peeloff(struct sock *sk, int len, char __user *optval, int __user *optlen)
> > +{
> > +	sctp_peeloff_arg_t peeloff;
> > +	struct file *newfile = NULL;
> > +	int retval = 0;
> > +
> > +	if (len < sizeof(sctp_peeloff_arg_t))
> > +		return -EINVAL;
> > +	len = sizeof(sctp_peeloff_arg_t);
> > +	if (copy_from_user(&peeloff, optval, len))
> > +		return -EFAULT;
> > +
> > +	retval = sctp_getsockopt_peeloff_common(sk, &peeloff, &newfile);
> > +	if (retval < 0)
> > +		goto out;
> >  
> >  	/* Return the fd mapped to the new socket.  */
> >  	if (put_user(len, optlen)) {
> > -		fput(newfile);
> > +		if (newfile)
> > +			fput(newfile);
> >  		put_unused_fd(retval);
> >  		return -EFAULT;
> >  	}
> > -	peeloff.sd = retval;
> > +
> >  	if (copy_to_user(optval, &peeloff, len)) {
> > -		fput(newfile);
> > +		if (newfile)
> > +			fput(newfile);
> > +		put_unused_fd(retval);
> > +		return -EFAULT;
> > +	}
> > +	fd_install(retval, newfile);
> > +out:
> > +	return retval;
> > +}
> > +
> > +static int sctp_getsockopt_peeloff_flags(struct sock *sk, int len, char __user *optval, int __user *optlen)
> > +{
> > +	sctp_peeloff_flags_arg_t peeloff;
> > +	struct file *newfile = NULL;
> > +	int retval = 0;
> > +
> > +	if (len < sizeof(sctp_peeloff_flags_arg_t))
> > +		return -EINVAL;
> > +	len = sizeof(sctp_peeloff_flags_arg_t);
> > +	if (copy_from_user(&peeloff, optval, len))
> > +		return -EFAULT;
> > +
> > +	retval = sctp_getsockopt_peeloff_common(sk, &peeloff.p_arg, &newfile);
> > +	if (retval < 0)
> > +		goto out;
> > +
> > +	set_close_on_exec(peeloff.p_arg.sd, !!(peeloff.flags & SOCK_CLOEXEC));
> > +
> > +	if (peeloff.flags & SOCK_NONBLOCK)
> > +		newfile->f_flags |= O_NONBLOCK;
> > +
> > +	/* Return the fd mapped to the new socket.  */
> > +	if (put_user(len, optlen)) {
> > +		if (newfile)
> > +			fput(newfile);
> > +		put_unused_fd(retval);
> > +		return -EFAULT;
> > +	}
> > +
> > +	if (copy_to_user(optval, &peeloff, len)) {
> > +		if (newfile)
> > +			fput(newfile);
> >  		put_unused_fd(retval);
> >  		return -EFAULT;
> >  	}
> > @@ -6758,6 +6815,9 @@ static int sctp_getsockopt(struct sock *sk, int level, int optname,
> >  	case SCTP_SOCKOPT_PEELOFF:
> >  		retval = sctp_getsockopt_peeloff(sk, len, optval, optlen);
> >  		break;
> > +	case SCTP_SOCKOPT_PEELOFF_FLAGS:
> > +		retval = sctp_getsockopt_peeloff_flags(sk, len, optval, optlen);
> > +		break;
> >  	case SCTP_PEER_ADDR_PARAMS:
> >  		retval = sctp_getsockopt_peer_addr_params(sk, len, optval,
> >  							  optlen);
> > --
> > 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
> > 
> 

Re: SCTP_SOCKOPT_PEELOFF is missing SOCK_CLOEXEC (and SOCK_NONBLOCK)

[Andreas Steinmetz]

A quick test with retrieving the flags with fcntl(F_GETFD/F_GETFL)
after peeloff shows that your patch works. Good job!

Am Sunday, den 25.06.2017, 08:06 -0400 schrieb Neil Horman:
> On Fri, Jun 23, 2017 at 04:33:15PM -0300, Marcelo Ricardo Leitner
> wrote:
> > On Fri, Jun 23, 2017 at 03:14:33PM -0400, Neil Horman wrote:
> > > On Wed, Jun 21, 2017 at 09:53:44PM -0400, Neil Horman wrote:
> > > > On Wed, Jun 21, 2017 at 03:53:13PM -0300, Marcelo Ricardo
> > > > Leitner wrote:
> > > > > On Wed, Jun 21, 2017 at 08:27:14AM -0400, Neil Horman wrote:
> > > > > > On Tue, Jun 20, 2017 at 04:21:45PM -0300, Marcelo Ricardo
> > > > > > Leitner wrote:
> > > > > > > On Tue, Jun 20, 2017 at 03:00:46PM -0400, Neil Horman
> > > > > > > wrote:
> > > > > > > > On Tue, Jun 20, 2017 at 12:41:47PM -0300, Marcelo
> > > > > > > > Ricardo Leitner wrote:
> > > > > > > 
> > > > > > > ...
> > > > > > > > > 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)
> > > > > > > > > 
> > > > > > > > 
> > > > > > > > Thinking about this some more, I'm a bit hesitant to
> > > > > > > > change the
> > > > > > > > sctp_peeloff_arg_t, since thats exposed to user
> > > > > > > > space.  Instead, what if we use
> > > > > > > 
> > > > > > > +1
> > > > > > > 
> > > > > > > > the sd value in the peeloff arg to fetch the
> > > > > > > > close_on_exec flag in the new fd?
> > > > > > > > Something like this (untested) patch:
> > > > > > > 
> > > > > > > Yes. :-) That's similar to what I proposed, though you
> > > > > > > used peeloff.sd
> > > > > > > to find the old fd and copy the flag from it and I used
> > > > > > > it as a pure
> > > > > > > 'flags' field instead.
> > > > > > > 
> > > > > > > I'm still not comfortable on hardwiring this copy. What
> > > > > > > if the
> > > > > > > application doesn't want to inherit the flag?
> > > > > > > accept() calls accept4(... , flags=0)
> > > > > > > dup2() calls dup3(... , flags=0)
> > > > > > > I don't see this direct inheritance anywhere else.
> > > > > > > 
> > > > > > 
> > > > > > I agree, but this strikes me as something of a unique
> > > > > > situation.  In alternate
> > > > > > cases of creating a new file descriptor within the same
> > > > > > process as a clone of an
> > > > > > existing fd, we have dup/dup2 and dup3, with the former
> > > > > > having defined behavior
> > > > > > of not copying the cloexec and nonblock flags, and the
> > > > > > latter allowing them to
> > > > > > be explicitly specified for the new fd.
> > > > > > 
> > > > > > 	In SCTP, we're creating a new fd, but have no express
> > > > > > mechanism for
> > > > > > defining the new flags.  We could, as you say, add a flags
> > > > > > field to the
> > > > > > peeloff_param_arg_t to provide that, but that has userspace
> > > > > > ABI ramifications,
> > > > > > and makes programs less portable.
> > > > > > 
> > > > > > 	Perhaps a new socket option SCTP_SOCKOPT_PEELOFF_FLAGS,
> > > > > > and
> > > > > > corresponding lksctp-tools library function
> > > > > > sctp_peeloff_flags, which accepts
> > > > > > the new fd's cloexec and nonblock flags as an
> > > > > > argument?  That way at least, we
> > > > > > could define the origional peeloff behavior as not copying
> > > > > > the flags, and allow
> > > > > > people to opt into the non-standard functions if they need
> > > > > > it.  That would be in
> > > > > > keeping with how dup/dup2/dup3 were developed.
> > > > > > 
> > > > > > Thoughts?
> > > > > > Neil
> > > > > 
> > > > > That works for me.
> > > > > 
> > > > > We can't rely on using peeloff.sd to carry the flags because
> > > > > the
> > > > > application may not have initialized it. It may be a variable
> > > > > in the
> > > > > stack on which application simply did peeloff.asoc = X and we
> > > > > would be
> > > > > working on unitialized data, so it's not safe.
> > > > > 
> > > > > On Andreas' idea to have a sctp_peeloff2_arg_t, it's also
> > > > > complicated
> > > > > because the application is allowed to use a bigger-than-
> > > > > needed buffer
> > > > > and in such cases it would lead us to the same situation as
> > > > > above.
> > > > > 
> > > > > So yes, I also think that the new SCTP_SOCKOPT_PEELOFF_FLAGS
> > > > > is the best
> > > > > way out here.
> > > > > 
> > > > > Cheers,
> > > > > Marcelo
> > > > > 
> > > > > 
> > > > 
> > > > I'll work up the patch tomorrow
> > > > Neil
> > > > 
> > > > --
> > > > 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.h
> > > > tml
> > > > 
> > > 
> > > Ok, sorry for the delay.  I tested this patch out with the lksctp
> > > test suite to
> > > validate that the traditional peeloff path works, but I don't
> > > have the userspace
> > > code written to test the peeloff_flags path.  Andreas, if you
> > > could validate
> > > that, I would appreciate it.  I'm not real thrilled with the
> > > export of the
> > > set_close_on_exec function, but I'm not sure I like using the
> > > internal function
> > > any better.  Either way, Andreas, if it works for you, I'll
> > > propose it
> > > officially and we can go from there.
> > 
> > What about exposing a flags arg on
> > sctp_getsockopt_peeloff_common()?
> > Then I guess you don't need such post-processing as you can pass it
> > to
> > get_unused_fd_flags() directly.
> > 
> >   Marcelo
> > 
> 
> Yeah, that makes sense, and then default the flags to 0 on
> sctp_getsockopt_peelof.  Andreas, can you please test the patch as-is 
> right now,
> just to make sure the path with flags set works properly, and I'll
> clean this up
> in parallel while you do that ahead of official submission?
> 
> Thanks
> Neil
> 
> > > 
> > > Neil
> > > 
> > > 
> > > diff --git a/fs/file.c b/fs/file.c
> > > index 1c2972e..a4521da 100644
> > > --- a/fs/file.c
> > > +++ b/fs/file.c
> > > @@ -807,6 +807,7 @@ void set_close_on_exec(unsigned int fd, int
> > > flag)
> > >  		__clear_close_on_exec(fd, fdt);
> > >  	spin_unlock(&files->file_lock);
> > >  }
> > > +EXPORT_SYMBOL(set_close_on_exec);
> > >  
> > >  bool get_close_on_exec(unsigned int fd)
> > >  {
> > > diff --git a/include/uapi/linux/sctp.h
> > > b/include/uapi/linux/sctp.h
> > > index ced9d8b..6217ff8 100644
> > > --- a/include/uapi/linux/sctp.h
> > > +++ b/include/uapi/linux/sctp.h
> > > @@ -121,6 +121,7 @@ typedef __s32 sctp_assoc_t;
> > >  #define SCTP_RESET_STREAMS	119
> > >  #define SCTP_RESET_ASSOC	120
> > >  #define SCTP_ADD_STREAMS	121
> > > +#define SCTP_SOCKOPT_PEELOFF_FLAGS 122
> > >  
> > >  /* PR-SCTP policies */
> > >  #define SCTP_PR_SCTP_NONE	0x0000
> > > @@ -978,6 +979,11 @@ typedef struct {
> > >  	int sd;
> > >  } sctp_peeloff_arg_t;
> > >  
> > > +typedef struct {
> > > +	sctp_peeloff_arg_t p_arg;
> > > +	unsigned flags;
> > > +} sctp_peeloff_flags_arg_t;
> > > +
> > >  /*
> > >   *  Peer Address Thresholds socket option
> > >   */
> > > diff --git a/net/sctp/socket.c b/net/sctp/socket.c
> > > index 7b6e20e..f210394 100644
> > > --- a/net/sctp/socket.c
> > > +++ b/net/sctp/socket.c
> > > @@ -4933,20 +4933,12 @@ int sctp_do_peeloff(struct sock *sk,
> > > sctp_assoc_t id, struct socket **sockp)
> > >  }
> > >  EXPORT_SYMBOL(sctp_do_peeloff);
> > >  
> > > -static int sctp_getsockopt_peeloff(struct sock *sk, int len,
> > > char __user *optval, int __user *optlen)
> > > +static int sctp_getsockopt_peeloff_common(struct sock *sk,
> > > sctp_peeloff_arg_t *peeloff, struct file **newfile)
> > >  {
> > > -	sctp_peeloff_arg_t peeloff;
> > >  	struct socket *newsock;
> > > -	struct file *newfile;
> > >  	int retval = 0;
> > >  
> > > -	if (len < sizeof(sctp_peeloff_arg_t))
> > > -		return -EINVAL;
> > > -	len = sizeof(sctp_peeloff_arg_t);
> > > -	if (copy_from_user(&peeloff, optval, len))
> > > -		return -EFAULT;
> > > -
> > > -	retval = sctp_do_peeloff(sk, peeloff.associd, &newsock);
> > > +	retval = sctp_do_peeloff(sk, peeloff->associd,
> > > &newsock);
> > >  	if (retval < 0)
> > >  		goto out;
> > >  
> > > @@ -4957,25 +4949,90 @@ static int sctp_getsockopt_peeloff(struct
> > > sock *sk, int len, char __user *optval
> > >  		goto out;
> > >  	}
> > >  
> > > -	newfile = sock_alloc_file(newsock, 0, NULL);
> > > -	if (IS_ERR(newfile)) {
> > > +	*newfile = sock_alloc_file(newsock, 0, NULL);
> > > +	if (IS_ERR(*newfile)) {
> > >  		put_unused_fd(retval);
> > >  		sock_release(newsock);
> > > -		return PTR_ERR(newfile);
> > > +		retval = PTR_ERR(*newfile);
> > > +		*newfile = NULL;
> > > +		return retval;
> > >  	}
> > >  
> > >  	pr_debug("%s: sk:%p, newsk:%p, sd:%d\n", __func__, sk,
> > > newsock->sk,
> > >  		 retval);
> > > +	
> > > +	peeloff->sd = retval;
> > > +out:
> > > +	return retval;
> > > +}
> > > +
> > > +static int sctp_getsockopt_peeloff(struct sock *sk, int len,
> > > char __user *optval, int __user *optlen)
> > > +{
> > > +	sctp_peeloff_arg_t peeloff;
> > > +	struct file *newfile = NULL;
> > > +	int retval = 0;
> > > +
> > > +	if (len < sizeof(sctp_peeloff_arg_t))
> > > +		return -EINVAL;
> > > +	len = sizeof(sctp_peeloff_arg_t);
> > > +	if (copy_from_user(&peeloff, optval, len))
> > > +		return -EFAULT;
> > > +
> > > +	retval = sctp_getsockopt_peeloff_common(sk, &peeloff,
> > > &newfile);
> > > +	if (retval < 0)
> > > +		goto out;
> > >  
> > >  	/* Return the fd mapped to the new socket.  */
> > >  	if (put_user(len, optlen)) {
> > > -		fput(newfile);
> > > +		if (newfile)
> > > +			fput(newfile);
> > >  		put_unused_fd(retval);
> > >  		return -EFAULT;
> > >  	}
> > > -	peeloff.sd = retval;
> > > +
> > >  	if (copy_to_user(optval, &peeloff, len)) {
> > > -		fput(newfile);
> > > +		if (newfile)
> > > +			fput(newfile);
> > > +		put_unused_fd(retval);
> > > +		return -EFAULT;
> > > +	}
> > > +	fd_install(retval, newfile);
> > > +out:
> > > +	return retval;
> > > +}
> > > +
> > > +static int sctp_getsockopt_peeloff_flags(struct sock *sk, int
> > > len, char __user *optval, int __user *optlen)
> > > +{
> > > +	sctp_peeloff_flags_arg_t peeloff;
> > > +	struct file *newfile = NULL;
> > > +	int retval = 0;
> > > +
> > > +	if (len < sizeof(sctp_peeloff_flags_arg_t))
> > > +		return -EINVAL;
> > > +	len = sizeof(sctp_peeloff_flags_arg_t);
> > > +	if (copy_from_user(&peeloff, optval, len))
> > > +		return -EFAULT;
> > > +
> > > +	retval = sctp_getsockopt_peeloff_common(sk,
> > > &peeloff.p_arg, &newfile);
> > > +	if (retval < 0)
> > > +		goto out;
> > > +
> > > +	set_close_on_exec(peeloff.p_arg.sd, !!(peeloff.flags &
> > > SOCK_CLOEXEC));
> > > +
> > > +	if (peeloff.flags & SOCK_NONBLOCK)
> > > +		newfile->f_flags |= O_NONBLOCK;
> > > +
> > > +	/* Return the fd mapped to the new socket.  */
> > > +	if (put_user(len, optlen)) {
> > > +		if (newfile)
> > > +			fput(newfile);
> > > +		put_unused_fd(retval);
> > > +		return -EFAULT;
> > > +	}
> > > +
> > > +	if (copy_to_user(optval, &peeloff, len)) {
> > > +		if (newfile)
> > > +			fput(newfile);
> > >  		put_unused_fd(retval);
> > >  		return -EFAULT;
> > >  	}
> > > @@ -6758,6 +6815,9 @@ static int sctp_getsockopt(struct sock *sk,
> > > int level, int optname,
> > >  	case SCTP_SOCKOPT_PEELOFF:
> > >  		retval = sctp_getsockopt_peeloff(sk, len,
> > > optval, optlen);
> > >  		break;
> > > +	case SCTP_SOCKOPT_PEELOFF_FLAGS:
> > > +		retval = sctp_getsockopt_peeloff_flags(sk, len,
> > > optval, optlen);
> > > +		break;
> > >  	case SCTP_PEER_ADDR_PARAMS:
> > >  		retval = sctp_getsockopt_peer_addr_params(sk,
> > > len, optval,
> > >  							  optlen
> > > );
> > > --
> > > 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.htm
> > > l
> > > 
-- 
Andreas Steinmetz                       SPAMmers use robotrap@domdv.de

Re: SCTP_SOCKOPT_PEELOFF is missing SOCK_CLOEXEC (and SOCK_NONBLOCK)

[Neil Horman]

On Mon, Jun 26, 2017 at 11:57:10AM +0200, Andreas Steinmetz wrote:
> A quick test with retrieving the flags with fcntl(F_GETFD/F_GETFL)
> after peeloff shows that your patch works. Good job!
> 
> Am Sunday, den 25.06.2017, 08:06 -0400 schrieb Neil Horman:
> > On Fri, Jun 23, 2017 at 04:33:15PM -0300, Marcelo Ricardo Leitner
> > wrote:
> > > On Fri, Jun 23, 2017 at 03:14:33PM -0400, Neil Horman wrote:
> > > > On Wed, Jun 21, 2017 at 09:53:44PM -0400, Neil Horman wrote:
> > > > > On Wed, Jun 21, 2017 at 03:53:13PM -0300, Marcelo Ricardo
> > > > > Leitner wrote:
> > > > > > On Wed, Jun 21, 2017 at 08:27:14AM -0400, Neil Horman wrote:
> > > > > > > On Tue, Jun 20, 2017 at 04:21:45PM -0300, Marcelo Ricardo
> > > > > > > Leitner wrote:
> > > > > > > > On Tue, Jun 20, 2017 at 03:00:46PM -0400, Neil Horman
> > > > > > > > wrote:
> > > > > > > > > On Tue, Jun 20, 2017 at 12:41:47PM -0300, Marcelo
> > > > > > > > > Ricardo Leitner wrote:
> > > > > > > > 
> > > > > > > > ...
> > > > > > > > > > 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)
> > > > > > > > > > 
> > > > > > > > > 
> > > > > > > > > Thinking about this some more, I'm a bit hesitant to
> > > > > > > > > change the
> > > > > > > > > sctp_peeloff_arg_t, since thats exposed to user
> > > > > > > > > space.  Instead, what if we use
> > > > > > > > 
> > > > > > > > +1
> > > > > > > > 
> > > > > > > > > the sd value in the peeloff arg to fetch the
> > > > > > > > > close_on_exec flag in the new fd?
> > > > > > > > > Something like this (untested) patch:
> > > > > > > > 
> > > > > > > > Yes. :-) That's similar to what I proposed, though you
> > > > > > > > used peeloff.sd
> > > > > > > > to find the old fd and copy the flag from it and I used
> > > > > > > > it as a pure
> > > > > > > > 'flags' field instead.
> > > > > > > > 
> > > > > > > > I'm still not comfortable on hardwiring this copy. What
> > > > > > > > if the
> > > > > > > > application doesn't want to inherit the flag?
> > > > > > > > accept() calls accept4(... , flags=0)
> > > > > > > > dup2() calls dup3(... , flags=0)
> > > > > > > > I don't see this direct inheritance anywhere else.
> > > > > > > > 
> > > > > > > 
> > > > > > > I agree, but this strikes me as something of a unique
> > > > > > > situation.  In alternate
> > > > > > > cases of creating a new file descriptor within the same
> > > > > > > process as a clone of an
> > > > > > > existing fd, we have dup/dup2 and dup3, with the former
> > > > > > > having defined behavior
> > > > > > > of not copying the cloexec and nonblock flags, and the
> > > > > > > latter allowing them to
> > > > > > > be explicitly specified for the new fd.
> > > > > > > 
> > > > > > > 	In SCTP, we're creating a new fd, but have no express
> > > > > > > mechanism for
> > > > > > > defining the new flags.  We could, as you say, add a flags
> > > > > > > field to the
> > > > > > > peeloff_param_arg_t to provide that, but that has userspace
> > > > > > > ABI ramifications,
> > > > > > > and makes programs less portable.
> > > > > > > 
> > > > > > > 	Perhaps a new socket option SCTP_SOCKOPT_PEELOFF_FLAGS,
> > > > > > > and
> > > > > > > corresponding lksctp-tools library function
> > > > > > > sctp_peeloff_flags, which accepts
> > > > > > > the new fd's cloexec and nonblock flags as an
> > > > > > > argument?  That way at least, we
> > > > > > > could define the origional peeloff behavior as not copying
> > > > > > > the flags, and allow
> > > > > > > people to opt into the non-standard functions if they need
> > > > > > > it.  That would be in
> > > > > > > keeping with how dup/dup2/dup3 were developed.
> > > > > > > 
> > > > > > > Thoughts?
> > > > > > > Neil
> > > > > > 
> > > > > > That works for me.
> > > > > > 
> > > > > > We can't rely on using peeloff.sd to carry the flags because
> > > > > > the
> > > > > > application may not have initialized it. It may be a variable
> > > > > > in the
> > > > > > stack on which application simply did peeloff.asoc = X and we
> > > > > > would be
> > > > > > working on unitialized data, so it's not safe.
> > > > > > 
> > > > > > On Andreas' idea to have a sctp_peeloff2_arg_t, it's also
> > > > > > complicated
> > > > > > because the application is allowed to use a bigger-than-
> > > > > > needed buffer
> > > > > > and in such cases it would lead us to the same situation as
> > > > > > above.
> > > > > > 
> > > > > > So yes, I also think that the new SCTP_SOCKOPT_PEELOFF_FLAGS
> > > > > > is the best
> > > > > > way out here.
> > > > > > 
> > > > > > Cheers,
> > > > > > Marcelo
> > > > > > 
> > > > > > 
> > > > > 
> > > > > I'll work up the patch tomorrow
> > > > > Neil
> > > > > 
> > > > > --
> > > > > 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.h
> > > > > tml
> > > > > 
> > > > 
> > > > Ok, sorry for the delay.  I tested this patch out with the lksctp
> > > > test suite to
> > > > validate that the traditional peeloff path works, but I don't
> > > > have the userspace
> > > > code written to test the peeloff_flags path.  Andreas, if you
> > > > could validate
> > > > that, I would appreciate it.  I'm not real thrilled with the
> > > > export of the
> > > > set_close_on_exec function, but I'm not sure I like using the
> > > > internal function
> > > > any better.  Either way, Andreas, if it works for you, I'll
> > > > propose it
> > > > officially and we can go from there.
> > > 
> > > What about exposing a flags arg on
> > > sctp_getsockopt_peeloff_common()?
> > > Then I guess you don't need such post-processing as you can pass it
> > > to
> > > get_unused_fd_flags() directly.
> > > 
> > >   Marcelo
> > > 
> > 
> > Yeah, that makes sense, and then default the flags to 0 on
> > sctp_getsockopt_peelof.  Andreas, can you please test the patch as-is 
> > right now,
> > just to make sure the path with flags set works properly, and I'll
> > clean this up
> > in parallel while you do that ahead of official submission?
> > 
> > Thanks
> > Neil
> > 
> > > > 
> > > > Neil
> > > > 
> > > > 
> > > > diff --git a/fs/file.c b/fs/file.c
> > > > index 1c2972e..a4521da 100644
> > > > --- a/fs/file.c
> > > > +++ b/fs/file.c
> > > > @@ -807,6 +807,7 @@ void set_close_on_exec(unsigned int fd, int
> > > > flag)
> > > >  		__clear_close_on_exec(fd, fdt);
> > > >  	spin_unlock(&files->file_lock);
> > > >  }
> > > > +EXPORT_SYMBOL(set_close_on_exec);
> > > >  
> > > >  bool get_close_on_exec(unsigned int fd)
> > > >  {
> > > > diff --git a/include/uapi/linux/sctp.h
> > > > b/include/uapi/linux/sctp.h
> > > > index ced9d8b..6217ff8 100644
> > > > --- a/include/uapi/linux/sctp.h
> > > > +++ b/include/uapi/linux/sctp.h
> > > > @@ -121,6 +121,7 @@ typedef __s32 sctp_assoc_t;
> > > >  #define SCTP_RESET_STREAMS	119
> > > >  #define SCTP_RESET_ASSOC	120
> > > >  #define SCTP_ADD_STREAMS	121
> > > > +#define SCTP_SOCKOPT_PEELOFF_FLAGS 122
> > > >  
> > > >  /* PR-SCTP policies */
> > > >  #define SCTP_PR_SCTP_NONE	0x0000
> > > > @@ -978,6 +979,11 @@ typedef struct {
> > > >  	int sd;
> > > >  } sctp_peeloff_arg_t;
> > > >  
> > > > +typedef struct {
> > > > +	sctp_peeloff_arg_t p_arg;
> > > > +	unsigned flags;
> > > > +} sctp_peeloff_flags_arg_t;
> > > > +
> > > >  /*
> > > >   *  Peer Address Thresholds socket option
> > > >   */
> > > > diff --git a/net/sctp/socket.c b/net/sctp/socket.c
> > > > index 7b6e20e..f210394 100644
> > > > --- a/net/sctp/socket.c
> > > > +++ b/net/sctp/socket.c
> > > > @@ -4933,20 +4933,12 @@ int sctp_do_peeloff(struct sock *sk,
> > > > sctp_assoc_t id, struct socket **sockp)
> > > >  }
> > > >  EXPORT_SYMBOL(sctp_do_peeloff);
> > > >  
> > > > -static int sctp_getsockopt_peeloff(struct sock *sk, int len,
> > > > char __user *optval, int __user *optlen)
> > > > +static int sctp_getsockopt_peeloff_common(struct sock *sk,
> > > > sctp_peeloff_arg_t *peeloff, struct file **newfile)
> > > >  {
> > > > -	sctp_peeloff_arg_t peeloff;
> > > >  	struct socket *newsock;
> > > > -	struct file *newfile;
> > > >  	int retval = 0;
> > > >  
> > > > -	if (len < sizeof(sctp_peeloff_arg_t))
> > > > -		return -EINVAL;
> > > > -	len = sizeof(sctp_peeloff_arg_t);
> > > > -	if (copy_from_user(&peeloff, optval, len))
> > > > -		return -EFAULT;
> > > > -
> > > > -	retval = sctp_do_peeloff(sk, peeloff.associd, &newsock);
> > > > +	retval = sctp_do_peeloff(sk, peeloff->associd,
> > > > &newsock);
> > > >  	if (retval < 0)
> > > >  		goto out;
> > > >  
> > > > @@ -4957,25 +4949,90 @@ static int sctp_getsockopt_peeloff(struct
> > > > sock *sk, int len, char __user *optval
> > > >  		goto out;
> > > >  	}
> > > >  
> > > > -	newfile = sock_alloc_file(newsock, 0, NULL);
> > > > -	if (IS_ERR(newfile)) {
> > > > +	*newfile = sock_alloc_file(newsock, 0, NULL);
> > > > +	if (IS_ERR(*newfile)) {
> > > >  		put_unused_fd(retval);
> > > >  		sock_release(newsock);
> > > > -		return PTR_ERR(newfile);
> > > > +		retval = PTR_ERR(*newfile);
> > > > +		*newfile = NULL;
> > > > +		return retval;
> > > >  	}
> > > >  
> > > >  	pr_debug("%s: sk:%p, newsk:%p, sd:%d\n", __func__, sk,
> > > > newsock->sk,
> > > >  		 retval);
> > > > +	
> > > > +	peeloff->sd = retval;
> > > > +out:
> > > > +	return retval;
> > > > +}
> > > > +
> > > > +static int sctp_getsockopt_peeloff(struct sock *sk, int len,
> > > > char __user *optval, int __user *optlen)
> > > > +{
> > > > +	sctp_peeloff_arg_t peeloff;
> > > > +	struct file *newfile = NULL;
> > > > +	int retval = 0;
> > > > +
> > > > +	if (len < sizeof(sctp_peeloff_arg_t))
> > > > +		return -EINVAL;
> > > > +	len = sizeof(sctp_peeloff_arg_t);
> > > > +	if (copy_from_user(&peeloff, optval, len))
> > > > +		return -EFAULT;
> > > > +
> > > > +	retval = sctp_getsockopt_peeloff_common(sk, &peeloff,
> > > > &newfile);
> > > > +	if (retval < 0)
> > > > +		goto out;
> > > >  
> > > >  	/* Return the fd mapped to the new socket.  */
> > > >  	if (put_user(len, optlen)) {
> > > > -		fput(newfile);
> > > > +		if (newfile)
> > > > +			fput(newfile);
> > > >  		put_unused_fd(retval);
> > > >  		return -EFAULT;
> > > >  	}
> > > > -	peeloff.sd = retval;
> > > > +
> > > >  	if (copy_to_user(optval, &peeloff, len)) {
> > > > -		fput(newfile);
> > > > +		if (newfile)
> > > > +			fput(newfile);
> > > > +		put_unused_fd(retval);
> > > > +		return -EFAULT;
> > > > +	}
> > > > +	fd_install(retval, newfile);
> > > > +out:
> > > > +	return retval;
> > > > +}
> > > > +
> > > > +static int sctp_getsockopt_peeloff_flags(struct sock *sk, int
> > > > len, char __user *optval, int __user *optlen)
> > > > +{
> > > > +	sctp_peeloff_flags_arg_t peeloff;
> > > > +	struct file *newfile = NULL;
> > > > +	int retval = 0;
> > > > +
> > > > +	if (len < sizeof(sctp_peeloff_flags_arg_t))
> > > > +		return -EINVAL;
> > > > +	len = sizeof(sctp_peeloff_flags_arg_t);
> > > > +	if (copy_from_user(&peeloff, optval, len))
> > > > +		return -EFAULT;
> > > > +
> > > > +	retval = sctp_getsockopt_peeloff_common(sk,
> > > > &peeloff.p_arg, &newfile);
> > > > +	if (retval < 0)
> > > > +		goto out;
> > > > +
> > > > +	set_close_on_exec(peeloff.p_arg.sd, !!(peeloff.flags &
> > > > SOCK_CLOEXEC));
> > > > +
> > > > +	if (peeloff.flags & SOCK_NONBLOCK)
> > > > +		newfile->f_flags |= O_NONBLOCK;
> > > > +
> > > > +	/* Return the fd mapped to the new socket.  */
> > > > +	if (put_user(len, optlen)) {
> > > > +		if (newfile)
> > > > +			fput(newfile);
> > > > +		put_unused_fd(retval);
> > > > +		return -EFAULT;
> > > > +	}
> > > > +
> > > > +	if (copy_to_user(optval, &peeloff, len)) {
> > > > +		if (newfile)
> > > > +			fput(newfile);
> > > >  		put_unused_fd(retval);
> > > >  		return -EFAULT;
> > > >  	}
> > > > @@ -6758,6 +6815,9 @@ static int sctp_getsockopt(struct sock *sk,
> > > > int level, int optname,
> > > >  	case SCTP_SOCKOPT_PEELOFF:
> > > >  		retval = sctp_getsockopt_peeloff(sk, len,
> > > > optval, optlen);
> > > >  		break;
> > > > +	case SCTP_SOCKOPT_PEELOFF_FLAGS:
> > > > +		retval = sctp_getsockopt_peeloff_flags(sk, len,
> > > > optval, optlen);
> > > > +		break;
> > > >  	case SCTP_PEER_ADDR_PARAMS:
> > > >  		retval = sctp_getsockopt_peer_addr_params(sk,
> > > > len, optval,
> > > >  							  optlen
> > > > );
> > > > --
> > > > 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.htm
> > > > l
> > > > 
> -- 
> 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
> 
Thank you, can I ask you to test this one as well?  It contains the clean up
that Marcello asked for.

Best
Neil


commit 07fb73580c766905023caabc4ac6b221e047e1eb
Author: root <root@amd-pence-02.lab.bos.redhat.com>
Date:   Mon Jun 26 08:58:56 2017 -0400

    Add peeloff-flags socket option

diff --git a/fs/file.c b/fs/file.c
index 1c2972e..a4521da 100644
--- a/fs/file.c
+++ b/fs/file.c
@@ -807,6 +807,7 @@ void set_close_on_exec(unsigned int fd, int flag)
 		__clear_close_on_exec(fd, fdt);
 	spin_unlock(&files->file_lock);
 }
+EXPORT_SYMBOL(set_close_on_exec);
 
 bool get_close_on_exec(unsigned int fd)
 {
diff --git a/include/uapi/linux/sctp.h b/include/uapi/linux/sctp.h
index ced9d8b..6217ff8 100644
--- a/include/uapi/linux/sctp.h
+++ b/include/uapi/linux/sctp.h
@@ -121,6 +121,7 @@ typedef __s32 sctp_assoc_t;
 #define SCTP_RESET_STREAMS	119
 #define SCTP_RESET_ASSOC	120
 #define SCTP_ADD_STREAMS	121
+#define SCTP_SOCKOPT_PEELOFF_FLAGS 122
 
 /* PR-SCTP policies */
 #define SCTP_PR_SCTP_NONE	0x0000
@@ -978,6 +979,11 @@ typedef struct {
 	int sd;
 } sctp_peeloff_arg_t;
 
+typedef struct {
+	sctp_peeloff_arg_t p_arg;
+	unsigned flags;
+} sctp_peeloff_flags_arg_t;
+
 /*
  *  Peer Address Thresholds socket option
  */
diff --git a/net/sctp/socket.c b/net/sctp/socket.c
index 7b6e20e..7341053 100644
--- a/net/sctp/socket.c
+++ b/net/sctp/socket.c
@@ -4933,20 +4933,12 @@ int sctp_do_peeloff(struct sock *sk, sctp_assoc_t id, struct socket **sockp)
 }
 EXPORT_SYMBOL(sctp_do_peeloff);
 
-static int sctp_getsockopt_peeloff(struct sock *sk, int len, char __user *optval, int __user *optlen)
+static int sctp_getsockopt_peeloff_common(struct sock *sk, sctp_peeloff_arg_t *peeloff, struct file **newfile, unsigned flags)
 {
-	sctp_peeloff_arg_t peeloff;
 	struct socket *newsock;
-	struct file *newfile;
 	int retval = 0;
 
-	if (len < sizeof(sctp_peeloff_arg_t))
-		return -EINVAL;
-	len = sizeof(sctp_peeloff_arg_t);
-	if (copy_from_user(&peeloff, optval, len))
-		return -EFAULT;
-
-	retval = sctp_do_peeloff(sk, peeloff.associd, &newsock);
+	retval = sctp_do_peeloff(sk, peeloff->associd, &newsock);
 	if (retval < 0)
 		goto out;
 
@@ -4957,25 +4949,90 @@ static int sctp_getsockopt_peeloff(struct sock *sk, int len, char __user *optval
 		goto out;
 	}
 
-	newfile = sock_alloc_file(newsock, 0, NULL);
-	if (IS_ERR(newfile)) {
+	*newfile = sock_alloc_file(newsock, 0, NULL);
+	if (IS_ERR(*newfile)) {
 		put_unused_fd(retval);
 		sock_release(newsock);
-		return PTR_ERR(newfile);
+		retval = PTR_ERR(*newfile);
+		*newfile = NULL;
+		return retval;
 	}
 
 	pr_debug("%s: sk:%p, newsk:%p, sd:%d\n", __func__, sk, newsock->sk,
 		 retval);
+	
+	peeloff->sd = retval;
+
+	set_close_on_exec(peeloff->sd, !!(flags & SOCK_CLOEXEC));
+
+	if (flags & SOCK_NONBLOCK)
+		(*newfile)->f_flags |= O_NONBLOCK;
+out:
+	return retval;
+}
+
+static int sctp_getsockopt_peeloff(struct sock *sk, int len, char __user *optval, int __user *optlen)
+{
+	sctp_peeloff_arg_t peeloff;
+	struct file *newfile = NULL;
+	int retval = 0;
+
+	if (len < sizeof(sctp_peeloff_arg_t))
+		return -EINVAL;
+	len = sizeof(sctp_peeloff_arg_t);
+	if (copy_from_user(&peeloff, optval, len))
+		return -EFAULT;
+
+	retval = sctp_getsockopt_peeloff_common(sk, &peeloff, &newfile, 0);
+	if (retval < 0)
+		goto out;
 
 	/* Return the fd mapped to the new socket.  */
 	if (put_user(len, optlen)) {
-		fput(newfile);
+		if (newfile)
+			fput(newfile);
 		put_unused_fd(retval);
 		return -EFAULT;
 	}
-	peeloff.sd = retval;
+
 	if (copy_to_user(optval, &peeloff, len)) {
-		fput(newfile);
+		if (newfile)
+			fput(newfile);
+		put_unused_fd(retval);
+		return -EFAULT;
+	}
+	fd_install(retval, newfile);
+out:
+	return retval;
+}
+
+static int sctp_getsockopt_peeloff_flags(struct sock *sk, int len, char __user *optval, int __user *optlen)
+{
+	sctp_peeloff_flags_arg_t peeloff;
+	struct file *newfile = NULL;
+	int retval = 0;
+
+	if (len < sizeof(sctp_peeloff_flags_arg_t))
+		return -EINVAL;
+	len = sizeof(sctp_peeloff_flags_arg_t);
+	if (copy_from_user(&peeloff, optval, len))
+		return -EFAULT;
+
+	retval = sctp_getsockopt_peeloff_common(sk, &peeloff.p_arg, &newfile, peeloff.flags);
+	if (retval < 0)
+		goto out;
+
+	/* Return the fd mapped to the new socket.  */
+	if (put_user(len, optlen)) {
+		if (newfile)
+			fput(newfile);
+		put_unused_fd(retval);
+		return -EFAULT;
+	}
+
+	if (copy_to_user(optval, &peeloff, len)) {
+		if (newfile)
+			fput(newfile);
 		put_unused_fd(retval);
 		return -EFAULT;
 	}
@@ -6758,6 +6815,9 @@ static int sctp_getsockopt(struct sock *sk, int level, int optname,
 	case SCTP_SOCKOPT_PEELOFF:
 		retval = sctp_getsockopt_peeloff(sk, len, optval, optlen);
 		break;
+	case SCTP_SOCKOPT_PEELOFF_FLAGS:
+		retval = sctp_getsockopt_peeloff_flags(sk, len, optval, optlen);
+		break;
 	case SCTP_PEER_ADDR_PARAMS:
 		retval = sctp_getsockopt_peer_addr_params(sk, len, optval,
 							  optlen);

Re: SCTP_SOCKOPT_PEELOFF is missing SOCK_CLOEXEC (and SOCK_NONBLOCK)

[Neil Horman]

Ping.  Sorry for the top post.  If this patch is eqiuvalently functional for you
Andreas, I'll post it officially
Neil

On Mon, Jun 26, 2017 at 01:44:21PM -0400, Neil Horman wrote:
> On Mon, Jun 26, 2017 at 11:57:10AM +0200, Andreas Steinmetz wrote:
> > A quick test with retrieving the flags with fcntl(F_GETFD/F_GETFL)
> > after peeloff shows that your patch works. Good job!
> > 
> > Am Sunday, den 25.06.2017, 08:06 -0400 schrieb Neil Horman:
> > > On Fri, Jun 23, 2017 at 04:33:15PM -0300, Marcelo Ricardo Leitner
> > > wrote:
> > > > On Fri, Jun 23, 2017 at 03:14:33PM -0400, Neil Horman wrote:
> > > > > On Wed, Jun 21, 2017 at 09:53:44PM -0400, Neil Horman wrote:
> > > > > > On Wed, Jun 21, 2017 at 03:53:13PM -0300, Marcelo Ricardo
> > > > > > Leitner wrote:
> > > > > > > On Wed, Jun 21, 2017 at 08:27:14AM -0400, Neil Horman wrote:
> > > > > > > > On Tue, Jun 20, 2017 at 04:21:45PM -0300, Marcelo Ricardo
> > > > > > > > Leitner wrote:
> > > > > > > > > On Tue, Jun 20, 2017 at 03:00:46PM -0400, Neil Horman
> > > > > > > > > wrote:
> > > > > > > > > > On Tue, Jun 20, 2017 at 12:41:47PM -0300, Marcelo
> > > > > > > > > > Ricardo Leitner wrote:
> > > > > > > > > 
> > > > > > > > > ...
> > > > > > > > > > > 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)
> > > > > > > > > > > 
> > > > > > > > > > 
> > > > > > > > > > Thinking about this some more, I'm a bit hesitant to
> > > > > > > > > > change the
> > > > > > > > > > sctp_peeloff_arg_t, since thats exposed to user
> > > > > > > > > > space.  Instead, what if we use
> > > > > > > > > 
> > > > > > > > > +1
> > > > > > > > > 
> > > > > > > > > > the sd value in the peeloff arg to fetch the
> > > > > > > > > > close_on_exec flag in the new fd?
> > > > > > > > > > Something like this (untested) patch:
> > > > > > > > > 
> > > > > > > > > Yes. :-) That's similar to what I proposed, though you
> > > > > > > > > used peeloff.sd
> > > > > > > > > to find the old fd and copy the flag from it and I used
> > > > > > > > > it as a pure
> > > > > > > > > 'flags' field instead.
> > > > > > > > > 
> > > > > > > > > I'm still not comfortable on hardwiring this copy. What
> > > > > > > > > if the
> > > > > > > > > application doesn't want to inherit the flag?
> > > > > > > > > accept() calls accept4(... , flags=0)
> > > > > > > > > dup2() calls dup3(... , flags=0)
> > > > > > > > > I don't see this direct inheritance anywhere else.
> > > > > > > > > 
> > > > > > > > 
> > > > > > > > I agree, but this strikes me as something of a unique
> > > > > > > > situation.  In alternate
> > > > > > > > cases of creating a new file descriptor within the same
> > > > > > > > process as a clone of an
> > > > > > > > existing fd, we have dup/dup2 and dup3, with the former
> > > > > > > > having defined behavior
> > > > > > > > of not copying the cloexec and nonblock flags, and the
> > > > > > > > latter allowing them to
> > > > > > > > be explicitly specified for the new fd.
> > > > > > > > 
> > > > > > > > 	In SCTP, we're creating a new fd, but have no express
> > > > > > > > mechanism for
> > > > > > > > defining the new flags.  We could, as you say, add a flags
> > > > > > > > field to the
> > > > > > > > peeloff_param_arg_t to provide that, but that has userspace
> > > > > > > > ABI ramifications,
> > > > > > > > and makes programs less portable.
> > > > > > > > 
> > > > > > > > 	Perhaps a new socket option SCTP_SOCKOPT_PEELOFF_FLAGS,
> > > > > > > > and
> > > > > > > > corresponding lksctp-tools library function
> > > > > > > > sctp_peeloff_flags, which accepts
> > > > > > > > the new fd's cloexec and nonblock flags as an
> > > > > > > > argument?  That way at least, we
> > > > > > > > could define the origional peeloff behavior as not copying
> > > > > > > > the flags, and allow
> > > > > > > > people to opt into the non-standard functions if they need
> > > > > > > > it.  That would be in
> > > > > > > > keeping with how dup/dup2/dup3 were developed.
> > > > > > > > 
> > > > > > > > Thoughts?
> > > > > > > > Neil
> > > > > > > 
> > > > > > > That works for me.
> > > > > > > 
> > > > > > > We can't rely on using peeloff.sd to carry the flags because
> > > > > > > the
> > > > > > > application may not have initialized it. It may be a variable
> > > > > > > in the
> > > > > > > stack on which application simply did peeloff.asoc = X and we
> > > > > > > would be
> > > > > > > working on unitialized data, so it's not safe.
> > > > > > > 
> > > > > > > On Andreas' idea to have a sctp_peeloff2_arg_t, it's also
> > > > > > > complicated
> > > > > > > because the application is allowed to use a bigger-than-
> > > > > > > needed buffer
> > > > > > > and in such cases it would lead us to the same situation as
> > > > > > > above.
> > > > > > > 
> > > > > > > So yes, I also think that the new SCTP_SOCKOPT_PEELOFF_FLAGS
> > > > > > > is the best
> > > > > > > way out here.
> > > > > > > 
> > > > > > > Cheers,
> > > > > > > Marcelo
> > > > > > > 
> > > > > > > 
> > > > > > 
> > > > > > I'll work up the patch tomorrow
> > > > > > Neil
> > > > > > 
> > > > > > --
> > > > > > 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.h
> > > > > > tml
> > > > > > 
> > > > > 
> > > > > Ok, sorry for the delay.  I tested this patch out with the lksctp
> > > > > test suite to
> > > > > validate that the traditional peeloff path works, but I don't
> > > > > have the userspace
> > > > > code written to test the peeloff_flags path.  Andreas, if you
> > > > > could validate
> > > > > that, I would appreciate it.  I'm not real thrilled with the
> > > > > export of the
> > > > > set_close_on_exec function, but I'm not sure I like using the
> > > > > internal function
> > > > > any better.  Either way, Andreas, if it works for you, I'll
> > > > > propose it
> > > > > officially and we can go from there.
> > > > 
> > > > What about exposing a flags arg on
> > > > sctp_getsockopt_peeloff_common()?
> > > > Then I guess you don't need such post-processing as you can pass it
> > > > to
> > > > get_unused_fd_flags() directly.
> > > > 
> > > >   Marcelo
> > > > 
> > > 
> > > Yeah, that makes sense, and then default the flags to 0 on
> > > sctp_getsockopt_peelof.  Andreas, can you please test the patch as-is 
> > > right now,
> > > just to make sure the path with flags set works properly, and I'll
> > > clean this up
> > > in parallel while you do that ahead of official submission?
> > > 
> > > Thanks
> > > Neil
> > > 
> > > > > 
> > > > > Neil
> > > > > 
> > > > > 
> > > > > diff --git a/fs/file.c b/fs/file.c
> > > > > index 1c2972e..a4521da 100644
> > > > > --- a/fs/file.c
> > > > > +++ b/fs/file.c
> > > > > @@ -807,6 +807,7 @@ void set_close_on_exec(unsigned int fd, int
> > > > > flag)
> > > > >  		__clear_close_on_exec(fd, fdt);
> > > > >  	spin_unlock(&files->file_lock);
> > > > >  }
> > > > > +EXPORT_SYMBOL(set_close_on_exec);
> > > > >  
> > > > >  bool get_close_on_exec(unsigned int fd)
> > > > >  {
> > > > > diff --git a/include/uapi/linux/sctp.h
> > > > > b/include/uapi/linux/sctp.h
> > > > > index ced9d8b..6217ff8 100644
> > > > > --- a/include/uapi/linux/sctp.h
> > > > > +++ b/include/uapi/linux/sctp.h
> > > > > @@ -121,6 +121,7 @@ typedef __s32 sctp_assoc_t;
> > > > >  #define SCTP_RESET_STREAMS	119
> > > > >  #define SCTP_RESET_ASSOC	120
> > > > >  #define SCTP_ADD_STREAMS	121
> > > > > +#define SCTP_SOCKOPT_PEELOFF_FLAGS 122
> > > > >  
> > > > >  /* PR-SCTP policies */
> > > > >  #define SCTP_PR_SCTP_NONE	0x0000
> > > > > @@ -978,6 +979,11 @@ typedef struct {
> > > > >  	int sd;
> > > > >  } sctp_peeloff_arg_t;
> > > > >  
> > > > > +typedef struct {
> > > > > +	sctp_peeloff_arg_t p_arg;
> > > > > +	unsigned flags;
> > > > > +} sctp_peeloff_flags_arg_t;
> > > > > +
> > > > >  /*
> > > > >   *  Peer Address Thresholds socket option
> > > > >   */
> > > > > diff --git a/net/sctp/socket.c b/net/sctp/socket.c
> > > > > index 7b6e20e..f210394 100644
> > > > > --- a/net/sctp/socket.c
> > > > > +++ b/net/sctp/socket.c
> > > > > @@ -4933,20 +4933,12 @@ int sctp_do_peeloff(struct sock *sk,
> > > > > sctp_assoc_t id, struct socket **sockp)
> > > > >  }
> > > > >  EXPORT_SYMBOL(sctp_do_peeloff);
> > > > >  
> > > > > -static int sctp_getsockopt_peeloff(struct sock *sk, int len,
> > > > > char __user *optval, int __user *optlen)
> > > > > +static int sctp_getsockopt_peeloff_common(struct sock *sk,
> > > > > sctp_peeloff_arg_t *peeloff, struct file **newfile)
> > > > >  {
> > > > > -	sctp_peeloff_arg_t peeloff;
> > > > >  	struct socket *newsock;
> > > > > -	struct file *newfile;
> > > > >  	int retval = 0;
> > > > >  
> > > > > -	if (len < sizeof(sctp_peeloff_arg_t))
> > > > > -		return -EINVAL;
> > > > > -	len = sizeof(sctp_peeloff_arg_t);
> > > > > -	if (copy_from_user(&peeloff, optval, len))
> > > > > -		return -EFAULT;
> > > > > -
> > > > > -	retval = sctp_do_peeloff(sk, peeloff.associd, &newsock);
> > > > > +	retval = sctp_do_peeloff(sk, peeloff->associd,
> > > > > &newsock);
> > > > >  	if (retval < 0)
> > > > >  		goto out;
> > > > >  
> > > > > @@ -4957,25 +4949,90 @@ static int sctp_getsockopt_peeloff(struct
> > > > > sock *sk, int len, char __user *optval
> > > > >  		goto out;
> > > > >  	}
> > > > >  
> > > > > -	newfile = sock_alloc_file(newsock, 0, NULL);
> > > > > -	if (IS_ERR(newfile)) {
> > > > > +	*newfile = sock_alloc_file(newsock, 0, NULL);
> > > > > +	if (IS_ERR(*newfile)) {
> > > > >  		put_unused_fd(retval);
> > > > >  		sock_release(newsock);
> > > > > -		return PTR_ERR(newfile);
> > > > > +		retval = PTR_ERR(*newfile);
> > > > > +		*newfile = NULL;
> > > > > +		return retval;
> > > > >  	}
> > > > >  
> > > > >  	pr_debug("%s: sk:%p, newsk:%p, sd:%d\n", __func__, sk,
> > > > > newsock->sk,
> > > > >  		 retval);
> > > > > +	
> > > > > +	peeloff->sd = retval;
> > > > > +out:
> > > > > +	return retval;
> > > > > +}
> > > > > +
> > > > > +static int sctp_getsockopt_peeloff(struct sock *sk, int len,
> > > > > char __user *optval, int __user *optlen)
> > > > > +{
> > > > > +	sctp_peeloff_arg_t peeloff;
> > > > > +	struct file *newfile = NULL;
> > > > > +	int retval = 0;
> > > > > +
> > > > > +	if (len < sizeof(sctp_peeloff_arg_t))
> > > > > +		return -EINVAL;
> > > > > +	len = sizeof(sctp_peeloff_arg_t);
> > > > > +	if (copy_from_user(&peeloff, optval, len))
> > > > > +		return -EFAULT;
> > > > > +
> > > > > +	retval = sctp_getsockopt_peeloff_common(sk, &peeloff,
> > > > > &newfile);
> > > > > +	if (retval < 0)
> > > > > +		goto out;
> > > > >  
> > > > >  	/* Return the fd mapped to the new socket.  */
> > > > >  	if (put_user(len, optlen)) {
> > > > > -		fput(newfile);
> > > > > +		if (newfile)
> > > > > +			fput(newfile);
> > > > >  		put_unused_fd(retval);
> > > > >  		return -EFAULT;
> > > > >  	}
> > > > > -	peeloff.sd = retval;
> > > > > +
> > > > >  	if (copy_to_user(optval, &peeloff, len)) {
> > > > > -		fput(newfile);
> > > > > +		if (newfile)
> > > > > +			fput(newfile);
> > > > > +		put_unused_fd(retval);
> > > > > +		return -EFAULT;
> > > > > +	}
> > > > > +	fd_install(retval, newfile);
> > > > > +out:
> > > > > +	return retval;
> > > > > +}
> > > > > +
> > > > > +static int sctp_getsockopt_peeloff_flags(struct sock *sk, int
> > > > > len, char __user *optval, int __user *optlen)
> > > > > +{
> > > > > +	sctp_peeloff_flags_arg_t peeloff;
> > > > > +	struct file *newfile = NULL;
> > > > > +	int retval = 0;
> > > > > +
> > > > > +	if (len < sizeof(sctp_peeloff_flags_arg_t))
> > > > > +		return -EINVAL;
> > > > > +	len = sizeof(sctp_peeloff_flags_arg_t);
> > > > > +	if (copy_from_user(&peeloff, optval, len))
> > > > > +		return -EFAULT;
> > > > > +
> > > > > +	retval = sctp_getsockopt_peeloff_common(sk,
> > > > > &peeloff.p_arg, &newfile);
> > > > > +	if (retval < 0)
> > > > > +		goto out;
> > > > > +
> > > > > +	set_close_on_exec(peeloff.p_arg.sd, !!(peeloff.flags &
> > > > > SOCK_CLOEXEC));
> > > > > +
> > > > > +	if (peeloff.flags & SOCK_NONBLOCK)
> > > > > +		newfile->f_flags |= O_NONBLOCK;
> > > > > +
> > > > > +	/* Return the fd mapped to the new socket.  */
> > > > > +	if (put_user(len, optlen)) {
> > > > > +		if (newfile)
> > > > > +			fput(newfile);
> > > > > +		put_unused_fd(retval);
> > > > > +		return -EFAULT;
> > > > > +	}
> > > > > +
> > > > > +	if (copy_to_user(optval, &peeloff, len)) {
> > > > > +		if (newfile)
> > > > > +			fput(newfile);
> > > > >  		put_unused_fd(retval);
> > > > >  		return -EFAULT;
> > > > >  	}
> > > > > @@ -6758,6 +6815,9 @@ static int sctp_getsockopt(struct sock *sk,
> > > > > int level, int optname,
> > > > >  	case SCTP_SOCKOPT_PEELOFF:
> > > > >  		retval = sctp_getsockopt_peeloff(sk, len,
> > > > > optval, optlen);
> > > > >  		break;
> > > > > +	case SCTP_SOCKOPT_PEELOFF_FLAGS:
> > > > > +		retval = sctp_getsockopt_peeloff_flags(sk, len,
> > > > > optval, optlen);
> > > > > +		break;
> > > > >  	case SCTP_PEER_ADDR_PARAMS:
> > > > >  		retval = sctp_getsockopt_peer_addr_params(sk,
> > > > > len, optval,
> > > > >  							  optlen
> > > > > );
> > > > > --
> > > > > 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.htm
> > > > > l
> > > > > 
> > -- 
> > 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
> > 
> Thank you, can I ask you to test this one as well?  It contains the clean up
> that Marcello asked for.
> 
> Best
> Neil
> 
> 
> commit 07fb73580c766905023caabc4ac6b221e047e1eb
> Author: root <root@amd-pence-02.lab.bos.redhat.com>
> Date:   Mon Jun 26 08:58:56 2017 -0400
> 
>     Add peeloff-flags socket option
> 
> diff --git a/fs/file.c b/fs/file.c
> index 1c2972e..a4521da 100644
> --- a/fs/file.c
> +++ b/fs/file.c
> @@ -807,6 +807,7 @@ void set_close_on_exec(unsigned int fd, int flag)
>  		__clear_close_on_exec(fd, fdt);
>  	spin_unlock(&files->file_lock);
>  }
> +EXPORT_SYMBOL(set_close_on_exec);
>  
>  bool get_close_on_exec(unsigned int fd)
>  {
> diff --git a/include/uapi/linux/sctp.h b/include/uapi/linux/sctp.h
> index ced9d8b..6217ff8 100644
> --- a/include/uapi/linux/sctp.h
> +++ b/include/uapi/linux/sctp.h
> @@ -121,6 +121,7 @@ typedef __s32 sctp_assoc_t;
>  #define SCTP_RESET_STREAMS	119
>  #define SCTP_RESET_ASSOC	120
>  #define SCTP_ADD_STREAMS	121
> +#define SCTP_SOCKOPT_PEELOFF_FLAGS 122
>  
>  /* PR-SCTP policies */
>  #define SCTP_PR_SCTP_NONE	0x0000
> @@ -978,6 +979,11 @@ typedef struct {
>  	int sd;
>  } sctp_peeloff_arg_t;
>  
> +typedef struct {
> +	sctp_peeloff_arg_t p_arg;
> +	unsigned flags;
> +} sctp_peeloff_flags_arg_t;
> +
>  /*
>   *  Peer Address Thresholds socket option
>   */
> diff --git a/net/sctp/socket.c b/net/sctp/socket.c
> index 7b6e20e..7341053 100644
> --- a/net/sctp/socket.c
> +++ b/net/sctp/socket.c
> @@ -4933,20 +4933,12 @@ int sctp_do_peeloff(struct sock *sk, sctp_assoc_t id, struct socket **sockp)
>  }
>  EXPORT_SYMBOL(sctp_do_peeloff);
>  
> -static int sctp_getsockopt_peeloff(struct sock *sk, int len, char __user *optval, int __user *optlen)
> +static int sctp_getsockopt_peeloff_common(struct sock *sk, sctp_peeloff_arg_t *peeloff, struct file **newfile, unsigned flags)
>  {
> -	sctp_peeloff_arg_t peeloff;
>  	struct socket *newsock;
> -	struct file *newfile;
>  	int retval = 0;
>  
> -	if (len < sizeof(sctp_peeloff_arg_t))
> -		return -EINVAL;
> -	len = sizeof(sctp_peeloff_arg_t);
> -	if (copy_from_user(&peeloff, optval, len))
> -		return -EFAULT;
> -
> -	retval = sctp_do_peeloff(sk, peeloff.associd, &newsock);
> +	retval = sctp_do_peeloff(sk, peeloff->associd, &newsock);
>  	if (retval < 0)
>  		goto out;
>  
> @@ -4957,25 +4949,90 @@ static int sctp_getsockopt_peeloff(struct sock *sk, int len, char __user *optval
>  		goto out;
>  	}
>  
> -	newfile = sock_alloc_file(newsock, 0, NULL);
> -	if (IS_ERR(newfile)) {
> +	*newfile = sock_alloc_file(newsock, 0, NULL);
> +	if (IS_ERR(*newfile)) {
>  		put_unused_fd(retval);
>  		sock_release(newsock);
> -		return PTR_ERR(newfile);
> +		retval = PTR_ERR(*newfile);
> +		*newfile = NULL;
> +		return retval;
>  	}
>  
>  	pr_debug("%s: sk:%p, newsk:%p, sd:%d\n", __func__, sk, newsock->sk,
>  		 retval);
> +	
> +	peeloff->sd = retval;
> +
> +	set_close_on_exec(peeloff->sd, !!(flags & SOCK_CLOEXEC));
> +
> +	if (flags & SOCK_NONBLOCK)
> +		(*newfile)->f_flags |= O_NONBLOCK;
> +out:
> +	return retval;
> +}
> +
> +static int sctp_getsockopt_peeloff(struct sock *sk, int len, char __user *optval, int __user *optlen)
> +{
> +	sctp_peeloff_arg_t peeloff;
> +	struct file *newfile = NULL;
> +	int retval = 0;
> +
> +	if (len < sizeof(sctp_peeloff_arg_t))
> +		return -EINVAL;
> +	len = sizeof(sctp_peeloff_arg_t);
> +	if (copy_from_user(&peeloff, optval, len))
> +		return -EFAULT;
> +
> +	retval = sctp_getsockopt_peeloff_common(sk, &peeloff, &newfile, 0);
> +	if (retval < 0)
> +		goto out;
>  
>  	/* Return the fd mapped to the new socket.  */
>  	if (put_user(len, optlen)) {
> -		fput(newfile);
> +		if (newfile)
> +			fput(newfile);
>  		put_unused_fd(retval);
>  		return -EFAULT;
>  	}
> -	peeloff.sd = retval;
> +
>  	if (copy_to_user(optval, &peeloff, len)) {
> -		fput(newfile);
> +		if (newfile)
> +			fput(newfile);
> +		put_unused_fd(retval);
> +		return -EFAULT;
> +	}
> +	fd_install(retval, newfile);
> +out:
> +	return retval;
> +}
> +
> +static int sctp_getsockopt_peeloff_flags(struct sock *sk, int len, char __user *optval, int __user *optlen)
> +{
> +	sctp_peeloff_flags_arg_t peeloff;
> +	struct file *newfile = NULL;
> +	int retval = 0;
> +
> +	if (len < sizeof(sctp_peeloff_flags_arg_t))
> +		return -EINVAL;
> +	len = sizeof(sctp_peeloff_flags_arg_t);
> +	if (copy_from_user(&peeloff, optval, len))
> +		return -EFAULT;
> +
> +	retval = sctp_getsockopt_peeloff_common(sk, &peeloff.p_arg, &newfile, peeloff.flags);
> +	if (retval < 0)
> +		goto out;
> +
> +	/* Return the fd mapped to the new socket.  */
> +	if (put_user(len, optlen)) {
> +		if (newfile)
> +			fput(newfile);
> +		put_unused_fd(retval);
> +		return -EFAULT;
> +	}
> +
> +	if (copy_to_user(optval, &peeloff, len)) {
> +		if (newfile)
> +			fput(newfile);
>  		put_unused_fd(retval);
>  		return -EFAULT;
>  	}
> @@ -6758,6 +6815,9 @@ static int sctp_getsockopt(struct sock *sk, int level, int optname,
>  	case SCTP_SOCKOPT_PEELOFF:
>  		retval = sctp_getsockopt_peeloff(sk, len, optval, optlen);
>  		break;
> +	case SCTP_SOCKOPT_PEELOFF_FLAGS:
> +		retval = sctp_getsockopt_peeloff_flags(sk, len, optval, optlen);
> +		break;
>  	case SCTP_PEER_ADDR_PARAMS:
>  		retval = sctp_getsockopt_peer_addr_params(sk, len, optval,
>  							  optlen);
> --
> 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
> 

Re: SCTP_SOCKOPT_PEELOFF is missing SOCK_CLOEXEC (and SOCK_NONBLOCK)

[Neil Horman]

On Mon, Jun 26, 2017 at 11:57:10AM +0200, Andreas Steinmetz wrote:
> A quick test with retrieving the flags with fcntl(F_GETFD/F_GETFL)
> after peeloff shows that your patch works. Good job!
> 
nm, I realized that I needed to update lksctp-tools for this too, and tested it
myself with that.  It seems to work for me, I'll post this officially shortly.

Neil