[PATCH net-next 0/3] Minor sctp cleanups

[PATCH net-next 0/3] Minor sctp cleanups

[Daniel Borkmann]

Daniel Borkmann (3):
  net: sctp: let sctp_destroy_sock destroy related members
  net: sctp: sctp_association_init: hold refs in reverse order
  net: sctp: minor: remove variable in sctp_init_sock

 net/sctp/associola.c   |  5 ++---
 net/sctp/endpointola.c |  7 -------
 net/sctp/socket.c      | 15 +++++++++++----
 3 files changed, 13 insertions(+), 14 deletions(-)

-- 
1.7.11.7

[PATCH net-next 1/3] net: sctp: let sctp_destroy_sock destroy related members

[Daniel Borkmann]

Release socket related data during sctp_destroy_sock() after we
called sctp_endpoint_free(), but still before we leave the socket
destruction callback. The socket's hmac and bind_hash are only
used in socket.c and actually not in endpointola.c, so also let
it collect the garbage there, no need to go this detour via
endpoints.

Signed-off-by: Daniel Borkmann <dborkman@redhat.com>
---
 net/sctp/endpointola.c | 7 -------
 net/sctp/socket.c      | 9 +++++++++
 2 files changed, 9 insertions(+), 7 deletions(-)

diff --git a/net/sctp/endpointola.c b/net/sctp/endpointola.c
index 5fbd7bc..ecfba70 100644
--- a/net/sctp/endpointola.c
+++ b/net/sctp/endpointola.c
@@ -248,9 +248,6 @@ static void sctp_endpoint_destroy(struct sctp_endpoint *ep)
 {
 	SCTP_ASSERT(ep->base.dead, "Endpoint is not dead", return);
 
-	/* Free up the HMAC transform. */
-	crypto_free_hash(sctp_sk(ep->base.sk)->hmac);
-
 	/* Free the digest buffer */
 	kfree(ep->digest);
 
@@ -270,10 +267,6 @@ static void sctp_endpoint_destroy(struct sctp_endpoint *ep)
 
 	memset(ep->secret_key, 0, sizeof(ep->secret_key));
 
-	/* Remove and free the port */
-	if (sctp_sk(ep->base.sk)->bind_hash)
-		sctp_put_port(ep->base.sk);
-
 	/* Give up our hold on the sock. */
 	if (ep->base.sk)
 		sock_put(ep->base.sk);
diff --git a/net/sctp/socket.c b/net/sctp/socket.c
index f631c5f..3267534 100644
--- a/net/sctp/socket.c
+++ b/net/sctp/socket.c
@@ -4007,7 +4007,16 @@ SCTP_STATIC void sctp_destroy_sock(struct sock *sk)
 		sp->do_auto_asconf = 0;
 		list_del(&sp->auto_asconf_list);
 	}
+
 	sctp_endpoint_free(sp->ep);
+
+	/* Free up the HMAC transform. */
+	crypto_free_hash(sp->hmac);
+
+	/* Remove and free the port */
+	if (sp->bind_hash)
+		sctp_put_port(sk);
+
 	local_bh_disable();
 	percpu_counter_dec(&sctp_sockets_allocated);
 	sock_prot_inuse_add(sock_net(sk), sk->sk_prot, -1);
-- 
1.7.11.7

Re: [PATCH net-next 1/3] net: sctp: let sctp_destroy_sock destroy related members

[Neil Horman]

On Fri, Jun 07, 2013 at 10:35:04AM +0200, Daniel Borkmann wrote:
> Release socket related data during sctp_destroy_sock() after we
> called sctp_endpoint_free(), but still before we leave the socket
> destruction callback. The socket's hmac and bind_hash are only
> used in socket.c and actually not in endpointola.c, so also let
> it collect the garbage there, no need to go this detour via
> endpoints.
> 
> Signed-off-by: Daniel Borkmann <dborkman@redhat.com>
> ---
>  net/sctp/endpointola.c | 7 -------
>  net/sctp/socket.c      | 9 +++++++++
>  2 files changed, 9 insertions(+), 7 deletions(-)
> 
> diff --git a/net/sctp/endpointola.c b/net/sctp/endpointola.c
> index 5fbd7bc..ecfba70 100644
> --- a/net/sctp/endpointola.c
> +++ b/net/sctp/endpointola.c
> @@ -248,9 +248,6 @@ static void sctp_endpoint_destroy(struct sctp_endpoint *ep)
>  {
>  	SCTP_ASSERT(ep->base.dead, "Endpoint is not dead", return);
>  
> -	/* Free up the HMAC transform. */
> -	crypto_free_hash(sctp_sk(ep->base.sk)->hmac);
> -
>  	/* Free the digest buffer */
>  	kfree(ep->digest);
>  
> @@ -270,10 +267,6 @@ static void sctp_endpoint_destroy(struct sctp_endpoint *ep)
>  
>  	memset(ep->secret_key, 0, sizeof(ep->secret_key));
>  
> -	/* Remove and free the port */
> -	if (sctp_sk(ep->base.sk)->bind_hash)
> -		sctp_put_port(ep->base.sk);
> -
>  	/* Give up our hold on the sock. */
>  	if (ep->base.sk)
>  		sock_put(ep->base.sk);
> diff --git a/net/sctp/socket.c b/net/sctp/socket.c
> index f631c5f..3267534 100644
> --- a/net/sctp/socket.c
> +++ b/net/sctp/socket.c
> @@ -4007,7 +4007,16 @@ SCTP_STATIC void sctp_destroy_sock(struct sock *sk)
>  		sp->do_auto_asconf = 0;
>  		list_del(&sp->auto_asconf_list);
>  	}
> +
>  	sctp_endpoint_free(sp->ep);
> +
> +	/* Free up the HMAC transform. */
> +	crypto_free_hash(sp->hmac);
> +
> +	/* Remove and free the port */
> +	if (sp->bind_hash)
> +		sctp_put_port(sk);
> +
>  	local_bh_disable();
>  	percpu_counter_dec(&sctp_sockets_allocated);
>  	sock_prot_inuse_add(sock_net(sk), sk->sk_prot, -1);
> -- 

I'm not sure this is safe.  Comment in sk_common_release indicates that the
network can still find the socket in the receive path.  What if we receive a
cookie chunk while the socket is being torn down?  We would wind up using the
hmac to unpack it potentially after you just freed it.  I think you need to wait
until you drop the last reference to the endpoint, not whenever you destroy the
local socket.  Note that sctp_endpoint_free doesn't actually free anything, it
just removes it from the hash list so it can't be found again, and drops a
refcount.  If a parallel recieve op has already found it, hmac may still be
used.

Neil

Re: [PATCH net-next 1/3] net: sctp: let sctp_destroy_sock destroy related members

[Daniel Borkmann]

On 06/07/2013 12:54 PM, Neil Horman wrote:
> I'm not sure this is safe.  Comment in sk_common_release indicates that the
> network can still find the socket in the receive path.  What if we receive a
> cookie chunk while the socket is being torn down?  We would wind up using the
> hmac to unpack it potentially after you just freed it.  I think you need to wait
> until you drop the last reference to the endpoint, not whenever you destroy the
> local socket.  Note that sctp_endpoint_free doesn't actually free anything, it
> just removes it from the hash list so it can't be found again, and drops a
> refcount.  If a parallel recieve op has already found it, hmac may still be
> used.

Agreed, you're right, thanks for pointing this out Neil! Is it *always* guaranteed
that at the time the endpoint is destroyed in a deferred way (e.g. exactly in such
a scenario you describe), the socket structure is still alive and not yet freed?
Either the ep->base.sk test in sctp_endpoint_destroy() would then be unnecessary
or, if necessary, we should move crypto_free_hash() and sctp_put_port() within this
body since they deref. socket members (but then that memory would be leaked in case
ep->base.sk is NULL). Probably, it might be best to add sth like this to explicitly
decouple it from the endpoint, which is then called when all refs are released from
the socket; then we could call this from __sk_free() via sk->sk_destruct():

static void sctp_sock_destruct(struct sock *sk)
{
	struct sctp_sock *sp = sctp_sk(sk);

	inet_sock_destruct(sk);

	/* Free up the HMAC transform. */
	crypto_free_hash(sp->hmac);

	/* Remove and free the port */
	if (sp->bind_hash)
		sctp_put_port(sk);
}

Re: [PATCH net-next 1/3] net: sctp: let sctp_destroy_sock destroy related members

[Neil Horman]

On Fri, Jun 07, 2013 at 01:36:04PM +0200, Daniel Borkmann wrote:
> On 06/07/2013 12:54 PM, Neil Horman wrote:
> >I'm not sure this is safe.  Comment in sk_common_release indicates that the
> >network can still find the socket in the receive path.  What if we receive a
> >cookie chunk while the socket is being torn down?  We would wind up using the
> >hmac to unpack it potentially after you just freed it.  I think you need to wait
> >until you drop the last reference to the endpoint, not whenever you destroy the
> >local socket.  Note that sctp_endpoint_free doesn't actually free anything, it
> >just removes it from the hash list so it can't be found again, and drops a
> >refcount.  If a parallel recieve op has already found it, hmac may still be
> >used.
> 
> Agreed, you're right, thanks for pointing this out Neil! Is it *always* guaranteed
> that at the time the endpoint is destroyed in a deferred way (e.g. exactly in such
> a scenario you describe), the socket structure is still alive and not yet freed?
> Either the ep->base.sk test in sctp_endpoint_destroy() would then be unnecessary
> or, if necessary, we should move crypto_free_hash() and sctp_put_port() within this
> body since they deref. socket members (but then that memory would be leaked in case
> ep->base.sk is NULL). Probably, it might be best to add sth like this to explicitly
> decouple it from the endpoint, which is then called when all refs are released from
> the socket; then we could call this from __sk_free() via sk->sk_destruct():
> 
Thats a good question, I'm on vacation right now, so I'm not looking to closely
at much (I've spent all day in a pool).  I think what you're proposing below
probably makes sense.  Since the hmac crypo instance is allocated when the
socket transitions to the listening state in sctp_listen, it makes sense to
destroy it in sctp_sock_destroy.  If we need to we can protect it as an rcu
variable to protect it against parallel reads from cookie processing.  If it
fails in that case, its irrelevant, as the local socket is shutting down anyway.

Best
Neil

> static void sctp_sock_destruct(struct sock *sk)
> {
> 	struct sctp_sock *sp = sctp_sk(sk);
> 
> 	inet_sock_destruct(sk);
> 
> 	/* Free up the HMAC transform. */
> 	crypto_free_hash(sp->hmac);
> 
> 	/* Remove and free the port */
> 	if (sp->bind_hash)
> 		sctp_put_port(sk);
> }
> 

Re: [PATCH net-next 1/3] net: sctp: let sctp_destroy_sock destroy related members

[Daniel Borkmann]

On 06/09/2013 02:20 AM, Neil Horman wrote:
> On Fri, Jun 07, 2013 at 01:36:04PM +0200, Daniel Borkmann wrote:
>> On 06/07/2013 12:54 PM, Neil Horman wrote:
>>> I'm not sure this is safe.  Comment in sk_common_release indicates that the
>>> network can still find the socket in the receive path.  What if we receive a
>>> cookie chunk while the socket is being torn down?  We would wind up using the
>>> hmac to unpack it potentially after you just freed it.  I think you need to wait
>>> until you drop the last reference to the endpoint, not whenever you destroy the
>>> local socket.  Note that sctp_endpoint_free doesn't actually free anything, it
>>> just removes it from the hash list so it can't be found again, and drops a
>>> refcount.  If a parallel recieve op has already found it, hmac may still be
>>> used.
>>
>> Agreed, you're right, thanks for pointing this out Neil! Is it *always* guaranteed
>> that at the time the endpoint is destroyed in a deferred way (e.g. exactly in such
>> a scenario you describe), the socket structure is still alive and not yet freed?
>> Either the ep->base.sk test in sctp_endpoint_destroy() would then be unnecessary
>> or, if necessary, we should move crypto_free_hash() and sctp_put_port() within this
>> body since they deref. socket members (but then that memory would be leaked in case
>> ep->base.sk is NULL). Probably, it might be best to add sth like this to explicitly
>> decouple it from the endpoint, which is then called when all refs are released from
>> the socket; then we could call this from __sk_free() via sk->sk_destruct():
>>
> Thats a good question, I'm on vacation right now, so I'm not looking to closely
> at much (I've spent all day in a pool).  I think what you're proposing below
> probably makes sense.  Since the hmac crypo instance is allocated when the

Cool, sounds relaxing. :-) Have nice holidays then!

> socket transitions to the listening state in sctp_listen, it makes sense to
> destroy it in sctp_sock_destroy.  If we need to we can protect it as an rcu
> variable to protect it against parallel reads from cookie processing.  If it
> fails in that case, its irrelevant, as the local socket is shutting down anyway.

I'll evaluate this further and then send a v2 of the set, but I think it makes
sense this way.

Thanks,

Daniel

[PATCH net-next 2/3] net: sctp: sctp_association_init: hold refs in reverse order

[Daniel Borkmann]

In case we need to bail out for whatever reason during assoc
init, we call sctp_endpoint_put() and then sock_put(), however,
we've hold both refs in reverse order, so first sctp_endpoint_hold()
and then sock_hold(). Reverse this, so that we have sock_hold()
with sctp_endpoint_hold() first and then in error case
sctp_endpoint_put() and then sock_put(). Actually shouldn't
matter much since we just increase an atomic, but that way, it's
more clean.

Signed-off-by: Daniel Borkmann <dborkman@redhat.com>
---
 net/sctp/associola.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/net/sctp/associola.c b/net/sctp/associola.c
index 91cfd8f..04795fb 100644
--- a/net/sctp/associola.c
+++ b/net/sctp/associola.c
@@ -86,11 +86,10 @@ static struct sctp_association *sctp_association_init(struct sctp_association *a
 
 	/* Discarding const is appropriate here.  */
 	asoc->ep = (struct sctp_endpoint *)ep;
-	sctp_endpoint_hold(asoc->ep);
-
-	/* Hold the sock.  */
 	asoc->base.sk = (struct sock *)sk;
+
 	sock_hold(asoc->base.sk);
+	sctp_endpoint_hold(asoc->ep);
 
 	/* Initialize the common base substructure.  */
 	asoc->base.type = SCTP_EP_TYPE_ASSOCIATION;
-- 
1.7.11.7

Re: [PATCH net-next 2/3] net: sctp: sctp_association_init: hold refs in reverse order

[Neil Horman]

On Fri, Jun 07, 2013 at 10:35:05AM +0200, Daniel Borkmann wrote:
> In case we need to bail out for whatever reason during assoc
> init, we call sctp_endpoint_put() and then sock_put(), however,
> we've hold both refs in reverse order, so first sctp_endpoint_hold()
> and then sock_hold(). Reverse this, so that we have sock_hold()
> with sctp_endpoint_hold() first and then in error case
> sctp_endpoint_put() and then sock_put(). Actually shouldn't
> matter much since we just increase an atomic, but that way, it's
> more clean.
> 
> Signed-off-by: Daniel Borkmann <dborkman@redhat.com>
> ---
>  net/sctp/associola.c | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/net/sctp/associola.c b/net/sctp/associola.c
> index 91cfd8f..04795fb 100644
> --- a/net/sctp/associola.c
> +++ b/net/sctp/associola.c
> @@ -86,11 +86,10 @@ static struct sctp_association *sctp_association_init(struct sctp_association *a
>  
>  	/* Discarding const is appropriate here.  */
>  	asoc->ep = (struct sctp_endpoint *)ep;
> -	sctp_endpoint_hold(asoc->ep);
> -
> -	/* Hold the sock.  */
>  	asoc->base.sk = (struct sock *)sk;
> +
>  	sock_hold(asoc->base.sk);
> +	sctp_endpoint_hold(asoc->ep);
>  
>  	/* Initialize the common base substructure.  */
>  	asoc->base.type = SCTP_EP_TYPE_ASSOCIATION;

This looks good, but you may want to instead reverse the order in which we do
the puts at fail_init, as other call sites that hold both endpoint socket do so
in endpoint, sock order, and it would probably be nice to be consistent in that
order.

Neil

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

Re: [PATCH net-next 2/3] net: sctp: sctp_association_init: hold refs in reverse order

[Daniel Borkmann]

On 06/07/2013 01:00 PM, Neil Horman wrote:
> On Fri, Jun 07, 2013 at 10:35:05AM +0200, Daniel Borkmann wrote:
>> In case we need to bail out for whatever reason during assoc
>> init, we call sctp_endpoint_put() and then sock_put(), however,
>> we've hold both refs in reverse order, so first sctp_endpoint_hold()
>> and then sock_hold(). Reverse this, so that we have sock_hold()
>> with sctp_endpoint_hold() first and then in error case
>> sctp_endpoint_put() and then sock_put(). Actually shouldn't
>> matter much since we just increase an atomic, but that way, it's
>> more clean.
>>
>> Signed-off-by: Daniel Borkmann <dborkman@redhat.com>
>> ---
>>   net/sctp/associola.c | 5 ++---
>>   1 file changed, 2 insertions(+), 3 deletions(-)
>>
>> diff --git a/net/sctp/associola.c b/net/sctp/associola.c
>> index 91cfd8f..04795fb 100644
>> --- a/net/sctp/associola.c
>> +++ b/net/sctp/associola.c
>> @@ -86,11 +86,10 @@ static struct sctp_association *sctp_association_init(struct sctp_association *a
>>
>>   	/* Discarding const is appropriate here.  */
>>   	asoc->ep = (struct sctp_endpoint *)ep;
>> -	sctp_endpoint_hold(asoc->ep);
>> -
>> -	/* Hold the sock.  */
>>   	asoc->base.sk = (struct sock *)sk;
>> +
>>   	sock_hold(asoc->base.sk);
>> +	sctp_endpoint_hold(asoc->ep);
>>
>>   	/* Initialize the common base substructure.  */
>>   	asoc->base.type = SCTP_EP_TYPE_ASSOCIATION;
>
> This looks good, but you may want to instead reverse the order in which we do
> the puts at fail_init, as other call sites that hold both endpoint socket do so
> in endpoint, sock order, and it would probably be nice to be consistent in that
> order.

Thanks, will do. When we have clarified the 1st patch, I'll simply respin the
entire patchset and send a v2.

Re: [PATCH net-next 2/3] net: sctp: sctp_association_init: hold refs in reverse order

[Neil Horman]

On Fri, Jun 07, 2013 at 01:38:56PM +0200, Daniel Borkmann wrote:
> On 06/07/2013 01:00 PM, Neil Horman wrote:
> >On Fri, Jun 07, 2013 at 10:35:05AM +0200, Daniel Borkmann wrote:
> >>In case we need to bail out for whatever reason during assoc
> >>init, we call sctp_endpoint_put() and then sock_put(), however,
> >>we've hold both refs in reverse order, so first sctp_endpoint_hold()
> >>and then sock_hold(). Reverse this, so that we have sock_hold()
> >>with sctp_endpoint_hold() first and then in error case
> >>sctp_endpoint_put() and then sock_put(). Actually shouldn't
> >>matter much since we just increase an atomic, but that way, it's
> >>more clean.
> >>
> >>Signed-off-by: Daniel Borkmann <dborkman@redhat.com>
> >>---
> >>  net/sctp/associola.c | 5 ++---
> >>  1 file changed, 2 insertions(+), 3 deletions(-)
> >>
> >>diff --git a/net/sctp/associola.c b/net/sctp/associola.c
> >>index 91cfd8f..04795fb 100644
> >>--- a/net/sctp/associola.c
> >>+++ b/net/sctp/associola.c
> >>@@ -86,11 +86,10 @@ static struct sctp_association *sctp_association_init(struct sctp_association *a
> >>
> >>  	/* Discarding const is appropriate here.  */
> >>  	asoc->ep = (struct sctp_endpoint *)ep;
> >>-	sctp_endpoint_hold(asoc->ep);
> >>-
> >>-	/* Hold the sock.  */
> >>  	asoc->base.sk = (struct sock *)sk;
> >>+
> >>  	sock_hold(asoc->base.sk);
> >>+	sctp_endpoint_hold(asoc->ep);
> >>
> >>  	/* Initialize the common base substructure.  */
> >>  	asoc->base.type = SCTP_EP_TYPE_ASSOCIATION;
> >
> >This looks good, but you may want to instead reverse the order in which we do
> >the puts at fail_init, as other call sites that hold both endpoint socket do so
> >in endpoint, sock order, and it would probably be nice to be consistent in that
> >order.
> 
> Thanks, will do. When we have clarified the 1st patch, I'll simply respin the
> entire patchset and send a v2.
> 
Sounds great, thanks.  Just FYI, I'm out of town so may not respond super fast.
Will do my best though.
Neil

[PATCH net-next 3/3] net: sctp: minor: remove variable in sctp_init_sock

[Daniel Borkmann]

It's only used at this one time, so we could remove it as well.
This is valid and also makes it more explicit/obvious that in case
of error the sp->ep is NULL here, i.e. for the sctp_destroy_sock()
check.

Signed-off-by: Daniel Borkmann <dborkman@redhat.com>
---
 net/sctp/socket.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/net/sctp/socket.c b/net/sctp/socket.c
index 3267534..e71d399 100644
--- a/net/sctp/socket.c
+++ b/net/sctp/socket.c
@@ -3862,7 +3862,6 @@ out:
 SCTP_STATIC int sctp_init_sock(struct sock *sk)
 {
 	struct net *net = sock_net(sk);
-	struct sctp_endpoint *ep;
 	struct sctp_sock *sp;
 
 	SCTP_DEBUG_PRINTK("sctp_init_sock(sk: %p)\n", sk);
@@ -3971,11 +3970,10 @@ SCTP_STATIC int sctp_init_sock(struct sock *sk)
 	 * change the data structure relationships, this may still
 	 * be useful for storing pre-connect address information.
 	 */
-	ep = sctp_endpoint_new(sk, GFP_KERNEL);
-	if (!ep)
+	sp->ep = sctp_endpoint_new(sk, GFP_KERNEL);
+	if (!sp->ep)
 		return -ENOMEM;
 
-	sp->ep = ep;
 	sp->hmac = NULL;
 
 	SCTP_DBG_OBJCNT_INC(sock);
-- 
1.7.11.7

Re: [PATCH net-next 3/3] net: sctp: minor: remove variable in sctp_init_sock

[Neil Horman]

On Fri, Jun 07, 2013 at 10:35:06AM +0200, Daniel Borkmann wrote:
> It's only used at this one time, so we could remove it as well.
> This is valid and also makes it more explicit/obvious that in case
> of error the sp->ep is NULL here, i.e. for the sctp_destroy_sock()
> check.
> 
> Signed-off-by: Daniel Borkmann <dborkman@redhat.com>
Acked-by: Neil Horman <nhorman@tuxdriver.com>

> ---
>  net/sctp/socket.c | 6 ++----
>  1 file changed, 2 insertions(+), 4 deletions(-)
> 
> diff --git a/net/sctp/socket.c b/net/sctp/socket.c
> index 3267534..e71d399 100644
> --- a/net/sctp/socket.c
> +++ b/net/sctp/socket.c
> @@ -3862,7 +3862,6 @@ out:
>  SCTP_STATIC int sctp_init_sock(struct sock *sk)
>  {
>  	struct net *net = sock_net(sk);
> -	struct sctp_endpoint *ep;
>  	struct sctp_sock *sp;
>  
>  	SCTP_DEBUG_PRINTK("sctp_init_sock(sk: %p)\n", sk);
> @@ -3971,11 +3970,10 @@ SCTP_STATIC int sctp_init_sock(struct sock *sk)
>  	 * change the data structure relationships, this may still
>  	 * be useful for storing pre-connect address information.
>  	 */
> -	ep = sctp_endpoint_new(sk, GFP_KERNEL);
> -	if (!ep)
> +	sp->ep = sctp_endpoint_new(sk, GFP_KERNEL);
> +	if (!sp->ep)
>  		return -ENOMEM;
>  
> -	sp->ep = ep;
>  	sp->hmac = NULL;
>  
>  	SCTP_DBG_OBJCNT_INC(sock);
> -- 
> 1.7.11.7
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-sctp" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>