Skip to contentSkip to conversation

Avoid link dependency of dlm on sctp module

PATCH In review Marcelo Ricardo Leitner 5 participants 14 messages Jul 14, 2015 – Jul 22, 2015

Subsystems (4)
Networking Network drivers SCTP protocol Distributed lock manager (DLM)
lore
Share & tools
lore

Marcelo Ricardo Leitner

author maintainer
#0 lore raw

v2 cover Avoid link dependency of dlm on sctp module

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 this call only 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.

"Kernel users" are identified by sockets that don't have a file descriptor attached to it, as only kernel users are allowed to do that and, if original socket has a file descriptor, one should just use the original option.

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

I tried minimizing the code duplication while adding the new option, but it's just better to add a whole new function to handle it. As the sockopt arguments are different, the only thing in common between both call handlers is the call to sctp_do_peeloff(). All rest would have to have checks around it.

RFC->v1, from Neil:
- For identifying kernel users, switched from segment_eq() to fd check
- Reused original option to avoid code duplication
v1->v2, from David:
- Back to new sockopt implementation, to avoid changing sockopt arg format.

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         | 37 +++++++++++++++++++++++++++++++++++++
 3 files changed, 57 insertions(+), 9 deletions(-)
Signature · 1 line
2.4.1
author maintainer
#1 lore raw

PATCH v2 1/2 sctp: add new getsockopt option SCTP_SOCKOPT_PEELOFF_KERNEL

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 <[email protected]>
---
 include/uapi/linux/sctp.h | 12 ++++++++++++
 net/sctp/socket.c         | 37 +++++++++++++++++++++++++++++++++++++
 2 files changed, 49 insertions(+)
include/uapi/linux/sctp.h +12 -0
index ce70fe6b45df3e841c35accbdb6379c16563893c..b3aad3ce456ab3c1ebf4d81fdb7269ba40b3d92a 100644
@@ -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
  */
net/sctp/socket.c +37 -0
index f1a65398f3118ab5d3a884e9c875620560e6b5ef..7968de7a1aeabd5cd0a0398461dbf2081bd4c5b7 100644
@@ -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);
Signature · 1 line
2.4.1
#2 lore raw

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

Quoted 107 lines · 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 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 <[email protected]>
---
 include/uapi/linux/sctp.h | 12 ++++++++++++
 net/sctp/socket.c         | 37 +++++++++++++++++++++++++++++++++++++
 2 files changed, 49 insertions(+)
include/uapi/linux/sctp.h +12 -0
index ce70fe6b45df3e841c35accbdb6379c16563893c..b3aad3ce456ab3c1ebf4d81fdb7269ba40b3d92a 100644
@@ -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
  */
net/sctp/socket.c +33 -0
index f1a65398f3118ab5d3a884e9c875620560e6b5ef..7968de7a1aeabd5cd0a0398461dbf2081bd4c5b7 100644
@@ -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

net/sctp/socket.c static int sctp_getsockopt(struct sock *sk, int level, int optname, · line 6030

Quoted 7 lines
net/sctp/socket.c +4 -0
@@ -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

Quoted 11 lines
net/sctp/socket.c +0 -0
 	case SCTP_PEER_ADDR_PARAMS:
 		retval = sctp_getsockopt_peer_addr_params(sk, len, optval,
 							  optlen);
Signature · 6 lines
2.4.1

--
To unsubscribe from this list: send the line "unsubscribe linux-sctp" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
author maintainer
#3 lore raw

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

Quoted 71 lines · Neil Horman
Quoted 70 lines · 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 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 <[email protected]>
---
  include/uapi/linux/sctp.h | 12 ++++++++++++
  net/sctp/socket.c         | 37 +++++++++++++++++++++++++++++++++++++
  2 files changed, 49 insertions(+)
include/uapi/linux/sctp.h +12 -0
index ce70fe6b45df3e841c35accbdb6379c16563893c..b3aad3ce456ab3c1ebf4d81fdb7269ba40b3d92a 100644
@@ -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
   */
net/sctp/socket.c +0 -0
index f1a65398f3118ab5d3a884e9c875620560e6b5ef..7968de7a1aeabd5cd0a0398461dbf2081bd4c5b7 100644
@@ -4504,6 +4504,39 @@ out:
  	return retval;
  }
Quoted 13 lines

net/sctp/socket.c out: · line 4519

Quoted 13 lines
net/sctp/socket.c +13 -0
+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) --^

Quoted 34 lines
Quoted 24 lines
net/sctp/socket.c +20 -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;
+}
+
  /* 7.1.13 Peer Address Parameters (SCTP_PEER_ADDR_PARAMS)
   *
   * Applications can enable or disable heartbeats for any peer address of

net/sctp/socket.c static int sctp_getsockopt(struct sock *sk, int level, int optname, · line 6030

Quoted 7 lines
net/sctp/socket.c +4 -0
@@ -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?

Quoted 1 line

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

Quoted 11 lines
Quoted 11 lines
net/sctp/socket.c +0 -1
  	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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

maintainer
#4 lore raw

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

In reply to Neil Horman

From: Neil Horman <[email protected]>
Date: Wed, 15 Jul 2015 09:18:28 -0400

Quoted 2 lines

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.

#5 lore raw

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

In reply to Marcelo Ricardo Leitner
Quoted 37 lines · 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 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 <[email protected]>
---
 include/uapi/linux/sctp.h | 12 ++++++++++++
 net/sctp/socket.c         | 37 +++++++++++++++++++++++++++++++++++++
 2 files changed, 49 insertions(+)
include/uapi/linux/sctp.h +0 -0
index ce70fe6b45df3e841c35accbdb6379c16563893c..b3aad3ce456ab3c1ebf4d81fdb7269ba40b3d92a 100644

include/uapi/linux/sctp.h typedef __s32 sctp_assoc_t; · line 111

Quoted 7 lines
include/uapi/linux/sctp.h +4 -0
@@ -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

Quoted 74 lines
include/uapi/linux/sctp.h +8 -0
 /* 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
  */
net/sctp/socket.c +37 -0
index f1a65398f3118ab5d3a884e9c875620560e6b5ef..7968de7a1aeabd5cd0a0398461dbf2081bd4c5b7 100644
@@ -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);
author maintainer
#6 lore raw

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

Quoted 33 lines · Vlad Yasevich
Quoted 32 lines · 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 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 <[email protected]>
---
 include/uapi/linux/sctp.h | 12 ++++++++++++
 net/sctp/socket.c         | 37 +++++++++++++++++++++++++++++++++++++
 2 files changed, 49 insertions(+)
Quoted 16 lines

include/uapi/linux/sctp.h typedef __s32 sctp_assoc_t; · line 111

Quoted 12 lines
include/uapi/linux/sctp.h +4 -0
index ce70fe6b45df3e841c35accbdb6379c16563893c..b3aad3ce456ab3c1ebf4d81fdb7269ba40b3d92a 100644
@@ -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.

Quoted 3 lines

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?

Quoted 8 lines

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

Quoted 83 lines

Thanks
-vlad

Quoted 64 lines
include/uapi/linux/sctp.h +8 -0
 /* 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
  */
net/sctp/socket.c +33 -0
index f1a65398f3118ab5d3a884e9c875620560e6b5ef..7968de7a1aeabd5cd0a0398461dbf2081bd4c5b7 100644
@@ -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

net/sctp/socket.c static int sctp_getsockopt(struct sock *sk, int level, int optname, · line 6033

Quoted 10 lines
net/sctp/socket.c +4 -0
@@ -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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

author maintainer
#7 lore raw

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

Quoted 1 line · Marcelo Ricardo Leitner

On Thu, Jul 16, 2015 at 09:50:16AM -0400, Vlad Yasevich wrote:

...

Quoted 11 lines
Quoted 8 lines

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

#8 lore raw

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

In reply to Marcelo Ricardo Leitner

From: Marcelo Ricardo Leitner

Quoted 28 lines

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 <[email protected]>

...

Quoted 9 lines
net/sctp/socket.c +9 -0
+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

net/sctp/socket.c out: · line 4525

Quoted 10 lines
net/sctp/socket.c +10 -0
+	 * 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

author maintainer
#9 lore raw

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

Em 22 de julho de 2015 10:13:22 BRT, David Laight <[email protected]> escreveu:

Quoted 59 lines

From: Marcelo Ricardo Leitner

Quoted 2 lines

Sent: 14 July 2015 18:13
SCTP has this operation to peel off associations from a given socket

and

Quoted 1 line

create a new socket using this association. We currently have two

ways

Quoted 11 lines

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

Quoted 4 lines

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

Quoted 5 lines

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

Quoted 1 line

uapi header. That's because hidding this wouldn't add any extra

security

Quoted 4 lines

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 <[email protected]>

...

Quoted 9 lines
net/sctp/socket.c +9 -0
+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

net/sctp/socket.c out: · line 4525

Quoted 10 lines
net/sctp/socket.c +10 -0
+	 * 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.

Quoted 1 line

David

Yes. It was just to avoid errors from static checkers, if any. Same for the __user in function prototype.

Signature · 1 line
Sent from mobile. Please excuse my brevity.
author maintainer
#10 lore raw

PATCH v2 2/2 dlm: avoid using sctp_do_peeloff directly

In reply to 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 <[email protected]>
---
 fs/dlm/lowcomms.c | 17 ++++++++---------
 1 file changed, 8 insertions(+), 9 deletions(-)
fs/dlm/lowcomms.c +8 -9
index d08e079ea5d3aa37cf685cce89eb00122fe7ba02..dd14fe05a69565d9ec061fc2c93671c49d12fdcb 100644
@@ -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;
Signature · 1 line
2.4.1
Acked-by on the v2 series
#11 lore raw
In reply to Marcelo Ricardo Leitner
Quoted 45 lines · 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 this call only 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.

"Kernel users" are identified by sockets that don't have a file descriptor attached to it, as only kernel users are allowed to do that and, if original socket has a file descriptor, one should just use the original option.

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

I tried minimizing the code duplication while adding the new option, but it's just better to add a whole new function to handle it. As the sockopt arguments are different, the only thing in common between both call handlers is the call to sctp_do_peeloff(). All rest would have to have checks around it.

RFC->v1, from Neil:
- For identifying kernel users, switched from segment_eq() to fd check
- Reused original option to avoid code duplication
v1->v2, from David:
- Back to new sockopt implementation, to avoid changing sockopt arg format.

Quoted 17 lines

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         | 37 +++++++++++++++++++++++++++++++++++++
 3 files changed, 57 insertions(+), 9 deletions(-)
Signature · 6 lines
2.4.1

--
To unsubscribe from this list: send the line "unsubscribe linux-sctp" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Series

Acked-by Neil Horman <[email protected]>
maintainer
#12 lore raw

From: Neil Horman <[email protected]>
Date: Wed, 15 Jul 2015 09:57:14 -0400

Quoted 2 lines

Series

Acked-by Neil Horman <[email protected]>

I don't like this at all.

I know it's a pain in the ass to have this dependency on SCTP, but calling exported functions is absolutely the right way to handle this kind of situation.

#13 lore raw
Quoted 11 lines · David Miller

From: Neil Horman <[email protected]>
Date: Wed, 15 Jul 2015 09:57:14 -0400

Quoted 2 lines

Series

Acked-by Neil Horman <[email protected]>

I don't like this at all.

I know it's a pain in the ass to have this dependency on SCTP, but calling exported functions is absolutely the right way to handle this kind of situation.

I have to disagree. Its certainly true that a direct call from the kernel is the right current way to handle a need for functionality in an external module, but I think its important to offer a mechanism that allows for modules to not load functionality that they don't need at run time (that is to say, if dlm decides to use tcp rather than sctp, the sctp module shouldn't be loaded).

Its not a crisis in either case, but it sure would be nice to not load a module just because a symbol reference exists.

Neil

Quoted 5 lines

--
To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html