Commit 1eed677933b816978abc4e3e18ecae5f254cb9be
Committed by
David S. Miller
1 parent
2baaa2d138
sctp: fix the transport dead race check by using atomic_add_unless on refcnt
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> Signed-off-by: David S. Miller <davem@davemloft.net>
Showing 3 changed files with 14 additions and 9 deletions Side-by-side Diff
include/net/sctp/structs.h
... | ... | @@ -955,7 +955,7 @@ |
955 | 955 | void sctp_transport_pmtu(struct sctp_transport *, struct sock *sk); |
956 | 956 | void sctp_transport_free(struct sctp_transport *); |
957 | 957 | void sctp_transport_reset_timers(struct sctp_transport *); |
958 | -void sctp_transport_hold(struct sctp_transport *); | |
958 | +int sctp_transport_hold(struct sctp_transport *); | |
959 | 959 | void sctp_transport_put(struct sctp_transport *); |
960 | 960 | void sctp_transport_update_rto(struct sctp_transport *, __u32); |
961 | 961 | void sctp_transport_raise_cwnd(struct sctp_transport *, __u32, __u32); |
net/sctp/input.c
... | ... | @@ -935,15 +935,22 @@ |
935 | 935 | struct sctp_transport **pt) |
936 | 936 | { |
937 | 937 | struct sctp_transport *t; |
938 | + struct sctp_association *asoc = NULL; | |
938 | 939 | |
940 | + rcu_read_lock(); | |
939 | 941 | t = sctp_addrs_lookup_transport(net, local, peer); |
940 | - if (!t || t->dead) | |
941 | - return NULL; | |
942 | + if (!t || !sctp_transport_hold(t)) | |
943 | + goto out; | |
942 | 944 | |
943 | - sctp_association_hold(t->asoc); | |
945 | + asoc = t->asoc; | |
946 | + sctp_association_hold(asoc); | |
944 | 947 | *pt = t; |
945 | 948 | |
946 | - return t->asoc; | |
949 | + sctp_transport_put(t); | |
950 | + | |
951 | +out: | |
952 | + rcu_read_unlock(); | |
953 | + return asoc; | |
947 | 954 | } |
948 | 955 | |
949 | 956 | /* Look up an association. protected by RCU read lock */ |
950 | 957 | |
... | ... | @@ -955,9 +962,7 @@ |
955 | 962 | { |
956 | 963 | struct sctp_association *asoc; |
957 | 964 | |
958 | - rcu_read_lock(); | |
959 | 965 | asoc = __sctp_lookup_association(net, laddr, paddr, transportp); |
960 | - rcu_read_unlock(); | |
961 | 966 | |
962 | 967 | return asoc; |
963 | 968 | } |
net/sctp/transport.c
... | ... | @@ -296,9 +296,9 @@ |
296 | 296 | } |
297 | 297 | |
298 | 298 | /* Hold a reference to a transport. */ |
299 | -void sctp_transport_hold(struct sctp_transport *transport) | |
299 | +int sctp_transport_hold(struct sctp_transport *transport) | |
300 | 300 | { |
301 | - atomic_inc(&transport->refcnt); | |
301 | + return atomic_add_unless(&transport->refcnt, 1, 0); | |
302 | 302 | } |
303 | 303 | |
304 | 304 | /* Release a reference to a transport and clean up |