[PATCH net 0/3] fix the transport dead race check by using atomic_add_unless on refcnt

[PATCH net 0/3] fix the transport dead race check by using atomic_add_unless on refcnt

[Xin Long]

Xin Long (3):
  sctp: fix the transport dead race check by using atomic_add_unless on
    refcnt
  sctp: hold transport before we access t->asoc in sctp proc
  sctp: remove the dead field of sctp_transport

 include/net/sctp/structs.h |  5 ++---
 net/sctp/input.c           | 17 +++++++++++------
 net/sctp/proc.c            | 12 ++++++++----
 net/sctp/sm_sideeffect.c   | 12 ------------
 net/sctp/transport.c       |  8 +++-----
 5 files changed, 24 insertions(+), 30 deletions(-)

-- 
2.1.0

[PATCH net 0/3] fix the transport dead race check by using atomic_add_unless on refcnt

[Xin Long]

Xin Long (3):
  sctp: fix the transport dead race check by using atomic_add_unless on
    refcnt
  sctp: hold transport before we access t->asoc in sctp proc
  sctp: remove the dead field of sctp_transport

 include/net/sctp/structs.h |  5 ++---
 net/sctp/input.c           | 17 +++++++++++------
 net/sctp/proc.c            | 12 ++++++++----
 net/sctp/sm_sideeffect.c   | 12 ------------
 net/sctp/transport.c       |  8 +++-----
 5 files changed, 24 insertions(+), 30 deletions(-)

-- 
2.1.0

[PATCH net 1/3] sctp: fix the transport dead race check by using atomic_add_unless on refcnt

[Xin Long]

Now when __sctp_lookup_association is running in BH, it will try to
check if t->dead is set, but meanwhile other CPUs may be freeing this
transport and this assoc and if it happens that
__sctp_lookup_association checked t->dead a bit too early, it may think
that the association is still good while it was already freed.

So we fix this race by using atomic_add_unless in sctp_transport_hold.
After we get one transport from hashtable, we will hold it only when
this transport's refcnt is not 0, so that we can make sure t->asoc
cannot be freed before we hold the asoc again.

Note that sctp association is not freed using RCU so we can't use
atomic_add_unless() with it as it may just be too late for that either.

Fixes: 4f0087812648 ("sctp: apply rhashtable api to send/recv path")
Reported-by: Vlad Yasevich <vyasevich@gmail.com>
Signed-off-by: Xin Long <lucien.xin@gmail.com>
---
 include/net/sctp/structs.h |  2 +-
 net/sctp/input.c           | 17 +++++++++++------
 net/sctp/transport.c       |  4 ++--
 3 files changed, 14 insertions(+), 9 deletions(-)

diff --git a/include/net/sctp/structs.h b/include/net/sctp/structs.h
index 20e7212..344da04 100644
--- a/include/net/sctp/structs.h
+++ b/include/net/sctp/structs.h
@@ -955,7 +955,7 @@ void sctp_transport_route(struct sctp_transport *, union sctp_addr *,
 void sctp_transport_pmtu(struct sctp_transport *, struct sock *sk);
 void sctp_transport_free(struct sctp_transport *);
 void sctp_transport_reset_timers(struct sctp_transport *);
-void sctp_transport_hold(struct sctp_transport *);
+int sctp_transport_hold(struct sctp_transport *);
 void sctp_transport_put(struct sctp_transport *);
 void sctp_transport_update_rto(struct sctp_transport *, __u32);
 void sctp_transport_raise_cwnd(struct sctp_transport *, __u32, __u32);
diff --git a/net/sctp/input.c b/net/sctp/input.c
index bf61dfb..49d2cc7 100644
--- a/net/sctp/input.c
+++ b/net/sctp/input.c
@@ -935,15 +935,22 @@ static struct sctp_association *__sctp_lookup_association(
 					struct sctp_transport **pt)
 {
 	struct sctp_transport *t;
+	struct sctp_association *asoc = NULL;
 
+	rcu_read_lock();
 	t = sctp_addrs_lookup_transport(net, local, peer);
-	if (!t || t->dead)
-		return NULL;
+	if (!t || !sctp_transport_hold(t))
+		goto out;
 
-	sctp_association_hold(t->asoc);
+	asoc = t->asoc;
+	sctp_association_hold(asoc);
 	*pt = t;
 
-	return t->asoc;
+	sctp_transport_put(t);
+
+out:
+	rcu_read_unlock();
+	return asoc;
 }
 
 /* Look up an association. protected by RCU read lock */
@@ -955,9 +962,7 @@ struct sctp_association *sctp_lookup_association(struct net *net,
 {
 	struct sctp_association *asoc;
 
-	rcu_read_lock();
 	asoc = __sctp_lookup_association(net, laddr, paddr, transportp);
-	rcu_read_unlock();
 
 	return asoc;
 }
diff --git a/net/sctp/transport.c b/net/sctp/transport.c
index aab9e3f..69f3799 100644
--- a/net/sctp/transport.c
+++ b/net/sctp/transport.c
@@ -296,9 +296,9 @@ void sctp_transport_route(struct sctp_transport *transport,
 }
 
 /* Hold a reference to a transport.  */
-void sctp_transport_hold(struct sctp_transport *transport)
+int sctp_transport_hold(struct sctp_transport *transport)
 {
-	atomic_inc(&transport->refcnt);
+	return atomic_add_unless(&transport->refcnt, 1, 0);
 }
 
 /* Release a reference to a transport and clean up
-- 
2.1.0

[PATCH net 1/3] sctp: fix the transport dead race check by using atomic_add_unless on refcnt

[Xin Long]

Now when __sctp_lookup_association is running in BH, it will try to
check if t->dead is set, but meanwhile other CPUs may be freeing this
transport and this assoc and if it happens that
__sctp_lookup_association checked t->dead a bit too early, it may think
that the association is still good while it was already freed.

So we fix this race by using atomic_add_unless in sctp_transport_hold.
After we get one transport from hashtable, we will hold it only when
this transport's refcnt is not 0, so that we can make sure t->asoc
cannot be freed before we hold the asoc again.

Note that sctp association is not freed using RCU so we can't use
atomic_add_unless() with it as it may just be too late for that either.

Fixes: 4f0087812648 ("sctp: apply rhashtable api to send/recv path")
Reported-by: Vlad Yasevich <vyasevich@gmail.com>
Signed-off-by: Xin Long <lucien.xin@gmail.com>
---
 include/net/sctp/structs.h |  2 +-
 net/sctp/input.c           | 17 +++++++++++------
 net/sctp/transport.c       |  4 ++--
 3 files changed, 14 insertions(+), 9 deletions(-)

diff --git a/include/net/sctp/structs.h b/include/net/sctp/structs.h
index 20e7212..344da04 100644
--- a/include/net/sctp/structs.h
+++ b/include/net/sctp/structs.h
@@ -955,7 +955,7 @@ void sctp_transport_route(struct sctp_transport *, union sctp_addr *,
 void sctp_transport_pmtu(struct sctp_transport *, struct sock *sk);
 void sctp_transport_free(struct sctp_transport *);
 void sctp_transport_reset_timers(struct sctp_transport *);
-void sctp_transport_hold(struct sctp_transport *);
+int sctp_transport_hold(struct sctp_transport *);
 void sctp_transport_put(struct sctp_transport *);
 void sctp_transport_update_rto(struct sctp_transport *, __u32);
 void sctp_transport_raise_cwnd(struct sctp_transport *, __u32, __u32);
diff --git a/net/sctp/input.c b/net/sctp/input.c
index bf61dfb..49d2cc7 100644
--- a/net/sctp/input.c
+++ b/net/sctp/input.c
@@ -935,15 +935,22 @@ static struct sctp_association *__sctp_lookup_association(
 					struct sctp_transport **pt)
 {
 	struct sctp_transport *t;
+	struct sctp_association *asoc = NULL;
 
+	rcu_read_lock();
 	t = sctp_addrs_lookup_transport(net, local, peer);
-	if (!t || t->dead)
-		return NULL;
+	if (!t || !sctp_transport_hold(t))
+		goto out;
 
-	sctp_association_hold(t->asoc);
+	asoc = t->asoc;
+	sctp_association_hold(asoc);
 	*pt = t;
 
-	return t->asoc;
+	sctp_transport_put(t);
+
+out:
+	rcu_read_unlock();
+	return asoc;
 }
 
 /* Look up an association. protected by RCU read lock */
@@ -955,9 +962,7 @@ struct sctp_association *sctp_lookup_association(struct net *net,
 {
 	struct sctp_association *asoc;
 
-	rcu_read_lock();
 	asoc = __sctp_lookup_association(net, laddr, paddr, transportp);
-	rcu_read_unlock();
 
 	return asoc;
 }
diff --git a/net/sctp/transport.c b/net/sctp/transport.c
index aab9e3f..69f3799 100644
--- a/net/sctp/transport.c
+++ b/net/sctp/transport.c
@@ -296,9 +296,9 @@ void sctp_transport_route(struct sctp_transport *transport,
 }
 
 /* Hold a reference to a transport.  */
-void sctp_transport_hold(struct sctp_transport *transport)
+int sctp_transport_hold(struct sctp_transport *transport)
 {
-	atomic_inc(&transport->refcnt);
+	return atomic_add_unless(&transport->refcnt, 1, 0);
 }
 
 /* Release a reference to a transport and clean up
-- 
2.1.0

[PATCH net 2/3] sctp: hold transport before we access t->asoc in sctp proc

[Xin Long]

Previously, before rhashtable, /proc assoc listing was done by
read-locking the entire hash entry and dumping all assocs at once, so we
were sure that the assoc wasn't freed because it wouldn't be possible to
remove it from the hash meanwhile.

Now we use rhashtable to list transports, and dump entries one by one.
That is, now we have to check if the assoc is still a good one, as the
transport we got may be being freed.

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

diff --git a/net/sctp/proc.c b/net/sctp/proc.c
index 684c5b3..c74a810 100644
--- a/net/sctp/proc.c
+++ b/net/sctp/proc.c
@@ -380,6 +380,8 @@ static int sctp_assocs_seq_show(struct seq_file *seq, void *v)
 	}
 
 	transport = (struct sctp_transport *)v;
+	if (!sctp_transport_hold(transport))
+		return 0;
 	assoc = transport->asoc;
 	epb = &assoc->base;
 	sk = epb->sk;
@@ -412,6 +414,8 @@ static int sctp_assocs_seq_show(struct seq_file *seq, void *v)
 		sk->sk_rcvbuf);
 	seq_printf(seq, "\n");
 
+	sctp_transport_put(transport);
+
 	return 0;
 }
 
@@ -489,6 +493,8 @@ static int sctp_remaddr_seq_show(struct seq_file *seq, void *v)
 	}
 
 	tsp = (struct sctp_transport *)v;
+	if (!sctp_transport_hold(tsp))
+		return 0;
 	assoc = tsp->asoc;
 
 	list_for_each_entry_rcu(tsp, &assoc->peer.transport_addr_list,
@@ -544,6 +550,8 @@ static int sctp_remaddr_seq_show(struct seq_file *seq, void *v)
 		seq_printf(seq, "\n");
 	}
 
+	sctp_transport_put(tsp);
+
 	return 0;
 }
 
-- 
2.1.0

[PATCH net 2/3] sctp: hold transport before we access t->asoc in sctp proc

[Xin Long]

Previously, before rhashtable, /proc assoc listing was done by
read-locking the entire hash entry and dumping all assocs at once, so we
were sure that the assoc wasn't freed because it wouldn't be possible to
remove it from the hash meanwhile.

Now we use rhashtable to list transports, and dump entries one by one.
That is, now we have to check if the assoc is still a good one, as the
transport we got may be being freed.

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

diff --git a/net/sctp/proc.c b/net/sctp/proc.c
index 684c5b3..c74a810 100644
--- a/net/sctp/proc.c
+++ b/net/sctp/proc.c
@@ -380,6 +380,8 @@ static int sctp_assocs_seq_show(struct seq_file *seq, void *v)
 	}
 
 	transport = (struct sctp_transport *)v;
+	if (!sctp_transport_hold(transport))
+		return 0;
 	assoc = transport->asoc;
 	epb = &assoc->base;
 	sk = epb->sk;
@@ -412,6 +414,8 @@ static int sctp_assocs_seq_show(struct seq_file *seq, void *v)
 		sk->sk_rcvbuf);
 	seq_printf(seq, "\n");
 
+	sctp_transport_put(transport);
+
 	return 0;
 }
 
@@ -489,6 +493,8 @@ static int sctp_remaddr_seq_show(struct seq_file *seq, void *v)
 	}
 
 	tsp = (struct sctp_transport *)v;
+	if (!sctp_transport_hold(tsp))
+		return 0;
 	assoc = tsp->asoc;
 
 	list_for_each_entry_rcu(tsp, &assoc->peer.transport_addr_list,
@@ -544,6 +550,8 @@ static int sctp_remaddr_seq_show(struct seq_file *seq, void *v)
 		seq_printf(seq, "\n");
 	}
 
+	sctp_transport_put(tsp);
+
 	return 0;
 }
 
-- 
2.1.0

[PATCH net 3/3] sctp: remove the dead field of sctp_transport

[Xin Long]

After we use refcnt to check if transport is alive, the dead can be
removed from sctp_transport.

The traversal of transport_addr_list in procfs dump is using
list_for_each_entry_rcu, no need to check if it has been freed.

sctp_generate_t3_rtx_event and sctp_generate_heartbeat_event is
protected by sock lock, it's not necessary to check dead, either.
also, the timers are cancelled when sctp_transport_free() is
called, that it doesn't wait for refcnt to reach 0 to cancel them.

Signed-off-by: Xin Long <lucien.xin@gmail.com>
---
 include/net/sctp/structs.h |  3 +--
 net/sctp/proc.c            |  4 ----
 net/sctp/sm_sideeffect.c   | 12 ------------
 net/sctp/transport.c       |  4 +---
 4 files changed, 2 insertions(+), 21 deletions(-)

diff --git a/include/net/sctp/structs.h b/include/net/sctp/structs.h
index 344da04..205630b 100644
--- a/include/net/sctp/structs.h
+++ b/include/net/sctp/structs.h
@@ -756,7 +756,6 @@ struct sctp_transport {
 
 	/* Reference counting. */
 	atomic_t refcnt;
-	__u32	 dead:1,
 		/* RTO-Pending : A flag used to track if one of the DATA
 		 *		chunks sent to this address is currently being
 		 *		used to compute a RTT. If this flag is 0,
@@ -766,7 +765,7 @@ struct sctp_transport {
 		 *		calculation completes (i.e. the DATA chunk
 		 *		is SACK'd) clear this flag.
 		 */
-		 rto_pending:1,
+	__u32	rto_pending:1,
 
 		/*
 		 * hb_sent : a flag that signals that we have a pending
diff --git a/net/sctp/proc.c b/net/sctp/proc.c
index c74a810..ded7d93 100644
--- a/net/sctp/proc.c
+++ b/net/sctp/proc.c
@@ -165,8 +165,6 @@ static void sctp_seq_dump_remote_addrs(struct seq_file *seq, struct sctp_associa
 	list_for_each_entry_rcu(transport, &assoc->peer.transport_addr_list,
 			transports) {
 		addr = &transport->ipaddr;
-		if (transport->dead)
-			continue;
 
 		af = sctp_get_af_specific(addr->sa.sa_family);
 		if (af->cmp_addr(addr, primary)) {
@@ -499,8 +497,6 @@ static int sctp_remaddr_seq_show(struct seq_file *seq, void *v)
 
 	list_for_each_entry_rcu(tsp, &assoc->peer.transport_addr_list,
 				transports) {
-		if (tsp->dead)
-			continue;
 		/*
 		 * The remote address (ADDR)
 		 */
diff --git a/net/sctp/sm_sideeffect.c b/net/sctp/sm_sideeffect.c
index 2e21384..b5327bb 100644
--- a/net/sctp/sm_sideeffect.c
+++ b/net/sctp/sm_sideeffect.c
@@ -259,12 +259,6 @@ void sctp_generate_t3_rtx_event(unsigned long peer)
 		goto out_unlock;
 	}
 
-	/* Is this transport really dead and just waiting around for
-	 * the timer to let go of the reference?
-	 */
-	if (transport->dead)
-		goto out_unlock;
-
 	/* Run through the state machine.  */
 	error = sctp_do_sm(net, SCTP_EVENT_T_TIMEOUT,
 			   SCTP_ST_TIMEOUT(SCTP_EVENT_TIMEOUT_T3_RTX),
@@ -380,12 +374,6 @@ void sctp_generate_heartbeat_event(unsigned long data)
 		goto out_unlock;
 	}
 
-	/* Is this structure just waiting around for us to actually
-	 * get destroyed?
-	 */
-	if (transport->dead)
-		goto out_unlock;
-
 	error = sctp_do_sm(net, SCTP_EVENT_T_TIMEOUT,
 			   SCTP_ST_TIMEOUT(SCTP_EVENT_TIMEOUT_HEARTBEAT),
 			   asoc->state, asoc->ep, asoc,
diff --git a/net/sctp/transport.c b/net/sctp/transport.c
index 69f3799..a431c14 100644
--- a/net/sctp/transport.c
+++ b/net/sctp/transport.c
@@ -132,8 +132,6 @@ fail:
  */
 void sctp_transport_free(struct sctp_transport *transport)
 {
-	transport->dead = 1;
-
 	/* Try to delete the heartbeat timer.  */
 	if (del_timer(&transport->hb_timer))
 		sctp_transport_put(transport);
@@ -169,7 +167,7 @@ static void sctp_transport_destroy_rcu(struct rcu_head *head)
  */
 static void sctp_transport_destroy(struct sctp_transport *transport)
 {
-	if (unlikely(!transport->dead)) {
+	if (unlikely(atomic_read(&transport->refcnt))) {
 		WARN(1, "Attempt to destroy undead transport %p!\n", transport);
 		return;
 	}
-- 
2.1.0

[PATCH net 3/3] sctp: remove the dead field of sctp_transport

[Xin Long]

After we use refcnt to check if transport is alive, the dead can be
removed from sctp_transport.

The traversal of transport_addr_list in procfs dump is using
list_for_each_entry_rcu, no need to check if it has been freed.

sctp_generate_t3_rtx_event and sctp_generate_heartbeat_event is
protected by sock lock, it's not necessary to check dead, either.
also, the timers are cancelled when sctp_transport_free() is
called, that it doesn't wait for refcnt to reach 0 to cancel them.

Signed-off-by: Xin Long <lucien.xin@gmail.com>
---
 include/net/sctp/structs.h |  3 +--
 net/sctp/proc.c            |  4 ----
 net/sctp/sm_sideeffect.c   | 12 ------------
 net/sctp/transport.c       |  4 +---
 4 files changed, 2 insertions(+), 21 deletions(-)

diff --git a/include/net/sctp/structs.h b/include/net/sctp/structs.h
index 344da04..205630b 100644
--- a/include/net/sctp/structs.h
+++ b/include/net/sctp/structs.h
@@ -756,7 +756,6 @@ struct sctp_transport {
 
 	/* Reference counting. */
 	atomic_t refcnt;
-	__u32	 dead:1,
 		/* RTO-Pending : A flag used to track if one of the DATA
 		 *		chunks sent to this address is currently being
 		 *		used to compute a RTT. If this flag is 0,
@@ -766,7 +765,7 @@ struct sctp_transport {
 		 *		calculation completes (i.e. the DATA chunk
 		 *		is SACK'd) clear this flag.
 		 */
-		 rto_pending:1,
+	__u32	rto_pending:1,
 
 		/*
 		 * hb_sent : a flag that signals that we have a pending
diff --git a/net/sctp/proc.c b/net/sctp/proc.c
index c74a810..ded7d93 100644
--- a/net/sctp/proc.c
+++ b/net/sctp/proc.c
@@ -165,8 +165,6 @@ static void sctp_seq_dump_remote_addrs(struct seq_file *seq, struct sctp_associa
 	list_for_each_entry_rcu(transport, &assoc->peer.transport_addr_list,
 			transports) {
 		addr = &transport->ipaddr;
-		if (transport->dead)
-			continue;
 
 		af = sctp_get_af_specific(addr->sa.sa_family);
 		if (af->cmp_addr(addr, primary)) {
@@ -499,8 +497,6 @@ static int sctp_remaddr_seq_show(struct seq_file *seq, void *v)
 
 	list_for_each_entry_rcu(tsp, &assoc->peer.transport_addr_list,
 				transports) {
-		if (tsp->dead)
-			continue;
 		/*
 		 * The remote address (ADDR)
 		 */
diff --git a/net/sctp/sm_sideeffect.c b/net/sctp/sm_sideeffect.c
index 2e21384..b5327bb 100644
--- a/net/sctp/sm_sideeffect.c
+++ b/net/sctp/sm_sideeffect.c
@@ -259,12 +259,6 @@ void sctp_generate_t3_rtx_event(unsigned long peer)
 		goto out_unlock;
 	}
 
-	/* Is this transport really dead and just waiting around for
-	 * the timer to let go of the reference?
-	 */
-	if (transport->dead)
-		goto out_unlock;
-
 	/* Run through the state machine.  */
 	error = sctp_do_sm(net, SCTP_EVENT_T_TIMEOUT,
 			   SCTP_ST_TIMEOUT(SCTP_EVENT_TIMEOUT_T3_RTX),
@@ -380,12 +374,6 @@ void sctp_generate_heartbeat_event(unsigned long data)
 		goto out_unlock;
 	}
 
-	/* Is this structure just waiting around for us to actually
-	 * get destroyed?
-	 */
-	if (transport->dead)
-		goto out_unlock;
-
 	error = sctp_do_sm(net, SCTP_EVENT_T_TIMEOUT,
 			   SCTP_ST_TIMEOUT(SCTP_EVENT_TIMEOUT_HEARTBEAT),
 			   asoc->state, asoc->ep, asoc,
diff --git a/net/sctp/transport.c b/net/sctp/transport.c
index 69f3799..a431c14 100644
--- a/net/sctp/transport.c
+++ b/net/sctp/transport.c
@@ -132,8 +132,6 @@ fail:
  */
 void sctp_transport_free(struct sctp_transport *transport)
 {
-	transport->dead = 1;
-
 	/* Try to delete the heartbeat timer.  */
 	if (del_timer(&transport->hb_timer))
 		sctp_transport_put(transport);
@@ -169,7 +167,7 @@ static void sctp_transport_destroy_rcu(struct rcu_head *head)
  */
 static void sctp_transport_destroy(struct sctp_transport *transport)
 {
-	if (unlikely(!transport->dead)) {
+	if (unlikely(atomic_read(&transport->refcnt))) {
 		WARN(1, "Attempt to destroy undead transport %p!\n", transport);
 		return;
 	}
-- 
2.1.0

Re: [PATCH net 3/3] sctp: remove the dead field of sctp_transport

[Marcelo Ricardo Leitner]

On Fri, Jan 22, 2016 at 01:49:09AM +0800, Xin Long wrote:
> After we use refcnt to check if transport is alive, the dead can be
> removed from sctp_transport.
> 
> The traversal of transport_addr_list in procfs dump is using
> list_for_each_entry_rcu, no need to check if it has been freed.
> 
> sctp_generate_t3_rtx_event and sctp_generate_heartbeat_event is
> protected by sock lock, it's not necessary to check dead, either.
> also, the timers are cancelled when sctp_transport_free() is
> called, that it doesn't wait for refcnt to reach 0 to cancel them.
> 
> Signed-off-by: Xin Long <lucien.xin@gmail.com>

Signed-off-by: Marcelo Ricardo Leitner <marcelo.leitner@gmail.com>

> ---
>  include/net/sctp/structs.h |  3 +--
>  net/sctp/proc.c            |  4 ----
>  net/sctp/sm_sideeffect.c   | 12 ------------
>  net/sctp/transport.c       |  4 +---
>  4 files changed, 2 insertions(+), 21 deletions(-)
> 
> diff --git a/include/net/sctp/structs.h b/include/net/sctp/structs.h
> index 344da04..205630b 100644
> --- a/include/net/sctp/structs.h
> +++ b/include/net/sctp/structs.h
> @@ -756,7 +756,6 @@ struct sctp_transport {
>  
>  	/* Reference counting. */
>  	atomic_t refcnt;
> -	__u32	 dead:1,
>  		/* RTO-Pending : A flag used to track if one of the DATA
>  		 *		chunks sent to this address is currently being
>  		 *		used to compute a RTT. If this flag is 0,
> @@ -766,7 +765,7 @@ struct sctp_transport {
>  		 *		calculation completes (i.e. the DATA chunk
>  		 *		is SACK'd) clear this flag.
>  		 */
> -		 rto_pending:1,
> +	__u32	rto_pending:1,
>  
>  		/*
>  		 * hb_sent : a flag that signals that we have a pending
> diff --git a/net/sctp/proc.c b/net/sctp/proc.c
> index c74a810..ded7d93 100644
> --- a/net/sctp/proc.c
> +++ b/net/sctp/proc.c
> @@ -165,8 +165,6 @@ static void sctp_seq_dump_remote_addrs(struct seq_file *seq, struct sctp_associa
>  	list_for_each_entry_rcu(transport, &assoc->peer.transport_addr_list,
>  			transports) {
>  		addr = &transport->ipaddr;
> -		if (transport->dead)
> -			continue;
>  
>  		af = sctp_get_af_specific(addr->sa.sa_family);
>  		if (af->cmp_addr(addr, primary)) {
> @@ -499,8 +497,6 @@ static int sctp_remaddr_seq_show(struct seq_file *seq, void *v)
>  
>  	list_for_each_entry_rcu(tsp, &assoc->peer.transport_addr_list,
>  				transports) {
> -		if (tsp->dead)
> -			continue;
>  		/*
>  		 * The remote address (ADDR)
>  		 */
> diff --git a/net/sctp/sm_sideeffect.c b/net/sctp/sm_sideeffect.c
> index 2e21384..b5327bb 100644
> --- a/net/sctp/sm_sideeffect.c
> +++ b/net/sctp/sm_sideeffect.c
> @@ -259,12 +259,6 @@ void sctp_generate_t3_rtx_event(unsigned long peer)
>  		goto out_unlock;
>  	}
>  
> -	/* Is this transport really dead and just waiting around for
> -	 * the timer to let go of the reference?
> -	 */
> -	if (transport->dead)
> -		goto out_unlock;
> -
>  	/* Run through the state machine.  */
>  	error = sctp_do_sm(net, SCTP_EVENT_T_TIMEOUT,
>  			   SCTP_ST_TIMEOUT(SCTP_EVENT_TIMEOUT_T3_RTX),
> @@ -380,12 +374,6 @@ void sctp_generate_heartbeat_event(unsigned long data)
>  		goto out_unlock;
>  	}
>  
> -	/* Is this structure just waiting around for us to actually
> -	 * get destroyed?
> -	 */
> -	if (transport->dead)
> -		goto out_unlock;
> -
>  	error = sctp_do_sm(net, SCTP_EVENT_T_TIMEOUT,
>  			   SCTP_ST_TIMEOUT(SCTP_EVENT_TIMEOUT_HEARTBEAT),
>  			   asoc->state, asoc->ep, asoc,
> diff --git a/net/sctp/transport.c b/net/sctp/transport.c
> index 69f3799..a431c14 100644
> --- a/net/sctp/transport.c
> +++ b/net/sctp/transport.c
> @@ -132,8 +132,6 @@ fail:
>   */
>  void sctp_transport_free(struct sctp_transport *transport)
>  {
> -	transport->dead = 1;
> -
>  	/* Try to delete the heartbeat timer.  */
>  	if (del_timer(&transport->hb_timer))
>  		sctp_transport_put(transport);
> @@ -169,7 +167,7 @@ static void sctp_transport_destroy_rcu(struct rcu_head *head)
>   */
>  static void sctp_transport_destroy(struct sctp_transport *transport)
>  {
> -	if (unlikely(!transport->dead)) {
> +	if (unlikely(atomic_read(&transport->refcnt))) {
>  		WARN(1, "Attempt to destroy undead transport %p!\n", transport);
>  		return;
>  	}
> -- 
> 2.1.0
> 
> --
> 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 3/3] sctp: remove the dead field of sctp_transport

[Marcelo Ricardo Leitner]

On Fri, Jan 22, 2016 at 01:49:09AM +0800, Xin Long wrote:
> After we use refcnt to check if transport is alive, the dead can be
> removed from sctp_transport.
> 
> The traversal of transport_addr_list in procfs dump is using
> list_for_each_entry_rcu, no need to check if it has been freed.
> 
> sctp_generate_t3_rtx_event and sctp_generate_heartbeat_event is
> protected by sock lock, it's not necessary to check dead, either.
> also, the timers are cancelled when sctp_transport_free() is
> called, that it doesn't wait for refcnt to reach 0 to cancel them.
> 
> Signed-off-by: Xin Long <lucien.xin@gmail.com>

Signed-off-by: Marcelo Ricardo Leitner <marcelo.leitner@gmail.com>

> ---
>  include/net/sctp/structs.h |  3 +--
>  net/sctp/proc.c            |  4 ----
>  net/sctp/sm_sideeffect.c   | 12 ------------
>  net/sctp/transport.c       |  4 +---
>  4 files changed, 2 insertions(+), 21 deletions(-)
> 
> diff --git a/include/net/sctp/structs.h b/include/net/sctp/structs.h
> index 344da04..205630b 100644
> --- a/include/net/sctp/structs.h
> +++ b/include/net/sctp/structs.h
> @@ -756,7 +756,6 @@ struct sctp_transport {
>  
>  	/* Reference counting. */
>  	atomic_t refcnt;
> -	__u32	 dead:1,
>  		/* RTO-Pending : A flag used to track if one of the DATA
>  		 *		chunks sent to this address is currently being
>  		 *		used to compute a RTT. If this flag is 0,
> @@ -766,7 +765,7 @@ struct sctp_transport {
>  		 *		calculation completes (i.e. the DATA chunk
>  		 *		is SACK'd) clear this flag.
>  		 */
> -		 rto_pending:1,
> +	__u32	rto_pending:1,
>  
>  		/*
>  		 * hb_sent : a flag that signals that we have a pending
> diff --git a/net/sctp/proc.c b/net/sctp/proc.c
> index c74a810..ded7d93 100644
> --- a/net/sctp/proc.c
> +++ b/net/sctp/proc.c
> @@ -165,8 +165,6 @@ static void sctp_seq_dump_remote_addrs(struct seq_file *seq, struct sctp_associa
>  	list_for_each_entry_rcu(transport, &assoc->peer.transport_addr_list,
>  			transports) {
>  		addr = &transport->ipaddr;
> -		if (transport->dead)
> -			continue;
>  
>  		af = sctp_get_af_specific(addr->sa.sa_family);
>  		if (af->cmp_addr(addr, primary)) {
> @@ -499,8 +497,6 @@ static int sctp_remaddr_seq_show(struct seq_file *seq, void *v)
>  
>  	list_for_each_entry_rcu(tsp, &assoc->peer.transport_addr_list,
>  				transports) {
> -		if (tsp->dead)
> -			continue;
>  		/*
>  		 * The remote address (ADDR)
>  		 */
> diff --git a/net/sctp/sm_sideeffect.c b/net/sctp/sm_sideeffect.c
> index 2e21384..b5327bb 100644
> --- a/net/sctp/sm_sideeffect.c
> +++ b/net/sctp/sm_sideeffect.c
> @@ -259,12 +259,6 @@ void sctp_generate_t3_rtx_event(unsigned long peer)
>  		goto out_unlock;
>  	}
>  
> -	/* Is this transport really dead and just waiting around for
> -	 * the timer to let go of the reference?
> -	 */
> -	if (transport->dead)
> -		goto out_unlock;
> -
>  	/* Run through the state machine.  */
>  	error = sctp_do_sm(net, SCTP_EVENT_T_TIMEOUT,
>  			   SCTP_ST_TIMEOUT(SCTP_EVENT_TIMEOUT_T3_RTX),
> @@ -380,12 +374,6 @@ void sctp_generate_heartbeat_event(unsigned long data)
>  		goto out_unlock;
>  	}
>  
> -	/* Is this structure just waiting around for us to actually
> -	 * get destroyed?
> -	 */
> -	if (transport->dead)
> -		goto out_unlock;
> -
>  	error = sctp_do_sm(net, SCTP_EVENT_T_TIMEOUT,
>  			   SCTP_ST_TIMEOUT(SCTP_EVENT_TIMEOUT_HEARTBEAT),
>  			   asoc->state, asoc->ep, asoc,
> diff --git a/net/sctp/transport.c b/net/sctp/transport.c
> index 69f3799..a431c14 100644
> --- a/net/sctp/transport.c
> +++ b/net/sctp/transport.c
> @@ -132,8 +132,6 @@ fail:
>   */
>  void sctp_transport_free(struct sctp_transport *transport)
>  {
> -	transport->dead = 1;
> -
>  	/* Try to delete the heartbeat timer.  */
>  	if (del_timer(&transport->hb_timer))
>  		sctp_transport_put(transport);
> @@ -169,7 +167,7 @@ static void sctp_transport_destroy_rcu(struct rcu_head *head)
>   */
>  static void sctp_transport_destroy(struct sctp_transport *transport)
>  {
> -	if (unlikely(!transport->dead)) {
> +	if (unlikely(atomic_read(&transport->refcnt))) {
>  		WARN(1, "Attempt to destroy undead transport %p!\n", transport);
>  		return;
>  	}
> -- 
> 2.1.0
> 
> --
> 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 2/3] sctp: hold transport before we access t->asoc in sctp proc

[Marcelo Ricardo Leitner]

On Fri, Jan 22, 2016 at 01:49:08AM +0800, Xin Long wrote:
> Previously, before rhashtable, /proc assoc listing was done by
> read-locking the entire hash entry and dumping all assocs at once, so we
> were sure that the assoc wasn't freed because it wouldn't be possible to
> remove it from the hash meanwhile.
> 
> Now we use rhashtable to list transports, and dump entries one by one.
> That is, now we have to check if the assoc is still a good one, as the
> transport we got may be being freed.
> 
> Signed-off-by: Xin Long <lucien.xin@gmail.com>

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

> ---
>  net/sctp/proc.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/net/sctp/proc.c b/net/sctp/proc.c
> index 684c5b3..c74a810 100644
> --- a/net/sctp/proc.c
> +++ b/net/sctp/proc.c
> @@ -380,6 +380,8 @@ static int sctp_assocs_seq_show(struct seq_file *seq, void *v)
>  	}
>  
>  	transport = (struct sctp_transport *)v;
> +	if (!sctp_transport_hold(transport))
> +		return 0;
>  	assoc = transport->asoc;
>  	epb = &assoc->base;
>  	sk = epb->sk;
> @@ -412,6 +414,8 @@ static int sctp_assocs_seq_show(struct seq_file *seq, void *v)
>  		sk->sk_rcvbuf);
>  	seq_printf(seq, "\n");
>  
> +	sctp_transport_put(transport);
> +
>  	return 0;
>  }
>  
> @@ -489,6 +493,8 @@ static int sctp_remaddr_seq_show(struct seq_file *seq, void *v)
>  	}
>  
>  	tsp = (struct sctp_transport *)v;
> +	if (!sctp_transport_hold(tsp))
> +		return 0;
>  	assoc = tsp->asoc;
>  
>  	list_for_each_entry_rcu(tsp, &assoc->peer.transport_addr_list,
> @@ -544,6 +550,8 @@ static int sctp_remaddr_seq_show(struct seq_file *seq, void *v)
>  		seq_printf(seq, "\n");
>  	}
>  
> +	sctp_transport_put(tsp);
> +
>  	return 0;
>  }
>  
> -- 
> 2.1.0
> 
> --
> 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 2/3] sctp: hold transport before we access t->asoc in sctp proc

[Marcelo Ricardo Leitner]

On Fri, Jan 22, 2016 at 01:49:08AM +0800, Xin Long wrote:
> Previously, before rhashtable, /proc assoc listing was done by
> read-locking the entire hash entry and dumping all assocs at once, so we
> were sure that the assoc wasn't freed because it wouldn't be possible to
> remove it from the hash meanwhile.
> 
> Now we use rhashtable to list transports, and dump entries one by one.
> That is, now we have to check if the assoc is still a good one, as the
> transport we got may be being freed.
> 
> Signed-off-by: Xin Long <lucien.xin@gmail.com>

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

> ---
>  net/sctp/proc.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/net/sctp/proc.c b/net/sctp/proc.c
> index 684c5b3..c74a810 100644
> --- a/net/sctp/proc.c
> +++ b/net/sctp/proc.c
> @@ -380,6 +380,8 @@ static int sctp_assocs_seq_show(struct seq_file *seq, void *v)
>  	}
>  
>  	transport = (struct sctp_transport *)v;
> +	if (!sctp_transport_hold(transport))
> +		return 0;
>  	assoc = transport->asoc;
>  	epb = &assoc->base;
>  	sk = epb->sk;
> @@ -412,6 +414,8 @@ static int sctp_assocs_seq_show(struct seq_file *seq, void *v)
>  		sk->sk_rcvbuf);
>  	seq_printf(seq, "\n");
>  
> +	sctp_transport_put(transport);
> +
>  	return 0;
>  }
>  
> @@ -489,6 +493,8 @@ static int sctp_remaddr_seq_show(struct seq_file *seq, void *v)
>  	}
>  
>  	tsp = (struct sctp_transport *)v;
> +	if (!sctp_transport_hold(tsp))
> +		return 0;
>  	assoc = tsp->asoc;
>  
>  	list_for_each_entry_rcu(tsp, &assoc->peer.transport_addr_list,
> @@ -544,6 +550,8 @@ static int sctp_remaddr_seq_show(struct seq_file *seq, void *v)
>  		seq_printf(seq, "\n");
>  	}
>  
> +	sctp_transport_put(tsp);
> +
>  	return 0;
>  }
>  
> -- 
> 2.1.0
> 
> --
> 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 2/3] sctp: hold transport before we access t->asoc in sctp proc

[Eric Dumazet]

On Fri, 2016-01-22 at 01:49 +0800, Xin Long wrote:
> Previously, before rhashtable, /proc assoc listing was done by
> read-locking the entire hash entry and dumping all assocs at once, so we
> were sure that the assoc wasn't freed because it wouldn't be possible to
> remove it from the hash meanwhile.
> 
> Now we use rhashtable to list transports, and dump entries one by one.
> That is, now we have to check if the assoc is still a good one, as the
> transport we got may be being freed.
> 
> Signed-off-by: Xin Long <lucien.xin@gmail.com>
> ---
>  net/sctp/proc.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/net/sctp/proc.c b/net/sctp/proc.c
> index 684c5b3..c74a810 100644
> --- a/net/sctp/proc.c
> +++ b/net/sctp/proc.c
> @@ -380,6 +380,8 @@ static int sctp_assocs_seq_show(struct seq_file *seq, void *v)
>  	}
>  
>  	transport = (struct sctp_transport *)v;

What protects you from this structure already being freed ?

> +	if (!sctp_transport_hold(transport))
> +		return 0;

If this is rcu, then you do not need to increment the refcount, and
decrement it later.

Re: [PATCH net 2/3] sctp: hold transport before we access t->asoc in sctp proc

[Eric Dumazet]

On Fri, 2016-01-22 at 01:49 +0800, Xin Long wrote:
> Previously, before rhashtable, /proc assoc listing was done by
> read-locking the entire hash entry and dumping all assocs at once, so we
> were sure that the assoc wasn't freed because it wouldn't be possible to
> remove it from the hash meanwhile.
> 
> Now we use rhashtable to list transports, and dump entries one by one.
> That is, now we have to check if the assoc is still a good one, as the
> transport we got may be being freed.
> 
> Signed-off-by: Xin Long <lucien.xin@gmail.com>
> ---
>  net/sctp/proc.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/net/sctp/proc.c b/net/sctp/proc.c
> index 684c5b3..c74a810 100644
> --- a/net/sctp/proc.c
> +++ b/net/sctp/proc.c
> @@ -380,6 +380,8 @@ static int sctp_assocs_seq_show(struct seq_file *seq, void *v)
>  	}
>  
>  	transport = (struct sctp_transport *)v;

What protects you from this structure already being freed ?

> +	if (!sctp_transport_hold(transport))
> +		return 0;

If this is rcu, then you do not need to increment the refcount, and
decrement it later.

Re: [PATCH net 2/3] sctp: hold transport before we access t->asoc in sctp proc

[Marcelo Ricardo Leitner]

On Thu, Jan 21, 2016 at 11:27:36AM -0800, Eric Dumazet wrote:
> On Fri, 2016-01-22 at 01:49 +0800, Xin Long wrote:
> > Previously, before rhashtable, /proc assoc listing was done by
> > read-locking the entire hash entry and dumping all assocs at once, so we
> > were sure that the assoc wasn't freed because it wouldn't be possible to
> > remove it from the hash meanwhile.
> > 
> > Now we use rhashtable to list transports, and dump entries one by one.
> > That is, now we have to check if the assoc is still a good one, as the
> > transport we got may be being freed.
> > 
> > Signed-off-by: Xin Long <lucien.xin@gmail.com>
> > ---
> >  net/sctp/proc.c | 8 ++++++++
> >  1 file changed, 8 insertions(+)
> > 
> > diff --git a/net/sctp/proc.c b/net/sctp/proc.c
> > index 684c5b3..c74a810 100644
> > --- a/net/sctp/proc.c
> > +++ b/net/sctp/proc.c
> > @@ -380,6 +380,8 @@ static int sctp_assocs_seq_show(struct seq_file *seq, void *v)
> >  	}
> >  
> >  	transport = (struct sctp_transport *)v;
> 
> What protects you from this structure already being freed ?

rcu, rhashtable_walk_start() at sctp_assocs_seq_start() starts an
(implicit from this POV) rcu_read_lock() for us which is unlocked only
when the walking is terminated, thus covering this _show.

> > +	if (!sctp_transport_hold(transport))
> > +		return 0;
> 
> If this is rcu, then you do not need to increment the refcount, and
> decrement it later.

It's an implicit hold on sctp asoc.

This code is using contents from asoc pointer, which is not proctected
by rcu. As transport has a hold on the asoc, it's enough to just hold
the transport and not the asoc too, as we had to do in the previous
patch.

  Marcelo

Re: [PATCH net 2/3] sctp: hold transport before we access t->asoc in sctp proc

[Marcelo Ricardo Leitner]

On Thu, Jan 21, 2016 at 11:27:36AM -0800, Eric Dumazet wrote:
> On Fri, 2016-01-22 at 01:49 +0800, Xin Long wrote:
> > Previously, before rhashtable, /proc assoc listing was done by
> > read-locking the entire hash entry and dumping all assocs at once, so we
> > were sure that the assoc wasn't freed because it wouldn't be possible to
> > remove it from the hash meanwhile.
> > 
> > Now we use rhashtable to list transports, and dump entries one by one.
> > That is, now we have to check if the assoc is still a good one, as the
> > transport we got may be being freed.
> > 
> > Signed-off-by: Xin Long <lucien.xin@gmail.com>
> > ---
> >  net/sctp/proc.c | 8 ++++++++
> >  1 file changed, 8 insertions(+)
> > 
> > diff --git a/net/sctp/proc.c b/net/sctp/proc.c
> > index 684c5b3..c74a810 100644
> > --- a/net/sctp/proc.c
> > +++ b/net/sctp/proc.c
> > @@ -380,6 +380,8 @@ static int sctp_assocs_seq_show(struct seq_file *seq, void *v)
> >  	}
> >  
> >  	transport = (struct sctp_transport *)v;
> 
> What protects you from this structure already being freed ?

rcu, rhashtable_walk_start() at sctp_assocs_seq_start() starts an
(implicit from this POV) rcu_read_lock() for us which is unlocked only
when the walking is terminated, thus covering this _show.

> > +	if (!sctp_transport_hold(transport))
> > +		return 0;
> 
> If this is rcu, then you do not need to increment the refcount, and
> decrement it later.

It's an implicit hold on sctp asoc.

This code is using contents from asoc pointer, which is not proctected
by rcu. As transport has a hold on the asoc, it's enough to just hold
the transport and not the asoc too, as we had to do in the previous
patch.

  Marcelo

Re: [PATCH net 2/3] sctp: hold transport before we access t->asoc in sctp proc

[Eric Dumazet]

On Thu, 2016-01-21 at 17:37 -0200, Marcelo Ricardo Leitner wrote:
> On Thu, Jan 21, 2016 at 11:27:36AM -0800, Eric Dumazet wrote:
> > On Fri, 2016-01-22 at 01:49 +0800, Xin Long wrote:
> > > Previously, before rhashtable, /proc assoc listing was done by
> > > read-locking the entire hash entry and dumping all assocs at once, so we
> > > were sure that the assoc wasn't freed because it wouldn't be possible to
> > > remove it from the hash meanwhile.
> > > 
> > > Now we use rhashtable to list transports, and dump entries one by one.
> > > That is, now we have to check if the assoc is still a good one, as the
> > > transport we got may be being freed.
> > > 
> > > Signed-off-by: Xin Long <lucien.xin@gmail.com>
> > > ---
> > >  net/sctp/proc.c | 8 ++++++++
> > >  1 file changed, 8 insertions(+)
> > > 
> > > diff --git a/net/sctp/proc.c b/net/sctp/proc.c
> > > index 684c5b3..c74a810 100644
> > > --- a/net/sctp/proc.c
> > > +++ b/net/sctp/proc.c
> > > @@ -380,6 +380,8 @@ static int sctp_assocs_seq_show(struct seq_file *seq, void *v)
> > >  	}
> > >  
> > >  	transport = (struct sctp_transport *)v;
> > 
> > What protects you from this structure already being freed ?
> 
> rcu, rhashtable_walk_start() at sctp_assocs_seq_start() starts an
> (implicit from this POV) rcu_read_lock() for us which is unlocked only
> when the walking is terminated, thus covering this _show.
> 
> > > +	if (!sctp_transport_hold(transport))
> > > +		return 0;
> > 
> > If this is rcu, then you do not need to increment the refcount, and
> > decrement it later.
> 
> It's an implicit hold on sctp asoc.
> 
> This code is using contents from asoc pointer, which is not proctected
> by rcu. As transport has a hold on the asoc, it's enough to just hold
> the transport and not the asoc too, as we had to do in the previous
> patch.

Then it means fast path also need to do this sctp_transport_hold() ?

If sctp_association_put() was called from sctp_transport_destroy_rcu()
(ie after rcu grace period), you would not need to increment/decrement
the transport refcount.

Normally, RCU protection does not need to change the refcount, unless we
need to keep an object alive after escaping the rcu section.

Re: [PATCH net 2/3] sctp: hold transport before we access t->asoc in sctp proc

[Eric Dumazet]

On Thu, 2016-01-21 at 17:37 -0200, Marcelo Ricardo Leitner wrote:
> On Thu, Jan 21, 2016 at 11:27:36AM -0800, Eric Dumazet wrote:
> > On Fri, 2016-01-22 at 01:49 +0800, Xin Long wrote:
> > > Previously, before rhashtable, /proc assoc listing was done by
> > > read-locking the entire hash entry and dumping all assocs at once, so we
> > > were sure that the assoc wasn't freed because it wouldn't be possible to
> > > remove it from the hash meanwhile.
> > > 
> > > Now we use rhashtable to list transports, and dump entries one by one.
> > > That is, now we have to check if the assoc is still a good one, as the
> > > transport we got may be being freed.
> > > 
> > > Signed-off-by: Xin Long <lucien.xin@gmail.com>
> > > ---
> > >  net/sctp/proc.c | 8 ++++++++
> > >  1 file changed, 8 insertions(+)
> > > 
> > > diff --git a/net/sctp/proc.c b/net/sctp/proc.c
> > > index 684c5b3..c74a810 100644
> > > --- a/net/sctp/proc.c
> > > +++ b/net/sctp/proc.c
> > > @@ -380,6 +380,8 @@ static int sctp_assocs_seq_show(struct seq_file *seq, void *v)
> > >  	}
> > >  
> > >  	transport = (struct sctp_transport *)v;
> > 
> > What protects you from this structure already being freed ?
> 
> rcu, rhashtable_walk_start() at sctp_assocs_seq_start() starts an
> (implicit from this POV) rcu_read_lock() for us which is unlocked only
> when the walking is terminated, thus covering this _show.
> 
> > > +	if (!sctp_transport_hold(transport))
> > > +		return 0;
> > 
> > If this is rcu, then you do not need to increment the refcount, and
> > decrement it later.
> 
> It's an implicit hold on sctp asoc.
> 
> This code is using contents from asoc pointer, which is not proctected
> by rcu. As transport has a hold on the asoc, it's enough to just hold
> the transport and not the asoc too, as we had to do in the previous
> patch.

Then it means fast path also need to do this sctp_transport_hold() ?

If sctp_association_put() was called from sctp_transport_destroy_rcu()
(ie after rcu grace period), you would not need to increment/decrement
the transport refcount.

Normally, RCU protection does not need to change the refcount, unless we
need to keep an object alive after escaping the rcu section.

Re: [PATCH net 2/3] sctp: hold transport before we access t->asoc in sctp proc

[Marcelo Ricardo Leitner]

On Thu, Jan 21, 2016 at 11:57:16AM -0800, Eric Dumazet wrote:
> On Thu, 2016-01-21 at 17:37 -0200, Marcelo Ricardo Leitner wrote:
> > On Thu, Jan 21, 2016 at 11:27:36AM -0800, Eric Dumazet wrote:
> > > On Fri, 2016-01-22 at 01:49 +0800, Xin Long wrote:
> > > > Previously, before rhashtable, /proc assoc listing was done by
> > > > read-locking the entire hash entry and dumping all assocs at once, so we
> > > > were sure that the assoc wasn't freed because it wouldn't be possible to
> > > > remove it from the hash meanwhile.
> > > > 
> > > > Now we use rhashtable to list transports, and dump entries one by one.
> > > > That is, now we have to check if the assoc is still a good one, as the
> > > > transport we got may be being freed.
> > > > 
> > > > Signed-off-by: Xin Long <lucien.xin@gmail.com>
> > > > ---
> > > >  net/sctp/proc.c | 8 ++++++++
> > > >  1 file changed, 8 insertions(+)
> > > > 
> > > > diff --git a/net/sctp/proc.c b/net/sctp/proc.c
> > > > index 684c5b3..c74a810 100644
> > > > --- a/net/sctp/proc.c
> > > > +++ b/net/sctp/proc.c
> > > > @@ -380,6 +380,8 @@ static int sctp_assocs_seq_show(struct seq_file *seq, void *v)
> > > >  	}
> > > >  
> > > >  	transport = (struct sctp_transport *)v;
> > > 
> > > What protects you from this structure already being freed ?
> > 
> > rcu, rhashtable_walk_start() at sctp_assocs_seq_start() starts an
> > (implicit from this POV) rcu_read_lock() for us which is unlocked only
> > when the walking is terminated, thus covering this _show.
> > 
> > > > +	if (!sctp_transport_hold(transport))
> > > > +		return 0;
> > > 
> > > If this is rcu, then you do not need to increment the refcount, and
> > > decrement it later.
> > 
> > It's an implicit hold on sctp asoc.
> > 
> > This code is using contents from asoc pointer, which is not proctected
> > by rcu. As transport has a hold on the asoc, it's enough to just hold
> > the transport and not the asoc too, as we had to do in the previous
> > patch.
> 
> Then it means fast path also need to do this sctp_transport_hold() ?

Well, kind of broad question, but I think so, yes. It's mostly done when
the transport is identified and fetched from rhashtable. Otherwise, we
probably already have the asoc and doesn't need this jump.

It's the first patch in this series. It's the only way we found to
safely transfer the ref from transport to asoc.

> If sctp_association_put() was called from sctp_transport_destroy_rcu()
> (ie after rcu grace period), you would not need to increment/decrement
> the transport refcount.
> 
> Normally, RCU protection does not need to change the refcount, unless we
> need to keep an object alive after escaping the rcu section.

sctp_association_put() was in sctp_transport_destroy_rcu(), but it
caused sctp-issues which Daniel fixed on 8c98653f0553 ("sctp:
sctp_close: fix release of bindings for deferred call_rcu's").

So in this case, we are not leaving the protected section but jumping
from a RCU-protected object (transport) to a non-protected one (asoc).

  Marcelo

Re: [PATCH net 2/3] sctp: hold transport before we access t->asoc in sctp proc

[Marcelo Ricardo Leitner]

On Thu, Jan 21, 2016 at 11:57:16AM -0800, Eric Dumazet wrote:
> On Thu, 2016-01-21 at 17:37 -0200, Marcelo Ricardo Leitner wrote:
> > On Thu, Jan 21, 2016 at 11:27:36AM -0800, Eric Dumazet wrote:
> > > On Fri, 2016-01-22 at 01:49 +0800, Xin Long wrote:
> > > > Previously, before rhashtable, /proc assoc listing was done by
> > > > read-locking the entire hash entry and dumping all assocs at once, so we
> > > > were sure that the assoc wasn't freed because it wouldn't be possible to
> > > > remove it from the hash meanwhile.
> > > > 
> > > > Now we use rhashtable to list transports, and dump entries one by one.
> > > > That is, now we have to check if the assoc is still a good one, as the
> > > > transport we got may be being freed.
> > > > 
> > > > Signed-off-by: Xin Long <lucien.xin@gmail.com>
> > > > ---
> > > >  net/sctp/proc.c | 8 ++++++++
> > > >  1 file changed, 8 insertions(+)
> > > > 
> > > > diff --git a/net/sctp/proc.c b/net/sctp/proc.c
> > > > index 684c5b3..c74a810 100644
> > > > --- a/net/sctp/proc.c
> > > > +++ b/net/sctp/proc.c
> > > > @@ -380,6 +380,8 @@ static int sctp_assocs_seq_show(struct seq_file *seq, void *v)
> > > >  	}
> > > >  
> > > >  	transport = (struct sctp_transport *)v;
> > > 
> > > What protects you from this structure already being freed ?
> > 
> > rcu, rhashtable_walk_start() at sctp_assocs_seq_start() starts an
> > (implicit from this POV) rcu_read_lock() for us which is unlocked only
> > when the walking is terminated, thus covering this _show.
> > 
> > > > +	if (!sctp_transport_hold(transport))
> > > > +		return 0;
> > > 
> > > If this is rcu, then you do not need to increment the refcount, and
> > > decrement it later.
> > 
> > It's an implicit hold on sctp asoc.
> > 
> > This code is using contents from asoc pointer, which is not proctected
> > by rcu. As transport has a hold on the asoc, it's enough to just hold
> > the transport and not the asoc too, as we had to do in the previous
> > patch.
> 
> Then it means fast path also need to do this sctp_transport_hold() ?

Well, kind of broad question, but I think so, yes. It's mostly done when
the transport is identified and fetched from rhashtable. Otherwise, we
probably already have the asoc and doesn't need this jump.

It's the first patch in this series. It's the only way we found to
safely transfer the ref from transport to asoc.

> If sctp_association_put() was called from sctp_transport_destroy_rcu()
> (ie after rcu grace period), you would not need to increment/decrement
> the transport refcount.
> 
> Normally, RCU protection does not need to change the refcount, unless we
> need to keep an object alive after escaping the rcu section.

sctp_association_put() was in sctp_transport_destroy_rcu(), but it
caused sctp-issues which Daniel fixed on 8c98653f0553 ("sctp:
sctp_close: fix release of bindings for deferred call_rcu's").

So in this case, we are not leaving the protected section but jumping
from a RCU-protected object (transport) to a non-protected one (asoc).

  Marcelo

Re: [PATCH net 1/3] sctp: fix the transport dead race check by using atomic_add_unless on refcnt

[Marcelo Ricardo Leitner]

On Fri, Jan 22, 2016 at 01:49:07AM +0800, Xin Long wrote:
> Now when __sctp_lookup_association is running in BH, it will try to
> check if t->dead is set, but meanwhile other CPUs may be freeing this
> transport and this assoc and if it happens that
> __sctp_lookup_association checked t->dead a bit too early, it may think
> that the association is still good while it was already freed.
> 
> So we fix this race by using atomic_add_unless in sctp_transport_hold.
> After we get one transport from hashtable, we will hold it only when
> this transport's refcnt is not 0, so that we can make sure t->asoc
> cannot be freed before we hold the asoc again.
> 
> Note that sctp association is not freed using RCU so we can't use
> atomic_add_unless() with it as it may just be too late for that either.
> 
> Fixes: 4f0087812648 ("sctp: apply rhashtable api to send/recv path")
> Reported-by: Vlad Yasevich <vyasevich@gmail.com>
> Signed-off-by: Xin Long <lucien.xin@gmail.com>

Signed-off-by: Marcelo Ricardo Leitner <marcelo.leitner@gmail.com>

> ---
>  include/net/sctp/structs.h |  2 +-
>  net/sctp/input.c           | 17 +++++++++++------
>  net/sctp/transport.c       |  4 ++--
>  3 files changed, 14 insertions(+), 9 deletions(-)
> 
> diff --git a/include/net/sctp/structs.h b/include/net/sctp/structs.h
> index 20e7212..344da04 100644
> --- a/include/net/sctp/structs.h
> +++ b/include/net/sctp/structs.h
> @@ -955,7 +955,7 @@ void sctp_transport_route(struct sctp_transport *, union sctp_addr *,
>  void sctp_transport_pmtu(struct sctp_transport *, struct sock *sk);
>  void sctp_transport_free(struct sctp_transport *);
>  void sctp_transport_reset_timers(struct sctp_transport *);
> -void sctp_transport_hold(struct sctp_transport *);
> +int sctp_transport_hold(struct sctp_transport *);
>  void sctp_transport_put(struct sctp_transport *);
>  void sctp_transport_update_rto(struct sctp_transport *, __u32);
>  void sctp_transport_raise_cwnd(struct sctp_transport *, __u32, __u32);
> diff --git a/net/sctp/input.c b/net/sctp/input.c
> index bf61dfb..49d2cc7 100644
> --- a/net/sctp/input.c
> +++ b/net/sctp/input.c
> @@ -935,15 +935,22 @@ static struct sctp_association *__sctp_lookup_association(
>  					struct sctp_transport **pt)
>  {
>  	struct sctp_transport *t;
> +	struct sctp_association *asoc = NULL;
>  
> +	rcu_read_lock();
>  	t = sctp_addrs_lookup_transport(net, local, peer);
> -	if (!t || t->dead)
> -		return NULL;
> +	if (!t || !sctp_transport_hold(t))
> +		goto out;
>  
> -	sctp_association_hold(t->asoc);
> +	asoc = t->asoc;
> +	sctp_association_hold(asoc);
>  	*pt = t;
>  
> -	return t->asoc;
> +	sctp_transport_put(t);
> +
> +out:
> +	rcu_read_unlock();
> +	return asoc;
>  }
>  
>  /* Look up an association. protected by RCU read lock */
> @@ -955,9 +962,7 @@ struct sctp_association *sctp_lookup_association(struct net *net,
>  {
>  	struct sctp_association *asoc;
>  
> -	rcu_read_lock();
>  	asoc = __sctp_lookup_association(net, laddr, paddr, transportp);
> -	rcu_read_unlock();
>  
>  	return asoc;
>  }
> diff --git a/net/sctp/transport.c b/net/sctp/transport.c
> index aab9e3f..69f3799 100644
> --- a/net/sctp/transport.c
> +++ b/net/sctp/transport.c
> @@ -296,9 +296,9 @@ void sctp_transport_route(struct sctp_transport *transport,
>  }
>  
>  /* Hold a reference to a transport.  */
> -void sctp_transport_hold(struct sctp_transport *transport)
> +int sctp_transport_hold(struct sctp_transport *transport)
>  {
> -	atomic_inc(&transport->refcnt);
> +	return atomic_add_unless(&transport->refcnt, 1, 0);
>  }
>  
>  /* Release a reference to a transport and clean up
> -- 
> 2.1.0
> 
> --
> 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 1/3] sctp: fix the transport dead race check by using atomic_add_unless on refcnt

[Marcelo Ricardo Leitner]

On Fri, Jan 22, 2016 at 01:49:07AM +0800, Xin Long wrote:
> Now when __sctp_lookup_association is running in BH, it will try to
> check if t->dead is set, but meanwhile other CPUs may be freeing this
> transport and this assoc and if it happens that
> __sctp_lookup_association checked t->dead a bit too early, it may think
> that the association is still good while it was already freed.
> 
> So we fix this race by using atomic_add_unless in sctp_transport_hold.
> After we get one transport from hashtable, we will hold it only when
> this transport's refcnt is not 0, so that we can make sure t->asoc
> cannot be freed before we hold the asoc again.
> 
> Note that sctp association is not freed using RCU so we can't use
> atomic_add_unless() with it as it may just be too late for that either.
> 
> Fixes: 4f0087812648 ("sctp: apply rhashtable api to send/recv path")
> Reported-by: Vlad Yasevich <vyasevich@gmail.com>
> Signed-off-by: Xin Long <lucien.xin@gmail.com>

Signed-off-by: Marcelo Ricardo Leitner <marcelo.leitner@gmail.com>

> ---
>  include/net/sctp/structs.h |  2 +-
>  net/sctp/input.c           | 17 +++++++++++------
>  net/sctp/transport.c       |  4 ++--
>  3 files changed, 14 insertions(+), 9 deletions(-)
> 
> diff --git a/include/net/sctp/structs.h b/include/net/sctp/structs.h
> index 20e7212..344da04 100644
> --- a/include/net/sctp/structs.h
> +++ b/include/net/sctp/structs.h
> @@ -955,7 +955,7 @@ void sctp_transport_route(struct sctp_transport *, union sctp_addr *,
>  void sctp_transport_pmtu(struct sctp_transport *, struct sock *sk);
>  void sctp_transport_free(struct sctp_transport *);
>  void sctp_transport_reset_timers(struct sctp_transport *);
> -void sctp_transport_hold(struct sctp_transport *);
> +int sctp_transport_hold(struct sctp_transport *);
>  void sctp_transport_put(struct sctp_transport *);
>  void sctp_transport_update_rto(struct sctp_transport *, __u32);
>  void sctp_transport_raise_cwnd(struct sctp_transport *, __u32, __u32);
> diff --git a/net/sctp/input.c b/net/sctp/input.c
> index bf61dfb..49d2cc7 100644
> --- a/net/sctp/input.c
> +++ b/net/sctp/input.c
> @@ -935,15 +935,22 @@ static struct sctp_association *__sctp_lookup_association(
>  					struct sctp_transport **pt)
>  {
>  	struct sctp_transport *t;
> +	struct sctp_association *asoc = NULL;
>  
> +	rcu_read_lock();
>  	t = sctp_addrs_lookup_transport(net, local, peer);
> -	if (!t || t->dead)
> -		return NULL;
> +	if (!t || !sctp_transport_hold(t))
> +		goto out;
>  
> -	sctp_association_hold(t->asoc);
> +	asoc = t->asoc;
> +	sctp_association_hold(asoc);
>  	*pt = t;
>  
> -	return t->asoc;
> +	sctp_transport_put(t);
> +
> +out:
> +	rcu_read_unlock();
> +	return asoc;
>  }
>  
>  /* Look up an association. protected by RCU read lock */
> @@ -955,9 +962,7 @@ struct sctp_association *sctp_lookup_association(struct net *net,
>  {
>  	struct sctp_association *asoc;
>  
> -	rcu_read_lock();
>  	asoc = __sctp_lookup_association(net, laddr, paddr, transportp);
> -	rcu_read_unlock();
>  
>  	return asoc;
>  }
> diff --git a/net/sctp/transport.c b/net/sctp/transport.c
> index aab9e3f..69f3799 100644
> --- a/net/sctp/transport.c
> +++ b/net/sctp/transport.c
> @@ -296,9 +296,9 @@ void sctp_transport_route(struct sctp_transport *transport,
>  }
>  
>  /* Hold a reference to a transport.  */
> -void sctp_transport_hold(struct sctp_transport *transport)
> +int sctp_transport_hold(struct sctp_transport *transport)
>  {
> -	atomic_inc(&transport->refcnt);
> +	return atomic_add_unless(&transport->refcnt, 1, 0);
>  }
>  
>  /* Release a reference to a transport and clean up
> -- 
> 2.1.0
> 
> --
> 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 1/3] sctp: fix the transport dead race check by using atomic_add_unless on refcnt

[Vlad Yasevich]

On 01/21/2016 12:49 PM, Xin Long wrote:
> Now when __sctp_lookup_association is running in BH, it will try to
> check if t->dead is set, but meanwhile other CPUs may be freeing this
> transport and this assoc and if it happens that
> __sctp_lookup_association checked t->dead a bit too early, it may think
> that the association is still good while it was already freed.
> 
> So we fix this race by using atomic_add_unless in sctp_transport_hold.
> After we get one transport from hashtable, we will hold it only when
> this transport's refcnt is not 0, so that we can make sure t->asoc
> cannot be freed before we hold the asoc again.

atomic_add_unless() uses atomic_read() to check the value.  Since there
don't appear to be any barriers, what guarantees that the value
read will not have been modified in another thread under a proper lock?


> 
> Note that sctp association is not freed using RCU so we can't use
> atomic_add_unless() with it as it may just be too late for that either.
> 
> Fixes: 4f0087812648 ("sctp: apply rhashtable api to send/recv path")
> Reported-by: Vlad Yasevich <vyasevich@gmail.com>
> Signed-off-by: Xin Long <lucien.xin@gmail.com>
> ---
>  include/net/sctp/structs.h |  2 +-
>  net/sctp/input.c           | 17 +++++++++++------
>  net/sctp/transport.c       |  4 ++--
>  3 files changed, 14 insertions(+), 9 deletions(-)
> 
> diff --git a/include/net/sctp/structs.h b/include/net/sctp/structs.h
> index 20e7212..344da04 100644
> --- a/include/net/sctp/structs.h
> +++ b/include/net/sctp/structs.h
> @@ -955,7 +955,7 @@ void sctp_transport_route(struct sctp_transport *, union sctp_addr *,
>  void sctp_transport_pmtu(struct sctp_transport *, struct sock *sk);
>  void sctp_transport_free(struct sctp_transport *);
>  void sctp_transport_reset_timers(struct sctp_transport *);
> -void sctp_transport_hold(struct sctp_transport *);
> +int sctp_transport_hold(struct sctp_transport *);
>  void sctp_transport_put(struct sctp_transport *);
>  void sctp_transport_update_rto(struct sctp_transport *, __u32);
>  void sctp_transport_raise_cwnd(struct sctp_transport *, __u32, __u32);
> diff --git a/net/sctp/input.c b/net/sctp/input.c
> index bf61dfb..49d2cc7 100644
> --- a/net/sctp/input.c
> +++ b/net/sctp/input.c
> @@ -935,15 +935,22 @@ static struct sctp_association *__sctp_lookup_association(
>  					struct sctp_transport **pt)
>  {
>  	struct sctp_transport *t;
> +	struct sctp_association *asoc = NULL;
>  
> +	rcu_read_lock();
>  	t = sctp_addrs_lookup_transport(net, local, peer);
> -	if (!t || t->dead)
> -		return NULL;
> +	if (!t || !sctp_transport_hold(t))
> +		goto out;
>  
> -	sctp_association_hold(t->asoc);
> +	asoc = t->asoc;
> +	sctp_association_hold(asoc);

I don't think you can modify the reference count on a transport, let alone
the association outside of a lock.

-vlad

>  	*pt = t;
>  
> -	return t->asoc;
> +	sctp_transport_put(t);
> +
> +out:
> +	rcu_read_unlock();
> +	return asoc;
>  }
>  
>  /* Look up an association. protected by RCU read lock */
> @@ -955,9 +962,7 @@ struct sctp_association *sctp_lookup_association(struct net *net,
>  {
>  	struct sctp_association *asoc;
>  
> -	rcu_read_lock();
>  	asoc = __sctp_lookup_association(net, laddr, paddr, transportp);
> -	rcu_read_unlock();
>  
>  	return asoc;
>  }
> diff --git a/net/sctp/transport.c b/net/sctp/transport.c
> index aab9e3f..69f3799 100644
> --- a/net/sctp/transport.c
> +++ b/net/sctp/transport.c
> @@ -296,9 +296,9 @@ void sctp_transport_route(struct sctp_transport *transport,
>  }
>  
>  /* Hold a reference to a transport.  */
> -void sctp_transport_hold(struct sctp_transport *transport)
> +int sctp_transport_hold(struct sctp_transport *transport)
>  {
> -	atomic_inc(&transport->refcnt);
> +	return atomic_add_unless(&transport->refcnt, 1, 0);
>  }
>  
>  /* Release a reference to a transport and clean up
> 

Re: [PATCH net 1/3] sctp: fix the transport dead race check by using atomic_add_unless on refcnt

[Vlad Yasevich]

On 01/21/2016 12:49 PM, Xin Long wrote:
> Now when __sctp_lookup_association is running in BH, it will try to
> check if t->dead is set, but meanwhile other CPUs may be freeing this
> transport and this assoc and if it happens that
> __sctp_lookup_association checked t->dead a bit too early, it may think
> that the association is still good while it was already freed.
> 
> So we fix this race by using atomic_add_unless in sctp_transport_hold.
> After we get one transport from hashtable, we will hold it only when
> this transport's refcnt is not 0, so that we can make sure t->asoc
> cannot be freed before we hold the asoc again.

atomic_add_unless() uses atomic_read() to check the value.  Since there
don't appear to be any barriers, what guarantees that the value
read will not have been modified in another thread under a proper lock?


> 
> Note that sctp association is not freed using RCU so we can't use
> atomic_add_unless() with it as it may just be too late for that either.
> 
> Fixes: 4f0087812648 ("sctp: apply rhashtable api to send/recv path")
> Reported-by: Vlad Yasevich <vyasevich@gmail.com>
> Signed-off-by: Xin Long <lucien.xin@gmail.com>
> ---
>  include/net/sctp/structs.h |  2 +-
>  net/sctp/input.c           | 17 +++++++++++------
>  net/sctp/transport.c       |  4 ++--
>  3 files changed, 14 insertions(+), 9 deletions(-)
> 
> diff --git a/include/net/sctp/structs.h b/include/net/sctp/structs.h
> index 20e7212..344da04 100644
> --- a/include/net/sctp/structs.h
> +++ b/include/net/sctp/structs.h
> @@ -955,7 +955,7 @@ void sctp_transport_route(struct sctp_transport *, union sctp_addr *,
>  void sctp_transport_pmtu(struct sctp_transport *, struct sock *sk);
>  void sctp_transport_free(struct sctp_transport *);
>  void sctp_transport_reset_timers(struct sctp_transport *);
> -void sctp_transport_hold(struct sctp_transport *);
> +int sctp_transport_hold(struct sctp_transport *);
>  void sctp_transport_put(struct sctp_transport *);
>  void sctp_transport_update_rto(struct sctp_transport *, __u32);
>  void sctp_transport_raise_cwnd(struct sctp_transport *, __u32, __u32);
> diff --git a/net/sctp/input.c b/net/sctp/input.c
> index bf61dfb..49d2cc7 100644
> --- a/net/sctp/input.c
> +++ b/net/sctp/input.c
> @@ -935,15 +935,22 @@ static struct sctp_association *__sctp_lookup_association(
>  					struct sctp_transport **pt)
>  {
>  	struct sctp_transport *t;
> +	struct sctp_association *asoc = NULL;
>  
> +	rcu_read_lock();
>  	t = sctp_addrs_lookup_transport(net, local, peer);
> -	if (!t || t->dead)
> -		return NULL;
> +	if (!t || !sctp_transport_hold(t))
> +		goto out;
>  
> -	sctp_association_hold(t->asoc);
> +	asoc = t->asoc;
> +	sctp_association_hold(asoc);

I don't think you can modify the reference count on a transport, let alone
the association outside of a lock.

-vlad

>  	*pt = t;
>  
> -	return t->asoc;
> +	sctp_transport_put(t);
> +
> +out:
> +	rcu_read_unlock();
> +	return asoc;
>  }
>  
>  /* Look up an association. protected by RCU read lock */
> @@ -955,9 +962,7 @@ struct sctp_association *sctp_lookup_association(struct net *net,
>  {
>  	struct sctp_association *asoc;
>  
> -	rcu_read_lock();
>  	asoc = __sctp_lookup_association(net, laddr, paddr, transportp);
> -	rcu_read_unlock();
>  
>  	return asoc;
>  }
> diff --git a/net/sctp/transport.c b/net/sctp/transport.c
> index aab9e3f..69f3799 100644
> --- a/net/sctp/transport.c
> +++ b/net/sctp/transport.c
> @@ -296,9 +296,9 @@ void sctp_transport_route(struct sctp_transport *transport,
>  }
>  
>  /* Hold a reference to a transport.  */
> -void sctp_transport_hold(struct sctp_transport *transport)
> +int sctp_transport_hold(struct sctp_transport *transport)
>  {
> -	atomic_inc(&transport->refcnt);
> +	return atomic_add_unless(&transport->refcnt, 1, 0);
>  }
>  
>  /* Release a reference to a transport and clean up
> 

Re: [PATCH net 1/3] sctp: fix the transport dead race check by using atomic_add_unless on refcnt

[Marcelo Ricardo Leitner]

On Fri, Jan 22, 2016 at 11:50:20AM -0500, Vlad Yasevich wrote:
> On 01/21/2016 12:49 PM, Xin Long wrote:
> > Now when __sctp_lookup_association is running in BH, it will try to
> > check if t->dead is set, but meanwhile other CPUs may be freeing this
> > transport and this assoc and if it happens that
> > __sctp_lookup_association checked t->dead a bit too early, it may think
> > that the association is still good while it was already freed.
> > 
> > So we fix this race by using atomic_add_unless in sctp_transport_hold.
> > After we get one transport from hashtable, we will hold it only when
> > this transport's refcnt is not 0, so that we can make sure t->asoc
> > cannot be freed before we hold the asoc again.
> 
> atomic_add_unless() uses atomic_read() to check the value.  Since there
> don't appear to be any barriers, what guarantees that the value
> read will not have been modified in another thread under a proper lock?
> 

atomic_read() is used only as a starting point.  If it got changed in
between, the new current value (return of atomic_cmpxchg) will be used
then.

> > 
> > Note that sctp association is not freed using RCU so we can't use
> > atomic_add_unless() with it as it may just be too late for that either.
> > 
> > Fixes: 4f0087812648 ("sctp: apply rhashtable api to send/recv path")
> > Reported-by: Vlad Yasevich <vyasevich@gmail.com>
> > Signed-off-by: Xin Long <lucien.xin@gmail.com>
> > ---
> >  include/net/sctp/structs.h |  2 +-
> >  net/sctp/input.c           | 17 +++++++++++------
> >  net/sctp/transport.c       |  4 ++--
> >  3 files changed, 14 insertions(+), 9 deletions(-)
> > 
> > diff --git a/include/net/sctp/structs.h b/include/net/sctp/structs.h
> > index 20e7212..344da04 100644
> > --- a/include/net/sctp/structs.h
> > +++ b/include/net/sctp/structs.h
> > @@ -955,7 +955,7 @@ void sctp_transport_route(struct sctp_transport *, union sctp_addr *,
> >  void sctp_transport_pmtu(struct sctp_transport *, struct sock *sk);
> >  void sctp_transport_free(struct sctp_transport *);
> >  void sctp_transport_reset_timers(struct sctp_transport *);
> > -void sctp_transport_hold(struct sctp_transport *);
> > +int sctp_transport_hold(struct sctp_transport *);
> >  void sctp_transport_put(struct sctp_transport *);
> >  void sctp_transport_update_rto(struct sctp_transport *, __u32);
> >  void sctp_transport_raise_cwnd(struct sctp_transport *, __u32, __u32);
> > diff --git a/net/sctp/input.c b/net/sctp/input.c
> > index bf61dfb..49d2cc7 100644
> > --- a/net/sctp/input.c
> > +++ b/net/sctp/input.c
> > @@ -935,15 +935,22 @@ static struct sctp_association *__sctp_lookup_association(
> >  					struct sctp_transport **pt)
> >  {
> >  	struct sctp_transport *t;
> > +	struct sctp_association *asoc = NULL;
> >  
> > +	rcu_read_lock();
> >  	t = sctp_addrs_lookup_transport(net, local, peer);
> > -	if (!t || t->dead)
> > -		return NULL;
> > +	if (!t || !sctp_transport_hold(t))
> > +		goto out;
> >  
> > -	sctp_association_hold(t->asoc);
> > +	asoc = t->asoc;
> > +	sctp_association_hold(asoc);
> 
> I don't think you can modify the reference count on a transport, let alone
> the association outside of a lock.

The transport memory is not freed, as it's protected by rcu_read_lock(),
so we are safe to use it yet.
atomic_ operations include an embedded lock instruction protecting the
counter itself, there shouldn't be a need to use another lock around it.

And in the code above, as we could grab a hold on the transport, means
the association was not freed yet because transports hold a ref on
assoc. That's why the dance: hold(transport) hold(assoc) put(transport)

  Marcelo

> 
> -vlad
> 
> >  	*pt = t;
> >  
> > -	return t->asoc;
> > +	sctp_transport_put(t);
> > +
> > +out:
> > +	rcu_read_unlock();
> > +	return asoc;
> >  }
> >  
> >  /* Look up an association. protected by RCU read lock */
> > @@ -955,9 +962,7 @@ struct sctp_association *sctp_lookup_association(struct net *net,
> >  {
> >  	struct sctp_association *asoc;
> >  
> > -	rcu_read_lock();
> >  	asoc = __sctp_lookup_association(net, laddr, paddr, transportp);
> > -	rcu_read_unlock();
> >  
> >  	return asoc;
> >  }
> > diff --git a/net/sctp/transport.c b/net/sctp/transport.c
> > index aab9e3f..69f3799 100644
> > --- a/net/sctp/transport.c
> > +++ b/net/sctp/transport.c
> > @@ -296,9 +296,9 @@ void sctp_transport_route(struct sctp_transport *transport,
> >  }
> >  
> >  /* Hold a reference to a transport.  */
> > -void sctp_transport_hold(struct sctp_transport *transport)
> > +int sctp_transport_hold(struct sctp_transport *transport)
> >  {
> > -	atomic_inc(&transport->refcnt);
> > +	return atomic_add_unless(&transport->refcnt, 1, 0);
> >  }
> >  
> >  /* Release a reference to a transport and clean up
> > 
> 
> --
> 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 1/3] sctp: fix the transport dead race check by using atomic_add_unless on refcnt

[Marcelo Ricardo Leitner]

On Fri, Jan 22, 2016 at 11:50:20AM -0500, Vlad Yasevich wrote:
> On 01/21/2016 12:49 PM, Xin Long wrote:
> > Now when __sctp_lookup_association is running in BH, it will try to
> > check if t->dead is set, but meanwhile other CPUs may be freeing this
> > transport and this assoc and if it happens that
> > __sctp_lookup_association checked t->dead a bit too early, it may think
> > that the association is still good while it was already freed.
> > 
> > So we fix this race by using atomic_add_unless in sctp_transport_hold.
> > After we get one transport from hashtable, we will hold it only when
> > this transport's refcnt is not 0, so that we can make sure t->asoc
> > cannot be freed before we hold the asoc again.
> 
> atomic_add_unless() uses atomic_read() to check the value.  Since there
> don't appear to be any barriers, what guarantees that the value
> read will not have been modified in another thread under a proper lock?
> 

atomic_read() is used only as a starting point.  If it got changed in
between, the new current value (return of atomic_cmpxchg) will be used
then.

> > 
> > Note that sctp association is not freed using RCU so we can't use
> > atomic_add_unless() with it as it may just be too late for that either.
> > 
> > Fixes: 4f0087812648 ("sctp: apply rhashtable api to send/recv path")
> > Reported-by: Vlad Yasevich <vyasevich@gmail.com>
> > Signed-off-by: Xin Long <lucien.xin@gmail.com>
> > ---
> >  include/net/sctp/structs.h |  2 +-
> >  net/sctp/input.c           | 17 +++++++++++------
> >  net/sctp/transport.c       |  4 ++--
> >  3 files changed, 14 insertions(+), 9 deletions(-)
> > 
> > diff --git a/include/net/sctp/structs.h b/include/net/sctp/structs.h
> > index 20e7212..344da04 100644
> > --- a/include/net/sctp/structs.h
> > +++ b/include/net/sctp/structs.h
> > @@ -955,7 +955,7 @@ void sctp_transport_route(struct sctp_transport *, union sctp_addr *,
> >  void sctp_transport_pmtu(struct sctp_transport *, struct sock *sk);
> >  void sctp_transport_free(struct sctp_transport *);
> >  void sctp_transport_reset_timers(struct sctp_transport *);
> > -void sctp_transport_hold(struct sctp_transport *);
> > +int sctp_transport_hold(struct sctp_transport *);
> >  void sctp_transport_put(struct sctp_transport *);
> >  void sctp_transport_update_rto(struct sctp_transport *, __u32);
> >  void sctp_transport_raise_cwnd(struct sctp_transport *, __u32, __u32);
> > diff --git a/net/sctp/input.c b/net/sctp/input.c
> > index bf61dfb..49d2cc7 100644
> > --- a/net/sctp/input.c
> > +++ b/net/sctp/input.c
> > @@ -935,15 +935,22 @@ static struct sctp_association *__sctp_lookup_association(
> >  					struct sctp_transport **pt)
> >  {
> >  	struct sctp_transport *t;
> > +	struct sctp_association *asoc = NULL;
> >  
> > +	rcu_read_lock();
> >  	t = sctp_addrs_lookup_transport(net, local, peer);
> > -	if (!t || t->dead)
> > -		return NULL;
> > +	if (!t || !sctp_transport_hold(t))
> > +		goto out;
> >  
> > -	sctp_association_hold(t->asoc);
> > +	asoc = t->asoc;
> > +	sctp_association_hold(asoc);
> 
> I don't think you can modify the reference count on a transport, let alone
> the association outside of a lock.

The transport memory is not freed, as it's protected by rcu_read_lock(),
so we are safe to use it yet.
atomic_ operations include an embedded lock instruction protecting the
counter itself, there shouldn't be a need to use another lock around it.

And in the code above, as we could grab a hold on the transport, means
the association was not freed yet because transports hold a ref on
assoc. That's why the dance: hold(transport) hold(assoc) put(transport)

  Marcelo

> 
> -vlad
> 
> >  	*pt = t;
> >  
> > -	return t->asoc;
> > +	sctp_transport_put(t);
> > +
> > +out:
> > +	rcu_read_unlock();
> > +	return asoc;
> >  }
> >  
> >  /* Look up an association. protected by RCU read lock */
> > @@ -955,9 +962,7 @@ struct sctp_association *sctp_lookup_association(struct net *net,
> >  {
> >  	struct sctp_association *asoc;
> >  
> > -	rcu_read_lock();
> >  	asoc = __sctp_lookup_association(net, laddr, paddr, transportp);
> > -	rcu_read_unlock();
> >  
> >  	return asoc;
> >  }
> > diff --git a/net/sctp/transport.c b/net/sctp/transport.c
> > index aab9e3f..69f3799 100644
> > --- a/net/sctp/transport.c
> > +++ b/net/sctp/transport.c
> > @@ -296,9 +296,9 @@ void sctp_transport_route(struct sctp_transport *transport,
> >  }
> >  
> >  /* Hold a reference to a transport.  */
> > -void sctp_transport_hold(struct sctp_transport *transport)
> > +int sctp_transport_hold(struct sctp_transport *transport)
> >  {
> > -	atomic_inc(&transport->refcnt);
> > +	return atomic_add_unless(&transport->refcnt, 1, 0);
> >  }
> >  
> >  /* Release a reference to a transport and clean up
> > 
> 
> --
> 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 1/3] sctp: fix the transport dead race check by using atomic_add_unless on refcnt

[Vlad Yasevich]

On 01/22/2016 12:18 PM, Marcelo Ricardo Leitner wrote:
> On Fri, Jan 22, 2016 at 11:50:20AM -0500, Vlad Yasevich wrote:
>> On 01/21/2016 12:49 PM, Xin Long wrote:
>>> Now when __sctp_lookup_association is running in BH, it will try to
>>> check if t->dead is set, but meanwhile other CPUs may be freeing this
>>> transport and this assoc and if it happens that
>>> __sctp_lookup_association checked t->dead a bit too early, it may think
>>> that the association is still good while it was already freed.
>>>
>>> So we fix this race by using atomic_add_unless in sctp_transport_hold.
>>> After we get one transport from hashtable, we will hold it only when
>>> this transport's refcnt is not 0, so that we can make sure t->asoc
>>> cannot be freed before we hold the asoc again.
>>
>> atomic_add_unless() uses atomic_read() to check the value.  Since there
>> don't appear to be any barriers, what guarantees that the value
>> read will not have been modified in another thread under a proper lock?
>>
> 
> atomic_read() is used only as a starting point.  If it got changed in
> between, the new current value (return of atomic_cmpxchg) will be used
> then.
> 
>>>
>>> Note that sctp association is not freed using RCU so we can't use
>>> atomic_add_unless() with it as it may just be too late for that either.
>>>
>>> Fixes: 4f0087812648 ("sctp: apply rhashtable api to send/recv path")
>>> Reported-by: Vlad Yasevich <vyasevich@gmail.com>
>>> Signed-off-by: Xin Long <lucien.xin@gmail.com>
>>> ---
>>>  include/net/sctp/structs.h |  2 +-
>>>  net/sctp/input.c           | 17 +++++++++++------
>>>  net/sctp/transport.c       |  4 ++--
>>>  3 files changed, 14 insertions(+), 9 deletions(-)
>>>
>>> diff --git a/include/net/sctp/structs.h b/include/net/sctp/structs.h
>>> index 20e7212..344da04 100644
>>> --- a/include/net/sctp/structs.h
>>> +++ b/include/net/sctp/structs.h
>>> @@ -955,7 +955,7 @@ void sctp_transport_route(struct sctp_transport *, union sctp_addr *,
>>>  void sctp_transport_pmtu(struct sctp_transport *, struct sock *sk);
>>>  void sctp_transport_free(struct sctp_transport *);
>>>  void sctp_transport_reset_timers(struct sctp_transport *);
>>> -void sctp_transport_hold(struct sctp_transport *);
>>> +int sctp_transport_hold(struct sctp_transport *);
>>>  void sctp_transport_put(struct sctp_transport *);
>>>  void sctp_transport_update_rto(struct sctp_transport *, __u32);
>>>  void sctp_transport_raise_cwnd(struct sctp_transport *, __u32, __u32);
>>> diff --git a/net/sctp/input.c b/net/sctp/input.c
>>> index bf61dfb..49d2cc7 100644
>>> --- a/net/sctp/input.c
>>> +++ b/net/sctp/input.c
>>> @@ -935,15 +935,22 @@ static struct sctp_association *__sctp_lookup_association(
>>>  					struct sctp_transport **pt)
>>>  {
>>>  	struct sctp_transport *t;
>>> +	struct sctp_association *asoc = NULL;
>>>  
>>> +	rcu_read_lock();
>>>  	t = sctp_addrs_lookup_transport(net, local, peer);
>>> -	if (!t || t->dead)
>>> -		return NULL;
>>> +	if (!t || !sctp_transport_hold(t))
>>> +		goto out;
>>>  
>>> -	sctp_association_hold(t->asoc);
>>> +	asoc = t->asoc;
>>> +	sctp_association_hold(asoc);
>>
>> I don't think you can modify the reference count on a transport, let alone
>> the association outside of a lock.
> 
> The transport memory is not freed, as it's protected by rcu_read_lock(),
> so we are safe to use it yet.
> atomic_ operations include an embedded lock instruction protecting the
> counter itself, there shouldn't be a need to use another lock around it.
> 
> And in the code above, as we could grab a hold on the transport, means
> the association was not freed yet because transports hold a ref on
> assoc. That's why the dance: hold(transport) hold(assoc) put(transport)
> 

OK,  I see how that holds together, but I think there might be hole wrt icmp
handling.  Some icmp processes assume transport can't disappear on them, but in
this case that last put(transport) may result in a call to sctp_transport_destroy()
and that might be bad.  I am looking at it now.

Thanks
-vlad

>   Marcelo
> 
>>
>> -vlad
>>
>>>  	*pt = t;
>>>  
>>> -	return t->asoc;
>>> +	sctp_transport_put(t);
>>> +
>>> +out:
>>> +	rcu_read_unlock();
>>> +	return asoc;
>>>  }
>>>  
>>>  /* Look up an association. protected by RCU read lock */
>>> @@ -955,9 +962,7 @@ struct sctp_association *sctp_lookup_association(struct net *net,
>>>  {
>>>  	struct sctp_association *asoc;
>>>  
>>> -	rcu_read_lock();
>>>  	asoc = __sctp_lookup_association(net, laddr, paddr, transportp);
>>> -	rcu_read_unlock();
>>>  
>>>  	return asoc;
>>>  }
>>> diff --git a/net/sctp/transport.c b/net/sctp/transport.c
>>> index aab9e3f..69f3799 100644
>>> --- a/net/sctp/transport.c
>>> +++ b/net/sctp/transport.c
>>> @@ -296,9 +296,9 @@ void sctp_transport_route(struct sctp_transport *transport,
>>>  }
>>>  
>>>  /* Hold a reference to a transport.  */
>>> -void sctp_transport_hold(struct sctp_transport *transport)
>>> +int sctp_transport_hold(struct sctp_transport *transport)
>>>  {
>>> -	atomic_inc(&transport->refcnt);
>>> +	return atomic_add_unless(&transport->refcnt, 1, 0);
>>>  }
>>>  
>>>  /* Release a reference to a transport and clean up
>>>
>>
>> --
>> 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 1/3] sctp: fix the transport dead race check by using atomic_add_unless on refcnt

[Vlad Yasevich]

On 01/22/2016 12:18 PM, Marcelo Ricardo Leitner wrote:
> On Fri, Jan 22, 2016 at 11:50:20AM -0500, Vlad Yasevich wrote:
>> On 01/21/2016 12:49 PM, Xin Long wrote:
>>> Now when __sctp_lookup_association is running in BH, it will try to
>>> check if t->dead is set, but meanwhile other CPUs may be freeing this
>>> transport and this assoc and if it happens that
>>> __sctp_lookup_association checked t->dead a bit too early, it may think
>>> that the association is still good while it was already freed.
>>>
>>> So we fix this race by using atomic_add_unless in sctp_transport_hold.
>>> After we get one transport from hashtable, we will hold it only when
>>> this transport's refcnt is not 0, so that we can make sure t->asoc
>>> cannot be freed before we hold the asoc again.
>>
>> atomic_add_unless() uses atomic_read() to check the value.  Since there
>> don't appear to be any barriers, what guarantees that the value
>> read will not have been modified in another thread under a proper lock?
>>
> 
> atomic_read() is used only as a starting point.  If it got changed in
> between, the new current value (return of atomic_cmpxchg) will be used
> then.
> 
>>>
>>> Note that sctp association is not freed using RCU so we can't use
>>> atomic_add_unless() with it as it may just be too late for that either.
>>>
>>> Fixes: 4f0087812648 ("sctp: apply rhashtable api to send/recv path")
>>> Reported-by: Vlad Yasevich <vyasevich@gmail.com>
>>> Signed-off-by: Xin Long <lucien.xin@gmail.com>
>>> ---
>>>  include/net/sctp/structs.h |  2 +-
>>>  net/sctp/input.c           | 17 +++++++++++------
>>>  net/sctp/transport.c       |  4 ++--
>>>  3 files changed, 14 insertions(+), 9 deletions(-)
>>>
>>> diff --git a/include/net/sctp/structs.h b/include/net/sctp/structs.h
>>> index 20e7212..344da04 100644
>>> --- a/include/net/sctp/structs.h
>>> +++ b/include/net/sctp/structs.h
>>> @@ -955,7 +955,7 @@ void sctp_transport_route(struct sctp_transport *, union sctp_addr *,
>>>  void sctp_transport_pmtu(struct sctp_transport *, struct sock *sk);
>>>  void sctp_transport_free(struct sctp_transport *);
>>>  void sctp_transport_reset_timers(struct sctp_transport *);
>>> -void sctp_transport_hold(struct sctp_transport *);
>>> +int sctp_transport_hold(struct sctp_transport *);
>>>  void sctp_transport_put(struct sctp_transport *);
>>>  void sctp_transport_update_rto(struct sctp_transport *, __u32);
>>>  void sctp_transport_raise_cwnd(struct sctp_transport *, __u32, __u32);
>>> diff --git a/net/sctp/input.c b/net/sctp/input.c
>>> index bf61dfb..49d2cc7 100644
>>> --- a/net/sctp/input.c
>>> +++ b/net/sctp/input.c
>>> @@ -935,15 +935,22 @@ static struct sctp_association *__sctp_lookup_association(
>>>  					struct sctp_transport **pt)
>>>  {
>>>  	struct sctp_transport *t;
>>> +	struct sctp_association *asoc = NULL;
>>>  
>>> +	rcu_read_lock();
>>>  	t = sctp_addrs_lookup_transport(net, local, peer);
>>> -	if (!t || t->dead)
>>> -		return NULL;
>>> +	if (!t || !sctp_transport_hold(t))
>>> +		goto out;
>>>  
>>> -	sctp_association_hold(t->asoc);
>>> +	asoc = t->asoc;
>>> +	sctp_association_hold(asoc);
>>
>> I don't think you can modify the reference count on a transport, let alone
>> the association outside of a lock.
> 
> The transport memory is not freed, as it's protected by rcu_read_lock(),
> so we are safe to use it yet.
> atomic_ operations include an embedded lock instruction protecting the
> counter itself, there shouldn't be a need to use another lock around it.
> 
> And in the code above, as we could grab a hold on the transport, means
> the association was not freed yet because transports hold a ref on
> assoc. That's why the dance: hold(transport) hold(assoc) put(transport)
> 

OK,  I see how that holds together, but I think there might be hole wrt icmp
handling.  Some icmp processes assume transport can't disappear on them, but in
this case that last put(transport) may result in a call to sctp_transport_destroy()
and that might be bad.  I am looking at it now.

Thanks
-vlad

>   Marcelo
> 
>>
>> -vlad
>>
>>>  	*pt = t;
>>>  
>>> -	return t->asoc;
>>> +	sctp_transport_put(t);
>>> +
>>> +out:
>>> +	rcu_read_unlock();
>>> +	return asoc;
>>>  }
>>>  
>>>  /* Look up an association. protected by RCU read lock */
>>> @@ -955,9 +962,7 @@ struct sctp_association *sctp_lookup_association(struct net *net,
>>>  {
>>>  	struct sctp_association *asoc;
>>>  
>>> -	rcu_read_lock();
>>>  	asoc = __sctp_lookup_association(net, laddr, paddr, transportp);
>>> -	rcu_read_unlock();
>>>  
>>>  	return asoc;
>>>  }
>>> diff --git a/net/sctp/transport.c b/net/sctp/transport.c
>>> index aab9e3f..69f3799 100644
>>> --- a/net/sctp/transport.c
>>> +++ b/net/sctp/transport.c
>>> @@ -296,9 +296,9 @@ void sctp_transport_route(struct sctp_transport *transport,
>>>  }
>>>  
>>>  /* Hold a reference to a transport.  */
>>> -void sctp_transport_hold(struct sctp_transport *transport)
>>> +int sctp_transport_hold(struct sctp_transport *transport)
>>>  {
>>> -	atomic_inc(&transport->refcnt);
>>> +	return atomic_add_unless(&transport->refcnt, 1, 0);
>>>  }
>>>  
>>>  /* Release a reference to a transport and clean up
>>>
>>
>> --
>> 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 1/3] sctp: fix the transport dead race check by using atomic_add_unless on refcnt

[David Miller]

From: Vlad Yasevich <vyasevich@gmail.com>
Date: Fri, 22 Jan 2016 13:54:09 -0500

> OK,  I see how that holds together, but I think there might be hole wrt icmp
> handling.  Some icmp processes assume transport can't disappear on them, but in
> this case that last put(transport) may result in a call to sctp_transport_destroy()
> and that might be bad.  I am looking at it now.

Vlad, this patch series is being held up because of this.  Please resolve
this one way or the other at your earliest possible convenience, thanks.

Re: [PATCH net 1/3] sctp: fix the transport dead race check by using atomic_add_unless on refcnt

[David Miller]

From: Vlad Yasevich <vyasevich@gmail.com>
Date: Fri, 22 Jan 2016 13:54:09 -0500

> OK,  I see how that holds together, but I think there might be hole wrt icmp
> handling.  Some icmp processes assume transport can't disappear on them, but in
> this case that last put(transport) may result in a call to sctp_transport_destroy()
> and that might be bad.  I am looking at it now.

Vlad, this patch series is being held up because of this.  Please resolve
this one way or the other at your earliest possible convenience, thanks.

Re: [PATCH net 0/3] fix the transport dead race check by using atomic_add_unless on refcnt

[Xin Long]

On Fri, Jan 22, 2016 at 1:49 AM, Xin Long <lucien.xin@gmail.com> wrote:

Fixing a bug that was introduce in rhashtable patchset and a follow-up
cleanup that is now possible

> Xin Long (3):
>   sctp: fix the transport dead race check by using atomic_add_unless on
>     refcnt
>   sctp: hold transport before we access t->asoc in sctp proc
>   sctp: remove the dead field of sctp_transport
>
>  include/net/sctp/structs.h |  5 ++---
>  net/sctp/input.c           | 17 +++++++++++------
>  net/sctp/proc.c            | 12 ++++++++----
>  net/sctp/sm_sideeffect.c   | 12 ------------
>  net/sctp/transport.c       |  8 +++-----
>  5 files changed, 24 insertions(+), 30 deletions(-)
>
> --
> 2.1.0
>

Re: [PATCH net 0/3] fix the transport dead race check by using atomic_add_unless on refcnt

[Xin Long]

On Fri, Jan 22, 2016 at 1:49 AM, Xin Long <lucien.xin@gmail.com> wrote:

Fixing a bug that was introduce in rhashtable patchset and a follow-up
cleanup that is now possible

> Xin Long (3):
>   sctp: fix the transport dead race check by using atomic_add_unless on
>     refcnt
>   sctp: hold transport before we access t->asoc in sctp proc
>   sctp: remove the dead field of sctp_transport
>
>  include/net/sctp/structs.h |  5 ++---
>  net/sctp/input.c           | 17 +++++++++++------
>  net/sctp/proc.c            | 12 ++++++++----
>  net/sctp/sm_sideeffect.c   | 12 ------------
>  net/sctp/transport.c       |  8 +++-----
>  5 files changed, 24 insertions(+), 30 deletions(-)
>
> --
> 2.1.0
>

Re: [PATCH net 0/3] fix the transport dead race check by using atomic_add_unless on refcnt

[David Miller]

From: Xin Long <lucien.xin@gmail.com>
Date: Fri, 22 Jan 2016 01:49:06 +0800

> Xin Long (3):
>   sctp: fix the transport dead race check by using atomic_add_unless on
>     refcnt
>   sctp: hold transport before we access t->asoc in sctp proc
>   sctp: remove the dead field of sctp_transport

Applied, thanks.

Re: [PATCH net 0/3] fix the transport dead race check by using atomic_add_unless on refcnt

[David Miller]

From: Xin Long <lucien.xin@gmail.com>
Date: Fri, 22 Jan 2016 01:49:06 +0800

> Xin Long (3):
>   sctp: fix the transport dead race check by using atomic_add_unless on
>     refcnt
>   sctp: hold transport before we access t->asoc in sctp proc
>   sctp: remove the dead field of sctp_transport

Applied, thanks.