SCTP_SOCKOPT_PEELOFF is missing SOCK_CLOEXEC (and SOCK_NONBLOCK)

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

Signature · 5 lines

Andreas Steinmetz                       SPAMmers use [email protected]
--
To unsubscribe from this list: send the line "unsubscribe linux-sctp" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Quoted 7 lines · 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.

Quoted 12 lines

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.

Signature · 5 lines

Andreas Steinmetz                       SPAMmers use [email protected]
--
To unsubscribe from this list: send the line "unsubscribe linux-sctp" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Try this patch, and confirm that it fixes the problem please?

Thanks!
Neil

net/sctp/socket.c +0 -0

index f16c8d9..a95e3d6 100644

Hi,

Quoted 35 lines · Neil Horman

Quoted 7 lines · 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.

Quoted 12 lines

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.

Signature · 5 lines

Andreas Steinmetz                       SPAMmers use [email protected]
--
To unsubscribe from this list: send the line "unsubscribe linux-sctp" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Try this patch, and confirm that it fixes the problem please?

Thanks!
Neil

net/sctp/socket.c +0 -0

index f16c8d9..a95e3d6 100644
@@ -4912,7 +4912,7 @@ int sctp_do_peeloff(struct sock *sk, sctp_assoc_t id, struct socket **sockp)
 		return -EINVAL;
 

net/sctp/socket.c int sctp_do_peeloff(struct sock *sk, sctp_assoc_t id, struct socket **sockp) · line 4915

Quoted 3 lines

net/sctp/socket.c +1 -1

 	/* 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<---

net/sctp/socket.c +4 -3

index e1fabf67d4ad..50021a925b16 100644
@@ -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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Quoted 38 lines · Marcelo Ricardo Leitner

Hi,

Quoted 35 lines · Neil Horman

Quoted 7 lines · 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.

Quoted 12 lines

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.

Signature · 5 lines

Andreas Steinmetz                       SPAMmers use [email protected]
--
To unsubscribe from this list: send the line "unsubscribe linux-sctp" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Try this patch, and confirm that it fixes the problem please?

Thanks!
Neil

net/sctp/socket.c +0 -0

index f16c8d9..a95e3d6 100644
@@ -4912,7 +4912,7 @@ int sctp_do_peeloff(struct sock *sk, sctp_assoc_t id, struct socket **sockp)
 		return -EINVAL;
 

Quoted 8 lines

net/sctp/socket.c int sctp_do_peeloff(struct sock *sk, sctp_assoc_t id, struct socket **sockp) · line 4915

Quoted 3 lines

net/sctp/socket.c +1 -1

 	/* 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.

Quoted 4 lines

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.

Quoted 7 lines

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

Quoted 23 lines

Thanks,
Marcelo

---8<---

net/sctp/socket.c +4 -3

index e1fabf67d4ad..50021a925b16 100644
@@ -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;
--

Quoted 4 lines

To unsubscribe from this list: send the line "unsubscribe linux-sctp" in the body of a message to [email protected]
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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Hi,

Quoted 33 lines · Neil Horman

Quoted 7 lines · 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.

Quoted 12 lines

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.

Signature · 5 lines

Andreas Steinmetz                       SPAMmers use [email protected]
--
To unsubscribe from this list: send the line "unsubscribe linux-sctp" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Try this patch, and confirm that it fixes the problem please?

Thanks!
Neil

net/sctp/socket.c +0 -0

index f16c8d9..a95e3d6 100644

net/sctp/socket.c int sctp_do_peeloff(struct sock *sk, sctp_assoc_t id, struct socket **sockp) · line 4915

Quoted 5 lines

net/sctp/socket.c +1 -1

@@ -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)

On Tue, Jun 20, 2017 at 12:41:47PM -0300, Marcelo Ricardo Leitner wrote:

Quoted 7 lines

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:

net/sctp/socket.c +12 -2

index f16c8d9..6386ac4 100644
@@ -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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Quoted 2 lines

On Tue, Jun 20, 2017 at 12:41:47PM -0300, Marcelo Ricardo Leitner wrote:

...

Quoted 11 lines

Quoted 10 lines

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)

Quoted 3 lines

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

Quoted 3 lines

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

Quoted 43 lines

net/sctp/socket.c +0 -0

index f16c8d9..6386ac4 100644
@@ -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;

net/sctp/socket.c +2 -0

+	struct file *oldfile
+	unsigned flags = 0;

  int retval = 0;

  if (len < sizeof(sctp_peeloff_arg_t))

net/sctp/socket.c +0 -0

@@ -4951,8 +4953,17 @@ static int sctp_getsockopt_peeloff(struct

sock *sk, int len, char __user *optval
  if (retval < 0)
  goto out;

net/sctp/socket.c +9 -0

+	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.  */

net/sctp/socket.c +1 -1

-	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 [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Quoted 1 line · Neil Horman

On Tue, Jun 20, 2017 at 12:41:47PM -0300, Marcelo Ricardo Leitner wrote:

...

Quoted 9 lines

Quoted 7 lines

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

Quoted 2 lines

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

Quoted 39 lines

net/sctp/socket.c +12 -2

index f16c8d9..6386ac4 100644
@@ -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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Quoted 22 lines · Marcelo Ricardo Leitner

Quoted 1 line · Neil Horman

On Tue, Jun 20, 2017 at 12:41:47PM -0300, Marcelo Ricardo Leitner wrote:

...

Quoted 9 lines

Quoted 7 lines

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

Quoted 2 lines

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.

Quoted 6 lines

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

Quoted 47 lines · Neil Horman

Quoted 22 lines · Marcelo Ricardo Leitner

Quoted 1 line · Neil Horman

On Tue, Jun 20, 2017 at 12:41:47PM -0300, Marcelo Ricardo Leitner wrote:

...

Quoted 9 lines

Quoted 7 lines

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

Quoted 2 lines

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.

Quoted 6 lines

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.

Quoted 2 lines

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

Quoted 62 lines · Marcelo Ricardo Leitner

Quoted 47 lines · Neil Horman

Quoted 22 lines · Marcelo Ricardo Leitner

Quoted 1 line · Neil Horman

On Tue, Jun 20, 2017 at 12:41:47PM -0300, Marcelo Ricardo Leitner wrote:

...

Quoted 9 lines

Quoted 7 lines

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

Quoted 2 lines

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.

Quoted 6 lines

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.

Quoted 2 lines

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.

Quoted 7 lines

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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Quoted 70 lines · Neil Horman

Quoted 62 lines · Marcelo Ricardo Leitner

Quoted 47 lines · Neil Horman

Quoted 22 lines · Marcelo Ricardo Leitner

Quoted 1 line · Neil Horman

On Tue, Jun 20, 2017 at 12:41:47PM -0300, Marcelo Ricardo Leitner wrote:

...

Quoted 9 lines

Quoted 7 lines

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

Quoted 2 lines

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.

Quoted 6 lines

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.

Quoted 2 lines

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.

Quoted 7 lines

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

Cheers,
Marcelo

Quoted 8 lines

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 [email protected]
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

fs/file.c +1 -0

index 1c2972e..a4521da 100644
@@ -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)
 {

include/uapi/linux/sctp.h +6 -0

index ced9d8b..6217ff8 100644
@@ -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
  */

net/sctp/socket.c +76 -17

index 7b6e20e..f210394 100644
@@ -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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Quoted 80 lines · Neil Horman

Quoted 70 lines · Neil Horman

Quoted 62 lines · Marcelo Ricardo Leitner

Quoted 47 lines · Neil Horman

Quoted 22 lines · Marcelo Ricardo Leitner

Quoted 1 line · Neil Horman

On Tue, Jun 20, 2017 at 12:41:47PM -0300, Marcelo Ricardo Leitner wrote:

...

Quoted 9 lines

Quoted 7 lines

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

Quoted 2 lines

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.

Quoted 6 lines

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.

Quoted 2 lines

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.

Quoted 7 lines

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

Cheers,
Marcelo

Quoted 8 lines

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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Quoted 7 lines

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

Quoted 179 lines

Neil

fs/file.c +1 -0

index 1c2972e..a4521da 100644
@@ -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)
 {

include/uapi/linux/sctp.h +6 -0

index ced9d8b..6217ff8 100644
@@ -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
  */

net/sctp/socket.c +76 -17

index 7b6e20e..f210394 100644
@@ -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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Quoted 135 lines · Marcelo Ricardo Leitner

Quoted 120 lines · Neil Horman

Quoted 111 lines · Neil Horman

Quoted 97 lines · Marcelo Ricardo Leitner

Quoted 76 lines · Neil Horman

Quoted 36 lines · Marcelo Ricardo Leitner

Quoted 2 lines · Neil Horman

On Tue, Jun 20, 2017 at 12:41:47PM -0300, Marcelo
Ricardo Leitner wrote:

...

Quoted 13 lines

Quoted 12 lines

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)

Quoted 4 lines

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

Quoted 3 lines

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.

Quoted 7 lines

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.

Quoted 2 lines

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.

Quoted 8 lines

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

Quoted 7 lines

--
To unsubscribe from this list: send the line "unsubscribe linux-sctp" in
the body of a message to [email protected] More majordomo info at  http://vger.kernel.org/majordomo-info.h
tml

Quoted 13 lines

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.

Quoted 8 lines

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

Quoted 199 lines

Quoted 199 lines

Neil

fs/file.c +1 -0

index 1c2972e..a4521da 100644
@@ -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)
 {

include/uapi/linux/sctp.h +1 -0

index ced9d8b..6217ff8 100644
@@ -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

include/uapi/linux/sctp.h +0 -0

@@ -978,6 +979,11 @@ typedef struct {
  	int sd;
  } sctp_peeloff_arg_t;

include/uapi/linux/sctp.h +5 -0

+typedef struct {
+	sctp_peeloff_arg_t p_arg;
+	unsigned flags;
+} sctp_peeloff_flags_arg_t;
+

 /*
  *  Peer Address Thresholds socket option
  */

net/sctp/socket.c +0 -0

index 7b6e20e..f210394 100644
@@ -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) {

net/sctp/socket.c +0 -1

-	sctp_peeloff_arg_t peeloff;

  struct socket *newsock;

net/sctp/socket.c +0 -1

-	struct file *newfile;

  int retval = 0;

net/sctp/socket.c +1 -7

-	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;

net/sctp/socket.c +0 -0

@@ -4957,25 +4949,90 @@ static int sctp_getsockopt_peeloff(struct

sock *sk, int len, char __user *optval
  goto out;
  }

net/sctp/socket.c +2 -2

-	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);

net/sctp/socket.c +3 -1

-		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);

net/sctp/socket.c +7 -0

+	
+	peeloff->sd = retval;
+out:
+	return retval;
+}
+
+static int sctp_getsockopt_peeloff(struct sock *sk, int len,

char __user *optval, int __user *optlen)

net/sctp/socket.c +12 -0

+{
+	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);

net/sctp/socket.c +2 -0

+	if (retval < 0)
+		goto out;

  /* Return the fd mapped to the new socket.  */ if (put_user(len, optlen)) {

net/sctp/socket.c +2 -1

-		fput(newfile);
+		if (newfile)
+			fput(newfile);

  put_unused_fd(retval);
  return -EFAULT;
  }

net/sctp/socket.c +1 -1

-	peeloff.sd = retval;
+

  if (copy_to_user(optval, &peeloff, len)) {

net/sctp/socket.c +11 -1

-		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)

net/sctp/socket.c +12 -0

+{
+	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);

net/sctp/socket.c +4 -0

+	if (retval < 0)
+		goto out;
+
+	set_close_on_exec(peeloff.p_arg.sd, !!(peeloff.flags &

SOCK_CLOEXEC));

net/sctp/socket.c +15 -0

+
+	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;
  }

net/sctp/socket.c +0 -0

@@ -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;

net/sctp/socket.c +2 -0

+	case SCTP_SOCKOPT_PEELOFF_FLAGS:
+		retval = sctp_getsockopt_peeloff_flags(sk, len,

optval, optlen);

net/sctp/socket.c +1 -0

+		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 [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.htm
l

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:

Quoted 347 lines

Quoted 135 lines · Marcelo Ricardo Leitner

Quoted 120 lines · Neil Horman

Quoted 111 lines · Neil Horman

Quoted 97 lines · Marcelo Ricardo Leitner

Quoted 76 lines · Neil Horman

Quoted 36 lines · Marcelo Ricardo Leitner

Quoted 2 lines · Neil Horman

On Tue, Jun 20, 2017 at 12:41:47PM -0300, Marcelo
Ricardo Leitner wrote:

...

Quoted 13 lines

Quoted 12 lines

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)

Quoted 4 lines

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

Quoted 3 lines

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.

Quoted 7 lines

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.

Quoted 2 lines

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.

Quoted 8 lines

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

Quoted 7 lines

--
To unsubscribe from this list: send the line "unsubscribe linux-sctp" in
the body of a message to [email protected] More majordomo info at  http://vger.kernel.org/majordomo-info.h
tml

Quoted 13 lines

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.

Quoted 8 lines

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

Quoted 191 lines

Quoted 191 lines

Neil

fs/file.c +1 -0

index 1c2972e..a4521da 100644
@@ -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)
 {

include/uapi/linux/sctp.h +1 -0

index ced9d8b..6217ff8 100644
@@ -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

include/uapi/linux/sctp.h +0 -0

@@ -978,6 +979,11 @@ typedef struct {
  	int sd;
  } sctp_peeloff_arg_t;

include/uapi/linux/sctp.h +5 -0

+typedef struct {
+	sctp_peeloff_arg_t p_arg;
+	unsigned flags;
+} sctp_peeloff_flags_arg_t;
+

 /*
  *  Peer Address Thresholds socket option
  */

net/sctp/socket.c +0 -0

index 7b6e20e..f210394 100644
@@ -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) {

net/sctp/socket.c +0 -1

-	sctp_peeloff_arg_t peeloff;

  struct socket *newsock;

net/sctp/socket.c +0 -1

-	struct file *newfile;

  int retval = 0;

net/sctp/socket.c +1 -7

-	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;

net/sctp/socket.c +0 -0

@@ -4957,25 +4949,90 @@ static int sctp_getsockopt_peeloff(struct

sock *sk, int len, char __user *optval
  goto out;
  }

net/sctp/socket.c +2 -2

-	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);

net/sctp/socket.c +3 -1

-		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);

net/sctp/socket.c +7 -0

+	
+	peeloff->sd = retval;
+out:
+	return retval;
+}
+
+static int sctp_getsockopt_peeloff(struct sock *sk, int len,

char __user *optval, int __user *optlen)

net/sctp/socket.c +12 -0

+{
+	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);

net/sctp/socket.c +2 -0

+	if (retval < 0)
+		goto out;

  /* Return the fd mapped to the new socket.  */ if (put_user(len, optlen)) {

net/sctp/socket.c +2 -1

-		fput(newfile);
+		if (newfile)
+			fput(newfile);

  put_unused_fd(retval);
  return -EFAULT;
  }

net/sctp/socket.c +1 -1

-	peeloff.sd = retval;
+

  if (copy_to_user(optval, &peeloff, len)) {

net/sctp/socket.c +11 -1

-		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)

net/sctp/socket.c +12 -0

+{
+	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);

net/sctp/socket.c +4 -0

+	if (retval < 0)
+		goto out;
+
+	set_close_on_exec(peeloff.p_arg.sd, !!(peeloff.flags &

SOCK_CLOEXEC));

net/sctp/socket.c +15 -0

+
+	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;
  }

net/sctp/socket.c +0 -0

@@ -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;

net/sctp/socket.c +2 -0

+	case SCTP_SOCKOPT_PEELOFF_FLAGS:
+		retval = sctp_getsockopt_peeloff_flags(sk, len,

optval, optlen);

net/sctp/socket.c +1 -0

+		break;

  case SCTP_PEER_ADDR_PARAMS:
  retval = sctp_getsockopt_peer_addr_params(sk, len, optval,
    optlen

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

);
--
To unsubscribe from this list: send the line "unsubscribe linux- sctp" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.htm
l

Signature · 5 lines

Andreas Steinmetz                       SPAMmers use [email protected]
--
To unsubscribe from this list: send the line "unsubscribe linux-sctp" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Quoted 351 lines · 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:

Quoted 347 lines

Quoted 135 lines · Marcelo Ricardo Leitner

Quoted 120 lines · Neil Horman

Quoted 111 lines · Neil Horman

Quoted 97 lines · Marcelo Ricardo Leitner

Quoted 76 lines · Neil Horman

Quoted 36 lines · Marcelo Ricardo Leitner

Quoted 2 lines · Neil Horman

On Tue, Jun 20, 2017 at 12:41:47PM -0300, Marcelo
Ricardo Leitner wrote:

...

Quoted 13 lines

Quoted 12 lines

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)

Quoted 4 lines

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

Quoted 3 lines

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.

Quoted 7 lines

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.

Quoted 2 lines

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.

Quoted 8 lines

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

Quoted 7 lines

--
To unsubscribe from this list: send the line "unsubscribe linux-sctp" in
the body of a message to [email protected] More majordomo info at  http://vger.kernel.org/majordomo-info.h
tml

Quoted 13 lines

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.

Quoted 8 lines

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

Quoted 191 lines

Quoted 191 lines

Neil

fs/file.c +1 -0

index 1c2972e..a4521da 100644
@@ -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)
 {

include/uapi/linux/sctp.h +1 -0

index ced9d8b..6217ff8 100644
@@ -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

include/uapi/linux/sctp.h +0 -0

@@ -978,6 +979,11 @@ typedef struct {
  	int sd;
  } sctp_peeloff_arg_t;

include/uapi/linux/sctp.h +5 -0

+typedef struct {
+	sctp_peeloff_arg_t p_arg;
+	unsigned flags;
+} sctp_peeloff_flags_arg_t;
+

 /*
  *  Peer Address Thresholds socket option
  */

net/sctp/socket.c +0 -0

index 7b6e20e..f210394 100644
@@ -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) {

net/sctp/socket.c +0 -1

-	sctp_peeloff_arg_t peeloff;

  struct socket *newsock;

net/sctp/socket.c +0 -1

-	struct file *newfile;

  int retval = 0;

net/sctp/socket.c +1 -7

-	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;

net/sctp/socket.c +0 -0

@@ -4957,25 +4949,90 @@ static int sctp_getsockopt_peeloff(struct

sock *sk, int len, char __user *optval
  goto out;
  }

net/sctp/socket.c +2 -2

-	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);

net/sctp/socket.c +3 -1

-		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);

net/sctp/socket.c +7 -0

+	
+	peeloff->sd = retval;
+out:
+	return retval;
+}
+
+static int sctp_getsockopt_peeloff(struct sock *sk, int len,

char __user *optval, int __user *optlen)

net/sctp/socket.c +12 -0

+{
+	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);

net/sctp/socket.c +2 -0

+	if (retval < 0)
+		goto out;

  /* Return the fd mapped to the new socket.  */ if (put_user(len, optlen)) {

net/sctp/socket.c +2 -1

-		fput(newfile);
+		if (newfile)
+			fput(newfile);

  put_unused_fd(retval);
  return -EFAULT;
  }

net/sctp/socket.c +1 -1

-	peeloff.sd = retval;
+

  if (copy_to_user(optval, &peeloff, len)) {

net/sctp/socket.c +11 -1

-		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)

net/sctp/socket.c +12 -0

+{
+	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);

net/sctp/socket.c +4 -0

+	if (retval < 0)
+		goto out;
+
+	set_close_on_exec(peeloff.p_arg.sd, !!(peeloff.flags &

SOCK_CLOEXEC));

net/sctp/socket.c +15 -0

+
+	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;
  }

net/sctp/socket.c +0 -0

@@ -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;

net/sctp/socket.c +2 -0

+	case SCTP_SOCKOPT_PEELOFF_FLAGS:
+		retval = sctp_getsockopt_peeloff_flags(sk, len,

optval, optlen);

net/sctp/socket.c +1 -0

+		break;

  case SCTP_PEER_ADDR_PARAMS:
  retval = sctp_getsockopt_peer_addr_params(sk, len, optval,
    optlen

Quoted 15 lines

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

);
--
To unsubscribe from this list: send the line "unsubscribe linux- sctp" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.htm
l

Signature · 5 lines

Andreas Steinmetz                       SPAMmers use [email protected]
--
To unsubscribe from this list: send the line "unsubscribe linux-sctp" in
the body of a message to [email protected]
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 <[email protected]>
Date:   Mon Jun 26 08:58:56 2017 -0400

Add peeloff-flags socket option

fs/file.c +1 -0

index 1c2972e..a4521da 100644
@@ -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)
 {

include/uapi/linux/sctp.h +6 -0

index ced9d8b..6217ff8 100644
@@ -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
  */

net/sctp/socket.c +76 -17

index 7b6e20e..7341053 100644
@@ -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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

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