Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
From:   Kuniyuki Iwashima <kuniyu@amazon.com>
To:     <edumazet@google.com>
CC:     <davem@davemloft.net>, <dsahern@kernel.org>, <kuba@kernel.org>,
        <kuniyu@amazon.com>, <linux-kernel@vger.kernel.org>,
        <netdev@vger.kernel.org>, <pabeni@redhat.com>,
        <threeearcat@gmail.com>
Subject: Re: general protection fault in raw_seq_start
Date:   Fri, 31 Mar 2023 00:17:25 -0700
Message-ID: <20230331071725.66950-1-kuniyu@amazon.com>
In-Reply-To: <CANn89iK5D75-SNg28ALi4Zr9JEHnreBpfu_pq0_zLe4jDLT5rw@mail.gmail.com>
References: <CANn89iK5D75-SNg28ALi4Zr9JEHnreBpfu_pq0_zLe4jDLT5rw@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: 8bit
Precedence: bulk

From:   Eric Dumazet <edumazet@google.com>
Date:   Fri, 31 Mar 2023 09:04:47 +0200
> On Thu, Mar 30, 2023 at 11:55 PM Kuniyuki Iwashima <kuniyu@amazon.com> wrote:
> 
> > Thanks for reporting the issue.
> >
> > It seems we need to use RCU variant in raw_get_first().
> > I'll post a patch.
> >
> > ---
> > diff --git a/net/ipv4/raw.c b/net/ipv4/raw.c
> > index 3cf68695b40d..fe0d1ad20b35 100644
> > --- a/net/ipv4/raw.c
> > +++ b/net/ipv4/raw.c
> > @@ -957,7 +957,7 @@ static struct sock *raw_get_first(struct seq_file *seq, int bucket)
> >         for (state->bucket = bucket; state->bucket < RAW_HTABLE_SIZE;
> >                         ++state->bucket) {
> >                 hlist = &h->ht[state->bucket];
> > -               sk_nulls_for_each(sk, hnode, hlist) {
> > +               sk_nulls_for_each_rcu(sk, hnode, hlist) {
> >                         if (sock_net(sk) == seq_file_net(seq))
> >                                 return sk;
> >
> 
> No, we do not want this.
> You missed that sk_nulls_for_each_rcu() needs a specific protocol
> (see Documentation/RCU/rculist_nulls.rst for details)

Ah, exactly SOCK_RAW does not have SLAB_TYPESAFE_BY_RCU.
Thank you for pointing this out!

And I found this seems wrong.

c25b7a7a565e ("inet: ping: use hlist_nulls rcu iterator during lookup")

> 
> RCU is needed in the data path, not for this control path.
> 
> My patch went too far in the RCU conversion. I did not think about
> syzbot harassing /proc files :)
> 
> We need raw_seq_start and friends to go back to use the lock.

Ok, then I'll change /proc/net/{raw, icmp} to use spinlock :)