All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH net 0/3] sctp: process the error returned from sctp_sock_migrate()
@ 2019-03-03  9:54 ` Xin Long
  0 siblings, 0 replies; 34+ messages in thread
From: Xin Long @ 2019-03-03  9:54 UTC (permalink / raw)
  To: network dev, linux-sctp; +Cc: davem, Marcelo Ricardo Leitner, Neil Horman

This patchset is to process the errs returned by sctp_auth_init_hmacs()
and sctp_bind_addr_dup() from sctp_sock_migrate(). And also fix a panic
caused by new ep->auth_hmacs was not set due to net->sctp.auth_enable
changed by sysctl before accepting an assoc.

Xin Long (3):
  sctp: sctp_sock_migrate() returns error if sctp_bind_addr_dup() fails
  sctp: move up sctp_auth_init_hmacs() in sctp_endpoint_init()
  sctp: call sctp_auth_init_hmacs() in sctp_sock_migrate()

 net/sctp/auth.c        |  6 ------
 net/sctp/endpointola.c | 18 ++++++++++--------
 net/sctp/socket.c      | 44 ++++++++++++++++++++++++++++++++++----------
 3 files changed, 44 insertions(+), 24 deletions(-)

-- 
2.1.0


^ permalink raw reply	[flat|nested] 34+ messages in thread

* [PATCH net 0/3] sctp: process the error returned from sctp_sock_migrate()
@ 2019-03-03  9:54 ` Xin Long
  0 siblings, 0 replies; 34+ messages in thread
From: Xin Long @ 2019-03-03  9:54 UTC (permalink / raw)
  To: network dev, linux-sctp; +Cc: davem, Marcelo Ricardo Leitner, Neil Horman

This patchset is to process the errs returned by sctp_auth_init_hmacs()
and sctp_bind_addr_dup() from sctp_sock_migrate(). And also fix a panic
caused by new ep->auth_hmacs was not set due to net->sctp.auth_enable
changed by sysctl before accepting an assoc.

Xin Long (3):
  sctp: sctp_sock_migrate() returns error if sctp_bind_addr_dup() fails
  sctp: move up sctp_auth_init_hmacs() in sctp_endpoint_init()
  sctp: call sctp_auth_init_hmacs() in sctp_sock_migrate()

 net/sctp/auth.c        |  6 ------
 net/sctp/endpointola.c | 18 ++++++++++--------
 net/sctp/socket.c      | 44 ++++++++++++++++++++++++++++++++++----------
 3 files changed, 44 insertions(+), 24 deletions(-)

-- 
2.1.0

^ permalink raw reply	[flat|nested] 34+ messages in thread

* [PATCH net 1/3] sctp: sctp_sock_migrate() returns error if sctp_bind_addr_dup() fails
  2019-03-03  9:54 ` Xin Long
@ 2019-03-03  9:54   ` Xin Long
  -1 siblings, 0 replies; 34+ messages in thread
From: Xin Long @ 2019-03-03  9:54 UTC (permalink / raw)
  To: network dev, linux-sctp; +Cc: davem, Marcelo Ricardo Leitner, Neil Horman

It should fail to create the new sk if sctp_bind_addr_dup() fails
when accepting or peeloff an association.

Signed-off-by: Xin Long <lucien.xin@gmail.com>
---
 net/sctp/socket.c | 34 ++++++++++++++++++++++++----------
 1 file changed, 24 insertions(+), 10 deletions(-)

diff --git a/net/sctp/socket.c b/net/sctp/socket.c
index a2771b3..22adb8d 100644
--- a/net/sctp/socket.c
+++ b/net/sctp/socket.c
@@ -102,9 +102,9 @@ static int sctp_send_asconf(struct sctp_association *asoc,
 			    struct sctp_chunk *chunk);
 static int sctp_do_bind(struct sock *, union sctp_addr *, int);
 static int sctp_autobind(struct sock *sk);
-static void sctp_sock_migrate(struct sock *oldsk, struct sock *newsk,
-			      struct sctp_association *assoc,
-			      enum sctp_socket_type type);
+static int sctp_sock_migrate(struct sock *oldsk, struct sock *newsk,
+			     struct sctp_association *assoc,
+			     enum sctp_socket_type type);
 
 static unsigned long sctp_memory_pressure;
 static atomic_long_t sctp_memory_allocated;
@@ -4655,7 +4655,11 @@ static struct sock *sctp_accept(struct sock *sk, int flags, int *err, bool kern)
 	/* Populate the fields of the newsk from the oldsk and migrate the
 	 * asoc to the newsk.
 	 */
-	sctp_sock_migrate(sk, newsk, asoc, SCTP_SOCKET_TCP);
+	error = sctp_sock_migrate(sk, newsk, asoc, SCTP_SOCKET_TCP);
+	if (error) {
+		sk_common_release(newsk);
+		newsk = NULL;
+	}
 
 out:
 	release_sock(sk);
@@ -5401,7 +5405,12 @@ int sctp_do_peeloff(struct sock *sk, sctp_assoc_t id, struct socket **sockp)
 	/* Populate the fields of the newsk from the oldsk and migrate the
 	 * asoc to the newsk.
 	 */
-	sctp_sock_migrate(sk, sock->sk, asoc, SCTP_SOCKET_UDP_HIGH_BANDWIDTH);
+	err = sctp_sock_migrate(sk, sock->sk, asoc,
+				SCTP_SOCKET_UDP_HIGH_BANDWIDTH);
+	if (err) {
+		sock_release(sock);
+		sock = NULL;
+	}
 
 	*sockp = sock;
 
@@ -8924,9 +8933,9 @@ static inline void sctp_copy_descendant(struct sock *sk_to,
 /* Populate the fields of the newsk from the oldsk and migrate the assoc
  * and its messages to the newsk.
  */
-static void sctp_sock_migrate(struct sock *oldsk, struct sock *newsk,
-			      struct sctp_association *assoc,
-			      enum sctp_socket_type type)
+static int sctp_sock_migrate(struct sock *oldsk, struct sock *newsk,
+			     struct sctp_association *assoc,
+			     enum sctp_socket_type type)
 {
 	struct sctp_sock *oldsp = sctp_sk(oldsk);
 	struct sctp_sock *newsp = sctp_sk(newsk);
@@ -8935,6 +8944,7 @@ static void sctp_sock_migrate(struct sock *oldsk, struct sock *newsk,
 	struct sk_buff *skb, *tmp;
 	struct sctp_ulpevent *event;
 	struct sctp_bind_hashbucket *head;
+	int err;
 
 	/* Migrate socket buffer sizes and all the socket level options to the
 	 * new socket.
@@ -8963,8 +8973,10 @@ static void sctp_sock_migrate(struct sock *oldsk, struct sock *newsk,
 	/* Copy the bind_addr list from the original endpoint to the new
 	 * endpoint so that we can handle restarts properly
 	 */
-	sctp_bind_addr_dup(&newsp->ep->base.bind_addr,
-				&oldsp->ep->base.bind_addr, GFP_KERNEL);
+	err = sctp_bind_addr_dup(&newsp->ep->base.bind_addr,
+				 &oldsp->ep->base.bind_addr, GFP_KERNEL);
+	if (err)
+		return err;
 
 	/* Move any messages in the old socket's receive queue that are for the
 	 * peeled off association to the new socket's receive queue.
@@ -9049,6 +9061,8 @@ static void sctp_sock_migrate(struct sock *oldsk, struct sock *newsk,
 	}
 
 	release_sock(newsk);
+
+	return 0;
 }
 
 
-- 
2.1.0


^ permalink raw reply related	[flat|nested] 34+ messages in thread

* [PATCH net 1/3] sctp: sctp_sock_migrate() returns error if sctp_bind_addr_dup() fails
@ 2019-03-03  9:54   ` Xin Long
  0 siblings, 0 replies; 34+ messages in thread
From: Xin Long @ 2019-03-03  9:54 UTC (permalink / raw)
  To: network dev, linux-sctp; +Cc: davem, Marcelo Ricardo Leitner, Neil Horman

It should fail to create the new sk if sctp_bind_addr_dup() fails
when accepting or peeloff an association.

Signed-off-by: Xin Long <lucien.xin@gmail.com>
---
 net/sctp/socket.c | 34 ++++++++++++++++++++++++----------
 1 file changed, 24 insertions(+), 10 deletions(-)

diff --git a/net/sctp/socket.c b/net/sctp/socket.c
index a2771b3..22adb8d 100644
--- a/net/sctp/socket.c
+++ b/net/sctp/socket.c
@@ -102,9 +102,9 @@ static int sctp_send_asconf(struct sctp_association *asoc,
 			    struct sctp_chunk *chunk);
 static int sctp_do_bind(struct sock *, union sctp_addr *, int);
 static int sctp_autobind(struct sock *sk);
-static void sctp_sock_migrate(struct sock *oldsk, struct sock *newsk,
-			      struct sctp_association *assoc,
-			      enum sctp_socket_type type);
+static int sctp_sock_migrate(struct sock *oldsk, struct sock *newsk,
+			     struct sctp_association *assoc,
+			     enum sctp_socket_type type);
 
 static unsigned long sctp_memory_pressure;
 static atomic_long_t sctp_memory_allocated;
@@ -4655,7 +4655,11 @@ static struct sock *sctp_accept(struct sock *sk, int flags, int *err, bool kern)
 	/* Populate the fields of the newsk from the oldsk and migrate the
 	 * asoc to the newsk.
 	 */
-	sctp_sock_migrate(sk, newsk, asoc, SCTP_SOCKET_TCP);
+	error = sctp_sock_migrate(sk, newsk, asoc, SCTP_SOCKET_TCP);
+	if (error) {
+		sk_common_release(newsk);
+		newsk = NULL;
+	}
 
 out:
 	release_sock(sk);
@@ -5401,7 +5405,12 @@ int sctp_do_peeloff(struct sock *sk, sctp_assoc_t id, struct socket **sockp)
 	/* Populate the fields of the newsk from the oldsk and migrate the
 	 * asoc to the newsk.
 	 */
-	sctp_sock_migrate(sk, sock->sk, asoc, SCTP_SOCKET_UDP_HIGH_BANDWIDTH);
+	err = sctp_sock_migrate(sk, sock->sk, asoc,
+				SCTP_SOCKET_UDP_HIGH_BANDWIDTH);
+	if (err) {
+		sock_release(sock);
+		sock = NULL;
+	}
 
 	*sockp = sock;
 
@@ -8924,9 +8933,9 @@ static inline void sctp_copy_descendant(struct sock *sk_to,
 /* Populate the fields of the newsk from the oldsk and migrate the assoc
  * and its messages to the newsk.
  */
-static void sctp_sock_migrate(struct sock *oldsk, struct sock *newsk,
-			      struct sctp_association *assoc,
-			      enum sctp_socket_type type)
+static int sctp_sock_migrate(struct sock *oldsk, struct sock *newsk,
+			     struct sctp_association *assoc,
+			     enum sctp_socket_type type)
 {
 	struct sctp_sock *oldsp = sctp_sk(oldsk);
 	struct sctp_sock *newsp = sctp_sk(newsk);
@@ -8935,6 +8944,7 @@ static void sctp_sock_migrate(struct sock *oldsk, struct sock *newsk,
 	struct sk_buff *skb, *tmp;
 	struct sctp_ulpevent *event;
 	struct sctp_bind_hashbucket *head;
+	int err;
 
 	/* Migrate socket buffer sizes and all the socket level options to the
 	 * new socket.
@@ -8963,8 +8973,10 @@ static void sctp_sock_migrate(struct sock *oldsk, struct sock *newsk,
 	/* Copy the bind_addr list from the original endpoint to the new
 	 * endpoint so that we can handle restarts properly
 	 */
-	sctp_bind_addr_dup(&newsp->ep->base.bind_addr,
-				&oldsp->ep->base.bind_addr, GFP_KERNEL);
+	err = sctp_bind_addr_dup(&newsp->ep->base.bind_addr,
+				 &oldsp->ep->base.bind_addr, GFP_KERNEL);
+	if (err)
+		return err;
 
 	/* Move any messages in the old socket's receive queue that are for the
 	 * peeled off association to the new socket's receive queue.
@@ -9049,6 +9061,8 @@ static void sctp_sock_migrate(struct sock *oldsk, struct sock *newsk,
 	}
 
 	release_sock(newsk);
+
+	return 0;
 }
 
 
-- 
2.1.0

^ permalink raw reply related	[flat|nested] 34+ messages in thread

* [PATCH net 2/3] sctp: move up sctp_auth_init_hmacs() in sctp_endpoint_init()
  2019-03-03  9:54   ` Xin Long
@ 2019-03-03  9:54     ` Xin Long
  -1 siblings, 0 replies; 34+ messages in thread
From: Xin Long @ 2019-03-03  9:54 UTC (permalink / raw)
  To: network dev, linux-sctp; +Cc: davem, Marcelo Ricardo Leitner, Neil Horman

sctp_auth_init_hmacs() is called only when ep->auth_enable is set.
It better to move up sctp_auth_init_hmacs() and remove auth_enable
check in it and check auth_enable only once in sctp_endpoint_init().

Signed-off-by: Xin Long <lucien.xin@gmail.com>
---
 net/sctp/auth.c        |  6 ------
 net/sctp/endpointola.c | 18 ++++++++++--------
 2 files changed, 10 insertions(+), 14 deletions(-)

diff --git a/net/sctp/auth.c b/net/sctp/auth.c
index 5b53761..39d72e5 100644
--- a/net/sctp/auth.c
+++ b/net/sctp/auth.c
@@ -471,12 +471,6 @@ int sctp_auth_init_hmacs(struct sctp_endpoint *ep, gfp_t gfp)
 	struct crypto_shash *tfm = NULL;
 	__u16   id;
 
-	/* If AUTH extension is disabled, we are done */
-	if (!ep->auth_enable) {
-		ep->auth_hmacs = NULL;
-		return 0;
-	}
-
 	/* If the transforms are already allocated, we are done */
 	if (ep->auth_hmacs)
 		return 0;
diff --git a/net/sctp/endpointola.c b/net/sctp/endpointola.c
index 40c7eb9..0448b68 100644
--- a/net/sctp/endpointola.c
+++ b/net/sctp/endpointola.c
@@ -107,6 +107,13 @@ static struct sctp_endpoint *sctp_endpoint_init(struct sctp_endpoint *ep,
 			auth_chunks->param_hdr.length =
 					htons(sizeof(struct sctp_paramhdr) + 2);
 		}
+
+		/* Allocate and initialize transorms arrays for supported
+		 * HMACs.
+		 */
+		err = sctp_auth_init_hmacs(ep, gfp);
+		if (err)
+			goto nomem;
 	}
 
 	/* Initialize the base structure. */
@@ -150,15 +157,10 @@ static struct sctp_endpoint *sctp_endpoint_init(struct sctp_endpoint *ep,
 	INIT_LIST_HEAD(&ep->endpoint_shared_keys);
 	null_key = sctp_auth_shkey_create(0, gfp);
 	if (!null_key)
-		goto nomem;
+		goto nomem_shkey;
 
 	list_add(&null_key->key_list, &ep->endpoint_shared_keys);
 
-	/* Allocate and initialize transorms arrays for supported HMACs. */
-	err = sctp_auth_init_hmacs(ep, gfp);
-	if (err)
-		goto nomem_hmacs;
-
 	/* Add the null key to the endpoint shared keys list and
 	 * set the hmcas and chunks pointers.
 	 */
@@ -169,8 +171,8 @@ static struct sctp_endpoint *sctp_endpoint_init(struct sctp_endpoint *ep,
 
 	return ep;
 
-nomem_hmacs:
-	sctp_auth_destroy_keys(&ep->endpoint_shared_keys);
+nomem_shkey:
+	sctp_auth_destroy_hmacs(ep->auth_hmacs);
 nomem:
 	/* Free all allocations */
 	kfree(auth_hmacs);
-- 
2.1.0


^ permalink raw reply related	[flat|nested] 34+ messages in thread

* [PATCH net 2/3] sctp: move up sctp_auth_init_hmacs() in sctp_endpoint_init()
@ 2019-03-03  9:54     ` Xin Long
  0 siblings, 0 replies; 34+ messages in thread
From: Xin Long @ 2019-03-03  9:54 UTC (permalink / raw)
  To: network dev, linux-sctp; +Cc: davem, Marcelo Ricardo Leitner, Neil Horman

sctp_auth_init_hmacs() is called only when ep->auth_enable is set.
It better to move up sctp_auth_init_hmacs() and remove auth_enable
check in it and check auth_enable only once in sctp_endpoint_init().

Signed-off-by: Xin Long <lucien.xin@gmail.com>
---
 net/sctp/auth.c        |  6 ------
 net/sctp/endpointola.c | 18 ++++++++++--------
 2 files changed, 10 insertions(+), 14 deletions(-)

diff --git a/net/sctp/auth.c b/net/sctp/auth.c
index 5b53761..39d72e5 100644
--- a/net/sctp/auth.c
+++ b/net/sctp/auth.c
@@ -471,12 +471,6 @@ int sctp_auth_init_hmacs(struct sctp_endpoint *ep, gfp_t gfp)
 	struct crypto_shash *tfm = NULL;
 	__u16   id;
 
-	/* If AUTH extension is disabled, we are done */
-	if (!ep->auth_enable) {
-		ep->auth_hmacs = NULL;
-		return 0;
-	}
-
 	/* If the transforms are already allocated, we are done */
 	if (ep->auth_hmacs)
 		return 0;
diff --git a/net/sctp/endpointola.c b/net/sctp/endpointola.c
index 40c7eb9..0448b68 100644
--- a/net/sctp/endpointola.c
+++ b/net/sctp/endpointola.c
@@ -107,6 +107,13 @@ static struct sctp_endpoint *sctp_endpoint_init(struct sctp_endpoint *ep,
 			auth_chunks->param_hdr.length  					htons(sizeof(struct sctp_paramhdr) + 2);
 		}
+
+		/* Allocate and initialize transorms arrays for supported
+		 * HMACs.
+		 */
+		err = sctp_auth_init_hmacs(ep, gfp);
+		if (err)
+			goto nomem;
 	}
 
 	/* Initialize the base structure. */
@@ -150,15 +157,10 @@ static struct sctp_endpoint *sctp_endpoint_init(struct sctp_endpoint *ep,
 	INIT_LIST_HEAD(&ep->endpoint_shared_keys);
 	null_key = sctp_auth_shkey_create(0, gfp);
 	if (!null_key)
-		goto nomem;
+		goto nomem_shkey;
 
 	list_add(&null_key->key_list, &ep->endpoint_shared_keys);
 
-	/* Allocate and initialize transorms arrays for supported HMACs. */
-	err = sctp_auth_init_hmacs(ep, gfp);
-	if (err)
-		goto nomem_hmacs;
-
 	/* Add the null key to the endpoint shared keys list and
 	 * set the hmcas and chunks pointers.
 	 */
@@ -169,8 +171,8 @@ static struct sctp_endpoint *sctp_endpoint_init(struct sctp_endpoint *ep,
 
 	return ep;
 
-nomem_hmacs:
-	sctp_auth_destroy_keys(&ep->endpoint_shared_keys);
+nomem_shkey:
+	sctp_auth_destroy_hmacs(ep->auth_hmacs);
 nomem:
 	/* Free all allocations */
 	kfree(auth_hmacs);
-- 
2.1.0

^ permalink raw reply related	[flat|nested] 34+ messages in thread

* [PATCH net 3/3] sctp: call sctp_auth_init_hmacs() in sctp_sock_migrate()
  2019-03-03  9:54     ` Xin Long
@ 2019-03-03  9:54       ` Xin Long
  -1 siblings, 0 replies; 34+ messages in thread
From: Xin Long @ 2019-03-03  9:54 UTC (permalink / raw)
  To: network dev, linux-sctp; +Cc: davem, Marcelo Ricardo Leitner, Neil Horman

New ep's auth_hmacs should be set if old ep's is set, in case that
net->sctp.auth_enable has been changed to 0 by users and new ep's
auth_hmacs couldn't be set in sctp_endpoint_init().

It can even crash kernel by doing:

  1. on server: sysctl -w net.sctp.auth_enable=1,
                sysctl -w net.sctp.addip_enable=1,
                sysctl -w net.sctp.addip_noauth_enable=0,
                listen() on server,
                sysctl -w net.sctp.auth_enable=0.
  2. on client: connect() to server.
  3. on server: accept() the asoc,
                sysctl -w net.sctp.auth_enable=1.
  4. on client: send() asconf packet to server.

The call trace:

  [  245.280251] BUG: unable to handle kernel NULL pointer dereference at 0000000000000008
  [  245.286872] RIP: 0010:sctp_auth_calculate_hmac+0xa3/0x140 [sctp]
  [  245.304572] Call Trace:
  [  245.305091]  <IRQ>
  [  245.311287]  sctp_sf_authenticate+0x110/0x160 [sctp]
  [  245.312311]  sctp_sf_eat_auth+0xf2/0x230 [sctp]
  [  245.313249]  sctp_do_sm+0x9a/0x2d0 [sctp]
  [  245.321483]  sctp_assoc_bh_rcv+0xed/0x1a0 [sctp]
  [  245.322495]  sctp_rcv+0xa66/0xc70 [sctp]

It's because the old ep->auth_hmacs wasn't copied to the new ep while
ep->auth_hmacs is used in sctp_auth_calculate_hmac() when processing
the incoming auth chunks, and it should have been done when migrating
sock.

Reported-by: Ying Xu <yinxu@redhat.com>
Signed-off-by: Xin Long <lucien.xin@gmail.com>
---
 net/sctp/socket.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/net/sctp/socket.c b/net/sctp/socket.c
index 22adb8d..def3335 100644
--- a/net/sctp/socket.c
+++ b/net/sctp/socket.c
@@ -8978,6 +8978,16 @@ static int sctp_sock_migrate(struct sock *oldsk, struct sock *newsk,
 	if (err)
 		return err;
 
+	/* New ep's auth_hmacs should be set if old ep's is set, in case
+	 * that net->sctp.auth_enable has been changed to 0 by users and
+	 * new ep's auth_hmacs couldn't be set in sctp_endpoint_init().
+	 */
+	if (oldsp->ep->auth_hmacs) {
+		err = sctp_auth_init_hmacs(newsp->ep, GFP_KERNEL);
+		if (err)
+			return err;
+	}
+
 	/* Move any messages in the old socket's receive queue that are for the
 	 * peeled off association to the new socket's receive queue.
 	 */
-- 
2.1.0


^ permalink raw reply related	[flat|nested] 34+ messages in thread

* [PATCH net 3/3] sctp: call sctp_auth_init_hmacs() in sctp_sock_migrate()
@ 2019-03-03  9:54       ` Xin Long
  0 siblings, 0 replies; 34+ messages in thread
From: Xin Long @ 2019-03-03  9:54 UTC (permalink / raw)
  To: network dev, linux-sctp; +Cc: davem, Marcelo Ricardo Leitner, Neil Horman

New ep's auth_hmacs should be set if old ep's is set, in case that
net->sctp.auth_enable has been changed to 0 by users and new ep's
auth_hmacs couldn't be set in sctp_endpoint_init().

It can even crash kernel by doing:

  1. on server: sysctl -w net.sctp.auth_enable=1,
                sysctl -w net.sctp.addip_enable=1,
                sysctl -w net.sctp.addip_noauth_enable=0,
                listen() on server,
                sysctl -w net.sctp.auth_enable=0.
  2. on client: connect() to server.
  3. on server: accept() the asoc,
                sysctl -w net.sctp.auth_enable=1.
  4. on client: send() asconf packet to server.

The call trace:

  [  245.280251] BUG: unable to handle kernel NULL pointer dereference at 0000000000000008
  [  245.286872] RIP: 0010:sctp_auth_calculate_hmac+0xa3/0x140 [sctp]
  [  245.304572] Call Trace:
  [  245.305091]  <IRQ>
  [  245.311287]  sctp_sf_authenticate+0x110/0x160 [sctp]
  [  245.312311]  sctp_sf_eat_auth+0xf2/0x230 [sctp]
  [  245.313249]  sctp_do_sm+0x9a/0x2d0 [sctp]
  [  245.321483]  sctp_assoc_bh_rcv+0xed/0x1a0 [sctp]
  [  245.322495]  sctp_rcv+0xa66/0xc70 [sctp]

It's because the old ep->auth_hmacs wasn't copied to the new ep while
ep->auth_hmacs is used in sctp_auth_calculate_hmac() when processing
the incoming auth chunks, and it should have been done when migrating
sock.

Reported-by: Ying Xu <yinxu@redhat.com>
Signed-off-by: Xin Long <lucien.xin@gmail.com>
---
 net/sctp/socket.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/net/sctp/socket.c b/net/sctp/socket.c
index 22adb8d..def3335 100644
--- a/net/sctp/socket.c
+++ b/net/sctp/socket.c
@@ -8978,6 +8978,16 @@ static int sctp_sock_migrate(struct sock *oldsk, struct sock *newsk,
 	if (err)
 		return err;
 
+	/* New ep's auth_hmacs should be set if old ep's is set, in case
+	 * that net->sctp.auth_enable has been changed to 0 by users and
+	 * new ep's auth_hmacs couldn't be set in sctp_endpoint_init().
+	 */
+	if (oldsp->ep->auth_hmacs) {
+		err = sctp_auth_init_hmacs(newsp->ep, GFP_KERNEL);
+		if (err)
+			return err;
+	}
+
 	/* Move any messages in the old socket's receive queue that are for the
 	 * peeled off association to the new socket's receive queue.
 	 */
-- 
2.1.0

^ permalink raw reply related	[flat|nested] 34+ messages in thread

* Re: [PATCH net 0/3] sctp: process the error returned from sctp_sock_migrate()
  2019-03-03  9:54 ` Xin Long
@ 2019-03-04 19:04   ` David Miller
  -1 siblings, 0 replies; 34+ messages in thread
From: David Miller @ 2019-03-04 19:04 UTC (permalink / raw)
  To: lucien.xin; +Cc: netdev, linux-sctp, marcelo.leitner, nhorman

From: Xin Long <lucien.xin@gmail.com>
Date: Sun,  3 Mar 2019 17:54:52 +0800

> This patchset is to process the errs returned by sctp_auth_init_hmacs()
> and sctp_bind_addr_dup() from sctp_sock_migrate(). And also fix a panic
> caused by new ep->auth_hmacs was not set due to net->sctp.auth_enable
> changed by sysctl before accepting an assoc.

I'll give Neil and Marcelo a change to review this.

^ permalink raw reply	[flat|nested] 34+ messages in thread

* Re: [PATCH net 0/3] sctp: process the error returned from sctp_sock_migrate()
@ 2019-03-04 19:04   ` David Miller
  0 siblings, 0 replies; 34+ messages in thread
From: David Miller @ 2019-03-04 19:04 UTC (permalink / raw)
  To: lucien.xin; +Cc: netdev, linux-sctp, marcelo.leitner, nhorman

From: Xin Long <lucien.xin@gmail.com>
Date: Sun,  3 Mar 2019 17:54:52 +0800

> This patchset is to process the errs returned by sctp_auth_init_hmacs()
> and sctp_bind_addr_dup() from sctp_sock_migrate(). And also fix a panic
> caused by new ep->auth_hmacs was not set due to net->sctp.auth_enable
> changed by sysctl before accepting an assoc.

I'll give Neil and Marcelo a change to review this.

^ permalink raw reply	[flat|nested] 34+ messages in thread

* Re: [PATCH net 1/3] sctp: sctp_sock_migrate() returns error if sctp_bind_addr_dup() fails
  2019-03-03  9:54   ` Xin Long
@ 2019-03-06 18:21     ` Neil Horman
  -1 siblings, 0 replies; 34+ messages in thread
From: Neil Horman @ 2019-03-06 18:21 UTC (permalink / raw)
  To: Xin Long; +Cc: network dev, linux-sctp, davem, Marcelo Ricardo Leitner

On Sun, Mar 03, 2019 at 05:54:53PM +0800, Xin Long wrote:
> It should fail to create the new sk if sctp_bind_addr_dup() fails
> when accepting or peeloff an association.
> 
> Signed-off-by: Xin Long <lucien.xin@gmail.com>
> ---
>  net/sctp/socket.c | 34 ++++++++++++++++++++++++----------
>  1 file changed, 24 insertions(+), 10 deletions(-)
> 
> diff --git a/net/sctp/socket.c b/net/sctp/socket.c
> index a2771b3..22adb8d 100644
> --- a/net/sctp/socket.c
> +++ b/net/sctp/socket.c
> @@ -102,9 +102,9 @@ static int sctp_send_asconf(struct sctp_association *asoc,
>  			    struct sctp_chunk *chunk);
>  static int sctp_do_bind(struct sock *, union sctp_addr *, int);
>  static int sctp_autobind(struct sock *sk);
> -static void sctp_sock_migrate(struct sock *oldsk, struct sock *newsk,
> -			      struct sctp_association *assoc,
> -			      enum sctp_socket_type type);
> +static int sctp_sock_migrate(struct sock *oldsk, struct sock *newsk,
> +			     struct sctp_association *assoc,
> +			     enum sctp_socket_type type);
>  
>  static unsigned long sctp_memory_pressure;
>  static atomic_long_t sctp_memory_allocated;
> @@ -4655,7 +4655,11 @@ static struct sock *sctp_accept(struct sock *sk, int flags, int *err, bool kern)
>  	/* Populate the fields of the newsk from the oldsk and migrate the
>  	 * asoc to the newsk.
>  	 */
> -	sctp_sock_migrate(sk, newsk, asoc, SCTP_SOCKET_TCP);
> +	error = sctp_sock_migrate(sk, newsk, asoc, SCTP_SOCKET_TCP);
> +	if (error) {
> +		sk_common_release(newsk);
sctp_sock_migrate may fail after the pending packets have been moved from the
old socket to the new socket.  Normally those packets will get purged by
successful transmission, or when the socket is closed (via sctp_close), but
neither of those cases applies here.  Whats going to dequeue and free any
pending skbs on the sk_receive_queue here?

> +		newsk = NULL;
> +	}
>  
>  out:
>  	release_sock(sk);
> @@ -5401,7 +5405,12 @@ int sctp_do_peeloff(struct sock *sk, sctp_assoc_t id, struct socket **sockp)
>  	/* Populate the fields of the newsk from the oldsk and migrate the
>  	 * asoc to the newsk.
>  	 */
> -	sctp_sock_migrate(sk, sock->sk, asoc, SCTP_SOCKET_UDP_HIGH_BANDWIDTH);
> +	err = sctp_sock_migrate(sk, sock->sk, asoc,
> +				SCTP_SOCKET_UDP_HIGH_BANDWIDTH);
> +	if (err) {
> +		sock_release(sock);
Same question here, what frees any pending skbs on the new socket, if the
migration fails after the skbs have been queued to it?

> +		sock = NULL;
> +	}
>  
>  	*sockp = sock;
>  
> @@ -8924,9 +8933,9 @@ static inline void sctp_copy_descendant(struct sock *sk_to,
>  /* Populate the fields of the newsk from the oldsk and migrate the assoc
>   * and its messages to the newsk.
>   */
> -static void sctp_sock_migrate(struct sock *oldsk, struct sock *newsk,
> -			      struct sctp_association *assoc,
> -			      enum sctp_socket_type type)
> +static int sctp_sock_migrate(struct sock *oldsk, struct sock *newsk,
> +			     struct sctp_association *assoc,
> +			     enum sctp_socket_type type)
>  {
>  	struct sctp_sock *oldsp = sctp_sk(oldsk);
>  	struct sctp_sock *newsp = sctp_sk(newsk);
> @@ -8935,6 +8944,7 @@ static void sctp_sock_migrate(struct sock *oldsk, struct sock *newsk,
>  	struct sk_buff *skb, *tmp;
>  	struct sctp_ulpevent *event;
>  	struct sctp_bind_hashbucket *head;
> +	int err;
>  
>  	/* Migrate socket buffer sizes and all the socket level options to the
>  	 * new socket.
> @@ -8963,8 +8973,10 @@ static void sctp_sock_migrate(struct sock *oldsk, struct sock *newsk,
>  	/* Copy the bind_addr list from the original endpoint to the new
>  	 * endpoint so that we can handle restarts properly
>  	 */
> -	sctp_bind_addr_dup(&newsp->ep->base.bind_addr,
> -				&oldsp->ep->base.bind_addr, GFP_KERNEL);
> +	err = sctp_bind_addr_dup(&newsp->ep->base.bind_addr,
> +				 &oldsp->ep->base.bind_addr, GFP_KERNEL);
> +	if (err)
> +		return err;
>  
>  	/* Move any messages in the old socket's receive queue that are for the
>  	 * peeled off association to the new socket's receive queue.
> @@ -9049,6 +9061,8 @@ static void sctp_sock_migrate(struct sock *oldsk, struct sock *newsk,
>  	}
>  
>  	release_sock(newsk);
> +
> +	return 0;
>  }
>  
>  
> -- 
> 2.1.0
> 
> 

^ permalink raw reply	[flat|nested] 34+ messages in thread

* Re: [PATCH net 1/3] sctp: sctp_sock_migrate() returns error if sctp_bind_addr_dup() fails
@ 2019-03-06 18:21     ` Neil Horman
  0 siblings, 0 replies; 34+ messages in thread
From: Neil Horman @ 2019-03-06 18:21 UTC (permalink / raw)
  To: Xin Long; +Cc: network dev, linux-sctp, davem, Marcelo Ricardo Leitner

On Sun, Mar 03, 2019 at 05:54:53PM +0800, Xin Long wrote:
> It should fail to create the new sk if sctp_bind_addr_dup() fails
> when accepting or peeloff an association.
> 
> Signed-off-by: Xin Long <lucien.xin@gmail.com>
> ---
>  net/sctp/socket.c | 34 ++++++++++++++++++++++++----------
>  1 file changed, 24 insertions(+), 10 deletions(-)
> 
> diff --git a/net/sctp/socket.c b/net/sctp/socket.c
> index a2771b3..22adb8d 100644
> --- a/net/sctp/socket.c
> +++ b/net/sctp/socket.c
> @@ -102,9 +102,9 @@ static int sctp_send_asconf(struct sctp_association *asoc,
>  			    struct sctp_chunk *chunk);
>  static int sctp_do_bind(struct sock *, union sctp_addr *, int);
>  static int sctp_autobind(struct sock *sk);
> -static void sctp_sock_migrate(struct sock *oldsk, struct sock *newsk,
> -			      struct sctp_association *assoc,
> -			      enum sctp_socket_type type);
> +static int sctp_sock_migrate(struct sock *oldsk, struct sock *newsk,
> +			     struct sctp_association *assoc,
> +			     enum sctp_socket_type type);
>  
>  static unsigned long sctp_memory_pressure;
>  static atomic_long_t sctp_memory_allocated;
> @@ -4655,7 +4655,11 @@ static struct sock *sctp_accept(struct sock *sk, int flags, int *err, bool kern)
>  	/* Populate the fields of the newsk from the oldsk and migrate the
>  	 * asoc to the newsk.
>  	 */
> -	sctp_sock_migrate(sk, newsk, asoc, SCTP_SOCKET_TCP);
> +	error = sctp_sock_migrate(sk, newsk, asoc, SCTP_SOCKET_TCP);
> +	if (error) {
> +		sk_common_release(newsk);
sctp_sock_migrate may fail after the pending packets have been moved from the
old socket to the new socket.  Normally those packets will get purged by
successful transmission, or when the socket is closed (via sctp_close), but
neither of those cases applies here.  Whats going to dequeue and free any
pending skbs on the sk_receive_queue here?

> +		newsk = NULL;
> +	}
>  
>  out:
>  	release_sock(sk);
> @@ -5401,7 +5405,12 @@ int sctp_do_peeloff(struct sock *sk, sctp_assoc_t id, struct socket **sockp)
>  	/* Populate the fields of the newsk from the oldsk and migrate the
>  	 * asoc to the newsk.
>  	 */
> -	sctp_sock_migrate(sk, sock->sk, asoc, SCTP_SOCKET_UDP_HIGH_BANDWIDTH);
> +	err = sctp_sock_migrate(sk, sock->sk, asoc,
> +				SCTP_SOCKET_UDP_HIGH_BANDWIDTH);
> +	if (err) {
> +		sock_release(sock);
Same question here, what frees any pending skbs on the new socket, if the
migration fails after the skbs have been queued to it?

> +		sock = NULL;
> +	}
>  
>  	*sockp = sock;
>  
> @@ -8924,9 +8933,9 @@ static inline void sctp_copy_descendant(struct sock *sk_to,
>  /* Populate the fields of the newsk from the oldsk and migrate the assoc
>   * and its messages to the newsk.
>   */
> -static void sctp_sock_migrate(struct sock *oldsk, struct sock *newsk,
> -			      struct sctp_association *assoc,
> -			      enum sctp_socket_type type)
> +static int sctp_sock_migrate(struct sock *oldsk, struct sock *newsk,
> +			     struct sctp_association *assoc,
> +			     enum sctp_socket_type type)
>  {
>  	struct sctp_sock *oldsp = sctp_sk(oldsk);
>  	struct sctp_sock *newsp = sctp_sk(newsk);
> @@ -8935,6 +8944,7 @@ static void sctp_sock_migrate(struct sock *oldsk, struct sock *newsk,
>  	struct sk_buff *skb, *tmp;
>  	struct sctp_ulpevent *event;
>  	struct sctp_bind_hashbucket *head;
> +	int err;
>  
>  	/* Migrate socket buffer sizes and all the socket level options to the
>  	 * new socket.
> @@ -8963,8 +8973,10 @@ static void sctp_sock_migrate(struct sock *oldsk, struct sock *newsk,
>  	/* Copy the bind_addr list from the original endpoint to the new
>  	 * endpoint so that we can handle restarts properly
>  	 */
> -	sctp_bind_addr_dup(&newsp->ep->base.bind_addr,
> -				&oldsp->ep->base.bind_addr, GFP_KERNEL);
> +	err = sctp_bind_addr_dup(&newsp->ep->base.bind_addr,
> +				 &oldsp->ep->base.bind_addr, GFP_KERNEL);
> +	if (err)
> +		return err;
>  
>  	/* Move any messages in the old socket's receive queue that are for the
>  	 * peeled off association to the new socket's receive queue.
> @@ -9049,6 +9061,8 @@ static void sctp_sock_migrate(struct sock *oldsk, struct sock *newsk,
>  	}
>  
>  	release_sock(newsk);
> +
> +	return 0;
>  }
>  
>  
> -- 
> 2.1.0
> 
> 

^ permalink raw reply	[flat|nested] 34+ messages in thread

* Re: [PATCH net 2/3] sctp: move up sctp_auth_init_hmacs() in sctp_endpoint_init()
  2019-03-03  9:54     ` Xin Long
@ 2019-03-06 18:24       ` Neil Horman
  -1 siblings, 0 replies; 34+ messages in thread
From: Neil Horman @ 2019-03-06 18:24 UTC (permalink / raw)
  To: Xin Long; +Cc: network dev, linux-sctp, davem, Marcelo Ricardo Leitner

On Sun, Mar 03, 2019 at 05:54:54PM +0800, Xin Long wrote:
> sctp_auth_init_hmacs() is called only when ep->auth_enable is set.
> It better to move up sctp_auth_init_hmacs() and remove auth_enable
> check in it and check auth_enable only once in sctp_endpoint_init().
> 
> Signed-off-by: Xin Long <lucien.xin@gmail.com>
> ---
>  net/sctp/auth.c        |  6 ------
>  net/sctp/endpointola.c | 18 ++++++++++--------
>  2 files changed, 10 insertions(+), 14 deletions(-)
> 
> diff --git a/net/sctp/auth.c b/net/sctp/auth.c
> index 5b53761..39d72e5 100644
> --- a/net/sctp/auth.c
> +++ b/net/sctp/auth.c
> @@ -471,12 +471,6 @@ int sctp_auth_init_hmacs(struct sctp_endpoint *ep, gfp_t gfp)
>  	struct crypto_shash *tfm = NULL;
>  	__u16   id;
>  
> -	/* If AUTH extension is disabled, we are done */
> -	if (!ep->auth_enable) {
> -		ep->auth_hmacs = NULL;
> -		return 0;
> -	}
> -
>  	/* If the transforms are already allocated, we are done */
>  	if (ep->auth_hmacs)
>  		return 0;
> diff --git a/net/sctp/endpointola.c b/net/sctp/endpointola.c
> index 40c7eb9..0448b68 100644
> --- a/net/sctp/endpointola.c
> +++ b/net/sctp/endpointola.c
> @@ -107,6 +107,13 @@ static struct sctp_endpoint *sctp_endpoint_init(struct sctp_endpoint *ep,
>  			auth_chunks->param_hdr.length =
>  					htons(sizeof(struct sctp_paramhdr) + 2);
>  		}
> +
> +		/* Allocate and initialize transorms arrays for supported
> +		 * HMACs.
> +		 */
> +		err = sctp_auth_init_hmacs(ep, gfp);
> +		if (err)
> +			goto nomem;
>  	}
>  
>  	/* Initialize the base structure. */
> @@ -150,15 +157,10 @@ static struct sctp_endpoint *sctp_endpoint_init(struct sctp_endpoint *ep,
>  	INIT_LIST_HEAD(&ep->endpoint_shared_keys);
>  	null_key = sctp_auth_shkey_create(0, gfp);
>  	if (!null_key)
> -		goto nomem;
> +		goto nomem_shkey;
>  
>  	list_add(&null_key->key_list, &ep->endpoint_shared_keys);
>  
> -	/* Allocate and initialize transorms arrays for supported HMACs. */
> -	err = sctp_auth_init_hmacs(ep, gfp);
> -	if (err)
> -		goto nomem_hmacs;
> -
>  	/* Add the null key to the endpoint shared keys list and
>  	 * set the hmcas and chunks pointers.
>  	 */
> @@ -169,8 +171,8 @@ static struct sctp_endpoint *sctp_endpoint_init(struct sctp_endpoint *ep,
>  
>  	return ep;
>  
> -nomem_hmacs:
> -	sctp_auth_destroy_keys(&ep->endpoint_shared_keys);
> +nomem_shkey:
> +	sctp_auth_destroy_hmacs(ep->auth_hmacs);
>  nomem:
>  	/* Free all allocations */
>  	kfree(auth_hmacs);
> -- 
> 2.1.0
> 
> 
Acked-by: Neil Horman <nhorman@tuxdriver.com>


^ permalink raw reply	[flat|nested] 34+ messages in thread

* Re: [PATCH net 2/3] sctp: move up sctp_auth_init_hmacs() in sctp_endpoint_init()
@ 2019-03-06 18:24       ` Neil Horman
  0 siblings, 0 replies; 34+ messages in thread
From: Neil Horman @ 2019-03-06 18:24 UTC (permalink / raw)
  To: Xin Long; +Cc: network dev, linux-sctp, davem, Marcelo Ricardo Leitner

On Sun, Mar 03, 2019 at 05:54:54PM +0800, Xin Long wrote:
> sctp_auth_init_hmacs() is called only when ep->auth_enable is set.
> It better to move up sctp_auth_init_hmacs() and remove auth_enable
> check in it and check auth_enable only once in sctp_endpoint_init().
> 
> Signed-off-by: Xin Long <lucien.xin@gmail.com>
> ---
>  net/sctp/auth.c        |  6 ------
>  net/sctp/endpointola.c | 18 ++++++++++--------
>  2 files changed, 10 insertions(+), 14 deletions(-)
> 
> diff --git a/net/sctp/auth.c b/net/sctp/auth.c
> index 5b53761..39d72e5 100644
> --- a/net/sctp/auth.c
> +++ b/net/sctp/auth.c
> @@ -471,12 +471,6 @@ int sctp_auth_init_hmacs(struct sctp_endpoint *ep, gfp_t gfp)
>  	struct crypto_shash *tfm = NULL;
>  	__u16   id;
>  
> -	/* If AUTH extension is disabled, we are done */
> -	if (!ep->auth_enable) {
> -		ep->auth_hmacs = NULL;
> -		return 0;
> -	}
> -
>  	/* If the transforms are already allocated, we are done */
>  	if (ep->auth_hmacs)
>  		return 0;
> diff --git a/net/sctp/endpointola.c b/net/sctp/endpointola.c
> index 40c7eb9..0448b68 100644
> --- a/net/sctp/endpointola.c
> +++ b/net/sctp/endpointola.c
> @@ -107,6 +107,13 @@ static struct sctp_endpoint *sctp_endpoint_init(struct sctp_endpoint *ep,
>  			auth_chunks->param_hdr.length >  					htons(sizeof(struct sctp_paramhdr) + 2);
>  		}
> +
> +		/* Allocate and initialize transorms arrays for supported
> +		 * HMACs.
> +		 */
> +		err = sctp_auth_init_hmacs(ep, gfp);
> +		if (err)
> +			goto nomem;
>  	}
>  
>  	/* Initialize the base structure. */
> @@ -150,15 +157,10 @@ static struct sctp_endpoint *sctp_endpoint_init(struct sctp_endpoint *ep,
>  	INIT_LIST_HEAD(&ep->endpoint_shared_keys);
>  	null_key = sctp_auth_shkey_create(0, gfp);
>  	if (!null_key)
> -		goto nomem;
> +		goto nomem_shkey;
>  
>  	list_add(&null_key->key_list, &ep->endpoint_shared_keys);
>  
> -	/* Allocate and initialize transorms arrays for supported HMACs. */
> -	err = sctp_auth_init_hmacs(ep, gfp);
> -	if (err)
> -		goto nomem_hmacs;
> -
>  	/* Add the null key to the endpoint shared keys list and
>  	 * set the hmcas and chunks pointers.
>  	 */
> @@ -169,8 +171,8 @@ static struct sctp_endpoint *sctp_endpoint_init(struct sctp_endpoint *ep,
>  
>  	return ep;
>  
> -nomem_hmacs:
> -	sctp_auth_destroy_keys(&ep->endpoint_shared_keys);
> +nomem_shkey:
> +	sctp_auth_destroy_hmacs(ep->auth_hmacs);
>  nomem:
>  	/* Free all allocations */
>  	kfree(auth_hmacs);
> -- 
> 2.1.0
> 
> 
Acked-by: Neil Horman <nhorman@tuxdriver.com>

^ permalink raw reply	[flat|nested] 34+ messages in thread

* Re: [PATCH net 3/3] sctp: call sctp_auth_init_hmacs() in sctp_sock_migrate()
  2019-03-03  9:54       ` Xin Long
@ 2019-03-06 18:26         ` Neil Horman
  -1 siblings, 0 replies; 34+ messages in thread
From: Neil Horman @ 2019-03-06 18:26 UTC (permalink / raw)
  To: Xin Long; +Cc: network dev, linux-sctp, davem, Marcelo Ricardo Leitner

On Sun, Mar 03, 2019 at 05:54:55PM +0800, Xin Long wrote:
> New ep's auth_hmacs should be set if old ep's is set, in case that
> net->sctp.auth_enable has been changed to 0 by users and new ep's
> auth_hmacs couldn't be set in sctp_endpoint_init().
> 
> It can even crash kernel by doing:
> 
>   1. on server: sysctl -w net.sctp.auth_enable=1,
>                 sysctl -w net.sctp.addip_enable=1,
>                 sysctl -w net.sctp.addip_noauth_enable=0,
>                 listen() on server,
>                 sysctl -w net.sctp.auth_enable=0.
>   2. on client: connect() to server.
>   3. on server: accept() the asoc,
>                 sysctl -w net.sctp.auth_enable=1.
>   4. on client: send() asconf packet to server.
> 
> The call trace:
> 
>   [  245.280251] BUG: unable to handle kernel NULL pointer dereference at 0000000000000008
>   [  245.286872] RIP: 0010:sctp_auth_calculate_hmac+0xa3/0x140 [sctp]
>   [  245.304572] Call Trace:
>   [  245.305091]  <IRQ>
>   [  245.311287]  sctp_sf_authenticate+0x110/0x160 [sctp]
>   [  245.312311]  sctp_sf_eat_auth+0xf2/0x230 [sctp]
>   [  245.313249]  sctp_do_sm+0x9a/0x2d0 [sctp]
>   [  245.321483]  sctp_assoc_bh_rcv+0xed/0x1a0 [sctp]
>   [  245.322495]  sctp_rcv+0xa66/0xc70 [sctp]
> 
> It's because the old ep->auth_hmacs wasn't copied to the new ep while
> ep->auth_hmacs is used in sctp_auth_calculate_hmac() when processing
> the incoming auth chunks, and it should have been done when migrating
> sock.
> 
> Reported-by: Ying Xu <yinxu@redhat.com>
> Signed-off-by: Xin Long <lucien.xin@gmail.com>
> ---
>  net/sctp/socket.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/net/sctp/socket.c b/net/sctp/socket.c
> index 22adb8d..def3335 100644
> --- a/net/sctp/socket.c
> +++ b/net/sctp/socket.c
> @@ -8978,6 +8978,16 @@ static int sctp_sock_migrate(struct sock *oldsk, struct sock *newsk,
>  	if (err)
>  		return err;
>  
> +	/* New ep's auth_hmacs should be set if old ep's is set, in case
> +	 * that net->sctp.auth_enable has been changed to 0 by users and
> +	 * new ep's auth_hmacs couldn't be set in sctp_endpoint_init().
> +	 */
> +	if (oldsp->ep->auth_hmacs) {
> +		err = sctp_auth_init_hmacs(newsp->ep, GFP_KERNEL);
> +		if (err)
> +			return err;
> +	}
> +
>  	/* Move any messages in the old socket's receive queue that are for the
>  	 * peeled off association to the new socket's receive queue.
>  	 */
> -- 
> 2.1.0
> 
> 
Acked-by: Neil Horman <nhorman@redhat.com>


^ permalink raw reply	[flat|nested] 34+ messages in thread

* Re: [PATCH net 3/3] sctp: call sctp_auth_init_hmacs() in sctp_sock_migrate()
@ 2019-03-06 18:26         ` Neil Horman
  0 siblings, 0 replies; 34+ messages in thread
From: Neil Horman @ 2019-03-06 18:26 UTC (permalink / raw)
  To: Xin Long; +Cc: network dev, linux-sctp, davem, Marcelo Ricardo Leitner

On Sun, Mar 03, 2019 at 05:54:55PM +0800, Xin Long wrote:
> New ep's auth_hmacs should be set if old ep's is set, in case that
> net->sctp.auth_enable has been changed to 0 by users and new ep's
> auth_hmacs couldn't be set in sctp_endpoint_init().
> 
> It can even crash kernel by doing:
> 
>   1. on server: sysctl -w net.sctp.auth_enable=1,
>                 sysctl -w net.sctp.addip_enable=1,
>                 sysctl -w net.sctp.addip_noauth_enable=0,
>                 listen() on server,
>                 sysctl -w net.sctp.auth_enable=0.
>   2. on client: connect() to server.
>   3. on server: accept() the asoc,
>                 sysctl -w net.sctp.auth_enable=1.
>   4. on client: send() asconf packet to server.
> 
> The call trace:
> 
>   [  245.280251] BUG: unable to handle kernel NULL pointer dereference at 0000000000000008
>   [  245.286872] RIP: 0010:sctp_auth_calculate_hmac+0xa3/0x140 [sctp]
>   [  245.304572] Call Trace:
>   [  245.305091]  <IRQ>
>   [  245.311287]  sctp_sf_authenticate+0x110/0x160 [sctp]
>   [  245.312311]  sctp_sf_eat_auth+0xf2/0x230 [sctp]
>   [  245.313249]  sctp_do_sm+0x9a/0x2d0 [sctp]
>   [  245.321483]  sctp_assoc_bh_rcv+0xed/0x1a0 [sctp]
>   [  245.322495]  sctp_rcv+0xa66/0xc70 [sctp]
> 
> It's because the old ep->auth_hmacs wasn't copied to the new ep while
> ep->auth_hmacs is used in sctp_auth_calculate_hmac() when processing
> the incoming auth chunks, and it should have been done when migrating
> sock.
> 
> Reported-by: Ying Xu <yinxu@redhat.com>
> Signed-off-by: Xin Long <lucien.xin@gmail.com>
> ---
>  net/sctp/socket.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/net/sctp/socket.c b/net/sctp/socket.c
> index 22adb8d..def3335 100644
> --- a/net/sctp/socket.c
> +++ b/net/sctp/socket.c
> @@ -8978,6 +8978,16 @@ static int sctp_sock_migrate(struct sock *oldsk, struct sock *newsk,
>  	if (err)
>  		return err;
>  
> +	/* New ep's auth_hmacs should be set if old ep's is set, in case
> +	 * that net->sctp.auth_enable has been changed to 0 by users and
> +	 * new ep's auth_hmacs couldn't be set in sctp_endpoint_init().
> +	 */
> +	if (oldsp->ep->auth_hmacs) {
> +		err = sctp_auth_init_hmacs(newsp->ep, GFP_KERNEL);
> +		if (err)
> +			return err;
> +	}
> +
>  	/* Move any messages in the old socket's receive queue that are for the
>  	 * peeled off association to the new socket's receive queue.
>  	 */
> -- 
> 2.1.0
> 
> 
Acked-by: Neil Horman <nhorman@redhat.com>

^ permalink raw reply	[flat|nested] 34+ messages in thread

* Re: [PATCH net 1/3] sctp: sctp_sock_migrate() returns error if sctp_bind_addr_dup() fails
  2019-03-06 18:21     ` Neil Horman
@ 2019-03-07 10:06       ` Xin Long
  -1 siblings, 0 replies; 34+ messages in thread
From: Xin Long @ 2019-03-07 10:06 UTC (permalink / raw)
  To: Neil Horman; +Cc: network dev, linux-sctp, davem, Marcelo Ricardo Leitner

On Thu, Mar 7, 2019 at 2:21 AM Neil Horman <nhorman@tuxdriver.com> wrote:
>
> On Sun, Mar 03, 2019 at 05:54:53PM +0800, Xin Long wrote:
> > It should fail to create the new sk if sctp_bind_addr_dup() fails
> > when accepting or peeloff an association.
> >
> > Signed-off-by: Xin Long <lucien.xin@gmail.com>
> > ---
> >  net/sctp/socket.c | 34 ++++++++++++++++++++++++----------
> >  1 file changed, 24 insertions(+), 10 deletions(-)
> >
> > diff --git a/net/sctp/socket.c b/net/sctp/socket.c
> > index a2771b3..22adb8d 100644
> > --- a/net/sctp/socket.c
> > +++ b/net/sctp/socket.c
> > @@ -102,9 +102,9 @@ static int sctp_send_asconf(struct sctp_association *asoc,
> >                           struct sctp_chunk *chunk);
> >  static int sctp_do_bind(struct sock *, union sctp_addr *, int);
> >  static int sctp_autobind(struct sock *sk);
> > -static void sctp_sock_migrate(struct sock *oldsk, struct sock *newsk,
> > -                           struct sctp_association *assoc,
> > -                           enum sctp_socket_type type);
> > +static int sctp_sock_migrate(struct sock *oldsk, struct sock *newsk,
> > +                          struct sctp_association *assoc,
> > +                          enum sctp_socket_type type);
> >
> >  static unsigned long sctp_memory_pressure;
> >  static atomic_long_t sctp_memory_allocated;
> > @@ -4655,7 +4655,11 @@ static struct sock *sctp_accept(struct sock *sk, int flags, int *err, bool kern)
> >       /* Populate the fields of the newsk from the oldsk and migrate the
> >        * asoc to the newsk.
> >        */
> > -     sctp_sock_migrate(sk, newsk, asoc, SCTP_SOCKET_TCP);
> > +     error = sctp_sock_migrate(sk, newsk, asoc, SCTP_SOCKET_TCP);
> > +     if (error) {
> > +             sk_common_release(newsk);
> sctp_sock_migrate may fail after the pending packets have been moved from the
> old socket to the new socket.  Normally those packets will get purged by
> successful transmission, or when the socket is closed (via sctp_close), but
> neither of those cases applies here.  Whats going to dequeue and free any
> pending skbs on the sk_receive_queue here?
>
> > +             newsk = NULL;
> > +     }
> >
> >  out:
> >       release_sock(sk);
> > @@ -5401,7 +5405,12 @@ int sctp_do_peeloff(struct sock *sk, sctp_assoc_t id, struct socket **sockp)
> >       /* Populate the fields of the newsk from the oldsk and migrate the
> >        * asoc to the newsk.
> >        */
> > -     sctp_sock_migrate(sk, sock->sk, asoc, SCTP_SOCKET_UDP_HIGH_BANDWIDTH);
> > +     err = sctp_sock_migrate(sk, sock->sk, asoc,
> > +                             SCTP_SOCKET_UDP_HIGH_BANDWIDTH);
> > +     if (err) {
> > +             sock_release(sock);
> Same question here, what frees any pending skbs on the new socket, if the
> migration fails after the skbs have been queued to it?
Luckily, in sctp_sock_migrate(), it can only fail before the skbs are
moved to newsk->sk_receive_queue. So when it returns err,
newsk->sk_receive_queue must be empty.

>
> > +             sock = NULL;
> > +     }
> >
> >       *sockp = sock;
> >
> > @@ -8924,9 +8933,9 @@ static inline void sctp_copy_descendant(struct sock *sk_to,
> >  /* Populate the fields of the newsk from the oldsk and migrate the assoc
> >   * and its messages to the newsk.
> >   */
> > -static void sctp_sock_migrate(struct sock *oldsk, struct sock *newsk,
> > -                           struct sctp_association *assoc,
> > -                           enum sctp_socket_type type)
> > +static int sctp_sock_migrate(struct sock *oldsk, struct sock *newsk,
> > +                          struct sctp_association *assoc,
> > +                          enum sctp_socket_type type)
> >  {
> >       struct sctp_sock *oldsp = sctp_sk(oldsk);
> >       struct sctp_sock *newsp = sctp_sk(newsk);
> > @@ -8935,6 +8944,7 @@ static void sctp_sock_migrate(struct sock *oldsk, struct sock *newsk,
> >       struct sk_buff *skb, *tmp;
> >       struct sctp_ulpevent *event;
> >       struct sctp_bind_hashbucket *head;
> > +     int err;
> >
> >       /* Migrate socket buffer sizes and all the socket level options to the
> >        * new socket.
> > @@ -8963,8 +8973,10 @@ static void sctp_sock_migrate(struct sock *oldsk, struct sock *newsk,
> >       /* Copy the bind_addr list from the original endpoint to the new
> >        * endpoint so that we can handle restarts properly
> >        */
> > -     sctp_bind_addr_dup(&newsp->ep->base.bind_addr,
> > -                             &oldsp->ep->base.bind_addr, GFP_KERNEL);
> > +     err = sctp_bind_addr_dup(&newsp->ep->base.bind_addr,
> > +                              &oldsp->ep->base.bind_addr, GFP_KERNEL);
> > +     if (err)
> > +             return err;
> >
> >       /* Move any messages in the old socket's receive queue that are for the
> >        * peeled off association to the new socket's receive queue.
> > @@ -9049,6 +9061,8 @@ static void sctp_sock_migrate(struct sock *oldsk, struct sock *newsk,
> >       }
> >
> >       release_sock(newsk);
> > +
> > +     return 0;
> >  }
> >
> >
> > --
> > 2.1.0
> >
> >

^ permalink raw reply	[flat|nested] 34+ messages in thread

* Re: [PATCH net 1/3] sctp: sctp_sock_migrate() returns error if sctp_bind_addr_dup() fails
@ 2019-03-07 10:06       ` Xin Long
  0 siblings, 0 replies; 34+ messages in thread
From: Xin Long @ 2019-03-07 10:06 UTC (permalink / raw)
  To: Neil Horman; +Cc: network dev, linux-sctp, davem, Marcelo Ricardo Leitner

On Thu, Mar 7, 2019 at 2:21 AM Neil Horman <nhorman@tuxdriver.com> wrote:
>
> On Sun, Mar 03, 2019 at 05:54:53PM +0800, Xin Long wrote:
> > It should fail to create the new sk if sctp_bind_addr_dup() fails
> > when accepting or peeloff an association.
> >
> > Signed-off-by: Xin Long <lucien.xin@gmail.com>
> > ---
> >  net/sctp/socket.c | 34 ++++++++++++++++++++++++----------
> >  1 file changed, 24 insertions(+), 10 deletions(-)
> >
> > diff --git a/net/sctp/socket.c b/net/sctp/socket.c
> > index a2771b3..22adb8d 100644
> > --- a/net/sctp/socket.c
> > +++ b/net/sctp/socket.c
> > @@ -102,9 +102,9 @@ static int sctp_send_asconf(struct sctp_association *asoc,
> >                           struct sctp_chunk *chunk);
> >  static int sctp_do_bind(struct sock *, union sctp_addr *, int);
> >  static int sctp_autobind(struct sock *sk);
> > -static void sctp_sock_migrate(struct sock *oldsk, struct sock *newsk,
> > -                           struct sctp_association *assoc,
> > -                           enum sctp_socket_type type);
> > +static int sctp_sock_migrate(struct sock *oldsk, struct sock *newsk,
> > +                          struct sctp_association *assoc,
> > +                          enum sctp_socket_type type);
> >
> >  static unsigned long sctp_memory_pressure;
> >  static atomic_long_t sctp_memory_allocated;
> > @@ -4655,7 +4655,11 @@ static struct sock *sctp_accept(struct sock *sk, int flags, int *err, bool kern)
> >       /* Populate the fields of the newsk from the oldsk and migrate the
> >        * asoc to the newsk.
> >        */
> > -     sctp_sock_migrate(sk, newsk, asoc, SCTP_SOCKET_TCP);
> > +     error = sctp_sock_migrate(sk, newsk, asoc, SCTP_SOCKET_TCP);
> > +     if (error) {
> > +             sk_common_release(newsk);
> sctp_sock_migrate may fail after the pending packets have been moved from the
> old socket to the new socket.  Normally those packets will get purged by
> successful transmission, or when the socket is closed (via sctp_close), but
> neither of those cases applies here.  Whats going to dequeue and free any
> pending skbs on the sk_receive_queue here?
>
> > +             newsk = NULL;
> > +     }
> >
> >  out:
> >       release_sock(sk);
> > @@ -5401,7 +5405,12 @@ int sctp_do_peeloff(struct sock *sk, sctp_assoc_t id, struct socket **sockp)
> >       /* Populate the fields of the newsk from the oldsk and migrate the
> >        * asoc to the newsk.
> >        */
> > -     sctp_sock_migrate(sk, sock->sk, asoc, SCTP_SOCKET_UDP_HIGH_BANDWIDTH);
> > +     err = sctp_sock_migrate(sk, sock->sk, asoc,
> > +                             SCTP_SOCKET_UDP_HIGH_BANDWIDTH);
> > +     if (err) {
> > +             sock_release(sock);
> Same question here, what frees any pending skbs on the new socket, if the
> migration fails after the skbs have been queued to it?
Luckily, in sctp_sock_migrate(), it can only fail before the skbs are
moved to newsk->sk_receive_queue. So when it returns err,
newsk->sk_receive_queue must be empty.

>
> > +             sock = NULL;
> > +     }
> >
> >       *sockp = sock;
> >
> > @@ -8924,9 +8933,9 @@ static inline void sctp_copy_descendant(struct sock *sk_to,
> >  /* Populate the fields of the newsk from the oldsk and migrate the assoc
> >   * and its messages to the newsk.
> >   */
> > -static void sctp_sock_migrate(struct sock *oldsk, struct sock *newsk,
> > -                           struct sctp_association *assoc,
> > -                           enum sctp_socket_type type)
> > +static int sctp_sock_migrate(struct sock *oldsk, struct sock *newsk,
> > +                          struct sctp_association *assoc,
> > +                          enum sctp_socket_type type)
> >  {
> >       struct sctp_sock *oldsp = sctp_sk(oldsk);
> >       struct sctp_sock *newsp = sctp_sk(newsk);
> > @@ -8935,6 +8944,7 @@ static void sctp_sock_migrate(struct sock *oldsk, struct sock *newsk,
> >       struct sk_buff *skb, *tmp;
> >       struct sctp_ulpevent *event;
> >       struct sctp_bind_hashbucket *head;
> > +     int err;
> >
> >       /* Migrate socket buffer sizes and all the socket level options to the
> >        * new socket.
> > @@ -8963,8 +8973,10 @@ static void sctp_sock_migrate(struct sock *oldsk, struct sock *newsk,
> >       /* Copy the bind_addr list from the original endpoint to the new
> >        * endpoint so that we can handle restarts properly
> >        */
> > -     sctp_bind_addr_dup(&newsp->ep->base.bind_addr,
> > -                             &oldsp->ep->base.bind_addr, GFP_KERNEL);
> > +     err = sctp_bind_addr_dup(&newsp->ep->base.bind_addr,
> > +                              &oldsp->ep->base.bind_addr, GFP_KERNEL);
> > +     if (err)
> > +             return err;
> >
> >       /* Move any messages in the old socket's receive queue that are for the
> >        * peeled off association to the new socket's receive queue.
> > @@ -9049,6 +9061,8 @@ static void sctp_sock_migrate(struct sock *oldsk, struct sock *newsk,
> >       }
> >
> >       release_sock(newsk);
> > +
> > +     return 0;
> >  }
> >
> >
> > --
> > 2.1.0
> >
> >

^ permalink raw reply	[flat|nested] 34+ messages in thread

* Re: [PATCH net 1/3] sctp: sctp_sock_migrate() returns error if sctp_bind_addr_dup() fails
  2019-03-07 10:06       ` Xin Long
@ 2019-03-07 11:59         ` Neil Horman
  -1 siblings, 0 replies; 34+ messages in thread
From: Neil Horman @ 2019-03-07 11:59 UTC (permalink / raw)
  To: Xin Long; +Cc: network dev, linux-sctp, davem, Marcelo Ricardo Leitner

On Thu, Mar 07, 2019 at 06:06:21PM +0800, Xin Long wrote:
> On Thu, Mar 7, 2019 at 2:21 AM Neil Horman <nhorman@tuxdriver.com> wrote:
> >
> > On Sun, Mar 03, 2019 at 05:54:53PM +0800, Xin Long wrote:
> > > It should fail to create the new sk if sctp_bind_addr_dup() fails
> > > when accepting or peeloff an association.
> > >
> > > Signed-off-by: Xin Long <lucien.xin@gmail.com>
> > > ---
> > >  net/sctp/socket.c | 34 ++++++++++++++++++++++++----------
> > >  1 file changed, 24 insertions(+), 10 deletions(-)
> > >
> > > diff --git a/net/sctp/socket.c b/net/sctp/socket.c
> > > index a2771b3..22adb8d 100644
> > > --- a/net/sctp/socket.c
> > > +++ b/net/sctp/socket.c
> > > @@ -102,9 +102,9 @@ static int sctp_send_asconf(struct sctp_association *asoc,
> > >                           struct sctp_chunk *chunk);
> > >  static int sctp_do_bind(struct sock *, union sctp_addr *, int);
> > >  static int sctp_autobind(struct sock *sk);
> > > -static void sctp_sock_migrate(struct sock *oldsk, struct sock *newsk,
> > > -                           struct sctp_association *assoc,
> > > -                           enum sctp_socket_type type);
> > > +static int sctp_sock_migrate(struct sock *oldsk, struct sock *newsk,
> > > +                          struct sctp_association *assoc,
> > > +                          enum sctp_socket_type type);
> > >
> > >  static unsigned long sctp_memory_pressure;
> > >  static atomic_long_t sctp_memory_allocated;
> > > @@ -4655,7 +4655,11 @@ static struct sock *sctp_accept(struct sock *sk, int flags, int *err, bool kern)
> > >       /* Populate the fields of the newsk from the oldsk and migrate the
> > >        * asoc to the newsk.
> > >        */
> > > -     sctp_sock_migrate(sk, newsk, asoc, SCTP_SOCKET_TCP);
> > > +     error = sctp_sock_migrate(sk, newsk, asoc, SCTP_SOCKET_TCP);
> > > +     if (error) {
> > > +             sk_common_release(newsk);
> > sctp_sock_migrate may fail after the pending packets have been moved from the
> > old socket to the new socket.  Normally those packets will get purged by
> > successful transmission, or when the socket is closed (via sctp_close), but
> > neither of those cases applies here.  Whats going to dequeue and free any
> > pending skbs on the sk_receive_queue here?
> >
> > > +             newsk = NULL;
> > > +     }
> > >
> > >  out:
> > >       release_sock(sk);
> > > @@ -5401,7 +5405,12 @@ int sctp_do_peeloff(struct sock *sk, sctp_assoc_t id, struct socket **sockp)
> > >       /* Populate the fields of the newsk from the oldsk and migrate the
> > >        * asoc to the newsk.
> > >        */
> > > -     sctp_sock_migrate(sk, sock->sk, asoc, SCTP_SOCKET_UDP_HIGH_BANDWIDTH);
> > > +     err = sctp_sock_migrate(sk, sock->sk, asoc,
> > > +                             SCTP_SOCKET_UDP_HIGH_BANDWIDTH);
> > > +     if (err) {
> > > +             sock_release(sock);
> > Same question here, what frees any pending skbs on the new socket, if the
> > migration fails after the skbs have been queued to it?
> Luckily, in sctp_sock_migrate(), it can only fail before the skbs are
> moved to newsk->sk_receive_queue. So when it returns err,
> newsk->sk_receive_queue must be empty.
> 
Yeah, you're right, I misread the location of the check, thinking it came after
the skb migration, this is fine.

> >
> > > +             sock = NULL;
> > > +     }
> > >
> > >       *sockp = sock;
> > >
> > > @@ -8924,9 +8933,9 @@ static inline void sctp_copy_descendant(struct sock *sk_to,
> > >  /* Populate the fields of the newsk from the oldsk and migrate the assoc
> > >   * and its messages to the newsk.
> > >   */
> > > -static void sctp_sock_migrate(struct sock *oldsk, struct sock *newsk,
> > > -                           struct sctp_association *assoc,
> > > -                           enum sctp_socket_type type)
> > > +static int sctp_sock_migrate(struct sock *oldsk, struct sock *newsk,
> > > +                          struct sctp_association *assoc,
> > > +                          enum sctp_socket_type type)
> > >  {
> > >       struct sctp_sock *oldsp = sctp_sk(oldsk);
> > >       struct sctp_sock *newsp = sctp_sk(newsk);
> > > @@ -8935,6 +8944,7 @@ static void sctp_sock_migrate(struct sock *oldsk, struct sock *newsk,
> > >       struct sk_buff *skb, *tmp;
> > >       struct sctp_ulpevent *event;
> > >       struct sctp_bind_hashbucket *head;
> > > +     int err;
> > >
> > >       /* Migrate socket buffer sizes and all the socket level options to the
> > >        * new socket.
> > > @@ -8963,8 +8973,10 @@ static void sctp_sock_migrate(struct sock *oldsk, struct sock *newsk,
> > >       /* Copy the bind_addr list from the original endpoint to the new
> > >        * endpoint so that we can handle restarts properly
> > >        */
> > > -     sctp_bind_addr_dup(&newsp->ep->base.bind_addr,
> > > -                             &oldsp->ep->base.bind_addr, GFP_KERNEL);
> > > +     err = sctp_bind_addr_dup(&newsp->ep->base.bind_addr,
> > > +                              &oldsp->ep->base.bind_addr, GFP_KERNEL);
> > > +     if (err)
> > > +             return err;
> > >
> > >       /* Move any messages in the old socket's receive queue that are for the
> > >        * peeled off association to the new socket's receive queue.
> > > @@ -9049,6 +9061,8 @@ static void sctp_sock_migrate(struct sock *oldsk, struct sock *newsk,
> > >       }
> > >
> > >       release_sock(newsk);
> > > +
> > > +     return 0;
> > >  }
> > >
> > >
> > > --
> > > 2.1.0
> > >
> > >
> 

^ permalink raw reply	[flat|nested] 34+ messages in thread

* Re: [PATCH net 1/3] sctp: sctp_sock_migrate() returns error if sctp_bind_addr_dup() fails
@ 2019-03-07 11:59         ` Neil Horman
  0 siblings, 0 replies; 34+ messages in thread
From: Neil Horman @ 2019-03-07 11:59 UTC (permalink / raw)
  To: Xin Long; +Cc: network dev, linux-sctp, davem, Marcelo Ricardo Leitner

On Thu, Mar 07, 2019 at 06:06:21PM +0800, Xin Long wrote:
> On Thu, Mar 7, 2019 at 2:21 AM Neil Horman <nhorman@tuxdriver.com> wrote:
> >
> > On Sun, Mar 03, 2019 at 05:54:53PM +0800, Xin Long wrote:
> > > It should fail to create the new sk if sctp_bind_addr_dup() fails
> > > when accepting or peeloff an association.
> > >
> > > Signed-off-by: Xin Long <lucien.xin@gmail.com>
> > > ---
> > >  net/sctp/socket.c | 34 ++++++++++++++++++++++++----------
> > >  1 file changed, 24 insertions(+), 10 deletions(-)
> > >
> > > diff --git a/net/sctp/socket.c b/net/sctp/socket.c
> > > index a2771b3..22adb8d 100644
> > > --- a/net/sctp/socket.c
> > > +++ b/net/sctp/socket.c
> > > @@ -102,9 +102,9 @@ static int sctp_send_asconf(struct sctp_association *asoc,
> > >                           struct sctp_chunk *chunk);
> > >  static int sctp_do_bind(struct sock *, union sctp_addr *, int);
> > >  static int sctp_autobind(struct sock *sk);
> > > -static void sctp_sock_migrate(struct sock *oldsk, struct sock *newsk,
> > > -                           struct sctp_association *assoc,
> > > -                           enum sctp_socket_type type);
> > > +static int sctp_sock_migrate(struct sock *oldsk, struct sock *newsk,
> > > +                          struct sctp_association *assoc,
> > > +                          enum sctp_socket_type type);
> > >
> > >  static unsigned long sctp_memory_pressure;
> > >  static atomic_long_t sctp_memory_allocated;
> > > @@ -4655,7 +4655,11 @@ static struct sock *sctp_accept(struct sock *sk, int flags, int *err, bool kern)
> > >       /* Populate the fields of the newsk from the oldsk and migrate the
> > >        * asoc to the newsk.
> > >        */
> > > -     sctp_sock_migrate(sk, newsk, asoc, SCTP_SOCKET_TCP);
> > > +     error = sctp_sock_migrate(sk, newsk, asoc, SCTP_SOCKET_TCP);
> > > +     if (error) {
> > > +             sk_common_release(newsk);
> > sctp_sock_migrate may fail after the pending packets have been moved from the
> > old socket to the new socket.  Normally those packets will get purged by
> > successful transmission, or when the socket is closed (via sctp_close), but
> > neither of those cases applies here.  Whats going to dequeue and free any
> > pending skbs on the sk_receive_queue here?
> >
> > > +             newsk = NULL;
> > > +     }
> > >
> > >  out:
> > >       release_sock(sk);
> > > @@ -5401,7 +5405,12 @@ int sctp_do_peeloff(struct sock *sk, sctp_assoc_t id, struct socket **sockp)
> > >       /* Populate the fields of the newsk from the oldsk and migrate the
> > >        * asoc to the newsk.
> > >        */
> > > -     sctp_sock_migrate(sk, sock->sk, asoc, SCTP_SOCKET_UDP_HIGH_BANDWIDTH);
> > > +     err = sctp_sock_migrate(sk, sock->sk, asoc,
> > > +                             SCTP_SOCKET_UDP_HIGH_BANDWIDTH);
> > > +     if (err) {
> > > +             sock_release(sock);
> > Same question here, what frees any pending skbs on the new socket, if the
> > migration fails after the skbs have been queued to it?
> Luckily, in sctp_sock_migrate(), it can only fail before the skbs are
> moved to newsk->sk_receive_queue. So when it returns err,
> newsk->sk_receive_queue must be empty.
> 
Yeah, you're right, I misread the location of the check, thinking it came after
the skb migration, this is fine.

> >
> > > +             sock = NULL;
> > > +     }
> > >
> > >       *sockp = sock;
> > >
> > > @@ -8924,9 +8933,9 @@ static inline void sctp_copy_descendant(struct sock *sk_to,
> > >  /* Populate the fields of the newsk from the oldsk and migrate the assoc
> > >   * and its messages to the newsk.
> > >   */
> > > -static void sctp_sock_migrate(struct sock *oldsk, struct sock *newsk,
> > > -                           struct sctp_association *assoc,
> > > -                           enum sctp_socket_type type)
> > > +static int sctp_sock_migrate(struct sock *oldsk, struct sock *newsk,
> > > +                          struct sctp_association *assoc,
> > > +                          enum sctp_socket_type type)
> > >  {
> > >       struct sctp_sock *oldsp = sctp_sk(oldsk);
> > >       struct sctp_sock *newsp = sctp_sk(newsk);
> > > @@ -8935,6 +8944,7 @@ static void sctp_sock_migrate(struct sock *oldsk, struct sock *newsk,
> > >       struct sk_buff *skb, *tmp;
> > >       struct sctp_ulpevent *event;
> > >       struct sctp_bind_hashbucket *head;
> > > +     int err;
> > >
> > >       /* Migrate socket buffer sizes and all the socket level options to the
> > >        * new socket.
> > > @@ -8963,8 +8973,10 @@ static void sctp_sock_migrate(struct sock *oldsk, struct sock *newsk,
> > >       /* Copy the bind_addr list from the original endpoint to the new
> > >        * endpoint so that we can handle restarts properly
> > >        */
> > > -     sctp_bind_addr_dup(&newsp->ep->base.bind_addr,
> > > -                             &oldsp->ep->base.bind_addr, GFP_KERNEL);
> > > +     err = sctp_bind_addr_dup(&newsp->ep->base.bind_addr,
> > > +                              &oldsp->ep->base.bind_addr, GFP_KERNEL);
> > > +     if (err)
> > > +             return err;
> > >
> > >       /* Move any messages in the old socket's receive queue that are for the
> > >        * peeled off association to the new socket's receive queue.
> > > @@ -9049,6 +9061,8 @@ static void sctp_sock_migrate(struct sock *oldsk, struct sock *newsk,
> > >       }
> > >
> > >       release_sock(newsk);
> > > +
> > > +     return 0;
> > >  }
> > >
> > >
> > > --
> > > 2.1.0
> > >
> > >
> 

^ permalink raw reply	[flat|nested] 34+ messages in thread

* Re: [PATCH net 0/3] sctp: process the error returned from sctp_sock_migrate()
  2019-03-03  9:54 ` Xin Long
@ 2019-03-07 12:11   ` Neil Horman
  -1 siblings, 0 replies; 34+ messages in thread
From: Neil Horman @ 2019-03-07 12:11 UTC (permalink / raw)
  To: Xin Long; +Cc: network dev, linux-sctp, davem, Marcelo Ricardo Leitner

On Sun, Mar 03, 2019 at 05:54:52PM +0800, Xin Long wrote:
> This patchset is to process the errs returned by sctp_auth_init_hmacs()
> and sctp_bind_addr_dup() from sctp_sock_migrate(). And also fix a panic
> caused by new ep->auth_hmacs was not set due to net->sctp.auth_enable
> changed by sysctl before accepting an assoc.
> 
> Xin Long (3):
>   sctp: sctp_sock_migrate() returns error if sctp_bind_addr_dup() fails
>   sctp: move up sctp_auth_init_hmacs() in sctp_endpoint_init()
>   sctp: call sctp_auth_init_hmacs() in sctp_sock_migrate()
> 
>  net/sctp/auth.c        |  6 ------
>  net/sctp/endpointola.c | 18 ++++++++++--------
>  net/sctp/socket.c      | 44 ++++++++++++++++++++++++++++++++++----------
>  3 files changed, 44 insertions(+), 24 deletions(-)
> 
> -- 
> 2.1.0
> 
> 
Series
Acked-by: Neil Horman <nhorman@tuxdriver.com>

^ permalink raw reply	[flat|nested] 34+ messages in thread

* Re: [PATCH net 0/3] sctp: process the error returned from sctp_sock_migrate()
@ 2019-03-07 12:11   ` Neil Horman
  0 siblings, 0 replies; 34+ messages in thread
From: Neil Horman @ 2019-03-07 12:11 UTC (permalink / raw)
  To: Xin Long; +Cc: network dev, linux-sctp, davem, Marcelo Ricardo Leitner

On Sun, Mar 03, 2019 at 05:54:52PM +0800, Xin Long wrote:
> This patchset is to process the errs returned by sctp_auth_init_hmacs()
> and sctp_bind_addr_dup() from sctp_sock_migrate(). And also fix a panic
> caused by new ep->auth_hmacs was not set due to net->sctp.auth_enable
> changed by sysctl before accepting an assoc.
> 
> Xin Long (3):
>   sctp: sctp_sock_migrate() returns error if sctp_bind_addr_dup() fails
>   sctp: move up sctp_auth_init_hmacs() in sctp_endpoint_init()
>   sctp: call sctp_auth_init_hmacs() in sctp_sock_migrate()
> 
>  net/sctp/auth.c        |  6 ------
>  net/sctp/endpointola.c | 18 ++++++++++--------
>  net/sctp/socket.c      | 44 ++++++++++++++++++++++++++++++++++----------
>  3 files changed, 44 insertions(+), 24 deletions(-)
> 
> -- 
> 2.1.0
> 
> 
Series
Acked-by: Neil Horman <nhorman@tuxdriver.com>

^ permalink raw reply	[flat|nested] 34+ messages in thread

* Re: [PATCH net 0/3] sctp: process the error returned from sctp_sock_migrate()
  2019-03-04 19:04   ` David Miller
@ 2019-03-07 14:59     ` Marcelo Ricardo Leitner
  -1 siblings, 0 replies; 34+ messages in thread
From: Marcelo Ricardo Leitner @ 2019-03-07 14:59 UTC (permalink / raw)
  To: David Miller; +Cc: lucien.xin, netdev, linux-sctp, nhorman

On Mon, Mar 04, 2019 at 11:04:46AM -0800, David Miller wrote:
> From: Xin Long <lucien.xin@gmail.com>
> Date: Sun,  3 Mar 2019 17:54:52 +0800
> 
> > This patchset is to process the errs returned by sctp_auth_init_hmacs()
> > and sctp_bind_addr_dup() from sctp_sock_migrate(). And also fix a panic
> > caused by new ep->auth_hmacs was not set due to net->sctp.auth_enable
> > changed by sysctl before accepting an assoc.
> 
> I'll give Neil and Marcelo a change to review this.

Please give me one more day. I'm just back from a large holiday.

Thanks,
Marcelo

^ permalink raw reply	[flat|nested] 34+ messages in thread

* Re: [PATCH net 0/3] sctp: process the error returned from sctp_sock_migrate()
@ 2019-03-07 14:59     ` Marcelo Ricardo Leitner
  0 siblings, 0 replies; 34+ messages in thread
From: Marcelo Ricardo Leitner @ 2019-03-07 14:59 UTC (permalink / raw)
  To: David Miller; +Cc: lucien.xin, netdev, linux-sctp, nhorman

On Mon, Mar 04, 2019 at 11:04:46AM -0800, David Miller wrote:
> From: Xin Long <lucien.xin@gmail.com>
> Date: Sun,  3 Mar 2019 17:54:52 +0800
> 
> > This patchset is to process the errs returned by sctp_auth_init_hmacs()
> > and sctp_bind_addr_dup() from sctp_sock_migrate(). And also fix a panic
> > caused by new ep->auth_hmacs was not set due to net->sctp.auth_enable
> > changed by sysctl before accepting an assoc.
> 
> I'll give Neil and Marcelo a change to review this.

Please give me one more day. I'm just back from a large holiday.

Thanks,
Marcelo

^ permalink raw reply	[flat|nested] 34+ messages in thread

* Re: [PATCH net 1/3] sctp: sctp_sock_migrate() returns error if sctp_bind_addr_dup() fails
  2019-03-03  9:54   ` Xin Long
@ 2019-03-07 18:25     ` Marcelo Ricardo Leitner
  -1 siblings, 0 replies; 34+ messages in thread
From: Marcelo Ricardo Leitner @ 2019-03-07 18:25 UTC (permalink / raw)
  To: Xin Long; +Cc: network dev, linux-sctp, davem, Neil Horman

On Sun, Mar 03, 2019 at 05:54:53PM +0800, Xin Long wrote:
> It should fail to create the new sk if sctp_bind_addr_dup() fails
> when accepting or peeloff an association.
> 
> Signed-off-by: Xin Long <lucien.xin@gmail.com>
> ---
>  net/sctp/socket.c | 34 ++++++++++++++++++++++++----------
>  1 file changed, 24 insertions(+), 10 deletions(-)
> 
> diff --git a/net/sctp/socket.c b/net/sctp/socket.c
> index a2771b3..22adb8d 100644
> --- a/net/sctp/socket.c
> +++ b/net/sctp/socket.c
> @@ -102,9 +102,9 @@ static int sctp_send_asconf(struct sctp_association *asoc,
>  			    struct sctp_chunk *chunk);
>  static int sctp_do_bind(struct sock *, union sctp_addr *, int);
>  static int sctp_autobind(struct sock *sk);
> -static void sctp_sock_migrate(struct sock *oldsk, struct sock *newsk,
> -			      struct sctp_association *assoc,
> -			      enum sctp_socket_type type);
> +static int sctp_sock_migrate(struct sock *oldsk, struct sock *newsk,
> +			     struct sctp_association *assoc,
> +			     enum sctp_socket_type type);
>  
>  static unsigned long sctp_memory_pressure;
>  static atomic_long_t sctp_memory_allocated;
> @@ -4655,7 +4655,11 @@ static struct sock *sctp_accept(struct sock *sk, int flags, int *err, bool kern)
>  	/* Populate the fields of the newsk from the oldsk and migrate the
>  	 * asoc to the newsk.
>  	 */
> -	sctp_sock_migrate(sk, newsk, asoc, SCTP_SOCKET_TCP);
> +	error = sctp_sock_migrate(sk, newsk, asoc, SCTP_SOCKET_TCP);
> +	if (error) {
> +		sk_common_release(newsk);
> +		newsk = NULL;
> +	}
>  
>  out:
>  	release_sock(sk);
> @@ -5401,7 +5405,12 @@ int sctp_do_peeloff(struct sock *sk, sctp_assoc_t id, struct socket **sockp)
>  	/* Populate the fields of the newsk from the oldsk and migrate the
>  	 * asoc to the newsk.
>  	 */
> -	sctp_sock_migrate(sk, sock->sk, asoc, SCTP_SOCKET_UDP_HIGH_BANDWIDTH);
> +	err = sctp_sock_migrate(sk, sock->sk, asoc,
> +				SCTP_SOCKET_UDP_HIGH_BANDWIDTH);
> +	if (err) {
> +		sock_release(sock);

I'm having a hard time understanding why it needs sock_release() here
and sk_common_release() in the above chunk. AFAICT by here (after
sctp_sock_migrate call) the sockets are pretty much the same. Mind
elaborating please?

> +		sock = NULL;
> +	}
>  
>  	*sockp = sock;
>  

^ permalink raw reply	[flat|nested] 34+ messages in thread

* Re: [PATCH net 1/3] sctp: sctp_sock_migrate() returns error if sctp_bind_addr_dup() fails
@ 2019-03-07 18:25     ` Marcelo Ricardo Leitner
  0 siblings, 0 replies; 34+ messages in thread
From: Marcelo Ricardo Leitner @ 2019-03-07 18:25 UTC (permalink / raw)
  To: Xin Long; +Cc: network dev, linux-sctp, davem, Neil Horman

On Sun, Mar 03, 2019 at 05:54:53PM +0800, Xin Long wrote:
> It should fail to create the new sk if sctp_bind_addr_dup() fails
> when accepting or peeloff an association.
> 
> Signed-off-by: Xin Long <lucien.xin@gmail.com>
> ---
>  net/sctp/socket.c | 34 ++++++++++++++++++++++++----------
>  1 file changed, 24 insertions(+), 10 deletions(-)
> 
> diff --git a/net/sctp/socket.c b/net/sctp/socket.c
> index a2771b3..22adb8d 100644
> --- a/net/sctp/socket.c
> +++ b/net/sctp/socket.c
> @@ -102,9 +102,9 @@ static int sctp_send_asconf(struct sctp_association *asoc,
>  			    struct sctp_chunk *chunk);
>  static int sctp_do_bind(struct sock *, union sctp_addr *, int);
>  static int sctp_autobind(struct sock *sk);
> -static void sctp_sock_migrate(struct sock *oldsk, struct sock *newsk,
> -			      struct sctp_association *assoc,
> -			      enum sctp_socket_type type);
> +static int sctp_sock_migrate(struct sock *oldsk, struct sock *newsk,
> +			     struct sctp_association *assoc,
> +			     enum sctp_socket_type type);
>  
>  static unsigned long sctp_memory_pressure;
>  static atomic_long_t sctp_memory_allocated;
> @@ -4655,7 +4655,11 @@ static struct sock *sctp_accept(struct sock *sk, int flags, int *err, bool kern)
>  	/* Populate the fields of the newsk from the oldsk and migrate the
>  	 * asoc to the newsk.
>  	 */
> -	sctp_sock_migrate(sk, newsk, asoc, SCTP_SOCKET_TCP);
> +	error = sctp_sock_migrate(sk, newsk, asoc, SCTP_SOCKET_TCP);
> +	if (error) {
> +		sk_common_release(newsk);
> +		newsk = NULL;
> +	}
>  
>  out:
>  	release_sock(sk);
> @@ -5401,7 +5405,12 @@ int sctp_do_peeloff(struct sock *sk, sctp_assoc_t id, struct socket **sockp)
>  	/* Populate the fields of the newsk from the oldsk and migrate the
>  	 * asoc to the newsk.
>  	 */
> -	sctp_sock_migrate(sk, sock->sk, asoc, SCTP_SOCKET_UDP_HIGH_BANDWIDTH);
> +	err = sctp_sock_migrate(sk, sock->sk, asoc,
> +				SCTP_SOCKET_UDP_HIGH_BANDWIDTH);
> +	if (err) {
> +		sock_release(sock);

I'm having a hard time understanding why it needs sock_release() here
and sk_common_release() in the above chunk. AFAICT by here (after
sctp_sock_migrate call) the sockets are pretty much the same. Mind
elaborating please?

> +		sock = NULL;
> +	}
>  
>  	*sockp = sock;
>  

^ permalink raw reply	[flat|nested] 34+ messages in thread

* Re: [PATCH net 1/3] sctp: sctp_sock_migrate() returns error if sctp_bind_addr_dup() fails
  2019-03-07 18:25     ` Marcelo Ricardo Leitner
@ 2019-03-08  3:48       ` Xin Long
  -1 siblings, 0 replies; 34+ messages in thread
From: Xin Long @ 2019-03-08  3:48 UTC (permalink / raw)
  To: Marcelo Ricardo Leitner; +Cc: network dev, linux-sctp, davem, Neil Horman

On Fri, Mar 8, 2019 at 2:25 AM Marcelo Ricardo Leitner
<marcelo.leitner@gmail.com> wrote:
>
> On Sun, Mar 03, 2019 at 05:54:53PM +0800, Xin Long wrote:
> > It should fail to create the new sk if sctp_bind_addr_dup() fails
> > when accepting or peeloff an association.
> >
> > Signed-off-by: Xin Long <lucien.xin@gmail.com>
> > ---
> >  net/sctp/socket.c | 34 ++++++++++++++++++++++++----------
> >  1 file changed, 24 insertions(+), 10 deletions(-)
> >
> > diff --git a/net/sctp/socket.c b/net/sctp/socket.c
> > index a2771b3..22adb8d 100644
> > --- a/net/sctp/socket.c
> > +++ b/net/sctp/socket.c
> > @@ -102,9 +102,9 @@ static int sctp_send_asconf(struct sctp_association *asoc,
> >                           struct sctp_chunk *chunk);
> >  static int sctp_do_bind(struct sock *, union sctp_addr *, int);
> >  static int sctp_autobind(struct sock *sk);
> > -static void sctp_sock_migrate(struct sock *oldsk, struct sock *newsk,
> > -                           struct sctp_association *assoc,
> > -                           enum sctp_socket_type type);
> > +static int sctp_sock_migrate(struct sock *oldsk, struct sock *newsk,
> > +                          struct sctp_association *assoc,
> > +                          enum sctp_socket_type type);
> >
> >  static unsigned long sctp_memory_pressure;
> >  static atomic_long_t sctp_memory_allocated;
> > @@ -4655,7 +4655,11 @@ static struct sock *sctp_accept(struct sock *sk, int flags, int *err, bool kern)
> >       /* Populate the fields of the newsk from the oldsk and migrate the
> >        * asoc to the newsk.
> >        */
> > -     sctp_sock_migrate(sk, newsk, asoc, SCTP_SOCKET_TCP);
> > +     error = sctp_sock_migrate(sk, newsk, asoc, SCTP_SOCKET_TCP);
> > +     if (error) {
> > +             sk_common_release(newsk);
> > +             newsk = NULL;
> > +     }
> >
> >  out:
> >       release_sock(sk);
> > @@ -5401,7 +5405,12 @@ int sctp_do_peeloff(struct sock *sk, sctp_assoc_t id, struct socket **sockp)
> >       /* Populate the fields of the newsk from the oldsk and migrate the
> >        * asoc to the newsk.
> >        */
> > -     sctp_sock_migrate(sk, sock->sk, asoc, SCTP_SOCKET_UDP_HIGH_BANDWIDTH);
> > +     err = sctp_sock_migrate(sk, sock->sk, asoc,
> > +                             SCTP_SOCKET_UDP_HIGH_BANDWIDTH);
> > +     if (err) {
> > +             sock_release(sock);
>
> I'm having a hard time understanding why it needs sock_release() here
> and sk_common_release() in the above chunk. AFAICT by here (after
> sctp_sock_migrate call) the sockets are pretty much the same. Mind
> elaborating please?
In sctp_do_peeloff(),'sock' is 'struct socket' created by sock_create(),
which should be freed by sock_release(struct socket *sock), otherwise
no place takes care of its freeing.
(sock_create() also create sock->sk)

In sctp_accept(), 'newsk' is 'struct sock' created by sk_alloc(), and
it should be freed by sk_common_release(). Note by then sock->sk is not
yet set to 'newsk', so when it return errs, outside can't free newsk
when freeing sock.
(sys_accept() calls sock_alloc() directly that doesn't set sock->sk)

^ permalink raw reply	[flat|nested] 34+ messages in thread

* Re: [PATCH net 1/3] sctp: sctp_sock_migrate() returns error if sctp_bind_addr_dup() fails
@ 2019-03-08  3:48       ` Xin Long
  0 siblings, 0 replies; 34+ messages in thread
From: Xin Long @ 2019-03-08  3:48 UTC (permalink / raw)
  To: Marcelo Ricardo Leitner; +Cc: network dev, linux-sctp, davem, Neil Horman

On Fri, Mar 8, 2019 at 2:25 AM Marcelo Ricardo Leitner
<marcelo.leitner@gmail.com> wrote:
>
> On Sun, Mar 03, 2019 at 05:54:53PM +0800, Xin Long wrote:
> > It should fail to create the new sk if sctp_bind_addr_dup() fails
> > when accepting or peeloff an association.
> >
> > Signed-off-by: Xin Long <lucien.xin@gmail.com>
> > ---
> >  net/sctp/socket.c | 34 ++++++++++++++++++++++++----------
> >  1 file changed, 24 insertions(+), 10 deletions(-)
> >
> > diff --git a/net/sctp/socket.c b/net/sctp/socket.c
> > index a2771b3..22adb8d 100644
> > --- a/net/sctp/socket.c
> > +++ b/net/sctp/socket.c
> > @@ -102,9 +102,9 @@ static int sctp_send_asconf(struct sctp_association *asoc,
> >                           struct sctp_chunk *chunk);
> >  static int sctp_do_bind(struct sock *, union sctp_addr *, int);
> >  static int sctp_autobind(struct sock *sk);
> > -static void sctp_sock_migrate(struct sock *oldsk, struct sock *newsk,
> > -                           struct sctp_association *assoc,
> > -                           enum sctp_socket_type type);
> > +static int sctp_sock_migrate(struct sock *oldsk, struct sock *newsk,
> > +                          struct sctp_association *assoc,
> > +                          enum sctp_socket_type type);
> >
> >  static unsigned long sctp_memory_pressure;
> >  static atomic_long_t sctp_memory_allocated;
> > @@ -4655,7 +4655,11 @@ static struct sock *sctp_accept(struct sock *sk, int flags, int *err, bool kern)
> >       /* Populate the fields of the newsk from the oldsk and migrate the
> >        * asoc to the newsk.
> >        */
> > -     sctp_sock_migrate(sk, newsk, asoc, SCTP_SOCKET_TCP);
> > +     error = sctp_sock_migrate(sk, newsk, asoc, SCTP_SOCKET_TCP);
> > +     if (error) {
> > +             sk_common_release(newsk);
> > +             newsk = NULL;
> > +     }
> >
> >  out:
> >       release_sock(sk);
> > @@ -5401,7 +5405,12 @@ int sctp_do_peeloff(struct sock *sk, sctp_assoc_t id, struct socket **sockp)
> >       /* Populate the fields of the newsk from the oldsk and migrate the
> >        * asoc to the newsk.
> >        */
> > -     sctp_sock_migrate(sk, sock->sk, asoc, SCTP_SOCKET_UDP_HIGH_BANDWIDTH);
> > +     err = sctp_sock_migrate(sk, sock->sk, asoc,
> > +                             SCTP_SOCKET_UDP_HIGH_BANDWIDTH);
> > +     if (err) {
> > +             sock_release(sock);
>
> I'm having a hard time understanding why it needs sock_release() here
> and sk_common_release() in the above chunk. AFAICT by here (after
> sctp_sock_migrate call) the sockets are pretty much the same. Mind
> elaborating please?
In sctp_do_peeloff(),'sock' is 'struct socket' created by sock_create(),
which should be freed by sock_release(struct socket *sock), otherwise
no place takes care of its freeing.
(sock_create() also create sock->sk)

In sctp_accept(), 'newsk' is 'struct sock' created by sk_alloc(), and
it should be freed by sk_common_release(). Note by then sock->sk is not
yet set to 'newsk', so when it return errs, outside can't free newsk
when freeing sock.
(sys_accept() calls sock_alloc() directly that doesn't set sock->sk)

^ permalink raw reply	[flat|nested] 34+ messages in thread

* Re: [PATCH net 1/3] sctp: sctp_sock_migrate() returns error if sctp_bind_addr_dup() fails
  2019-03-08  3:48       ` Xin Long
@ 2019-03-08 16:59         ` Marcelo Ricardo Leitner
  -1 siblings, 0 replies; 34+ messages in thread
From: Marcelo Ricardo Leitner @ 2019-03-08 16:59 UTC (permalink / raw)
  To: Xin Long; +Cc: network dev, linux-sctp, davem, Neil Horman

On Fri, Mar 08, 2019 at 11:48:10AM +0800, Xin Long wrote:
> On Fri, Mar 8, 2019 at 2:25 AM Marcelo Ricardo Leitner
> <marcelo.leitner@gmail.com> wrote:
> >
> > On Sun, Mar 03, 2019 at 05:54:53PM +0800, Xin Long wrote:
> > > It should fail to create the new sk if sctp_bind_addr_dup() fails
> > > when accepting or peeloff an association.
> > >
> > > Signed-off-by: Xin Long <lucien.xin@gmail.com>
> > > ---
> > >  net/sctp/socket.c | 34 ++++++++++++++++++++++++----------
> > >  1 file changed, 24 insertions(+), 10 deletions(-)
> > >
> > > diff --git a/net/sctp/socket.c b/net/sctp/socket.c
> > > index a2771b3..22adb8d 100644
> > > --- a/net/sctp/socket.c
> > > +++ b/net/sctp/socket.c
> > > @@ -102,9 +102,9 @@ static int sctp_send_asconf(struct sctp_association *asoc,
> > >                           struct sctp_chunk *chunk);
> > >  static int sctp_do_bind(struct sock *, union sctp_addr *, int);
> > >  static int sctp_autobind(struct sock *sk);
> > > -static void sctp_sock_migrate(struct sock *oldsk, struct sock *newsk,
> > > -                           struct sctp_association *assoc,
> > > -                           enum sctp_socket_type type);
> > > +static int sctp_sock_migrate(struct sock *oldsk, struct sock *newsk,
> > > +                          struct sctp_association *assoc,
> > > +                          enum sctp_socket_type type);
> > >
> > >  static unsigned long sctp_memory_pressure;
> > >  static atomic_long_t sctp_memory_allocated;
> > > @@ -4655,7 +4655,11 @@ static struct sock *sctp_accept(struct sock *sk, int flags, int *err, bool kern)
> > >       /* Populate the fields of the newsk from the oldsk and migrate the
> > >        * asoc to the newsk.
> > >        */
> > > -     sctp_sock_migrate(sk, newsk, asoc, SCTP_SOCKET_TCP);
> > > +     error = sctp_sock_migrate(sk, newsk, asoc, SCTP_SOCKET_TCP);
> > > +     if (error) {
> > > +             sk_common_release(newsk);
> > > +             newsk = NULL;
> > > +     }
> > >
> > >  out:
> > >       release_sock(sk);
> > > @@ -5401,7 +5405,12 @@ int sctp_do_peeloff(struct sock *sk, sctp_assoc_t id, struct socket **sockp)
> > >       /* Populate the fields of the newsk from the oldsk and migrate the
> > >        * asoc to the newsk.
> > >        */
> > > -     sctp_sock_migrate(sk, sock->sk, asoc, SCTP_SOCKET_UDP_HIGH_BANDWIDTH);
> > > +     err = sctp_sock_migrate(sk, sock->sk, asoc,
> > > +                             SCTP_SOCKET_UDP_HIGH_BANDWIDTH);
> > > +     if (err) {
> > > +             sock_release(sock);
> >
> > I'm having a hard time understanding why it needs sock_release() here
> > and sk_common_release() in the above chunk. AFAICT by here (after
> > sctp_sock_migrate call) the sockets are pretty much the same. Mind
> > elaborating please?
> In sctp_do_peeloff(),'sock' is 'struct socket' created by sock_create(),
> which should be freed by sock_release(struct socket *sock), otherwise
> no place takes care of its freeing.
> (sock_create() also create sock->sk)
> 
> In sctp_accept(), 'newsk' is 'struct sock' created by sk_alloc(), and
> it should be freed by sk_common_release(). Note by then sock->sk is not
> yet set to 'newsk', so when it return errs, outside can't free newsk
> when freeing sock.
> (sys_accept() calls sock_alloc() directly that doesn't set sock->sk)

Thanks.

  Marcelo

^ permalink raw reply	[flat|nested] 34+ messages in thread

* Re: [PATCH net 1/3] sctp: sctp_sock_migrate() returns error if sctp_bind_addr_dup() fails
@ 2019-03-08 16:59         ` Marcelo Ricardo Leitner
  0 siblings, 0 replies; 34+ messages in thread
From: Marcelo Ricardo Leitner @ 2019-03-08 16:59 UTC (permalink / raw)
  To: Xin Long; +Cc: network dev, linux-sctp, davem, Neil Horman

On Fri, Mar 08, 2019 at 11:48:10AM +0800, Xin Long wrote:
> On Fri, Mar 8, 2019 at 2:25 AM Marcelo Ricardo Leitner
> <marcelo.leitner@gmail.com> wrote:
> >
> > On Sun, Mar 03, 2019 at 05:54:53PM +0800, Xin Long wrote:
> > > It should fail to create the new sk if sctp_bind_addr_dup() fails
> > > when accepting or peeloff an association.
> > >
> > > Signed-off-by: Xin Long <lucien.xin@gmail.com>
> > > ---
> > >  net/sctp/socket.c | 34 ++++++++++++++++++++++++----------
> > >  1 file changed, 24 insertions(+), 10 deletions(-)
> > >
> > > diff --git a/net/sctp/socket.c b/net/sctp/socket.c
> > > index a2771b3..22adb8d 100644
> > > --- a/net/sctp/socket.c
> > > +++ b/net/sctp/socket.c
> > > @@ -102,9 +102,9 @@ static int sctp_send_asconf(struct sctp_association *asoc,
> > >                           struct sctp_chunk *chunk);
> > >  static int sctp_do_bind(struct sock *, union sctp_addr *, int);
> > >  static int sctp_autobind(struct sock *sk);
> > > -static void sctp_sock_migrate(struct sock *oldsk, struct sock *newsk,
> > > -                           struct sctp_association *assoc,
> > > -                           enum sctp_socket_type type);
> > > +static int sctp_sock_migrate(struct sock *oldsk, struct sock *newsk,
> > > +                          struct sctp_association *assoc,
> > > +                          enum sctp_socket_type type);
> > >
> > >  static unsigned long sctp_memory_pressure;
> > >  static atomic_long_t sctp_memory_allocated;
> > > @@ -4655,7 +4655,11 @@ static struct sock *sctp_accept(struct sock *sk, int flags, int *err, bool kern)
> > >       /* Populate the fields of the newsk from the oldsk and migrate the
> > >        * asoc to the newsk.
> > >        */
> > > -     sctp_sock_migrate(sk, newsk, asoc, SCTP_SOCKET_TCP);
> > > +     error = sctp_sock_migrate(sk, newsk, asoc, SCTP_SOCKET_TCP);
> > > +     if (error) {
> > > +             sk_common_release(newsk);
> > > +             newsk = NULL;
> > > +     }
> > >
> > >  out:
> > >       release_sock(sk);
> > > @@ -5401,7 +5405,12 @@ int sctp_do_peeloff(struct sock *sk, sctp_assoc_t id, struct socket **sockp)
> > >       /* Populate the fields of the newsk from the oldsk and migrate the
> > >        * asoc to the newsk.
> > >        */
> > > -     sctp_sock_migrate(sk, sock->sk, asoc, SCTP_SOCKET_UDP_HIGH_BANDWIDTH);
> > > +     err = sctp_sock_migrate(sk, sock->sk, asoc,
> > > +                             SCTP_SOCKET_UDP_HIGH_BANDWIDTH);
> > > +     if (err) {
> > > +             sock_release(sock);
> >
> > I'm having a hard time understanding why it needs sock_release() here
> > and sk_common_release() in the above chunk. AFAICT by here (after
> > sctp_sock_migrate call) the sockets are pretty much the same. Mind
> > elaborating please?
> In sctp_do_peeloff(),'sock' is 'struct socket' created by sock_create(),
> which should be freed by sock_release(struct socket *sock), otherwise
> no place takes care of its freeing.
> (sock_create() also create sock->sk)
> 
> In sctp_accept(), 'newsk' is 'struct sock' created by sk_alloc(), and
> it should be freed by sk_common_release(). Note by then sock->sk is not
> yet set to 'newsk', so when it return errs, outside can't free newsk
> when freeing sock.
> (sys_accept() calls sock_alloc() directly that doesn't set sock->sk)

Thanks.

  Marcelo

^ permalink raw reply	[flat|nested] 34+ messages in thread

* Re: [PATCH net 0/3] sctp: process the error returned from sctp_sock_migrate()
  2019-03-03  9:54 ` Xin Long
@ 2019-03-08 17:00   ` Marcelo Ricardo Leitner
  -1 siblings, 0 replies; 34+ messages in thread
From: Marcelo Ricardo Leitner @ 2019-03-08 17:00 UTC (permalink / raw)
  To: Xin Long; +Cc: network dev, linux-sctp, davem, Neil Horman

On Sun, Mar 03, 2019 at 05:54:52PM +0800, Xin Long wrote:
> This patchset is to process the errs returned by sctp_auth_init_hmacs()
> and sctp_bind_addr_dup() from sctp_sock_migrate(). And also fix a panic
> caused by new ep->auth_hmacs was not set due to net->sctp.auth_enable
> changed by sysctl before accepting an assoc.
> 
> Xin Long (3):
>   sctp: sctp_sock_migrate() returns error if sctp_bind_addr_dup() fails
>   sctp: move up sctp_auth_init_hmacs() in sctp_endpoint_init()
>   sctp: call sctp_auth_init_hmacs() in sctp_sock_migrate()

Series
Acked-by: Marcelo Ricardo Leitner <marcelo.leitner@gmail.com>

^ permalink raw reply	[flat|nested] 34+ messages in thread

* Re: [PATCH net 0/3] sctp: process the error returned from sctp_sock_migrate()
@ 2019-03-08 17:00   ` Marcelo Ricardo Leitner
  0 siblings, 0 replies; 34+ messages in thread
From: Marcelo Ricardo Leitner @ 2019-03-08 17:00 UTC (permalink / raw)
  To: Xin Long; +Cc: network dev, linux-sctp, davem, Neil Horman

On Sun, Mar 03, 2019 at 05:54:52PM +0800, Xin Long wrote:
> This patchset is to process the errs returned by sctp_auth_init_hmacs()
> and sctp_bind_addr_dup() from sctp_sock_migrate(). And also fix a panic
> caused by new ep->auth_hmacs was not set due to net->sctp.auth_enable
> changed by sysctl before accepting an assoc.
> 
> Xin Long (3):
>   sctp: sctp_sock_migrate() returns error if sctp_bind_addr_dup() fails
>   sctp: move up sctp_auth_init_hmacs() in sctp_endpoint_init()
>   sctp: call sctp_auth_init_hmacs() in sctp_sock_migrate()

Series
Acked-by: Marcelo Ricardo Leitner <marcelo.leitner@gmail.com>

^ permalink raw reply	[flat|nested] 34+ messages in thread

* Re: [PATCH net 0/3] sctp: process the error returned from sctp_sock_migrate()
  2019-03-08 17:00   ` Marcelo Ricardo Leitner
@ 2019-03-08 19:43     ` David Miller
  -1 siblings, 0 replies; 34+ messages in thread
From: David Miller @ 2019-03-08 19:43 UTC (permalink / raw)
  To: marcelo.leitner; +Cc: lucien.xin, netdev, linux-sctp, nhorman

From: Marcelo Ricardo Leitner <marcelo.leitner@gmail.com>
Date: Fri, 8 Mar 2019 14:00:03 -0300

> On Sun, Mar 03, 2019 at 05:54:52PM +0800, Xin Long wrote:
>> This patchset is to process the errs returned by sctp_auth_init_hmacs()
>> and sctp_bind_addr_dup() from sctp_sock_migrate(). And also fix a panic
>> caused by new ep->auth_hmacs was not set due to net->sctp.auth_enable
>> changed by sysctl before accepting an assoc.
>> 
>> Xin Long (3):
>>   sctp: sctp_sock_migrate() returns error if sctp_bind_addr_dup() fails
>>   sctp: move up sctp_auth_init_hmacs() in sctp_endpoint_init()
>>   sctp: call sctp_auth_init_hmacs() in sctp_sock_migrate()
> 
> Series
> Acked-by: Marcelo Ricardo Leitner <marcelo.leitner@gmail.com>

Series applied.

^ permalink raw reply	[flat|nested] 34+ messages in thread

* Re: [PATCH net 0/3] sctp: process the error returned from sctp_sock_migrate()
@ 2019-03-08 19:43     ` David Miller
  0 siblings, 0 replies; 34+ messages in thread
From: David Miller @ 2019-03-08 19:43 UTC (permalink / raw)
  To: marcelo.leitner; +Cc: lucien.xin, netdev, linux-sctp, nhorman

From: Marcelo Ricardo Leitner <marcelo.leitner@gmail.com>
Date: Fri, 8 Mar 2019 14:00:03 -0300

> On Sun, Mar 03, 2019 at 05:54:52PM +0800, Xin Long wrote:
>> This patchset is to process the errs returned by sctp_auth_init_hmacs()
>> and sctp_bind_addr_dup() from sctp_sock_migrate(). And also fix a panic
>> caused by new ep->auth_hmacs was not set due to net->sctp.auth_enable
>> changed by sysctl before accepting an assoc.
>> 
>> Xin Long (3):
>>   sctp: sctp_sock_migrate() returns error if sctp_bind_addr_dup() fails
>>   sctp: move up sctp_auth_init_hmacs() in sctp_endpoint_init()
>>   sctp: call sctp_auth_init_hmacs() in sctp_sock_migrate()
> 
> Series
> Acked-by: Marcelo Ricardo Leitner <marcelo.leitner@gmail.com>

Series applied.

^ permalink raw reply	[flat|nested] 34+ messages in thread

end of thread, other threads:[~2019-03-08 19:43 UTC | newest]

Thread overview: 34+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-03-03  9:54 [PATCH net 0/3] sctp: process the error returned from sctp_sock_migrate() Xin Long
2019-03-03  9:54 ` Xin Long
2019-03-03  9:54 ` [PATCH net 1/3] sctp: sctp_sock_migrate() returns error if sctp_bind_addr_dup() fails Xin Long
2019-03-03  9:54   ` Xin Long
2019-03-03  9:54   ` [PATCH net 2/3] sctp: move up sctp_auth_init_hmacs() in sctp_endpoint_init() Xin Long
2019-03-03  9:54     ` Xin Long
2019-03-03  9:54     ` [PATCH net 3/3] sctp: call sctp_auth_init_hmacs() in sctp_sock_migrate() Xin Long
2019-03-03  9:54       ` Xin Long
2019-03-06 18:26       ` Neil Horman
2019-03-06 18:26         ` Neil Horman
2019-03-06 18:24     ` [PATCH net 2/3] sctp: move up sctp_auth_init_hmacs() in sctp_endpoint_init() Neil Horman
2019-03-06 18:24       ` Neil Horman
2019-03-06 18:21   ` [PATCH net 1/3] sctp: sctp_sock_migrate() returns error if sctp_bind_addr_dup() fails Neil Horman
2019-03-06 18:21     ` Neil Horman
2019-03-07 10:06     ` Xin Long
2019-03-07 10:06       ` Xin Long
2019-03-07 11:59       ` Neil Horman
2019-03-07 11:59         ` Neil Horman
2019-03-07 18:25   ` Marcelo Ricardo Leitner
2019-03-07 18:25     ` Marcelo Ricardo Leitner
2019-03-08  3:48     ` Xin Long
2019-03-08  3:48       ` Xin Long
2019-03-08 16:59       ` Marcelo Ricardo Leitner
2019-03-08 16:59         ` Marcelo Ricardo Leitner
2019-03-04 19:04 ` [PATCH net 0/3] sctp: process the error returned from sctp_sock_migrate() David Miller
2019-03-04 19:04   ` David Miller
2019-03-07 14:59   ` Marcelo Ricardo Leitner
2019-03-07 14:59     ` Marcelo Ricardo Leitner
2019-03-07 12:11 ` Neil Horman
2019-03-07 12:11   ` Neil Horman
2019-03-08 17:00 ` Marcelo Ricardo Leitner
2019-03-08 17:00   ` Marcelo Ricardo Leitner
2019-03-08 19:43   ` David Miller
2019-03-08 19:43     ` David Miller

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.