Re: [PATCH net] sctp: do sanity checks before migrating the asoc

[Marcelo Ricardo Leitner <marcelo.leitner@gmail.com>]

Em 19-01-2016 17:55, Vlad Yasevich escreveu:
> On 01/19/2016 02:31 PM, Marcelo Ricardo Leitner wrote:
>> Em 19-01-2016 16:37, Vlad Yasevich escreveu:
>>> On 01/19/2016 10:59 AM, Marcelo Ricardo Leitner wrote:
>>>> Yes, not thrilled here either about connect-to-self.
>>>>
>>>> But there is a big difference on how both works. For rx we can just look for wanted skbs
>>>> in rx queue, as they aren't going anywhere, but for tx I don't think we can easily block
>>>> sctp_wfree() call because that may be happening on another CPU (or am I mistaken here?
>>>> sctp still doesn't have RFS but even irqbalance could affect this AFAICT) and more than
>>>> one skb may be in transit at a time.
>>>
>>> The way it's done now, we wouldn't have to block sctp_wfree.  Chunks are released under
>>> lock when they are acked, so we are OK here.  The tx completions will just put 1 byte back
>>> to the socket associated with the tx'ed skb, and that should still be ok as
>>> sctp_packet_release_owner will call sk_free().
>>
>> Please let me rephrase it. I'm actually worried about the asoc->base.sk part of the story
>> and how it's fetched in sctp_wfree(). I think we can update that sk pointer after
>> sock_wfree() has fetched it but not used it yet, possibly leading to accounting it twice,
>> one during migration and one on sock_wfree.
>> In sock_wfree() it will update some sk stats like sk->sk_wmem_alloc, among others.
>
> sctp_wfree() is only used on skbs that were created as sctp chunks to be transmitted.
> Right now, these skbs aren't actually submitted to the IP or to nic to be transmitted.
> They are queued at the association level (either in transports or in the outqueue).
> They are only freed during ACK processing.
>
> The ACK processing happens under a socket lock and thus asoc->base.sk can not move.
>
> The migration process also happens under a socket lock.  As a result, during migration
> we are guaranteed the chunk queues remain consistent and that asoc->base.sk linkage
> remains consistent.  In fact, if you look at the sctp_sock_migrate, we lock both
> sockets when we reassign the assoc->base.sk so we know both sockets are properly locked.
>
> So, I am not sure that what you are worried about can happen.  Please feel free to
> double-check the above of course.

Ohh, right. That makes sense. I'll rework the patch. Thanks Vlad.

   Marcelo