fix the transport dead race check by using atomic_add_unless on refcnt

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

---
 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(-)

include/net/sctp/structs.h +1 -2

index 344da04..205630b 100644
@@ -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

net/sctp/proc.c +0 -4

index c74a810..ded7d93 100644
@@ -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)
 		 */

net/sctp/sm_sideeffect.c +0 -12

index 2e21384..b5327bb 100644
@@ -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,

net/sctp/transport.c +1 -3

index 69f3799..a431c14 100644
@@ -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;
 	}

Signature · 6 lines

2.1.0

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

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

---
 net/sctp/proc.c | 8 ++++++++
 1 file changed, 8 insertions(+)

net/sctp/proc.c +8 -0

index 684c5b3..c74a810 100644
@@ -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;
 }
 

Signature · 6 lines

2.1.0

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

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

---
 net/sctp/proc.c | 8 ++++++++
 1 file changed, 8 insertions(+)

net/sctp/proc.c +0 -0

index 684c5b3..c74a810 100644

Quoted 21 lines · 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 <[email protected]>

---
 net/sctp/proc.c | 8 ++++++++
 1 file changed, 8 insertions(+)

net/sctp/proc.c +0 -0

index 684c5b3..c74a810 100644
@@ -380,6 +380,8 @@ static int sctp_assocs_seq_show(struct seq_file *seq, void *v)
 	}
 

net/sctp/proc.c static int sctp_assocs_seq_show(struct seq_file *seq, void *v) · line 382

Quoted 1 line

net/sctp/proc.c +0 -0

 	transport = (struct sctp_transport *)v;

What protects you from this structure already being freed ?

net/sctp/proc.c static int sctp_assocs_seq_show(struct seq_file *seq, void *v) · line 384

Quoted 2 lines

net/sctp/proc.c +2 -0

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

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

Quoted 22 lines · Eric Dumazet

Quoted 21 lines · 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 <[email protected]>

---
 net/sctp/proc.c | 8 ++++++++
 1 file changed, 8 insertions(+)

net/sctp/proc.c +0 -0

index 684c5b3..c74a810 100644
@@ -380,6 +380,8 @@ static int sctp_assocs_seq_show(struct seq_file *seq, void *v)
 	}
 

Quoted 3 lines

net/sctp/proc.c static int sctp_assocs_seq_show(struct seq_file *seq, void *v) · line 382

Quoted 1 line

net/sctp/proc.c +0 -0

 	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.

Quoted 5 lines

net/sctp/proc.c static int sctp_assocs_seq_show(struct seq_file *seq, void *v) · line 384

Quoted 2 lines

net/sctp/proc.c +2 -0

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

Quoted 37 lines · Marcelo Ricardo Leitner

Quoted 22 lines · Eric Dumazet

Quoted 21 lines · 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 <[email protected]>

---
 net/sctp/proc.c | 8 ++++++++
 1 file changed, 8 insertions(+)

net/sctp/proc.c +0 -0

index 684c5b3..c74a810 100644
@@ -380,6 +380,8 @@ static int sctp_assocs_seq_show(struct seq_file *seq, void *v)
 	}
 

Quoted 3 lines

net/sctp/proc.c static int sctp_assocs_seq_show(struct seq_file *seq, void *v) · line 382

Quoted 1 line

net/sctp/proc.c +0 -0

 	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.

Quoted 5 lines

net/sctp/proc.c static int sctp_assocs_seq_show(struct seq_file *seq, void *v) · line 384

Quoted 2 lines

net/sctp/proc.c +2 -0

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

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

Quoted 6 lines

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.

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

Signed-off-by Xin Long <[email protected]>

---
 include/net/sctp/structs.h |  2 +-
 net/sctp/input.c           | 17 +++++++++++------
 net/sctp/transport.c       |  4 ++--
 3 files changed, 14 insertions(+), 9 deletions(-)

include/net/sctp/structs.h +1 -1

index 20e7212..344da04 100644
@@ -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);

net/sctp/input.c +11 -6

index bf61dfb..49d2cc7 100644
@@ -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;
 }

net/sctp/transport.c +2 -2

index aab9e3f..69f3799 100644
@@ -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

Signature · 6 lines

2.1.0

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

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

Signed-off-by Xin Long <[email protected]>

---
 include/net/sctp/structs.h |  2 +-
 net/sctp/input.c           | 17 +++++++++++------
 net/sctp/transport.c       |  4 ++--
 3 files changed, 14 insertions(+), 9 deletions(-)

include/net/sctp/structs.h +1 -1

index 20e7212..344da04 100644
@@ -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);

net/sctp/input.c +1 -0

index bf61dfb..49d2cc7 100644
@@ -935,15 +935,22 @@ static struct sctp_association *__sctp_lookup_association(
 					struct sctp_transport **pt)
 {
 	struct sctp_transport *t;
+	struct sctp_association *asoc = NULL;
 

net/sctp/input.c +5 -3

 	*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;
 }

net/sctp/transport.c +2 -2

index aab9e3f..69f3799 100644
@@ -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

Quoted 6 lines · 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.

Quoted 4 lines

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?

Quoted 43 lines

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

Signed-off-by Xin Long <[email protected]>

---
 include/net/sctp/structs.h |  2 +-
 net/sctp/input.c           | 17 +++++++++++------
 net/sctp/transport.c       |  4 ++--
 3 files changed, 14 insertions(+), 9 deletions(-)

include/net/sctp/structs.h +1 -1

index 20e7212..344da04 100644
@@ -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);

net/sctp/input.c +4 -2

index bf61dfb..49d2cc7 100644
@@ -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;
 

net/sctp/input.c static struct sctp_association *__sctp_lookup_association( · line 946

Quoted 3 lines

net/sctp/input.c +2 -1

-	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

Quoted 28 lines

net/sctp/input.c +5 -3

 	*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;
 }

net/sctp/transport.c +0 -0

index aab9e3f..69f3799 100644
@@ -296,9 +296,9 @@ void sctp_transport_route(struct sctp_transport *transport,
 }

net/sctp/transport.c void sctp_transport_route(struct sctp_transport *transport, · line 304

Quoted 10 lines

net/sctp/transport.c +2 -2

 
 /* 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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Quoted 7 lines · Vlad Yasevich

Quoted 6 lines · 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.

Quoted 9 lines

Quoted 4 lines

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.

Quoted 43 lines

Quoted 43 lines

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

Signed-off-by Xin Long <[email protected]>

---
 include/net/sctp/structs.h |  2 +-
 net/sctp/input.c           | 17 +++++++++++------
 net/sctp/transport.c       |  4 ++--
 3 files changed, 14 insertions(+), 9 deletions(-)

include/net/sctp/structs.h +1 -1

index 20e7212..344da04 100644
@@ -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);

net/sctp/input.c +4 -2

index bf61dfb..49d2cc7 100644
@@ -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;
 

Quoted 6 lines

net/sctp/input.c static struct sctp_association *__sctp_lookup_association( · line 946

Quoted 3 lines

net/sctp/input.c +2 -1

-	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

Quoted 47 lines

-vlad

Quoted 28 lines

net/sctp/input.c +5 -3

 	*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;
 }

net/sctp/transport.c +0 -0

index aab9e3f..69f3799 100644
@@ -296,9 +296,9 @@ void sctp_transport_route(struct sctp_transport *transport,
 }

net/sctp/transport.c void sctp_transport_route(struct sctp_transport *transport, · line 304

Quoted 10 lines

net/sctp/transport.c +2 -2

 
 /* 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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

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.

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

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