Netdev Archive on lore.kernel.org
 help / color / Atom feed
From: Michal Kubecek <mkubecek@suse.cz>
To: Eric Dumazet <edumazet@google.com>
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 22:13:48 +0100
Message-ID: <20191120211348.GD29650@unicorn.suse.cz> (raw)
In-Reply-To: <CANn89iJeq2CCBrdgt=fFxG3Uk7f4CHbLfsOM2S8q3ucC6znzEA@mail.gmail.com>

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.

Michal

  reply index

Thread overview: 9+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-11-20  8:39 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 [this message]
2019-11-20 21:54                 ` Eric Dumazet

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=20191120211348.GD29650@unicorn.suse.cz \
    --to=mkubecek@suse.cz \
    --cc=edumazet@google.com \
    --cc=eric.dumazet@gmail.com \
    --cc=firo.yang@suse.com \
    --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

Netdev Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/netdev/0 netdev/git/0.git
	git clone --mirror https://lore.kernel.org/netdev/1 netdev/git/1.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 netdev netdev/ https://lore.kernel.org/netdev \
		netdev@vger.kernel.org
	public-inbox-index netdev

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.netdev


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git