[PATCH 1/2] sctp: SCTP_SOCKOPT_PEELOFF return socket pointer for kernel users

[PATCH 1/2] sctp: SCTP_SOCKOPT_PEELOFF return socket pointer for kernel users

[Marcelo Ricardo Leitner]

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(-)

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) {
-- 
2.4.1

[PATCH 2/2] dlm: avoid using sctp_do_peeloff directly

[Marcelo Ricardo Leitner]

This patch reverts 2f2d76cc3e93 ("dlm: Do not allocate a fd for
peeloff") but also takes benefit on an updated sockopt
SCTP_SOCKOPT_PEELOFF, which now avoids allocating file descriptors for
kernel users.

By this we avoid creating a direct dependency from dlm to sctp module,
which can then be left unloaded if dlm is not really using it.

Note that this was preferred other than a module split as it once was
split and was merged back in 2007 by commit 6ed7257b4670 ("[DLM]
Consolidate transport protocols") so that we don't revert it.

Signed-off-by: Marcelo Ricardo Leitner <marcelo.leitner@gmail.com>
---
 fs/dlm/lowcomms.c | 17 ++++++++---------
 1 file changed, 8 insertions(+), 9 deletions(-)

diff --git a/fs/dlm/lowcomms.c b/fs/dlm/lowcomms.c
index 754fd6c0b7470bab272b071e6ca6e4969e4e4209..fb094b5ab0347a7b8659c2dc6cc3b32638d63638 100644
--- a/fs/dlm/lowcomms.c
+++ b/fs/dlm/lowcomms.c
@@ -52,7 +52,6 @@
 #include <linux/mutex.h>
 #include <linux/sctp.h>
 #include <linux/slab.h>
-#include <net/sctp/sctp.h>
 #include <net/ipv6.h>
 
 #include "dlm_internal.h"
@@ -671,6 +670,8 @@ static void process_sctp_notification(struct connection *con,
 			int prim_len, ret;
 			int addr_len;
 			struct connection *new_con;
+			sctp_peeloff_arg_t parg;
+			int parglen = sizeof(parg);
 
 			/*
 			 * We get this before any data for an association.
@@ -719,19 +720,17 @@ static void process_sctp_notification(struct connection *con,
 				return;
 
 			/* Peel off a new sock */
-			lock_sock(con->sock->sk);
-			ret = sctp_do_peeloff(con->sock->sk,
-				sn->sn_assoc_change.sac_assoc_id,
-				&new_con->sock);
-			release_sock(con->sock->sk);
+			parg.associd = sn->sn_assoc_change.sac_assoc_id;
+			ret = kernel_getsockopt(con->sock, IPPROTO_SCTP,
+						SCTP_SOCKOPT_PEELOFF,
+						(void *)&parg, &parglen);
 			if (ret < 0) {
 				log_print("Can't peel off a socket for "
 					  "connection %d to node %d: err=%d",
-					  (int)sn->sn_assoc_change.sac_assoc_id,
-					  nodeid, ret);
+					  parg.associd, nodeid, ret);
 				return;
 			}
-			add_sock(new_con->sock, new_con);
+			add_sock(parg.sock, new_con);
 
 			linger.l_onoff = 1;
 			linger.l_linger = 0;
-- 
2.4.1

Re: [PATCH 1/2] sctp: SCTP_SOCKOPT_PEELOFF return socket pointer for kernel users

[Neil Horman]

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>

Re: [PATCH 2/2] dlm: avoid using sctp_do_peeloff directly

[Neil Horman]

On Thu, Jul 09, 2015 at 11:15:20AM -0300, Marcelo Ricardo Leitner wrote:
> This patch reverts 2f2d76cc3e93 ("dlm: Do not allocate a fd for
> peeloff") but also takes benefit on an updated sockopt
> SCTP_SOCKOPT_PEELOFF, which now avoids allocating file descriptors for
> kernel users.
> 
> By this we avoid creating a direct dependency from dlm to sctp module,
> which can then be left unloaded if dlm is not really using it.
> 
> Note that this was preferred other than a module split as it once was
> split and was merged back in 2007 by commit 6ed7257b4670 ("[DLM]
> Consolidate transport protocols") so that we don't revert it.
> 
> Signed-off-by: Marcelo Ricardo Leitner <marcelo.leitner@gmail.com>

Acked-by: Neil Horman <nhorman@tuxdriver.com>

Re: [PATCH 1/2] sctp: SCTP_SOCKOPT_PEELOFF return socket pointer for kernel users

[David Miller]

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.

Re: [PATCH 1/2] sctp: SCTP_SOCKOPT_PEELOFF return socket pointer for kernel users

[Neil Horman]

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

Re: [PATCH 1/2] sctp: SCTP_SOCKOPT_PEELOFF return socket pointer for kernel users

[Marcelo Ricardo Leitner]

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

Re: [PATCH 1/2] sctp: SCTP_SOCKOPT_PEELOFF return socket pointer for kernel users

[David Miller]

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.

Re: [PATCH 1/2] sctp: SCTP_SOCKOPT_PEELOFF return socket pointer for kernel users

[Marcelo Ricardo Leitner]

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

Re: [PATCH 1/2] sctp: SCTP_SOCKOPT_PEELOFF return socket pointer for kernel users

[David Miller]

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.

Re: [PATCH 1/2] sctp: SCTP_SOCKOPT_PEELOFF return socket pointer for kernel users

[Marcelo Ricardo Leitner]

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