[RFC PATCH 0/2] sctp: add new getsockopt option SCTP_SOCKOPT_PEELOFF_KERNEL

[RFC PATCH 0/2] sctp: add new getsockopt option SCTP_SOCKOPT_PEELOFF_KERNEL

[Marcelo Ricardo Leitner]

Hi,

I'm trying to remove a direct dependency of dlm module on sctp one.
Currently dlm code is calling sctp_do_peeloff() directly and only this
call is causing the load of sctp module together with dlm. For that, we
have basically 3 options:
- Doing a module split on dlm
  - which I'm avoiding because it was already split and was merged (more
    info on patch2 changelog)
  - and the sctp code on it is rather small if compared with sctp module
    itself
- Using some other infra that gets indirectly activated, like getsockopt()
  - It was like this before, but the exposed sockopt created a file
    descriptor for the new socket and that create some serious issues.
    More info on 2f2d76cc3e93 ("dlm: Do not allocate a fd for peeloff")
- Doing something like ipv6_stub (which is used by vxlan) or similar
  - but I don't feel that's a good way out here, it doesn't feel right.

So I'm approaching this by going with 2nd option again but this time
also creating a new sockopt that is only accessible for kernel users of
this protocol, so that we are safe to directly return a struct socket *
via getsockopt() results. This is the tricky part of it of this series. 

It smells hacky yes but currently most of sctp calls are wrapped behind
kernel_*(). Even if we set a flag (like netlink does) saying that this
is a kernel socket, we still have the issue of getting the function call
through and returning such non-usual return value.

I kept __user marker on sctp_getsockopt_peeloff_kernel() prototype and
its helpers just to avoid issues with static checkers.

Kernel path not really tested yet.. mainly willing to know what do you
think, is this feasible? getsockopt option only reachable by kernel
itself? Couldn't find any other like this.

Thanks,
Marcelo

Marcelo Ricardo Leitner (2):
  sctp: add new getsockopt option SCTP_SOCKOPT_PEELOFF_KERNEL
  dlm: avoid using sctp_do_peeloff directly

 fs/dlm/lowcomms.c         | 17 ++++++++---------
 include/uapi/linux/sctp.h | 12 ++++++++++++
 net/sctp/socket.c         | 39 +++++++++++++++++++++++++++++++++++++++
 3 files changed, 59 insertions(+), 9 deletions(-)

-- 
2.4.1

[RFC PATCH 1/2] sctp: add new getsockopt option SCTP_SOCKOPT_PEELOFF_KERNEL

[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 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.

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         | 39 +++++++++++++++++++++++++++++++++++++++
 2 files changed, 51 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 f09de7fac2e6acddad8b2e046dbf626e329cb674..dab6f9be260229f012e10e8f67c80ce99c0d2d06 100644
--- a/net/sctp/socket.c
+++ b/net/sctp/socket.c
@@ -4448,6 +4448,32 @@ int sctp_do_peeloff(struct sock *sk, sctp_assoc_t id, struct socket **sockp)
 }
 EXPORT_SYMBOL(sctp_do_peeloff);
 
+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;
+
+	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;
+}
+
 static int sctp_getsockopt_peeloff(struct sock *sk, int len, char __user *optval, int __user *optlen)
 {
 	sctp_peeloff_arg_t peeloff;
@@ -5943,6 +5969,11 @@ static int sctp_getsockopt_recvnxtinfo(struct sock *sk,	int len,
 	return 0;
 }
 
+static inline bool sctp_is_kernel(void)
+{
+	return segment_eq(get_fs(), KERNEL_DS);
+}
+
 static int sctp_getsockopt(struct sock *sk, int level, int optname,
 			   char __user *optval, int __user *optlen)
 {
@@ -5986,6 +6017,14 @@ 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:
+		if (!sctp_is_kernel()) {
+			retval = -EPERM;
+			break;
+		}
+		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);
-- 
2.4.1

[RFC 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 makes use of a new sockopt:
SCTP_SOCKOPT_PEELOFF_KERNEL, which avoids allocating file descriptors
while doing this operation.

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..aa50131e51ceaf2d56cc2252fe6c0c17b80af769 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_kernel_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_KERNEL,
+						(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.socket, new_con);
 
 			linger.l_onoff = 1;
 			linger.l_linger = 0;
-- 
2.4.1

Re: [RFC PATCH 0/2] sctp: add new getsockopt option SCTP_SOCKOPT_PEELOFF_KERNEL

[Neil Horman]

On Tue, Jun 16, 2015 at 07:42:31PM -0300, Marcelo Ricardo Leitner wrote:
> Hi,
> 
> I'm trying to remove a direct dependency of dlm module on sctp one.
> Currently dlm code is calling sctp_do_peeloff() directly and only this
> call is causing the load of sctp module together with dlm. For that, we
> have basically 3 options:
> - Doing a module split on dlm
>   - which I'm avoiding because it was already split and was merged (more
>     info on patch2 changelog)
>   - and the sctp code on it is rather small if compared with sctp module
>     itself
> - Using some other infra that gets indirectly activated, like getsockopt()
>   - It was like this before, but the exposed sockopt created a file
>     descriptor for the new socket and that create some serious issues.
>     More info on 2f2d76cc3e93 ("dlm: Do not allocate a fd for peeloff")
> - Doing something like ipv6_stub (which is used by vxlan) or similar
>   - but I don't feel that's a good way out here, it doesn't feel right.
> 
> So I'm approaching this by going with 2nd option again but this time
> also creating a new sockopt that is only accessible for kernel users of
> this protocol, so that we are safe to directly return a struct socket *
> via getsockopt() results. This is the tricky part of it of this series. 
> 
> It smells hacky yes but currently most of sctp calls are wrapped behind
> kernel_*(). Even if we set a flag (like netlink does) saying that this
> is a kernel socket, we still have the issue of getting the function call
> through and returning such non-usual return value.
> 
> I kept __user marker on sctp_getsockopt_peeloff_kernel() prototype and
> its helpers just to avoid issues with static checkers.
> 
> Kernel path not really tested yet.. mainly willing to know what do you
> think, is this feasible? getsockopt option only reachable by kernel
> itself? Couldn't find any other like this.
> 
> Thanks,
> Marcelo
> 
> Marcelo Ricardo Leitner (2):
>   sctp: add new getsockopt option SCTP_SOCKOPT_PEELOFF_KERNEL
>   dlm: avoid using sctp_do_peeloff directly
> 
>  fs/dlm/lowcomms.c         | 17 ++++++++---------
>  include/uapi/linux/sctp.h | 12 ++++++++++++
>  net/sctp/socket.c         | 39 +++++++++++++++++++++++++++++++++++++++
>  3 files changed, 59 insertions(+), 9 deletions(-)
> 
> -- 
> 2.4.1
> 
> 

Why not just use the existing PEELOFF socket option with the kernel_getsockopt
interface, and sockfd_lookup to translate the returned value back to a socket
struct?  That seems less redundant and less hack-ish to me.

Neil

Re: [RFC PATCH 0/2] sctp: add new getsockopt option SCTP_SOCKOPT_PEELOFF_KERNEL

[Marcelo Ricardo Leitner]

On 17-06-2015 07:21, Neil Horman wrote:
> On Tue, Jun 16, 2015 at 07:42:31PM -0300, Marcelo Ricardo Leitner wrote:
>> Hi,
>>
>> I'm trying to remove a direct dependency of dlm module on sctp one.
>> Currently dlm code is calling sctp_do_peeloff() directly and only this
>> call is causing the load of sctp module together with dlm. For that, we
>> have basically 3 options:
>> - Doing a module split on dlm
>>    - which I'm avoiding because it was already split and was merged (more
>>      info on patch2 changelog)
>>    - and the sctp code on it is rather small if compared with sctp module
>>      itself
>> - Using some other infra that gets indirectly activated, like getsockopt()
>>    - It was like this before, but the exposed sockopt created a file
>>      descriptor for the new socket and that create some serious issues.
>>      More info on 2f2d76cc3e93 ("dlm: Do not allocate a fd for peeloff")
>> - Doing something like ipv6_stub (which is used by vxlan) or similar
>>    - but I don't feel that's a good way out here, it doesn't feel right.
>>
>> So I'm approaching this by going with 2nd option again but this time
>> also creating a new sockopt that is only accessible for kernel users of
>> this protocol, so that we are safe to directly return a struct socket *
>> via getsockopt() results. This is the tricky part of it of this series.
>>
>> It smells hacky yes but currently most of sctp calls are wrapped behind
>> kernel_*(). Even if we set a flag (like netlink does) saying that this
>> is a kernel socket, we still have the issue of getting the function call
>> through and returning such non-usual return value.
>>
>> I kept __user marker on sctp_getsockopt_peeloff_kernel() prototype and
>> its helpers just to avoid issues with static checkers.
>>
>> Kernel path not really tested yet.. mainly willing to know what do you
>> think, is this feasible? getsockopt option only reachable by kernel
>> itself? Couldn't find any other like this.
>>
>> Thanks,
>> Marcelo
>>
>> Marcelo Ricardo Leitner (2):
>>    sctp: add new getsockopt option SCTP_SOCKOPT_PEELOFF_KERNEL
>>    dlm: avoid using sctp_do_peeloff directly
>>
>>   fs/dlm/lowcomms.c         | 17 ++++++++---------
>>   include/uapi/linux/sctp.h | 12 ++++++++++++
>>   net/sctp/socket.c         | 39 +++++++++++++++++++++++++++++++++++++++
>>   3 files changed, 59 insertions(+), 9 deletions(-)
>>
>> --
>> 2.4.1
>>
>>
>
> Why not just use the existing PEELOFF socket option with the kernel_getsockopt
> interface, and sockfd_lookup to translate the returned value back to a socket
> struct?  That seems less redundant and less hack-ish to me.

It was like that before commit 2f2d76cc3e93 ("dlm: Do not allocate a fd 
for peeloff"), but it caused serious issues due to the fd allocation, so 
that's what I'm willing to avoid now.

References:
http://article.gmane.org/gmane.linux.network.drbd/22529
https://bugzilla.redhat.com/show_bug.cgi?id=1075629 (this one is closed, 
sorry)

   Marcelo

Re: [RFC PATCH 0/2] sctp: add new getsockopt option SCTP_SOCKOPT_PEELOFF_KERNEL

[Neil Horman]

On Wed, Jun 17, 2015 at 08:38:10AM -0300, Marcelo Ricardo Leitner wrote:
> On 17-06-2015 07:21, Neil Horman wrote:
> >On Tue, Jun 16, 2015 at 07:42:31PM -0300, Marcelo Ricardo Leitner wrote:
> >>Hi,
> >>
> >>I'm trying to remove a direct dependency of dlm module on sctp one.
> >>Currently dlm code is calling sctp_do_peeloff() directly and only this
> >>call is causing the load of sctp module together with dlm. For that, we
> >>have basically 3 options:
> >>- Doing a module split on dlm
> >>   - which I'm avoiding because it was already split and was merged (more
> >>     info on patch2 changelog)
> >>   - and the sctp code on it is rather small if compared with sctp module
> >>     itself
> >>- Using some other infra that gets indirectly activated, like getsockopt()
> >>   - It was like this before, but the exposed sockopt created a file
> >>     descriptor for the new socket and that create some serious issues.
> >>     More info on 2f2d76cc3e93 ("dlm: Do not allocate a fd for peeloff")
> >>- Doing something like ipv6_stub (which is used by vxlan) or similar
> >>   - but I don't feel that's a good way out here, it doesn't feel right.
> >>
> >>So I'm approaching this by going with 2nd option again but this time
> >>also creating a new sockopt that is only accessible for kernel users of
> >>this protocol, so that we are safe to directly return a struct socket *
> >>via getsockopt() results. This is the tricky part of it of this series.
> >>
> >>It smells hacky yes but currently most of sctp calls are wrapped behind
> >>kernel_*(). Even if we set a flag (like netlink does) saying that this
> >>is a kernel socket, we still have the issue of getting the function call
> >>through and returning such non-usual return value.
> >>
> >>I kept __user marker on sctp_getsockopt_peeloff_kernel() prototype and
> >>its helpers just to avoid issues with static checkers.
> >>
> >>Kernel path not really tested yet.. mainly willing to know what do you
> >>think, is this feasible? getsockopt option only reachable by kernel
> >>itself? Couldn't find any other like this.
> >>
> >>Thanks,
> >>Marcelo
> >>
> >>Marcelo Ricardo Leitner (2):
> >>   sctp: add new getsockopt option SCTP_SOCKOPT_PEELOFF_KERNEL
> >>   dlm: avoid using sctp_do_peeloff directly
> >>
> >>  fs/dlm/lowcomms.c         | 17 ++++++++---------
> >>  include/uapi/linux/sctp.h | 12 ++++++++++++
> >>  net/sctp/socket.c         | 39 +++++++++++++++++++++++++++++++++++++++
> >>  3 files changed, 59 insertions(+), 9 deletions(-)
> >>
> >>--
> >>2.4.1
> >>
> >>
> >
> >Why not just use the existing PEELOFF socket option with the kernel_getsockopt
> >interface, and sockfd_lookup to translate the returned value back to a socket
> >struct?  That seems less redundant and less hack-ish to me.
> 
> It was like that before commit 2f2d76cc3e93 ("dlm: Do not allocate a fd for
> peeloff"), but it caused serious issues due to the fd allocation, so that's
> what I'm willing to avoid now.
> 
> References:
> http://article.gmane.org/gmane.linux.network.drbd/22529
> https://bugzilla.redhat.com/show_bug.cgi?id=1075629 (this one is closed,
> sorry)
> 
>   Marcelo
> 
Ah, I see.  You're using the new socket option as a differentiator to just skip
the creation of an FD.

I get your reasoning, but I'm still not in love with the idea of duplicating
code paths to avoid that action.  Can we use some data inside the socket
structure to do this differentiation?  Specifically here I'm thinking of
sock->file.  IIRC that will be non-null for any sockets created in user space,
but will always be NULL for dlm created sockets (since we use sock_create
directly to create them.  If that is a sufficient differentiator, then we can
just optionally allocate the new socket fd for the peeled off socket, iff the
parent sock->file pointer is non-null.

Thoughts?
Neil

Re: [RFC PATCH 0/2] sctp: add new getsockopt option SCTP_SOCKOPT_PEELOFF_KERNEL

[Marcelo Ricardo Leitner]

On 17-06-2015 09:20, Neil Horman wrote:
> On Wed, Jun 17, 2015 at 08:38:10AM -0300, Marcelo Ricardo Leitner wrote:
>> On 17-06-2015 07:21, Neil Horman wrote:
>>> On Tue, Jun 16, 2015 at 07:42:31PM -0300, Marcelo Ricardo Leitner wrote:
>>>> Hi,
>>>>
>>>> I'm trying to remove a direct dependency of dlm module on sctp one.
>>>> Currently dlm code is calling sctp_do_peeloff() directly and only this
>>>> call is causing the load of sctp module together with dlm. For that, we
>>>> have basically 3 options:
>>>> - Doing a module split on dlm
>>>>    - which I'm avoiding because it was already split and was merged (more
>>>>      info on patch2 changelog)
>>>>    - and the sctp code on it is rather small if compared with sctp module
>>>>      itself
>>>> - Using some other infra that gets indirectly activated, like getsockopt()
>>>>    - It was like this before, but the exposed sockopt created a file
>>>>      descriptor for the new socket and that create some serious issues.
>>>>      More info on 2f2d76cc3e93 ("dlm: Do not allocate a fd for peeloff")
>>>> - Doing something like ipv6_stub (which is used by vxlan) or similar
>>>>    - but I don't feel that's a good way out here, it doesn't feel right.
>>>>
>>>> So I'm approaching this by going with 2nd option again but this time
>>>> also creating a new sockopt that is only accessible for kernel users of
>>>> this protocol, so that we are safe to directly return a struct socket *
>>>> via getsockopt() results. This is the tricky part of it of this series.
>>>>
>>>> It smells hacky yes but currently most of sctp calls are wrapped behind
>>>> kernel_*(). Even if we set a flag (like netlink does) saying that this
>>>> is a kernel socket, we still have the issue of getting the function call
>>>> through and returning such non-usual return value.
>>>>
>>>> I kept __user marker on sctp_getsockopt_peeloff_kernel() prototype and
>>>> its helpers just to avoid issues with static checkers.
>>>>
>>>> Kernel path not really tested yet.. mainly willing to know what do you
>>>> think, is this feasible? getsockopt option only reachable by kernel
>>>> itself? Couldn't find any other like this.
>>>>
>>>> Thanks,
>>>> Marcelo
>>>>
>>>> Marcelo Ricardo Leitner (2):
>>>>    sctp: add new getsockopt option SCTP_SOCKOPT_PEELOFF_KERNEL
>>>>    dlm: avoid using sctp_do_peeloff directly
>>>>
>>>>   fs/dlm/lowcomms.c         | 17 ++++++++---------
>>>>   include/uapi/linux/sctp.h | 12 ++++++++++++
>>>>   net/sctp/socket.c         | 39 +++++++++++++++++++++++++++++++++++++++
>>>>   3 files changed, 59 insertions(+), 9 deletions(-)
>>>>
>>>> --
>>>> 2.4.1
>>>>
>>>>
>>>
>>> Why not just use the existing PEELOFF socket option with the kernel_getsockopt
>>> interface, and sockfd_lookup to translate the returned value back to a socket
>>> struct?  That seems less redundant and less hack-ish to me.
>>
>> It was like that before commit 2f2d76cc3e93 ("dlm: Do not allocate a fd for
>> peeloff"), but it caused serious issues due to the fd allocation, so that's
>> what I'm willing to avoid now.
>>
>> References:
>> http://article.gmane.org/gmane.linux.network.drbd/22529
>> https://bugzilla.redhat.com/show_bug.cgi?id=1075629 (this one is closed,
>> sorry)
>>
>>    Marcelo
>>
> Ah, I see.  You're using the new socket option as a differentiator to just skip
> the creation of an FD.

Exactly.

> I get your reasoning, but I'm still not in love with the idea of duplicating
> code paths to avoid that action.  Can we use some data inside the socket
> structure to do this differentiation?  Specifically here I'm thinking of
> sock->file.  IIRC that will be non-null for any sockets created in user space,

I had thought about using some socket flags like netlink does but 
couldn't get around with that. Hadn't thought about sock->file though, 
nice idea.

> but will always be NULL for dlm created sockets (since we use sock_create
> directly to create them.  If that is a sufficient differentiator, then we can
> just optionally allocate the new socket fd for the peeled off socket, iff the
> parent sock->file pointer is non-null.
>
> Thoughts?
> Neil

We can re-use the current code path, by either checking it via 
sock->file or via get_fs(). That will require us to change the option 
arg format so we keep it nice and clean but as it would be kernel-side 
only, it should be ok right? It currently is:

typedef struct {
         sctp_assoc_t associd;
         int sd;
} sctp_peeloff_arg_t;

And we would have to fit a pointer in there, something like:
typedef union {
	struct {
	        sctp_assoc_t associd;
	        int sd;
	};
	void *sock;
} sctp_peeloff_arg_t;

Sounds good?

   Marcelo

Re: [RFC PATCH 0/2] sctp: add new getsockopt option SCTP_SOCKOPT_PEELOFF_KERNEL

[Neil Horman]

On Wed, Jun 17, 2015 at 09:40:32AM -0300, Marcelo Ricardo Leitner wrote:
> On 17-06-2015 09:20, Neil Horman wrote:
> >On Wed, Jun 17, 2015 at 08:38:10AM -0300, Marcelo Ricardo Leitner wrote:
> >>On 17-06-2015 07:21, Neil Horman wrote:
> >>>On Tue, Jun 16, 2015 at 07:42:31PM -0300, Marcelo Ricardo Leitner wrote:
> >>>>Hi,
> >>>>
> >>>>I'm trying to remove a direct dependency of dlm module on sctp one.
> >>>>Currently dlm code is calling sctp_do_peeloff() directly and only this
> >>>>call is causing the load of sctp module together with dlm. For that, we
> >>>>have basically 3 options:
> >>>>- Doing a module split on dlm
> >>>>   - which I'm avoiding because it was already split and was merged (more
> >>>>     info on patch2 changelog)
> >>>>   - and the sctp code on it is rather small if compared with sctp module
> >>>>     itself
> >>>>- Using some other infra that gets indirectly activated, like getsockopt()
> >>>>   - It was like this before, but the exposed sockopt created a file
> >>>>     descriptor for the new socket and that create some serious issues.
> >>>>     More info on 2f2d76cc3e93 ("dlm: Do not allocate a fd for peeloff")
> >>>>- Doing something like ipv6_stub (which is used by vxlan) or similar
> >>>>   - but I don't feel that's a good way out here, it doesn't feel right.
> >>>>
> >>>>So I'm approaching this by going with 2nd option again but this time
> >>>>also creating a new sockopt that is only accessible for kernel users of
> >>>>this protocol, so that we are safe to directly return a struct socket *
> >>>>via getsockopt() results. This is the tricky part of it of this series.
> >>>>
> >>>>It smells hacky yes but currently most of sctp calls are wrapped behind
> >>>>kernel_*(). Even if we set a flag (like netlink does) saying that this
> >>>>is a kernel socket, we still have the issue of getting the function call
> >>>>through and returning such non-usual return value.
> >>>>
> >>>>I kept __user marker on sctp_getsockopt_peeloff_kernel() prototype and
> >>>>its helpers just to avoid issues with static checkers.
> >>>>
> >>>>Kernel path not really tested yet.. mainly willing to know what do you
> >>>>think, is this feasible? getsockopt option only reachable by kernel
> >>>>itself? Couldn't find any other like this.
> >>>>
> >>>>Thanks,
> >>>>Marcelo
> >>>>
> >>>>Marcelo Ricardo Leitner (2):
> >>>>   sctp: add new getsockopt option SCTP_SOCKOPT_PEELOFF_KERNEL
> >>>>   dlm: avoid using sctp_do_peeloff directly
> >>>>
> >>>>  fs/dlm/lowcomms.c         | 17 ++++++++---------
> >>>>  include/uapi/linux/sctp.h | 12 ++++++++++++
> >>>>  net/sctp/socket.c         | 39 +++++++++++++++++++++++++++++++++++++++
> >>>>  3 files changed, 59 insertions(+), 9 deletions(-)
> >>>>
> >>>>--
> >>>>2.4.1
> >>>>
> >>>>
> >>>
> >>>Why not just use the existing PEELOFF socket option with the kernel_getsockopt
> >>>interface, and sockfd_lookup to translate the returned value back to a socket
> >>>struct?  That seems less redundant and less hack-ish to me.
> >>
> >>It was like that before commit 2f2d76cc3e93 ("dlm: Do not allocate a fd for
> >>peeloff"), but it caused serious issues due to the fd allocation, so that's
> >>what I'm willing to avoid now.
> >>
> >>References:
> >>http://article.gmane.org/gmane.linux.network.drbd/22529
> >>https://bugzilla.redhat.com/show_bug.cgi?id=1075629 (this one is closed,
> >>sorry)
> >>
> >>   Marcelo
> >>
> >Ah, I see.  You're using the new socket option as a differentiator to just skip
> >the creation of an FD.
> 
> Exactly.
> 
> >I get your reasoning, but I'm still not in love with the idea of duplicating
> >code paths to avoid that action.  Can we use some data inside the socket
> >structure to do this differentiation?  Specifically here I'm thinking of
> >sock->file.  IIRC that will be non-null for any sockets created in user space,
> 
> I had thought about using some socket flags like netlink does but couldn't
> get around with that. Hadn't thought about sock->file though, nice idea.
> 
> >but will always be NULL for dlm created sockets (since we use sock_create
> >directly to create them.  If that is a sufficient differentiator, then we can
> >just optionally allocate the new socket fd for the peeled off socket, iff the
> >parent sock->file pointer is non-null.
> >
> >Thoughts?
> >Neil
> 
> We can re-use the current code path, by either checking it via sock->file or
> via get_fs(). That will require us to change the option arg format so we
> keep it nice and clean but as it would be kernel-side only, it should be ok
> right? It currently is:
> 
> typedef struct {
>         sctp_assoc_t associd;
>         int sd;
> } sctp_peeloff_arg_t;
> 
> And we would have to fit a pointer in there, something like:
> typedef union {
> 	struct {
> 	        sctp_assoc_t associd;
> 	        int sd;
> 	};
> 	void *sock;
> } sctp_peeloff_arg_t;
> 
> Sounds good?
> 
Yes, sounds reasonable.

Thanks!
Neil

>   Marcelo
> 
> --
> 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
> 

Re: [RFC PATCH 0/2] sctp: add new getsockopt option SCTP_SOCKOPT_PEELOFF_KERNEL

[Marcelo Ricardo Leitner]

On 17-06-2015 10:16, Neil Horman wrote:
> On Wed, Jun 17, 2015 at 09:40:32AM -0300, Marcelo Ricardo Leitner wrote:
>> On 17-06-2015 09:20, Neil Horman wrote:
>>> On Wed, Jun 17, 2015 at 08:38:10AM -0300, Marcelo Ricardo Leitner wrote:
>>>> On 17-06-2015 07:21, Neil Horman wrote:
>>>>> On Tue, Jun 16, 2015 at 07:42:31PM -0300, Marcelo Ricardo Leitner wrote:
>>>>>> Hi,
>>>>>>
>>>>>> I'm trying to remove a direct dependency of dlm module on sctp one.
>>>>>> Currently dlm code is calling sctp_do_peeloff() directly and only this
>>>>>> call is causing the load of sctp module together with dlm. For that, we
>>>>>> have basically 3 options:
>>>>>> - Doing a module split on dlm
>>>>>>    - which I'm avoiding because it was already split and was merged (more
>>>>>>      info on patch2 changelog)
>>>>>>    - and the sctp code on it is rather small if compared with sctp module
>>>>>>      itself
>>>>>> - Using some other infra that gets indirectly activated, like getsockopt()
>>>>>>    - It was like this before, but the exposed sockopt created a file
>>>>>>      descriptor for the new socket and that create some serious issues.
>>>>>>      More info on 2f2d76cc3e93 ("dlm: Do not allocate a fd for peeloff")
>>>>>> - Doing something like ipv6_stub (which is used by vxlan) or similar
>>>>>>    - but I don't feel that's a good way out here, it doesn't feel right.
>>>>>>
>>>>>> So I'm approaching this by going with 2nd option again but this time
>>>>>> also creating a new sockopt that is only accessible for kernel users of
>>>>>> this protocol, so that we are safe to directly return a struct socket *
>>>>>> via getsockopt() results. This is the tricky part of it of this series.
>>>>>>
>>>>>> It smells hacky yes but currently most of sctp calls are wrapped behind
>>>>>> kernel_*(). Even if we set a flag (like netlink does) saying that this
>>>>>> is a kernel socket, we still have the issue of getting the function call
>>>>>> through and returning such non-usual return value.
>>>>>>
>>>>>> I kept __user marker on sctp_getsockopt_peeloff_kernel() prototype and
>>>>>> its helpers just to avoid issues with static checkers.
>>>>>>
>>>>>> Kernel path not really tested yet.. mainly willing to know what do you
>>>>>> think, is this feasible? getsockopt option only reachable by kernel
>>>>>> itself? Couldn't find any other like this.
>>>>>>
>>>>>> Thanks,
>>>>>> Marcelo
>>>>>>
>>>>>> Marcelo Ricardo Leitner (2):
>>>>>>    sctp: add new getsockopt option SCTP_SOCKOPT_PEELOFF_KERNEL
>>>>>>    dlm: avoid using sctp_do_peeloff directly
>>>>>>
>>>>>>   fs/dlm/lowcomms.c         | 17 ++++++++---------
>>>>>>   include/uapi/linux/sctp.h | 12 ++++++++++++
>>>>>>   net/sctp/socket.c         | 39 +++++++++++++++++++++++++++++++++++++++
>>>>>>   3 files changed, 59 insertions(+), 9 deletions(-)
>>>>>>
>>>>>> --
>>>>>> 2.4.1
>>>>>>
>>>>>>
>>>>>
>>>>> Why not just use the existing PEELOFF socket option with the kernel_getsockopt
>>>>> interface, and sockfd_lookup to translate the returned value back to a socket
>>>>> struct?  That seems less redundant and less hack-ish to me.
>>>>
>>>> It was like that before commit 2f2d76cc3e93 ("dlm: Do not allocate a fd for
>>>> peeloff"), but it caused serious issues due to the fd allocation, so that's
>>>> what I'm willing to avoid now.
>>>>
>>>> References:
>>>> http://article.gmane.org/gmane.linux.network.drbd/22529
>>>> https://bugzilla.redhat.com/show_bug.cgi?id=1075629 (this one is closed,
>>>> sorry)
>>>>
>>>>    Marcelo
>>>>
>>> Ah, I see.  You're using the new socket option as a differentiator to just skip
>>> the creation of an FD.
>>
>> Exactly.
>>
>>> I get your reasoning, but I'm still not in love with the idea of duplicating
>>> code paths to avoid that action.  Can we use some data inside the socket
>>> structure to do this differentiation?  Specifically here I'm thinking of
>>> sock->file.  IIRC that will be non-null for any sockets created in user space,
>>
>> I had thought about using some socket flags like netlink does but couldn't
>> get around with that. Hadn't thought about sock->file though, nice idea.
>>
>>> but will always be NULL for dlm created sockets (since we use sock_create
>>> directly to create them.  If that is a sufficient differentiator, then we can
>>> just optionally allocate the new socket fd for the peeled off socket, iff the
>>> parent sock->file pointer is non-null.
>>>
>>> Thoughts?
>>> Neil
>>
>> We can re-use the current code path, by either checking it via sock->file or
>> via get_fs(). That will require us to change the option arg format so we
>> keep it nice and clean but as it would be kernel-side only, it should be ok
>> right? It currently is:
>>
>> typedef struct {
>>          sctp_assoc_t associd;
>>          int sd;
>> } sctp_peeloff_arg_t;
>>
>> And we would have to fit a pointer in there, something like:
>> typedef union {
>> 	struct {
>> 	        sctp_assoc_t associd;
>> 	        int sd;
>> 	};
>> 	void *sock;
>> } sctp_peeloff_arg_t;
>>
>> Sounds good?
>>
> Yes, sounds reasonable.
>
> Thanks!
> Neil

Cool, thanks Neil. I'll rework these now but will post the new version 
probably by next week only, as we can get dlm properly tested too.

Cheers,
Marcelo

Re: [RFC PATCH 0/2] sctp: add new getsockopt option SCTP_SOCKOPT_PEELOFF_KERNEL

[Neil Horman]

On Wed, Jun 17, 2015 at 10:40:26AM -0300, Marcelo Ricardo Leitner wrote:
> On 17-06-2015 10:16, Neil Horman wrote:
> >On Wed, Jun 17, 2015 at 09:40:32AM -0300, Marcelo Ricardo Leitner wrote:
> >>On 17-06-2015 09:20, Neil Horman wrote:
> >>>On Wed, Jun 17, 2015 at 08:38:10AM -0300, Marcelo Ricardo Leitner wrote:
> >>>>On 17-06-2015 07:21, Neil Horman wrote:
> >>>>>On Tue, Jun 16, 2015 at 07:42:31PM -0300, Marcelo Ricardo Leitner wrote:
> >>>>>>Hi,
> >>>>>>
> >>>>>>I'm trying to remove a direct dependency of dlm module on sctp one.
> >>>>>>Currently dlm code is calling sctp_do_peeloff() directly and only this
> >>>>>>call is causing the load of sctp module together with dlm. For that, we
> >>>>>>have basically 3 options:
> >>>>>>- Doing a module split on dlm
> >>>>>>   - which I'm avoiding because it was already split and was merged (more
> >>>>>>     info on patch2 changelog)
> >>>>>>   - and the sctp code on it is rather small if compared with sctp module
> >>>>>>     itself
> >>>>>>- Using some other infra that gets indirectly activated, like getsockopt()
> >>>>>>   - It was like this before, but the exposed sockopt created a file
> >>>>>>     descriptor for the new socket and that create some serious issues.
> >>>>>>     More info on 2f2d76cc3e93 ("dlm: Do not allocate a fd for peeloff")
> >>>>>>- Doing something like ipv6_stub (which is used by vxlan) or similar
> >>>>>>   - but I don't feel that's a good way out here, it doesn't feel right.
> >>>>>>
> >>>>>>So I'm approaching this by going with 2nd option again but this time
> >>>>>>also creating a new sockopt that is only accessible for kernel users of
> >>>>>>this protocol, so that we are safe to directly return a struct socket *
> >>>>>>via getsockopt() results. This is the tricky part of it of this series.
> >>>>>>
> >>>>>>It smells hacky yes but currently most of sctp calls are wrapped behind
> >>>>>>kernel_*(). Even if we set a flag (like netlink does) saying that this
> >>>>>>is a kernel socket, we still have the issue of getting the function call
> >>>>>>through and returning such non-usual return value.
> >>>>>>
> >>>>>>I kept __user marker on sctp_getsockopt_peeloff_kernel() prototype and
> >>>>>>its helpers just to avoid issues with static checkers.
> >>>>>>
> >>>>>>Kernel path not really tested yet.. mainly willing to know what do you
> >>>>>>think, is this feasible? getsockopt option only reachable by kernel
> >>>>>>itself? Couldn't find any other like this.
> >>>>>>
> >>>>>>Thanks,
> >>>>>>Marcelo
> >>>>>>
> >>>>>>Marcelo Ricardo Leitner (2):
> >>>>>>   sctp: add new getsockopt option SCTP_SOCKOPT_PEELOFF_KERNEL
> >>>>>>   dlm: avoid using sctp_do_peeloff directly
> >>>>>>
> >>>>>>  fs/dlm/lowcomms.c         | 17 ++++++++---------
> >>>>>>  include/uapi/linux/sctp.h | 12 ++++++++++++
> >>>>>>  net/sctp/socket.c         | 39 +++++++++++++++++++++++++++++++++++++++
> >>>>>>  3 files changed, 59 insertions(+), 9 deletions(-)
> >>>>>>
> >>>>>>--
> >>>>>>2.4.1
> >>>>>>
> >>>>>>
> >>>>>
> >>>>>Why not just use the existing PEELOFF socket option with the kernel_getsockopt
> >>>>>interface, and sockfd_lookup to translate the returned value back to a socket
> >>>>>struct?  That seems less redundant and less hack-ish to me.
> >>>>
> >>>>It was like that before commit 2f2d76cc3e93 ("dlm: Do not allocate a fd for
> >>>>peeloff"), but it caused serious issues due to the fd allocation, so that's
> >>>>what I'm willing to avoid now.
> >>>>
> >>>>References:
> >>>>http://article.gmane.org/gmane.linux.network.drbd/22529
> >>>>https://bugzilla.redhat.com/show_bug.cgi?id=1075629 (this one is closed,
> >>>>sorry)
> >>>>
> >>>>   Marcelo
> >>>>
> >>>Ah, I see.  You're using the new socket option as a differentiator to just skip
> >>>the creation of an FD.
> >>
> >>Exactly.
> >>
> >>>I get your reasoning, but I'm still not in love with the idea of duplicating
> >>>code paths to avoid that action.  Can we use some data inside the socket
> >>>structure to do this differentiation?  Specifically here I'm thinking of
> >>>sock->file.  IIRC that will be non-null for any sockets created in user space,
> >>
> >>I had thought about using some socket flags like netlink does but couldn't
> >>get around with that. Hadn't thought about sock->file though, nice idea.
> >>
> >>>but will always be NULL for dlm created sockets (since we use sock_create
> >>>directly to create them.  If that is a sufficient differentiator, then we can
> >>>just optionally allocate the new socket fd for the peeled off socket, iff the
> >>>parent sock->file pointer is non-null.
> >>>
> >>>Thoughts?
> >>>Neil
> >>
> >>We can re-use the current code path, by either checking it via sock->file or
> >>via get_fs(). That will require us to change the option arg format so we
> >>keep it nice and clean but as it would be kernel-side only, it should be ok
> >>right? It currently is:
> >>
> >>typedef struct {
> >>         sctp_assoc_t associd;
> >>         int sd;
> >>} sctp_peeloff_arg_t;
> >>
> >>And we would have to fit a pointer in there, something like:
> >>typedef union {
> >>	struct {
> >>	        sctp_assoc_t associd;
> >>	        int sd;
> >>	};
> >>	void *sock;
> >>} sctp_peeloff_arg_t;
> >>
> >>Sounds good?
> >>
> >Yes, sounds reasonable.
> >
> >Thanks!
> >Neil
> 
> Cool, thanks Neil. I'll rework these now but will post the new version
> probably by next week only, as we can get dlm properly tested too.
> 
Worksforme :)
Neil

> Cheers,
> Marcelo
> 
>