From fba4c330c5b9d1beeae08a42ed4430f5055e7f27 Mon Sep 17 00:00:00 2001 From: Xin Long Date: Fri, 22 Jan 2016 01:49:08 +0800 Subject: sctp: hold transport before we access t->asoc in sctp proc 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 Reviewed-by: Marcelo Ricardo Leitner Signed-off-by: David S. Miller --- net/sctp/proc.c | 8 ++++++++ 1 file changed, 8 insertions(+) (limited to 'net/sctp/proc.c') diff --git a/net/sctp/proc.c b/net/sctp/proc.c index 684c5b31563b..c74a810150aa 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; } -- cgit v1.2.3 From 47faa1e4c50ec26e6e75dcd1ce53f064bd45f729 Mon Sep 17 00:00:00 2001 From: Xin Long Date: Fri, 22 Jan 2016 01:49:09 +0800 Subject: sctp: remove the dead field of sctp_transport 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 Signed-off-by: Marcelo Ricardo Leitner Signed-off-by: David S. Miller --- 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(-) (limited to 'net/sctp/proc.c') diff --git a/include/net/sctp/structs.h b/include/net/sctp/structs.h index 344da04e2e30..205630bb5010 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 c74a810150aa..ded7d931a6a5 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 2e21384697c2..b5327bb77458 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 69f3799570a7..a431c14044a4 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; } -- cgit v1.2.3