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

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

On Wed, Feb 03, 2016 at 05:13:25PM +0100, Dmitry Vyukov wrote:
> On Tue, Jan 19, 2016 at 9:08 PM, Marcelo Ricardo Leitner
> <marcelo.leitner@gmail.com> wrote:
> > 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.
> 
> 
> Hi Marcelo,
> 
> Any updates on this? I still see the leak.

Hi Dmitry,

No, not yet, and I'll be out for 3 weeks starting monday. So if I don't
get it by sunday, it will be a while, sorry.

  Marcelo