netdev - Re: [PATCH v1 net 1/2] raw: Fix NULL deref in raw_get

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Tue, 4 Apr 2023 14:56:18 +0800
From:   Jason Xing <kerneljasonxing@...il.com>
To:     Eric Dumazet <edumazet@...gle.com>
Cc:     Kuniyuki Iwashima <kuniyu@...zon.com>,
        "David S. Miller" <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>,
        Paolo Abeni <pabeni@...hat.com>,
        Kuniyuki Iwashima <kuni1840@...il.com>, netdev@...r.kernel.org,
        syzbot <syzkaller@...glegroups.com>,
        "Dae R . Jeong" <threeearcat@...il.com>
Subject: Re: [PATCH v1 net 1/2] raw: Fix NULL deref in raw_get_next().

On Tue, Apr 4, 2023 at 12:07 PM Eric Dumazet <edumazet@...gle.com> wrote:
>
> On Tue, Apr 4, 2023 at 4:46 AM Jason Xing <kerneljasonxing@...il.com> wrote:
> >
> > I would like to ask two questions which make me confused:
> > 1) Why would we use spin_lock to protect the socket in a raw hashtable
> > for reader's safety under the rcu protection? Normally, if we use the
> > RCU protection, we only make sure that we need to destroy the socket
> > by calling call_rcu() which would prevent the READER of the socket
> > from getting a NULL pointer.
>
> Yes, but then we can not sleep or yield the cpu.

Indeed. We also cannot sleep/yield under the protection of the spin
lock. And I checked the caller in fs/seq_file.c and noticed that we
have no chance to sleep/yield between ->start and ->stop.

So I wonder why we couldn't use RCU directly like the patch[1] you
proposed before and choose deliberately to switch to spin lock? Spin
lock for the whole hashinfo to protect the reader side is heavy, and
RCU outperforms spin lock in this case, I think.

[1]: commit 0daf07e52709 ("raw: convert raw sockets to RCU")

Thanks,
Jason
>
> > 2) Using spin lock when we're cating /proc/net/raw file may
> > block/postpone the action of hashing socket somewhere else?
>
> /proc/net/raw file access is rare, and limited in duration (at most
> one page filled by system call)
>
> Use of RCU was only intended in my original patch to solve deadlock issues
> under packet floods, like DOS attacks.
>
> Really using RCU in the data/fast path makes sense (and we did that already).
> For control paths (or /proc/.... files), not so much.