On Wed, Jan 6, 2016 at 3:07 AM, Vlad Yasevich <vyasevich@xxxxxxxxx> wrote:
> On 12/30/2015 10:50 AM, Xin Long wrote:
>> apply lookup apis to two functions, for __sctp_endpoint_lookup_assoc
>> and __sctp_lookup_association, it's invoked in the protection of sock
>> lock, it will be safe, but sctp_lookup_association need to call
>> rcu_read_lock() and to detect the t->dead to protect it.
>>
>> Signed-off-by: Xin Long <lucien.xin@xxxxxxxxx>
>> Signed-off-by: Marcelo Ricardo Leitner <marcelo.leitner@xxxxxxxxx>
>> ---
>> net/sctp/associola.c | 5 +++++
>> net/sctp/endpointola.c | 35 ++++++++---------------------------
>> net/sctp/input.c | 39 ++++++++++-----------------------------
>> net/sctp/protocol.c | 6 ++++++
>> 4 files changed, 29 insertions(+), 56 deletions(-)
>>
>> diff --git a/net/sctp/associola.c b/net/sctp/associola.c
>> index 559afd0..2bf8ec9 100644
>> --- a/net/sctp/associola.c
>> +++ b/net/sctp/associola.c
>> @@ -383,6 +383,7 @@ void sctp_association_free(struct sctp_association *asoc)
>> list_for_each_safe(pos, temp, &asoc->peer.transport_addr_list) {
>> transport = list_entry(pos, struct sctp_transport, transports);
>> list_del_rcu(pos);
>> + sctp_unhash_transport(transport);
>> sctp_transport_free(transport);
>> }
>>
>> @@ -500,6 +501,8 @@ void sctp_assoc_rm_peer(struct sctp_association *asoc,
>>
>> /* Remove this peer from the list. */
>> list_del_rcu(&peer->transports);
>> + /* Remove this peer from the transport hashtable */
>> + sctp_unhash_transport(peer);
>>
>> /* Get the first transport of asoc. */
>> pos = asoc->peer.transport_addr_list.next;
>> @@ -699,6 +702,8 @@ struct sctp_transport *sctp_assoc_add_peer(struct sctp_association *asoc,
>> /* Attach the remote transport to our asoc. */
>> list_add_tail_rcu(&peer->transports, &asoc->peer.transport_addr_list);
>> asoc->peer.transport_count++;
>> + /* Add this peer into the transport hashtable */
>> + sctp_hash_transport(peer);
>
> This is actually problematic. The issue is that transports are unhashed when removed.
> however, transport removal happens after the association has been declared dead and
> should have been removed from the hash and marked unreachable.
>
> As a result, with the code above, you can now find and return a dead association.
> Checking for 'dead' state is racy.
>
> The best solution I've come up with is to hash the transports in sctp_hash_established()
> and clean-up in __sctp_unhash_established(), and then handle ADD-IP case separately.
>
> The above would also remove the necessity to check for temporary associations, since they
> should never be hashed.
>
> -vlad
>
yes, you're right, im thinking if we can unhash transport before the association
declares dead in sctp_association_free, like:
list_for_each_safe(pos, temp, &asoc->peer.transport_addr_list) {
transport = list_entry(pos, struct sctp_transport, transports);
sctp_unhash_transport(transport);
}
asoc->base.dead = true;
--
To unsubscribe from this list: send the line "unsubscribe linux-sctp" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
