netdev - Re: [PATCH net] net: implement lockless setsockopt(SO_PEEK

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 for Android: free password hash cracker in your pocket

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

Date: Mon, 19 Feb 2024 18:00:28 +0100
From: Eric Dumazet <edumazet@...gle.com>
To: Willem de Bruijn <willemdebruijn.kernel@...il.com>
Cc: "David S . Miller" <davem@...emloft.net>, Jakub Kicinski <kuba@...nel.org>, 
	Paolo Abeni <pabeni@...hat.com>, netdev@...r.kernel.org, eric.dumazet@...il.com, 
	Daan De Meyer <daan.j.demeyer@...il.com>, Kuniyuki Iwashima <kuniyu@...zon.com>, 
	Martin KaFai Lau <martin.lau@...nel.org>, David Ahern <dsahern@...nel.org>
Subject: Re: [PATCH net] net: implement lockless setsockopt(SO_PEEK_OFF)

On Mon, Feb 19, 2024 at 5:07 PM Willem de Bruijn
<willemdebruijn.kernel@...il.com> wrote:
>
> Eric Dumazet wrote:
> > syzbot reported a lockdep violation [1] involving af_unix
> > support of SO_PEEK_OFF.
> >
> > Since SO_PEEK_OFF is inherently not thread safe (it uses a per-socket
> > sk_peek_off field), there is really no point to enforce a pointless
> > thread safety in the kernel.
>
> Would it be sufficient to just move the setsockopt, so that the
> socket lock is not taken, but iolock still is?

Probably, if we focus on the lockdep issue rather than the general
SO_PEEK_OFF mechanism.

We could remove unix_set_peek_off() in net-next,
unless someone explains why keeping a locking on iolock is needed.


>
> Agreed with the general principle that this whole interface is not
> thread safe. So agreed on simplifying. Doubly so for the (lockless)
> UDP path.
>
> sk_peek_offset(), sk_peek_offset_fwd() and sk_peek_offset_bwd() calls
> currently all take place inside a single iolock critical section. If
> not taking iolock, perhaps it would be better if sk_peek_off is at
> least only read once in that critical section, rather than reread
> in sk_peek_offset_fwd() and sk_peek_offset_bwd()?

Note that for sk_peek_offset_bwd() operations, there is no prior read
of sk_peek_off,
since the caller does not use MSG_PEEK (only UDP does a read in an
attempt to avoid the lock...)

static inline int sk_peek_offset(const struct sock *sk, int flags)
{
    if (unlikely(flags & MSG_PEEK))
        return READ_ONCE(sk->sk_peek_off);

    return 0;
}




>
> >
> > After this patch :
> >
> > - setsockopt(SO_PEEK_OFF) no longer acquires the socket lock.
> >
> > - skb_consume_udp() no longer has to acquire the socket lock.
> >
> > - af_unix no longer needs a special version of sk_set_peek_off(),
> >   because it does not lock u->iolock anymore.
> >
> > As a followup, we could replace prot->set_peek_off to be a boolean
> > and avoid an indirect call, since we always use sk_set_peek_off().
>