netdev.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Eric Dumazet <edumazet@google.com>
To: Michal Kubecek <mkubecek@suse.cz>
Cc: Eric Dumazet <eric.dumazet@gmail.com>,
	netdev <netdev@vger.kernel.org>, Firo Yang <firo.yang@suse.com>
Subject: Re: possible race in __inet_lookup_established()
Date: Wed, 20 Nov 2019 13:54:18 -0800	[thread overview]
Message-ID: <CANn89i+a7LHSN6sx2NCUXyUph6Uk7B5vh5ZTUAoVExphN0GmTQ@mail.gmail.com> (raw)
In-Reply-To: <20191120211348.GD29650@unicorn.suse.cz>

On Wed, Nov 20, 2019 at 1:13 PM Michal Kubecek <mkubecek@suse.cz> wrote:
>
> On Wed, Nov 20, 2019 at 12:57:48PM -0800, Eric Dumazet wrote:
> > On Wed, Nov 20, 2019 at 12:49 PM Michal Kubecek <mkubecek@suse.cz> wrote:
> > > Firo suggested something like
> > >
> > > ------------------------------------------------------------------------
> > > --- a/net/ipv4/inet_hashtables.c
> > > +++ b/net/ipv4/inet_hashtables.c
> > > @@ -362,6 +362,8 @@ struct sock *__inet_lookup_established(struct net *net,
> > >
> > >  begin:
> > >         sk_nulls_for_each_rcu(sk, node, &head->chain) {
> > > +               if (unlikely(!node))
> > > +                       goto begin;
> > >                 if (sk->sk_hash != hash)
> > >                         continue;
> > >                 if (likely(INET_MATCH(sk, net, acookie,
> > > ------------------------------------------------------------------------
> > >
> > > It depends on implementation details but I believe it would work. It
> > > would be nicer if we could detect the switch to a listening socket but
> > > I don't see how to make such test race free without introducing
> > > unacceptable performance penalty.
> >
> > No, we do not want to add more checks in the fast path really.
> >
> > I was more thinking about not breaking the RCU invariants.
> >
> > (ie : adding back the nulls stuff that I removed in 3b24d854cb35
> > ("tcp/dccp: do not touch
> > listener sk_refcnt under synflood")
>
> Yes, that would do the trick. It would add some cycles to listener
> lookup but that is less harm than slowing down established socket
> lookup.
>

It should not change cycles spent in listener lookup.

Only the test to check for the iteration end will not use NULL, that's about it.

      reply	other threads:[~2019-11-20 21:54 UTC|newest]

Thread overview: 9+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-11-20  8:39 possible race in __inet_lookup_established() Michal Kubecek
     [not found] ` <CANn89iJYXh7AwK8_Aiz3wXqugG0icPNW6OPsPxwOvpH90kr+Ew@mail.gmail.com>
2019-11-20 18:10   ` Michal Kubecek
2019-11-20 19:13     ` Eric Dumazet
2019-11-20 19:52       ` Michal Kubecek
2019-11-20 20:04         ` Eric Dumazet
2019-11-20 20:49           ` Michal Kubecek
2019-11-20 20:57             ` Eric Dumazet
2019-11-20 21:13               ` Michal Kubecek
2019-11-20 21:54                 ` Eric Dumazet [this message]

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=CANn89i+a7LHSN6sx2NCUXyUph6Uk7B5vh5ZTUAoVExphN0GmTQ@mail.gmail.com \
    --to=edumazet@google.com \
    --cc=eric.dumazet@gmail.com \
    --cc=firo.yang@suse.com \
    --cc=mkubecek@suse.cz \
    --cc=netdev@vger.kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).