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@xxxxxxxxxxxxxxx > > > > > 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@xxxxxxxxxxxxxxx > > > > More majordomo info at http://vger.kernel.org/majordomo-info.htm > > > > l > > > > > -- > Andreas Steinmetz SPAMmers use robotrap@xxxxxxxx > -- > To unsubscribe from this list: send the line "unsubscribe linux-sctp" in > the body of a message to majordomo@xxxxxxxxxxxxxxx > 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@xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
