| Message ID | 4dfd0ee7ac0aac0791812217e990e2ae7ff86955.1434645734.git.marcelo.leitner@gmail.com |
|---|---|
| State | Rejected, archived |
| Delegated to: | David Miller |
| Headers | show |
On Thu, Jul 09, 2015 at 11:15:19AM -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 directly used > > This patch then updates SCTP_SOCKOPT_PEELOFF so that for kernel users of > this protocol it will not allocate a file descriptor but instead just > return the socket pointer directly. > > If called by an user application it will work as before. > > Signed-off-by: Marcelo Ricardo Leitner <marcelo.leitner@gmail.com> Acked-by: Neil Horman <nhorman@tuxdriver.com> -- 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 <marcelo.leitner@gmail.com> Date: Thu, 9 Jul 2015 11:15:19 -0300 > 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 directly used > > This patch then updates SCTP_SOCKOPT_PEELOFF so that for kernel users of > this protocol it will not allocate a file descriptor but instead just > return the socket pointer directly. > > If called by an user application it will work as before. > > Signed-off-by: Marcelo Ricardo Leitner <marcelo.leitner@gmail.com> I do not like this at all. Socket option implementations should not change their behavior or what datastructures they consume or return just because the socket happens to be a kernel socket. I'm not applying this series, sorry. Also, your patch series lacked an intial "PATCH 0/N" posting, so you could at least spend the time to discuss this patch series at a high level and explain your overall motivations. -- 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 Fri, Jul 10, 2015 at 06:21:14PM -0700, David Miller wrote: > From: Marcelo Ricardo Leitner <marcelo.leitner@gmail.com> > Date: Thu, 9 Jul 2015 11:15:19 -0300 > > > 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 directly used > > > > This patch then updates SCTP_SOCKOPT_PEELOFF so that for kernel users of > > this protocol it will not allocate a file descriptor but instead just > > return the socket pointer directly. > > > > If called by an user application it will work as before. > > > > Signed-off-by: Marcelo Ricardo Leitner <marcelo.leitner@gmail.com> > > I do not like this at all. > > Socket option implementations should not change their behavior or what > datastructures they consume or return just because the socket happens > to be a kernel socket. > But in this case its necessecary, as the kernel here can't allocate an fd, due to serious leakage (see commit 2f2d76cc3e938389feee671b46252dde6880b3b7). Initially Marcelo had created duplicate code paths, one to return an fd, one to return a file struct. If you would rather go in that direction, I'm sure he can propose it again, but that seems less correct to me than this solution. > I'm not applying this series, sorry. > > Also, your patch series lacked an intial "PATCH 0/N" posting, so you > could at least spend the time to discuss this patch series at a high > level and explain your overall motivations. > That was in the initial posting. It should have been reposted, but if you're interested: http://marc.info/?l=linux-sctp&m=143449456219518&w=2 Regards Neil -- 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 13-07-2015 07:39, Neil Horman wrote: > On Fri, Jul 10, 2015 at 06:21:14PM -0700, David Miller wrote: >> From: Marcelo Ricardo Leitner <marcelo.leitner@gmail.com> >> Date: Thu, 9 Jul 2015 11:15:19 -0300 >> >>> 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 directly used >>> >>> This patch then updates SCTP_SOCKOPT_PEELOFF so that for kernel users of >>> this protocol it will not allocate a file descriptor but instead just >>> return the socket pointer directly. >>> >>> If called by an user application it will work as before. >>> >>> Signed-off-by: Marcelo Ricardo Leitner <marcelo.leitner@gmail.com> >> >> I do not like this at all. >> >> Socket option implementations should not change their behavior or what >> datastructures they consume or return just because the socket happens >> to be a kernel socket. >> > But in this case its necessecary, as the kernel here can't allocate an fd, due > to serious leakage (see commit 2f2d76cc3e938389feee671b46252dde6880b3b7). > Initially Marcelo had created duplicate code paths, one to return an fd, one to > return a file struct. If you would rather go in that direction, I'm sure he can > propose it again, but that seems less correct to me than this solution. Yes. dlm is the only user of this option within kernel today and it causes serious problems, as Neil just referenced. Another good result of this implementation is that we are preventing such leakage from happening again in the future. >> I'm not applying this series, sorry. >> >> Also, your patch series lacked an intial "PATCH 0/N" posting, so you >> could at least spend the time to discuss this patch series at a high >> level and explain your overall motivations. >> > That was in the initial posting. It should have been reposted, but if you're > interested: > http://marc.info/?l=linux-sctp&m=143449456219518&w=2 My bad. Won't happen again. Thanks, 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: Neil Horman <nhorman@tuxdriver.com> Date: Mon, 13 Jul 2015 06:39:11 -0400 > Initially Marcelo had created duplicate code paths, one to return an > fd, one to return a file struct. If you would rather go in that > direction, I'm sure he can propose it again, but that seems less > correct to me than this solution. That's much better. -- 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 13-07-2015 15:59, David Miller wrote: > From: Neil Horman <nhorman@tuxdriver.com> > Date: Mon, 13 Jul 2015 06:39:11 -0400 > >> Initially Marcelo had created duplicate code paths, one to return an >> fd, one to return a file struct. If you would rather go in that >> direction, I'm sure he can propose it again, but that seems less >> correct to me than this solution. > > That's much better. I'm not sure what you mean. Is the new option better or the history/description? 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 <marcelo.leitner@gmail.com> Date: Mon, 13 Jul 2015 16:05:27 -0300 > On 13-07-2015 15:59, David Miller wrote: >> From: Neil Horman <nhorman@tuxdriver.com> >> Date: Mon, 13 Jul 2015 06:39:11 -0400 >> >>> Initially Marcelo had created duplicate code paths, one to return an >>> fd, one to return a file struct. If you would rather go in that >>> direction, I'm sure he can propose it again, but that seems less >>> correct to me than this solution. >> >> That's much better. > > I'm not sure what you mean. Is the new option better or the > history/description? I mean that adding an explicit function for these internal kernel users to call is better. -- 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 13-07-2015 16:58, David Miller wrote: > From: Marcelo Ricardo Leitner <marcelo.leitner@gmail.com> > Date: Mon, 13 Jul 2015 16:05:27 -0300 > >> On 13-07-2015 15:59, David Miller wrote: >>> From: Neil Horman <nhorman@tuxdriver.com> >>> Date: Mon, 13 Jul 2015 06:39:11 -0400 >>> >>>> Initially Marcelo had created duplicate code paths, one to return an >>>> fd, one to return a file struct. If you would rather go in that >>>> direction, I'm sure he can propose it again, but that seems less >>>> correct to me than this solution. >>> >>> That's much better. >> >> I'm not sure what you mean. Is the new option better or the >> history/description? > > I mean that adding an explicit function for these internal kernel > users to call is better. Okay. I'll try to minimize that code duplication then. Thanks 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
diff --git a/include/uapi/linux/sctp.h b/include/uapi/linux/sctp.h index ce70fe6b45df3e841c35accbdb6379c16563893c..9e15fc06ba553c7e33f729872bb2dfaa2e21b0d8 100644 --- a/include/uapi/linux/sctp.h +++ b/include/uapi/linux/sctp.h @@ -887,9 +887,12 @@ struct sctp_assoc_stats { /* This is the structure that is passed as an argument(optval) to * getsockopt(SCTP_SOCKOPT_PEELOFF). */ -typedef struct { - sctp_assoc_t associd; - int sd; +typedef union { + struct { + sctp_assoc_t associd; + int sd; + }; + void *sock; } sctp_peeloff_arg_t; /* diff --git a/net/sctp/socket.c b/net/sctp/socket.c index f09de7fac2e6acddad8b2e046dbf626e329cb674..ff1138558687e15ee486e84c0916ad81f01ca734 100644 --- a/net/sctp/socket.c +++ b/net/sctp/socket.c @@ -4465,6 +4465,19 @@ static int sctp_getsockopt_peeloff(struct sock *sk, int len, char __user *optval if (retval < 0) goto out; + /* If the owner of parent sock is the kernel, that is, if a file + * descriptor wasn't allocated to it, return the socket pointer + * directly instead of allocating a file descriptor. + */ + if (!sk->sk_socket->file) { + peeloff.sock = newsock; + if (copy_to_user(optval, &peeloff, len)) { + sock_release(newsock); + return -EFAULT; + } + return retval; + } + /* Map the socket to an unused fd that can be returned to the user. */ retval = get_unused_fd_flags(0); if (retval < 0) {
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 directly used This patch then updates SCTP_SOCKOPT_PEELOFF so that for kernel users of this protocol it will not allocate a file descriptor but instead just return the socket pointer directly. If called by an user application it will work as before. Signed-off-by: Marcelo Ricardo Leitner <marcelo.leitner@gmail.com> --- include/uapi/linux/sctp.h | 9 ++++++--- net/sctp/socket.c | 13 +++++++++++++ 2 files changed, 19 insertions(+), 3 deletions(-)