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.html > > > > > > > Ok, sorry for the delay. I tested this patch out with the lksctp test suite to > > validate that the traditional peeloff path works, but I don't have the userspace > > code written to test the peeloff_flags path. Andreas, if you could validate > > that, I would appreciate it. I'm not real thrilled with the export of the > > set_close_on_exec function, but I'm not sure I like using the internal function > > any better. Either way, Andreas, if it works for you, I'll propose it > > officially and we can go from there. > > What about exposing a flags arg on sctp_getsockopt_peeloff_common()? > Then I guess you don't need such post-processing as you can pass it to > get_unused_fd_flags() directly. > > Marcelo > Yeah, that makes sense, and then default the flags to 0 on sctp_getsockopt_peelof. Andreas, can you please test the patch as-is right now, just to make sure the path with flags set works properly, and I'll clean this up in parallel while you do that ahead of official submission? Thanks Neil > > > > Neil > > > > > > diff --git a/fs/file.c b/fs/file.c > > index 1c2972e..a4521da 100644 > > --- a/fs/file.c > > +++ b/fs/file.c > > @@ -807,6 +807,7 @@ void set_close_on_exec(unsigned int fd, int flag) > > __clear_close_on_exec(fd, fdt); > > spin_unlock(&files->file_lock); > > } > > +EXPORT_SYMBOL(set_close_on_exec); > > > > bool get_close_on_exec(unsigned int fd) > > { > > diff --git a/include/uapi/linux/sctp.h b/include/uapi/linux/sctp.h > > index ced9d8b..6217ff8 100644 > > --- a/include/uapi/linux/sctp.h > > +++ b/include/uapi/linux/sctp.h > > @@ -121,6 +121,7 @@ typedef __s32 sctp_assoc_t; > > #define SCTP_RESET_STREAMS 119 > > #define SCTP_RESET_ASSOC 120 > > #define SCTP_ADD_STREAMS 121 > > +#define SCTP_SOCKOPT_PEELOFF_FLAGS 122 > > > > /* PR-SCTP policies */ > > #define SCTP_PR_SCTP_NONE 0x0000 > > @@ -978,6 +979,11 @@ typedef struct { > > int sd; > > } sctp_peeloff_arg_t; > > > > +typedef struct { > > + sctp_peeloff_arg_t p_arg; > > + unsigned flags; > > +} sctp_peeloff_flags_arg_t; > > + > > /* > > * Peer Address Thresholds socket option > > */ > > diff --git a/net/sctp/socket.c b/net/sctp/socket.c > > index 7b6e20e..f210394 100644 > > --- a/net/sctp/socket.c > > +++ b/net/sctp/socket.c > > @@ -4933,20 +4933,12 @@ int sctp_do_peeloff(struct sock *sk, sctp_assoc_t id, struct socket **sockp) > > } > > EXPORT_SYMBOL(sctp_do_peeloff); > > > > -static int sctp_getsockopt_peeloff(struct sock *sk, int len, char __user *optval, int __user *optlen) > > +static int sctp_getsockopt_peeloff_common(struct sock *sk, sctp_peeloff_arg_t *peeloff, struct file **newfile) > > { > > - sctp_peeloff_arg_t peeloff; > > struct socket *newsock; > > - struct file *newfile; > > int retval = 0; > > > > - if (len < sizeof(sctp_peeloff_arg_t)) > > - return -EINVAL; > > - len = sizeof(sctp_peeloff_arg_t); > > - if (copy_from_user(&peeloff, optval, len)) > > - return -EFAULT; > > - > > - retval = sctp_do_peeloff(sk, peeloff.associd, &newsock); > > + retval = sctp_do_peeloff(sk, peeloff->associd, &newsock); > > if (retval < 0) > > goto out; > > > > @@ -4957,25 +4949,90 @@ static int sctp_getsockopt_peeloff(struct sock *sk, int len, char __user *optval > > goto out; > > } > > > > - newfile = sock_alloc_file(newsock, 0, NULL); > > - if (IS_ERR(newfile)) { > > + *newfile = sock_alloc_file(newsock, 0, NULL); > > + if (IS_ERR(*newfile)) { > > put_unused_fd(retval); > > sock_release(newsock); > > - return PTR_ERR(newfile); > > + retval = PTR_ERR(*newfile); > > + *newfile = NULL; > > + return retval; > > } > > > > pr_debug("%s: sk:%p, newsk:%p, sd:%d\n", __func__, sk, newsock->sk, > > retval); > > + > > + peeloff->sd = retval; > > +out: > > + return retval; > > +} > > + > > +static int sctp_getsockopt_peeloff(struct sock *sk, int len, char __user *optval, int __user *optlen) > > +{ > > + sctp_peeloff_arg_t peeloff; > > + struct file *newfile = NULL; > > + int retval = 0; > > + > > + if (len < sizeof(sctp_peeloff_arg_t)) > > + return -EINVAL; > > + len = sizeof(sctp_peeloff_arg_t); > > + if (copy_from_user(&peeloff, optval, len)) > > + return -EFAULT; > > + > > + retval = sctp_getsockopt_peeloff_common(sk, &peeloff, &newfile); > > + if (retval < 0) > > + goto out; > > > > /* Return the fd mapped to the new socket. */ > > if (put_user(len, optlen)) { > > - fput(newfile); > > + if (newfile) > > + fput(newfile); > > put_unused_fd(retval); > > return -EFAULT; > > } > > - peeloff.sd = retval; > > + > > if (copy_to_user(optval, &peeloff, len)) { > > - fput(newfile); > > + if (newfile) > > + fput(newfile); > > + put_unused_fd(retval); > > + return -EFAULT; > > + } > > + fd_install(retval, newfile); > > +out: > > + return retval; > > +} > > + > > +static int sctp_getsockopt_peeloff_flags(struct sock *sk, int len, char __user *optval, int __user *optlen) > > +{ > > + sctp_peeloff_flags_arg_t peeloff; > > + struct file *newfile = NULL; > > + int retval = 0; > > + > > + if (len < sizeof(sctp_peeloff_flags_arg_t)) > > + return -EINVAL; > > + len = sizeof(sctp_peeloff_flags_arg_t); > > + if (copy_from_user(&peeloff, optval, len)) > > + return -EFAULT; > > + > > + retval = sctp_getsockopt_peeloff_common(sk, &peeloff.p_arg, &newfile); > > + if (retval < 0) > > + goto out; > > + > > + set_close_on_exec(peeloff.p_arg.sd, !!(peeloff.flags & SOCK_CLOEXEC)); > > + > > + if (peeloff.flags & SOCK_NONBLOCK) > > + newfile->f_flags |= O_NONBLOCK; > > + > > + /* Return the fd mapped to the new socket. */ > > + if (put_user(len, optlen)) { > > + if (newfile) > > + fput(newfile); > > + put_unused_fd(retval); > > + return -EFAULT; > > + } > > + > > + if (copy_to_user(optval, &peeloff, len)) { > > + if (newfile) > > + fput(newfile); > > put_unused_fd(retval); > > return -EFAULT; > > } > > @@ -6758,6 +6815,9 @@ static int sctp_getsockopt(struct sock *sk, int level, int optname, > > case SCTP_SOCKOPT_PEELOFF: > > retval = sctp_getsockopt_peeloff(sk, len, optval, optlen); > > break; > > + case SCTP_SOCKOPT_PEELOFF_FLAGS: > > + retval = sctp_getsockopt_peeloff_flags(sk, len, optval, optlen); > > + break; > > case SCTP_PEER_ADDR_PARAMS: > > retval = sctp_getsockopt_peer_addr_params(sk, len, optval, > > optlen); > > -- > > To unsubscribe from this list: send the line "unsubscribe linux-sctp" in > > the body of a message to majordomo@xxxxxxxxxxxxxxx > > More majordomo info at http://vger.kernel.org/majordomo-info.html > > > -- To unsubscribe from this list: send the line "unsubscribe linux-sctp" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
