Re: [PATCH] SCTP: fix race between sctp_bind_addr_free() and sctp_bind_addr_conflict()

[Jacek Luczak <difrost.kernel@gmail.com>]

2011/5/18 Wei Yongjun <yjwei@cn.fujitsu.com>:
>
>> Le mercredi 18 mai 2011 à 10:06 +0200, Jacek Luczak a écrit :
>>> 2011/5/18 Eric Dumazet <eric.dumazet@gmail.com>:
>>>> If you're removing items from this list, you must be a writer here, with
>>>> exclusive access. So rcu_read_lock()/rcu_read_unlock() is not necessary.
>>> I could agree to some extend ... but strict RCU section IMO is needed here.
>>> I can check this if the issue exists.
>>>
>> I can tell you for sure rcu_read_lock() is not needed here. Its only
>> showing confusion from code's author.
>>
>> Please read Documentation/RCU/listRCU.txt for concise explanations,
>> line 117.
>>
>>
>>>> Therefore, I guess following code is better :
>>>>
>>>> list_for_each_entry(addr, &bp->address_list, list) {
>>>>        list_del_rcu(&addr->list);
>>>>        call_rcu(&addr->rcu, sctp_local_addr_free);
>>>>        SCTP_DBG_OBJCNT_DEC(addr);
>>>> }
>>>>
>>>> Then, why dont you fix sctp_bind_addr_clean() instead ?
>>>>
>>>> if 'struct sctp_sockaddr_entry' is recu protected, then all frees should
>>>> be protected as well.
>>> The _clean() as claimed by Vlad is called many times from various places
>>> in code and this could give a overhead. I guess Vlad would need to comment.
>> I guess a full review of this code is needed. You'll have to prove
>> sctp_bind_addr_clean() is always called after one RCU grace period if
>> you want to leave it as is.
>>
>> You cant get RCU for free, some rules must be followed or you risk
>> crashes.
>>
>
> fix the race between sctp_bind_addr_free() and sctp_bind_addr_conflict(), maybe you just
> need to remove the socket from the port hash before empty the bind address list.
> some thing like this, not test.
>
> diff --git a/net/sctp/endpointola.c b/net/sctp/endpointola.c
> index c8cc24e..924d846 100644
> --- a/net/sctp/endpointola.c
> +++ b/net/sctp/endpointola.c
> @@ -268,12 +268,13 @@ static void sctp_endpoint_destroy(struct sctp_endpoint *ep)
>
>        /* Cleanup. */
>        sctp_inq_free(&ep->base.inqueue);
> -       sctp_bind_addr_free(&ep->base.bind_addr);
>
>        /* Remove and free the port */
>        if (sctp_sk(ep->base.sk)->bind_hash)
>                sctp_put_port(ep->base.sk);
>
> +       sctp_bind_addr_free(&ep->base.bind_addr);
> +
>        /* Give up our hold on the sock. */
>        if (ep->base.sk)
>                sock_put(ep->base.sk);
>

Done tests with that one and looks like it survive the flood.

How then this will be handled is up to you. Both ways fix
the issue which makes me happy as damn hell.

@Eric, if you will take a look into the code you might notice
that there are few places where list operations could be
optimised and the main question here is do we really care
to have the data ,,safe'' so that things like that won't popup.
The good example can be the set of _local_ functions.
Ahhh... and I'm aware of how tricky can be abuse of RCU.

-Jacek