| Message ID | 6091a8542d13f43fbe1abfa25062d28d15b15e66.1436891629.git.marcelo.leitner@gmail.com |
|---|---|
| State | Rejected, archived |
| Delegated to: | David Miller |
| Headers | show |
On Tue, Jul 14, 2015 at 02:13:24PM -0300, Marcelo Ricardo Leitner wrote: > SCTP has this operation to peel off associations from a given socket and > create a new socket using this association. We currently have two ways > to use this operation: > - via getsockopt(), on which it will also create and return a file > descriptor for this new socket > - via sctp_do_peeloff(), which is for kernel only > > The caveat with using sctp_do_peeloff() directly is that it creates a > dependency to SCTP module, while all other operations are handled via > kernel_{socket,sendmsg,getsockopt...}() interface. This causes the > kernel to load SCTP module even when it's not really used. > > This patch then creates a new sockopt that is to be used only by kernel > users of this protocol. This new sockopt will not allocate a file > descriptor but instead just return the socket pointer directly. > > Kernel users are actually identified by if the parent socket has or not > a fd attached to it. If not, it's a kernel a user. > > If called by an user application, it will just return -EPERM. > > Even though it's not intended for user applications, it's listed under > uapi header. That's because hidding this wouldn't add any extra security > and to keep the sockopt list in one place, so it's easy to check > available numbers to use. > > Signed-off-by: Marcelo Ricardo Leitner <marcelo.leitner@gmail.com> > --- > include/uapi/linux/sctp.h | 12 ++++++++++++ > net/sctp/socket.c | 37 +++++++++++++++++++++++++++++++++++++ > 2 files changed, 49 insertions(+) > > diff --git a/include/uapi/linux/sctp.h b/include/uapi/linux/sctp.h > index ce70fe6b45df3e841c35accbdb6379c16563893c..b3aad3ce456ab3c1ebf4d81fdb7269ba40b3d92a 100644 > --- a/include/uapi/linux/sctp.h > +++ b/include/uapi/linux/sctp.h > @@ -105,6 +105,10 @@ typedef __s32 sctp_assoc_t; > #define SCTP_SOCKOPT_BINDX_ADD 100 /* BINDX requests for adding addrs */ > #define SCTP_SOCKOPT_BINDX_REM 101 /* BINDX requests for removing addrs. */ > #define SCTP_SOCKOPT_PEELOFF 102 /* peel off association. */ > +#define SCTP_SOCKOPT_PEELOFF_KERNEL 103 /* peel off association. > + * only valid for kernel > + * users > + */ > /* Options 104-106 are deprecated and removed. Do not use this space */ > #define SCTP_SOCKOPT_CONNECTX_OLD 107 /* CONNECTX old requests. */ > #define SCTP_GET_PEER_ADDRS 108 /* Get all peer address. */ > @@ -892,6 +896,14 @@ typedef struct { > int sd; > } sctp_peeloff_arg_t; > > +/* This is the union that is passed as an argument(optval) to > + * getsockopt(SCTP_SOCKOPT_PEELOFF_KERNEL). > + */ > +typedef union { > + sctp_assoc_t associd; > + struct socket *socket; > +} sctp_peeloff_kernel_arg_t; > + > /* > * Peer Address Thresholds socket option > */ > diff --git a/net/sctp/socket.c b/net/sctp/socket.c > index f1a65398f3118ab5d3a884e9c875620560e6b5ef..7968de7a1aeabd5cd0a0398461dbf2081bd4c5b7 100644 > --- a/net/sctp/socket.c > +++ b/net/sctp/socket.c > @@ -4504,6 +4504,39 @@ out: > return retval; > } > > +static int sctp_getsockopt_peeloff_kernel(struct sock *sk, int len, > + char __user *optval, int __user *optlen) > +{ > + sctp_peeloff_kernel_arg_t peeloff; > + struct socket *newsock; > + int retval = 0; > + > + /* We only allow this operation if parent socket also hadn't a > + * file descriptor allocated to it, mainly as a way to make sure > + * that this is really a kernel socket. > + */ > + if (sk->sk_socket->file) > + return -EPERM; > + > + if (len < sizeof(sctp_peeloff_kernel_arg_t)) > + return -EINVAL; > + len = sizeof(sctp_peeloff_kernel_arg_t); > + if (copy_from_user(&peeloff, optval, len)) > + return -EFAULT; > + > + retval = sctp_do_peeloff(sk, peeloff.associd, &newsock); > + if (retval < 0) > + goto out; > + > + peeloff.socket = newsock; > + if (copy_to_user(optval, &peeloff, len)) { > + sock_release(newsock); > + return -EFAULT; > + } > +out: > + return retval; > +} > + > /* 7.1.13 Peer Address Parameters (SCTP_PEER_ADDR_PARAMS) > * > * Applications can enable or disable heartbeats for any peer address of > @@ -5991,6 +6024,10 @@ 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_KERNEL: > + retval = sctp_getsockopt_peeloff_kernel(sk, len, optval, > + optlen); > + break; Do we need to do something here to prevent user space calls from inadvertently accessing this option? Neil > case SCTP_PEER_ADDR_PARAMS: > retval = sctp_getsockopt_peer_addr_params(sk, len, optval, > optlen); > -- > 2.4.1 > > -- > To unsubscribe from this list: send the line "unsubscribe linux-sctp" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 15-07-2015 10:18, Neil Horman wrote: > On Tue, Jul 14, 2015 at 02:13:24PM -0300, Marcelo Ricardo Leitner wrote: >> SCTP has this operation to peel off associations from a given socket and >> create a new socket using this association. We currently have two ways >> to use this operation: >> - via getsockopt(), on which it will also create and return a file >> descriptor for this new socket >> - via sctp_do_peeloff(), which is for kernel only >> >> The caveat with using sctp_do_peeloff() directly is that it creates a >> dependency to SCTP module, while all other operations are handled via >> kernel_{socket,sendmsg,getsockopt...}() interface. This causes the >> kernel to load SCTP module even when it's not really used. >> >> This patch then creates a new sockopt that is to be used only by kernel >> users of this protocol. This new sockopt will not allocate a file >> descriptor but instead just return the socket pointer directly. >> >> Kernel users are actually identified by if the parent socket has or not >> a fd attached to it. If not, it's a kernel a user. >> >> If called by an user application, it will just return -EPERM. >> >> Even though it's not intended for user applications, it's listed under >> uapi header. That's because hidding this wouldn't add any extra security >> and to keep the sockopt list in one place, so it's easy to check >> available numbers to use. >> >> Signed-off-by: Marcelo Ricardo Leitner <marcelo.leitner@gmail.com> >> --- >> include/uapi/linux/sctp.h | 12 ++++++++++++ >> net/sctp/socket.c | 37 +++++++++++++++++++++++++++++++++++++ >> 2 files changed, 49 insertions(+) >> >> diff --git a/include/uapi/linux/sctp.h b/include/uapi/linux/sctp.h >> index ce70fe6b45df3e841c35accbdb6379c16563893c..b3aad3ce456ab3c1ebf4d81fdb7269ba40b3d92a 100644 >> --- a/include/uapi/linux/sctp.h >> +++ b/include/uapi/linux/sctp.h >> @@ -105,6 +105,10 @@ typedef __s32 sctp_assoc_t; >> #define SCTP_SOCKOPT_BINDX_ADD 100 /* BINDX requests for adding addrs */ >> #define SCTP_SOCKOPT_BINDX_REM 101 /* BINDX requests for removing addrs. */ >> #define SCTP_SOCKOPT_PEELOFF 102 /* peel off association. */ >> +#define SCTP_SOCKOPT_PEELOFF_KERNEL 103 /* peel off association. >> + * only valid for kernel >> + * users >> + */ >> /* Options 104-106 are deprecated and removed. Do not use this space */ >> #define SCTP_SOCKOPT_CONNECTX_OLD 107 /* CONNECTX old requests. */ >> #define SCTP_GET_PEER_ADDRS 108 /* Get all peer address. */ >> @@ -892,6 +896,14 @@ typedef struct { >> int sd; >> } sctp_peeloff_arg_t; >> >> +/* This is the union that is passed as an argument(optval) to >> + * getsockopt(SCTP_SOCKOPT_PEELOFF_KERNEL). >> + */ >> +typedef union { >> + sctp_assoc_t associd; >> + struct socket *socket; >> +} sctp_peeloff_kernel_arg_t; >> + >> /* >> * Peer Address Thresholds socket option >> */ >> diff --git a/net/sctp/socket.c b/net/sctp/socket.c >> index f1a65398f3118ab5d3a884e9c875620560e6b5ef..7968de7a1aeabd5cd0a0398461dbf2081bd4c5b7 100644 >> --- a/net/sctp/socket.c >> +++ b/net/sctp/socket.c >> @@ -4504,6 +4504,39 @@ out: >> return retval; >> } >> >> +static int sctp_getsockopt_peeloff_kernel(struct sock *sk, int len, >> + char __user *optval, int __user *optlen) >> +{ >> + sctp_peeloff_kernel_arg_t peeloff; >> + struct socket *newsock; >> + int retval = 0; >> + >> + /* We only allow this operation if parent socket also hadn't a >> + * file descriptor allocated to it, mainly as a way to make sure >> + * that this is really a kernel socket. >> + */ >> + if (sk->sk_socket->file) >> + return -EPERM; (A) --^ >> + >> + if (len < sizeof(sctp_peeloff_kernel_arg_t)) >> + return -EINVAL; >> + len = sizeof(sctp_peeloff_kernel_arg_t); >> + if (copy_from_user(&peeloff, optval, len)) >> + return -EFAULT; >> + >> + retval = sctp_do_peeloff(sk, peeloff.associd, &newsock); >> + if (retval < 0) >> + goto out; >> + >> + peeloff.socket = newsock; >> + if (copy_to_user(optval, &peeloff, len)) { >> + sock_release(newsock); >> + return -EFAULT; >> + } >> +out: >> + return retval; >> +} >> + >> /* 7.1.13 Peer Address Parameters (SCTP_PEER_ADDR_PARAMS) >> * >> * Applications can enable or disable heartbeats for any peer address of >> @@ -5991,6 +6024,10 @@ 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_KERNEL: >> + retval = sctp_getsockopt_peeloff_kernel(sk, len, optval, >> + optlen); >> + break; > Do we need to do something here to prevent user space calls from inadvertently > accessing this option? > > Neil No because such protection is inside sctp_getsockopt_peeloff_kernel() now, on point (A) above. It seemed better to keep this switch case aligned and move this protection inside it. Marcelo >> case SCTP_PEER_ADDR_PARAMS: >> retval = sctp_getsockopt_peer_addr_params(sk, len, optval, >> optlen); >> -- >> 2.4.1 >> >> -- >> To unsubscribe from this list: send the line "unsubscribe linux-sctp" in >> the body of a message to majordomo@vger.kernel.org >> More majordomo info at http://vger.kernel.org/majordomo-info.html >> -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
From: Neil Horman <nhorman@tuxdriver.com> Date: Wed, 15 Jul 2015 09:18:28 -0400 > Do we need to do something here to prevent user space calls from inadvertently > accessing this option? And this is also not what I had anticipated was the implementation. I didn't mean that adding a new option number specially for the kernel was the right thing to do. I meant that having a specific function GPL exported to modules that the external code calls would do the trick. That's why we have things like kernel_sendmsg() et al. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 07/14/2015 01:13 PM, Marcelo Ricardo Leitner wrote: > SCTP has this operation to peel off associations from a given socket and > create a new socket using this association. We currently have two ways > to use this operation: > - via getsockopt(), on which it will also create and return a file > descriptor for this new socket > - via sctp_do_peeloff(), which is for kernel only > > The caveat with using sctp_do_peeloff() directly is that it creates a > dependency to SCTP module, while all other operations are handled via > kernel_{socket,sendmsg,getsockopt...}() interface. This causes the > kernel to load SCTP module even when it's not really used. > > This patch then creates a new sockopt that is to be used only by kernel > users of this protocol. This new sockopt will not allocate a file > descriptor but instead just return the socket pointer directly. > > Kernel users are actually identified by if the parent socket has or not > a fd attached to it. If not, it's a kernel a user. > > If called by an user application, it will just return -EPERM. > > Even though it's not intended for user applications, it's listed under > uapi header. That's because hidding this wouldn't add any extra security > and to keep the sockopt list in one place, so it's easy to check > available numbers to use. > > Signed-off-by: Marcelo Ricardo Leitner <marcelo.leitner@gmail.com> > --- > include/uapi/linux/sctp.h | 12 ++++++++++++ > net/sctp/socket.c | 37 +++++++++++++++++++++++++++++++++++++ > 2 files changed, 49 insertions(+) > > diff --git a/include/uapi/linux/sctp.h b/include/uapi/linux/sctp.h > index ce70fe6b45df3e841c35accbdb6379c16563893c..b3aad3ce456ab3c1ebf4d81fdb7269ba40b3d92a 100644 > --- a/include/uapi/linux/sctp.h > +++ b/include/uapi/linux/sctp.h > @@ -105,6 +105,10 @@ typedef __s32 sctp_assoc_t; > #define SCTP_SOCKOPT_BINDX_ADD 100 /* BINDX requests for adding addrs */ > #define SCTP_SOCKOPT_BINDX_REM 101 /* BINDX requests for removing addrs. */ > #define SCTP_SOCKOPT_PEELOFF 102 /* peel off association. */ > +#define SCTP_SOCKOPT_PEELOFF_KERNEL 103 /* peel off association. > + * only valid for kernel > + * users > + */ I am not sure how much I like stuff this like in the uapi. This stuff is exposed to the user and I'd much rather we try and hide this from the user completely. I understand that you are dealing with a rather ugly dependency, but this is not the only one in the kernel. There are dependencies like this elsewhere as well. I am not familiar enough with DLM and its history, but my question is this: If dlm always peels off a socket for a new associations, why is it using 1-to-many api in the first place? Doing a quick scan of DLM lowcoms code for sctp specific things, I see nothing that has specific dependencies on 1-to-many api. It might be simpler to switch to using 1-to-1 api, similar to dlm tcp and eliminate this dependency. Is that a naive point of view? Thanks -vlad > /* Options 104-106 are deprecated and removed. Do not use this space */ > #define SCTP_SOCKOPT_CONNECTX_OLD 107 /* CONNECTX old requests. */ > #define SCTP_GET_PEER_ADDRS 108 /* Get all peer address. */ > @@ -892,6 +896,14 @@ typedef struct { > int sd; > } sctp_peeloff_arg_t; > > +/* This is the union that is passed as an argument(optval) to > + * getsockopt(SCTP_SOCKOPT_PEELOFF_KERNEL). > + */ > +typedef union { > + sctp_assoc_t associd; > + struct socket *socket; > +} sctp_peeloff_kernel_arg_t; > + > /* > * Peer Address Thresholds socket option > */ > diff --git a/net/sctp/socket.c b/net/sctp/socket.c > index f1a65398f3118ab5d3a884e9c875620560e6b5ef..7968de7a1aeabd5cd0a0398461dbf2081bd4c5b7 100644 > --- a/net/sctp/socket.c > +++ b/net/sctp/socket.c > @@ -4504,6 +4504,39 @@ out: > return retval; > } > > +static int sctp_getsockopt_peeloff_kernel(struct sock *sk, int len, > + char __user *optval, int __user *optlen) > +{ > + sctp_peeloff_kernel_arg_t peeloff; > + struct socket *newsock; > + int retval = 0; > + > + /* We only allow this operation if parent socket also hadn't a > + * file descriptor allocated to it, mainly as a way to make sure > + * that this is really a kernel socket. > + */ > + if (sk->sk_socket->file) > + return -EPERM; > + > + if (len < sizeof(sctp_peeloff_kernel_arg_t)) > + return -EINVAL; > + len = sizeof(sctp_peeloff_kernel_arg_t); > + if (copy_from_user(&peeloff, optval, len)) > + return -EFAULT; > + > + retval = sctp_do_peeloff(sk, peeloff.associd, &newsock); > + if (retval < 0) > + goto out; > + > + peeloff.socket = newsock; > + if (copy_to_user(optval, &peeloff, len)) { > + sock_release(newsock); > + return -EFAULT; > + } > +out: > + return retval; > +} > + > /* 7.1.13 Peer Address Parameters (SCTP_PEER_ADDR_PARAMS) > * > * Applications can enable or disable heartbeats for any peer address of > @@ -5991,6 +6024,10 @@ 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_KERNEL: > + retval = sctp_getsockopt_peeloff_kernel(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 netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Jul 16, 2015 at 09:50:16AM -0400, Vlad Yasevich wrote: > On 07/14/2015 01:13 PM, Marcelo Ricardo Leitner wrote: > > SCTP has this operation to peel off associations from a given socket and > > create a new socket using this association. We currently have two ways > > to use this operation: > > - via getsockopt(), on which it will also create and return a file > > descriptor for this new socket > > - via sctp_do_peeloff(), which is for kernel only > > > > The caveat with using sctp_do_peeloff() directly is that it creates a > > dependency to SCTP module, while all other operations are handled via > > kernel_{socket,sendmsg,getsockopt...}() interface. This causes the > > kernel to load SCTP module even when it's not really used. > > > > This patch then creates a new sockopt that is to be used only by kernel > > users of this protocol. This new sockopt will not allocate a file > > descriptor but instead just return the socket pointer directly. > > > > Kernel users are actually identified by if the parent socket has or not > > a fd attached to it. If not, it's a kernel a user. > > > > If called by an user application, it will just return -EPERM. > > > > Even though it's not intended for user applications, it's listed under > > uapi header. That's because hidding this wouldn't add any extra security > > and to keep the sockopt list in one place, so it's easy to check > > available numbers to use. > > > > Signed-off-by: Marcelo Ricardo Leitner <marcelo.leitner@gmail.com> > > --- > > include/uapi/linux/sctp.h | 12 ++++++++++++ > > net/sctp/socket.c | 37 +++++++++++++++++++++++++++++++++++++ > > 2 files changed, 49 insertions(+) > > > > diff --git a/include/uapi/linux/sctp.h b/include/uapi/linux/sctp.h > > index ce70fe6b45df3e841c35accbdb6379c16563893c..b3aad3ce456ab3c1ebf4d81fdb7269ba40b3d92a 100644 > > --- a/include/uapi/linux/sctp.h > > +++ b/include/uapi/linux/sctp.h > > @@ -105,6 +105,10 @@ typedef __s32 sctp_assoc_t; > > #define SCTP_SOCKOPT_BINDX_ADD 100 /* BINDX requests for adding addrs */ > > #define SCTP_SOCKOPT_BINDX_REM 101 /* BINDX requests for removing addrs. */ > > #define SCTP_SOCKOPT_PEELOFF 102 /* peel off association. */ > > +#define SCTP_SOCKOPT_PEELOFF_KERNEL 103 /* peel off association. > > + * only valid for kernel > > + * users > > + */ > > I am not sure how much I like stuff this like in the uapi. This stuff is > exposed to the user and I'd much rather we try and hide this from > the user completely. We can hide it, but as is it would create hidden IDs and adding new gets complicated, one would have to check two different lists to find free IDs. Neil's suggestion is much cleaner on this aspect, but has the caveat on changing sockopt arg format. > I understand that you are dealing with a rather ugly dependency, but this is > not the only one in the kernel. There are dependencies like this elsewhere > as well. Doesn't mean we cannot fix one or another every now and then, right? > I am not familiar enough with DLM and its history, but my question is this: > If dlm always peels off a socket for a new associations, why is it using > 1-to-many api in the first place? Doing a quick scan of DLM lowcoms code > for sctp specific things, I see nothing that has specific dependencies > on 1-to-many api. It might be simpler to switch to using 1-to-1 api, similar > to dlm tcp and eliminate this dependency. > > Is that a naive point of view? Not at all, that's a very good question. I also don't know much of DLM code itself, I'll check that. Thanks, Marcelo > Thanks > -vlad > > > /* Options 104-106 are deprecated and removed. Do not use this space */ > > #define SCTP_SOCKOPT_CONNECTX_OLD 107 /* CONNECTX old requests. */ > > #define SCTP_GET_PEER_ADDRS 108 /* Get all peer address. */ > > @@ -892,6 +896,14 @@ typedef struct { > > int sd; > > } sctp_peeloff_arg_t; > > > > +/* This is the union that is passed as an argument(optval) to > > + * getsockopt(SCTP_SOCKOPT_PEELOFF_KERNEL). > > + */ > > +typedef union { > > + sctp_assoc_t associd; > > + struct socket *socket; > > +} sctp_peeloff_kernel_arg_t; > > + > > /* > > * Peer Address Thresholds socket option > > */ > > diff --git a/net/sctp/socket.c b/net/sctp/socket.c > > index f1a65398f3118ab5d3a884e9c875620560e6b5ef..7968de7a1aeabd5cd0a0398461dbf2081bd4c5b7 100644 > > --- a/net/sctp/socket.c > > +++ b/net/sctp/socket.c > > @@ -4504,6 +4504,39 @@ out: > > return retval; > > } > > > > +static int sctp_getsockopt_peeloff_kernel(struct sock *sk, int len, > > + char __user *optval, int __user *optlen) > > +{ > > + sctp_peeloff_kernel_arg_t peeloff; > > + struct socket *newsock; > > + int retval = 0; > > + > > + /* We only allow this operation if parent socket also hadn't a > > + * file descriptor allocated to it, mainly as a way to make sure > > + * that this is really a kernel socket. > > + */ > > + if (sk->sk_socket->file) > > + return -EPERM; > > + > > + if (len < sizeof(sctp_peeloff_kernel_arg_t)) > > + return -EINVAL; > > + len = sizeof(sctp_peeloff_kernel_arg_t); > > + if (copy_from_user(&peeloff, optval, len)) > > + return -EFAULT; > > + > > + retval = sctp_do_peeloff(sk, peeloff.associd, &newsock); > > + if (retval < 0) > > + goto out; > > + > > + peeloff.socket = newsock; > > + if (copy_to_user(optval, &peeloff, len)) { > > + sock_release(newsock); > > + return -EFAULT; > > + } > > +out: > > + return retval; > > +} > > + > > /* 7.1.13 Peer Address Parameters (SCTP_PEER_ADDR_PARAMS) > > * > > * Applications can enable or disable heartbeats for any peer address of > > @@ -5991,6 +6024,10 @@ 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_KERNEL: > > + retval = sctp_getsockopt_peeloff_kernel(sk, len, optval, > > + optlen); > > + break; > > case SCTP_PEER_ADDR_PARAMS: > > retval = sctp_getsockopt_peer_addr_params(sk, len, optval, > > optlen); > > > > -- > To unsubscribe from this list: send the line "unsubscribe linux-sctp" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Jul 16, 2015 at 11:03:14AM -0300, Marcelo Ricardo Leitner wrote: > On Thu, Jul 16, 2015 at 09:50:16AM -0400, Vlad Yasevich wrote: ... > > I am not familiar enough with DLM and its history, but my question is this: > > If dlm always peels off a socket for a new associations, why is it using > > 1-to-many api in the first place? Doing a quick scan of DLM lowcoms code > > for sctp specific things, I see nothing that has specific dependencies > > on 1-to-many api. It might be simpler to switch to using 1-to-1 api, similar > > to dlm tcp and eliminate this dependency. > > > > Is that a naive point of view? > > Not at all, that's a very good question. I also don't know much of DLM > code itself, I'll check that. Sounds like DLM is using 1-to-many just in an attempt to use multi-homing, but we can do that with 1-to-1 too. I'll draft a patch and see how it goes, and perhaps with that we can avoid/postpone this question regarding indirect call to sctp_do_peeloff by kernel. Marcelo -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
From: Marcelo Ricardo Leitner > Sent: 14 July 2015 18:13 > SCTP has this operation to peel off associations from a given socket and > create a new socket using this association. We currently have two ways > to use this operation: > - via getsockopt(), on which it will also create and return a file > descriptor for this new socket > - via sctp_do_peeloff(), which is for kernel only > > The caveat with using sctp_do_peeloff() directly is that it creates a > dependency to SCTP module, while all other operations are handled via > kernel_{socket,sendmsg,getsockopt...}() interface. This causes the > kernel to load SCTP module even when it's not really used. > > This patch then creates a new sockopt that is to be used only by kernel > users of this protocol. This new sockopt will not allocate a file > descriptor but instead just return the socket pointer directly. > > Kernel users are actually identified by if the parent socket has or not > a fd attached to it. If not, it's a kernel a user. > > If called by an user application, it will just return -EPERM. > > Even though it's not intended for user applications, it's listed under > uapi header. That's because hidding this wouldn't add any extra security > and to keep the sockopt list in one place, so it's easy to check > available numbers to use. > > Signed-off-by: Marcelo Ricardo Leitner <marcelo.leitner@gmail.com> ... > +static int sctp_getsockopt_peeloff_kernel(struct sock *sk, int len, > + char __user *optval, int __user *optlen) > +{ > + sctp_peeloff_kernel_arg_t peeloff; > + struct socket *newsock; > + int retval = 0; > + > + /* We only allow this operation if parent socket also hadn't a > + * file descriptor allocated to it, mainly as a way to make sure > + * that this is really a kernel socket. > + */ > + if (sk->sk_socket->file) > + return -EPERM; > + > + if (len < sizeof(sctp_peeloff_kernel_arg_t)) > + return -EINVAL; > + len = sizeof(sctp_peeloff_kernel_arg_t); > + if (copy_from_user(&peeloff, optval, len)) > + return -EFAULT; You can't need copy_from_user() here, the buffer would surely be kernel. David -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Em 22 de julho de 2015 10:13:22 BRT, David Laight <David.Laight@ACULAB.COM> escreveu: >From: Marcelo Ricardo Leitner >> Sent: 14 July 2015 18:13 >> SCTP has this operation to peel off associations from a given socket >and >> create a new socket using this association. We currently have two >ways >> to use this operation: >> - via getsockopt(), on which it will also create and return a file >> descriptor for this new socket >> - via sctp_do_peeloff(), which is for kernel only >> >> The caveat with using sctp_do_peeloff() directly is that it creates a >> dependency to SCTP module, while all other operations are handled via >> kernel_{socket,sendmsg,getsockopt...}() interface. This causes the >> kernel to load SCTP module even when it's not really used. >> >> This patch then creates a new sockopt that is to be used only by >kernel >> users of this protocol. This new sockopt will not allocate a file >> descriptor but instead just return the socket pointer directly. >> >> Kernel users are actually identified by if the parent socket has or >not >> a fd attached to it. If not, it's a kernel a user. >> >> If called by an user application, it will just return -EPERM. >> >> Even though it's not intended for user applications, it's listed >under >> uapi header. That's because hidding this wouldn't add any extra >security >> and to keep the sockopt list in one place, so it's easy to check >> available numbers to use. >> >> Signed-off-by: Marcelo Ricardo Leitner <marcelo.leitner@gmail.com> >... >> +static int sctp_getsockopt_peeloff_kernel(struct sock *sk, int len, >> + char __user *optval, int __user *optlen) >> +{ >> + sctp_peeloff_kernel_arg_t peeloff; >> + struct socket *newsock; >> + int retval = 0; >> + >> + /* We only allow this operation if parent socket also hadn't a >> + * file descriptor allocated to it, mainly as a way to make sure >> + * that this is really a kernel socket. >> + */ >> + if (sk->sk_socket->file) >> + return -EPERM; >> + >> + if (len < sizeof(sctp_peeloff_kernel_arg_t)) >> + return -EINVAL; >> + len = sizeof(sctp_peeloff_kernel_arg_t); >> + if (copy_from_user(&peeloff, optval, len)) >> + return -EFAULT; > >You can't need copy_from_user() here, the buffer would surely be >kernel. > > David Yes. It was just to avoid errors from static checkers, if any. Same for the __user in function prototype.
diff --git a/include/uapi/linux/sctp.h b/include/uapi/linux/sctp.h index ce70fe6b45df3e841c35accbdb6379c16563893c..b3aad3ce456ab3c1ebf4d81fdb7269ba40b3d92a 100644 --- a/include/uapi/linux/sctp.h +++ b/include/uapi/linux/sctp.h @@ -105,6 +105,10 @@ typedef __s32 sctp_assoc_t; #define SCTP_SOCKOPT_BINDX_ADD 100 /* BINDX requests for adding addrs */ #define SCTP_SOCKOPT_BINDX_REM 101 /* BINDX requests for removing addrs. */ #define SCTP_SOCKOPT_PEELOFF 102 /* peel off association. */ +#define SCTP_SOCKOPT_PEELOFF_KERNEL 103 /* peel off association. + * only valid for kernel + * users + */ /* Options 104-106 are deprecated and removed. Do not use this space */ #define SCTP_SOCKOPT_CONNECTX_OLD 107 /* CONNECTX old requests. */ #define SCTP_GET_PEER_ADDRS 108 /* Get all peer address. */ @@ -892,6 +896,14 @@ typedef struct { int sd; } sctp_peeloff_arg_t; +/* This is the union that is passed as an argument(optval) to + * getsockopt(SCTP_SOCKOPT_PEELOFF_KERNEL). + */ +typedef union { + sctp_assoc_t associd; + struct socket *socket; +} sctp_peeloff_kernel_arg_t; + /* * Peer Address Thresholds socket option */ diff --git a/net/sctp/socket.c b/net/sctp/socket.c index f1a65398f3118ab5d3a884e9c875620560e6b5ef..7968de7a1aeabd5cd0a0398461dbf2081bd4c5b7 100644 --- a/net/sctp/socket.c +++ b/net/sctp/socket.c @@ -4504,6 +4504,39 @@ out: return retval; } +static int sctp_getsockopt_peeloff_kernel(struct sock *sk, int len, + char __user *optval, int __user *optlen) +{ + sctp_peeloff_kernel_arg_t peeloff; + struct socket *newsock; + int retval = 0; + + /* We only allow this operation if parent socket also hadn't a + * file descriptor allocated to it, mainly as a way to make sure + * that this is really a kernel socket. + */ + if (sk->sk_socket->file) + return -EPERM; + + if (len < sizeof(sctp_peeloff_kernel_arg_t)) + return -EINVAL; + len = sizeof(sctp_peeloff_kernel_arg_t); + if (copy_from_user(&peeloff, optval, len)) + return -EFAULT; + + retval = sctp_do_peeloff(sk, peeloff.associd, &newsock); + if (retval < 0) + goto out; + + peeloff.socket = newsock; + if (copy_to_user(optval, &peeloff, len)) { + sock_release(newsock); + return -EFAULT; + } +out: + return retval; +} + /* 7.1.13 Peer Address Parameters (SCTP_PEER_ADDR_PARAMS) * * Applications can enable or disable heartbeats for any peer address of @@ -5991,6 +6024,10 @@ 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_KERNEL: + retval = sctp_getsockopt_peeloff_kernel(sk, len, optval, + optlen); + break; case SCTP_PEER_ADDR_PARAMS: retval = sctp_getsockopt_peer_addr_params(sk, len, optval, optlen);
SCTP has this operation to peel off associations from a given socket and create a new socket using this association. We currently have two ways to use this operation: - via getsockopt(), on which it will also create and return a file descriptor for this new socket - via sctp_do_peeloff(), which is for kernel only The caveat with using sctp_do_peeloff() directly is that it creates a dependency to SCTP module, while all other operations are handled via kernel_{socket,sendmsg,getsockopt...}() interface. This causes the kernel to load SCTP module even when it's not really used. This patch then creates a new sockopt that is to be used only by kernel users of this protocol. This new sockopt will not allocate a file descriptor but instead just return the socket pointer directly. Kernel users are actually identified by if the parent socket has or not a fd attached to it. If not, it's a kernel a user. If called by an user application, it will just return -EPERM. Even though it's not intended for user applications, it's listed under uapi header. That's because hidding this wouldn't add any extra security and to keep the sockopt list in one place, so it's easy to check available numbers to use. Signed-off-by: Marcelo Ricardo Leitner <marcelo.leitner@gmail.com> --- include/uapi/linux/sctp.h | 12 ++++++++++++ net/sctp/socket.c | 37 +++++++++++++++++++++++++++++++++++++ 2 files changed, 49 insertions(+)