Re: SCTP_SOCKOPT_PEELOFF is missing SOCK_CLOEXEC (and SOCK_NONBLOCK)

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Tue, Jun 20, 2017 at 12:41:47PM -0300, Marcelo Ricardo Leitner wrote:
> Hi,
> 
> On Tue, Jun 20, 2017 at 10:39:51AM -0400, Neil Horman wrote:
> > On Tue, Jun 20, 2017 at 03:24:13PM +0200, Andreas Steinmetz wrote:
> > > [please CC me, I'm not subscribed]
> > > 
> > > It seems that if one does a getsockopt(SCTP_SOCKOPT_PEELOFF) a.k.a.
> > > sctp_peeloff(), even if the socket descriptor from which the
> > > association is to be peeled off has SOCK_CLOEXEC/SOCK_NONBLOCK set,
> > > the peeled off socket descriptor doesn't have so.
> > > 
> > > It would be advisable to either clone these flags or add an atomic
> > > version of SCTP_SOCKOPT_PEELOFF (accept4() style).
> > > 
> > > The missing SOCK_CLOEXEC requires unintentional additional locking
> > > which typically is prone to errors and can slow down processing.
> > > -- 
> > > Andreas Steinmetz                       SPAMmers use robotrap@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
> > > 
> > 
> > 
> > Try this patch, and confirm that it fixes the problem please?
> > 
> > Thanks!
> > Neil
> > 
> > 
> > diff --git a/net/sctp/socket.c b/net/sctp/socket.c
> > index f16c8d9..a95e3d6 100644
> > --- a/net/sctp/socket.c
> > +++ b/net/sctp/socket.c
> > @@ -4912,7 +4912,7 @@ int sctp_do_peeloff(struct sock *sk, sctp_assoc_t id, struct socket **sockp)
> >  		return -EINVAL;
> >  
> >  	/* Create a new socket.  */
> > -	err = sock_create(sk->sk_family, SOCK_SEQPACKET, IPPROTO_SCTP, &sock);
> > +	err = sock_create(sk->sk_family, sk->sk_type, IPPROTO_SCTP, &sock);
> 
> I don't think this will work. Syscall socket() filters out these
> flags and handle them separetly after calling sock_create() with just
> the type, so I don't think this information is preserved in there.
> 
> Interesting to note that according to man page dup() syscall also
> doesn't duplicate these flags. One has to use dup3() instead in order to
> be able to specify them.
> 
> Maybe by extending sctp_peeloff_arg_t to have a flags attribute in
> there, we can allow the application to specify it and feed into
> get_unused_fd_flags() call in sctp_getsockopt_peeloff() instead, or even
> just overload the sd, which is currently an output-only value, to
> contain flags as the patch below. (We probably should add some sanity
> checking in there, though)
> 
Thinking about this some more, I'm a bit hesitant to change the
sctp_peeloff_arg_t, since thats exposed to user space.  Instead, what if we use
the sd value in the peeloff arg to fetch the close_on_exec flag in the new fd?
Something like this (untested) patch:


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



[Index of Archives]     [Linux Networking Development]     [Linux OMAP]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux