| Message ID | 16453bea94a6fc43d657139dff2ce0b5924e2a1f.1354817574.git.tgraf@suug.ch |
|---|---|
| State | Superseded, archived |
| Delegated to: | David Miller |
| Headers | show |
On 12/06/2012 01:15 PM, Thomas Graf wrote: > peer.transport_addr_list is currently only protected by sk_sock > which is inpractical to acquire for procfs dumping purposes. > > This patch adds RCU protection allowing for the procfs readers to > enter RCU read-side critical sections. > > Modification of the list continues to be serialized via sk_lock. > > Cc: Vlad Yasevich <vyasevich@gmail.com> > Cc: Neil Horman <nhorman@tuxdriver.com> > Signed-off-by: Thomas Graf <tgraf@suug.ch> > --- > include/net/sctp/structs.h | 2 ++ > net/sctp/associola.c | 4 ++-- > net/sctp/proc.c | 8 ++++++-- > net/sctp/transport.c | 18 +++++++++++++----- > 4 files changed, 23 insertions(+), 9 deletions(-) > > diff --git a/include/net/sctp/structs.h b/include/net/sctp/structs.h > index 64158aa..5d6987b 100644 > --- a/include/net/sctp/structs.h > +++ b/include/net/sctp/structs.h > @@ -948,6 +948,8 @@ struct sctp_transport { > > /* 64-bit random number sent with heartbeat. */ > __u64 hb_nonce; > + > + struct rcu_head rcu; > }; > > struct sctp_transport *sctp_transport_new(struct net *, const union sctp_addr *, > diff --git a/net/sctp/associola.c b/net/sctp/associola.c > index b1ef3bc..c826bb8 100644 > --- a/net/sctp/associola.c > +++ b/net/sctp/associola.c > @@ -565,7 +565,7 @@ void sctp_assoc_rm_peer(struct sctp_association *asoc, > sctp_assoc_update_retran_path(asoc); > > /* Remove this peer from the list. */ > - list_del(&peer->transports); > + list_del_rcu(&peer->transports); > > /* Get the first transport of asoc. */ > pos = asoc->peer.transport_addr_list.next; > @@ -765,7 +765,7 @@ struct sctp_transport *sctp_assoc_add_peer(struct sctp_association *asoc, > peer->state = peer_state; > > /* Attach the remote transport to our asoc. */ > - list_add_tail(&peer->transports, &asoc->peer.transport_addr_list); > + list_add_tail_rcu(&peer->transports, &asoc->peer.transport_addr_list); > asoc->peer.transport_count++; > > /* If we do not yet have a primary path, set one. */ > diff --git a/net/sctp/proc.c b/net/sctp/proc.c > index ec9b0c8..36a3f9d 100644 > --- a/net/sctp/proc.c > +++ b/net/sctp/proc.c > @@ -159,7 +159,8 @@ static void sctp_seq_dump_remote_addrs(struct seq_file *seq, struct sctp_associa > struct sctp_af *af; > > primary = &assoc->peer.primary_addr; > - list_for_each_entry(transport, &assoc->peer.transport_addr_list, > + rcu_read_lock(); > + list_for_each_entry_rcu(transport, &assoc->peer.transport_addr_list, > transports) { > addr = &transport->ipaddr; > af = sctp_get_af_specific(addr->sa.sa_family); > @@ -168,6 +169,7 @@ static void sctp_seq_dump_remote_addrs(struct seq_file *seq, struct sctp_associa > } > af->seq_dump_addr(seq, addr); > } > + rcu_read_unlock(); > } > > static void * sctp_eps_seq_start(struct seq_file *seq, loff_t *pos) > @@ -438,11 +440,12 @@ static int sctp_remaddr_seq_show(struct seq_file *seq, void *v) > head = &sctp_assoc_hashtable[hash]; > sctp_local_bh_disable(); > read_lock(&head->lock); > + rcu_read_lock(); > sctp_for_each_hentry(epb, node, &head->chain) { > if (!net_eq(sock_net(epb->sk), seq_file_net(seq))) > continue; > assoc = sctp_assoc(epb); > - list_for_each_entry(tsp, &assoc->peer.transport_addr_list, > + list_for_each_entry_rcu(tsp, &assoc->peer.transport_addr_list, > transports) { > /* > * The remote address (ADDR) > @@ -489,6 +492,7 @@ static int sctp_remaddr_seq_show(struct seq_file *seq, void *v) > } > } > > + rcu_read_unlock(); > read_unlock(&head->lock); > sctp_local_bh_enable(); > > diff --git a/net/sctp/transport.c b/net/sctp/transport.c > index 206cf52..1295aec 100644 > --- a/net/sctp/transport.c > +++ b/net/sctp/transport.c > @@ -163,13 +163,11 @@ void sctp_transport_free(struct sctp_transport *transport) > sctp_transport_put(transport); > } > > -/* Destroy the transport data structure. > - * Assumes there are no more users of this structure. > - */ > -static void sctp_transport_destroy(struct sctp_transport *transport) > +static void sctp_transport_destroy_rcu(struct rcu_head *head) > { > - SCTP_ASSERT(transport->dead, "Transport is not dead", return); > + struct sctp_transport *transport; > > + transport = container_of(head, struct sctp_transport, rcu); > if (transport->asoc) > sctp_association_put(transport->asoc); > > @@ -180,6 +178,16 @@ static void sctp_transport_destroy(struct sctp_transport *transport) > SCTP_DBG_OBJCNT_DEC(transport); > } > > +/* Destroy the transport data structure. > + * Assumes there are no more users of this structure. > + */ > +static void sctp_transport_destroy(struct sctp_transport *transport) > +{ > + SCTP_ASSERT(transport->dead, "Transport is not dead", return); > + > + call_rcu(&transport->rcu, sctp_transport_destroy_rcu); > +} > + > /* Start T3_rtx timer if it is not already running and update the heartbeat > * timer. This routine is called every time a DATA chunk is sent. > */ > We may want to mark transports as dead sooner. Probably right about the time we pull them off the list. When displaying, we may want to look at transport->dead, and skip them. It will reduce the probability that we would be looking at a transport that's about to go away. Otherwise, looks very nice. -vlad -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 12/06/12 at 01:35pm, Vlad Yasevich wrote: > We may want to mark transports as dead sooner. Probably right about > the time we pull them off the list. We mark it dead in sctp_transport_free() which is called at the end of sctp_assoc_rm_peer(). Do you want to mark it dead at the beginning of sctp_assoc_rm_peer() as well? (We still need to mark in sctp_transport_free() anyway). > When displaying, we may want to > look at transport->dead, and skip them. It will reduce the probability > that we would be looking at a transport that's about to go away. Agreed. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 12/06/2012 01:44 PM, Thomas Graf wrote: > On 12/06/12 at 01:35pm, Vlad Yasevich wrote: >> We may want to mark transports as dead sooner. Probably right about >> the time we pull them off the list. > > We mark it dead in sctp_transport_free() which is called at the > end of sctp_assoc_rm_peer(). Do you want to mark it dead at the > beginning of sctp_assoc_rm_peer() as well? (We still need to > mark in sctp_transport_free() anyway). Crud.. sctp_transport_free() is called directly in places... Hmm... the one in sctp_association_free() may need to be list_del_rcu()... Ok, we can leave the dead handling the way it is.. -vlad > >> When displaying, we may want to >> look at transport->dead, and skip them. It will reduce the probability >> that we would be looking at a transport that's about to go away. > > Agreed. > -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 12/06/12 at 01:57pm, Vlad Yasevich wrote: > On 12/06/2012 01:44 PM, Thomas Graf wrote: > >On 12/06/12 at 01:35pm, Vlad Yasevich wrote: > >>We may want to mark transports as dead sooner. Probably right about > >>the time we pull them off the list. > > > >We mark it dead in sctp_transport_free() which is called at the > >end of sctp_assoc_rm_peer(). Do you want to mark it dead at the > >beginning of sctp_assoc_rm_peer() as well? (We still need to > >mark in sctp_transport_free() anyway). > > Crud.. sctp_transport_free() is called directly in places... Hmm... > the one in sctp_association_free() may need to be list_del_rcu()... It's not really needed but it wouldn't be wrong from a documentation perspective. The assoc is always unhashed while holding head->lock before sctp_association_free() and all current RCU readers of transport_addr_list access the the assoc while holding a read-lock on head->lock. Let me respin this patch and do a list_del_rcu() there to document the RCU'iness of it. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 12/06/2012 02:08 PM, Thomas Graf wrote: > On 12/06/12 at 01:57pm, Vlad Yasevich wrote: >> On 12/06/2012 01:44 PM, Thomas Graf wrote: >>> On 12/06/12 at 01:35pm, Vlad Yasevich wrote: >>>> We may want to mark transports as dead sooner. Probably right about >>>> the time we pull them off the list. >>> >>> We mark it dead in sctp_transport_free() which is called at the >>> end of sctp_assoc_rm_peer(). Do you want to mark it dead at the >>> beginning of sctp_assoc_rm_peer() as well? (We still need to >>> mark in sctp_transport_free() anyway). >> >> Crud.. sctp_transport_free() is called directly in places... Hmm... >> the one in sctp_association_free() may need to be list_del_rcu()... > > It's not really needed but it wouldn't be wrong from a > documentation perspective. The assoc is always unhashed > while holding head->lock before sctp_association_free() > and all current RCU readers of transport_addr_list access > the the assoc while holding a read-lock on head->lock. > > Let me respin this patch and do a list_del_rcu() there > to document the RCU'iness of it. > Right, but there may be chunks that have cached association with a ref before sctp_association_free() is called. Now, after free they may be looking at the transport list for whatever reason... Most places check assoc->dead, but I don't want to get caught. So, there is a remote chance that someone may look at transports and would crash without rcu. -vlad -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 12/06/12 at 02:14pm, Vlad Yasevich wrote: > Right, but there may be chunks that have cached association with a ref > before sctp_association_free() is called. Now, after free they may > be looking at the transport list for whatever reason... Most places > check assoc->dead, but I don't want to get caught. So, there is > a remote chance that someone may look at transports and would crash > without rcu. You are right, sctp_associate_free() can be called even though there may still be multiple refs on that assoc around. We are currently fine as I believe sk_lock serializes all the accesses but better be safe than sorry. I've respun the patches in a v2 patch series. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/include/net/sctp/structs.h b/include/net/sctp/structs.h index 64158aa..5d6987b 100644 --- a/include/net/sctp/structs.h +++ b/include/net/sctp/structs.h @@ -948,6 +948,8 @@ struct sctp_transport { /* 64-bit random number sent with heartbeat. */ __u64 hb_nonce; + + struct rcu_head rcu; }; struct sctp_transport *sctp_transport_new(struct net *, const union sctp_addr *, diff --git a/net/sctp/associola.c b/net/sctp/associola.c index b1ef3bc..c826bb8 100644 --- a/net/sctp/associola.c +++ b/net/sctp/associola.c @@ -565,7 +565,7 @@ void sctp_assoc_rm_peer(struct sctp_association *asoc, sctp_assoc_update_retran_path(asoc); /* Remove this peer from the list. */ - list_del(&peer->transports); + list_del_rcu(&peer->transports); /* Get the first transport of asoc. */ pos = asoc->peer.transport_addr_list.next; @@ -765,7 +765,7 @@ struct sctp_transport *sctp_assoc_add_peer(struct sctp_association *asoc, peer->state = peer_state; /* Attach the remote transport to our asoc. */ - list_add_tail(&peer->transports, &asoc->peer.transport_addr_list); + list_add_tail_rcu(&peer->transports, &asoc->peer.transport_addr_list); asoc->peer.transport_count++; /* If we do not yet have a primary path, set one. */ diff --git a/net/sctp/proc.c b/net/sctp/proc.c index ec9b0c8..36a3f9d 100644 --- a/net/sctp/proc.c +++ b/net/sctp/proc.c @@ -159,7 +159,8 @@ static void sctp_seq_dump_remote_addrs(struct seq_file *seq, struct sctp_associa struct sctp_af *af; primary = &assoc->peer.primary_addr; - list_for_each_entry(transport, &assoc->peer.transport_addr_list, + rcu_read_lock(); + list_for_each_entry_rcu(transport, &assoc->peer.transport_addr_list, transports) { addr = &transport->ipaddr; af = sctp_get_af_specific(addr->sa.sa_family); @@ -168,6 +169,7 @@ static void sctp_seq_dump_remote_addrs(struct seq_file *seq, struct sctp_associa } af->seq_dump_addr(seq, addr); } + rcu_read_unlock(); } static void * sctp_eps_seq_start(struct seq_file *seq, loff_t *pos) @@ -438,11 +440,12 @@ static int sctp_remaddr_seq_show(struct seq_file *seq, void *v) head = &sctp_assoc_hashtable[hash]; sctp_local_bh_disable(); read_lock(&head->lock); + rcu_read_lock(); sctp_for_each_hentry(epb, node, &head->chain) { if (!net_eq(sock_net(epb->sk), seq_file_net(seq))) continue; assoc = sctp_assoc(epb); - list_for_each_entry(tsp, &assoc->peer.transport_addr_list, + list_for_each_entry_rcu(tsp, &assoc->peer.transport_addr_list, transports) { /* * The remote address (ADDR) @@ -489,6 +492,7 @@ static int sctp_remaddr_seq_show(struct seq_file *seq, void *v) } } + rcu_read_unlock(); read_unlock(&head->lock); sctp_local_bh_enable(); diff --git a/net/sctp/transport.c b/net/sctp/transport.c index 206cf52..1295aec 100644 --- a/net/sctp/transport.c +++ b/net/sctp/transport.c @@ -163,13 +163,11 @@ void sctp_transport_free(struct sctp_transport *transport) sctp_transport_put(transport); } -/* Destroy the transport data structure. - * Assumes there are no more users of this structure. - */ -static void sctp_transport_destroy(struct sctp_transport *transport) +static void sctp_transport_destroy_rcu(struct rcu_head *head) { - SCTP_ASSERT(transport->dead, "Transport is not dead", return); + struct sctp_transport *transport; + transport = container_of(head, struct sctp_transport, rcu); if (transport->asoc) sctp_association_put(transport->asoc); @@ -180,6 +178,16 @@ static void sctp_transport_destroy(struct sctp_transport *transport) SCTP_DBG_OBJCNT_DEC(transport); } +/* Destroy the transport data structure. + * Assumes there are no more users of this structure. + */ +static void sctp_transport_destroy(struct sctp_transport *transport) +{ + SCTP_ASSERT(transport->dead, "Transport is not dead", return); + + call_rcu(&transport->rcu, sctp_transport_destroy_rcu); +} + /* Start T3_rtx timer if it is not already running and update the heartbeat * timer. This routine is called every time a DATA chunk is sent. */
peer.transport_addr_list is currently only protected by sk_sock which is inpractical to acquire for procfs dumping purposes. This patch adds RCU protection allowing for the procfs readers to enter RCU read-side critical sections. Modification of the list continues to be serialized via sk_lock. Cc: Vlad Yasevich <vyasevich@gmail.com> Cc: Neil Horman <nhorman@tuxdriver.com> Signed-off-by: Thomas Graf <tgraf@suug.ch> --- include/net/sctp/structs.h | 2 ++ net/sctp/associola.c | 4 ++-- net/sctp/proc.c | 8 ++++++-- net/sctp/transport.c | 18 +++++++++++++----- 4 files changed, 23 insertions(+), 9 deletions(-)