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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <65d37cdc26e65_1f47ea29474@willemb.c.googlers.com.notmuch>
Date: Mon, 19 Feb 2024 11:07:56 -0500
From: Willem de Bruijn <willemdebruijn.kernel@...il.com>
To: Eric Dumazet <edumazet@...gle.com>, 
 "David S . Miller" <davem@...emloft.net>, 
 Jakub Kicinski <kuba@...nel.org>, 
 Paolo Abeni <pabeni@...hat.com>
Cc: netdev@...r.kernel.org, 
 eric.dumazet@...il.com, 
 Eric Dumazet <edumazet@...gle.com>, 
 Willem de Bruijn <willemdebruijn.kernel@...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)

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?

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()?

> 
> 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().