netdev - Re: [PATCH bpf-next v3 03/16] xdp: add proper __rcu annotations to redirect map entries

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: <87r1guovg2.fsf@toke.dk>
Date:   Tue, 22 Jun 2021 15:55:25 +0200
From:   Toke Høiland-Jørgensen <toke@...hat.com>
To:     Daniel Borkmann <daniel@...earbox.net>,
        "Paul E . McKenney" <paulmck@...nel.org>
Cc:     Martin KaFai Lau <kafai@...com>,
        Hangbin Liu <liuhangbin@...il.com>,
        Jesper Dangaard Brouer <brouer@...hat.com>,
        Magnus Karlsson <magnus.karlsson@...il.com>,
        Jakub Kicinski <kuba@...nel.org>, bpf@...r.kernel.org,
        netdev@...r.kernel.org
Subject: Re: [PATCH bpf-next v3 03/16] xdp: add proper __rcu annotations to
 redirect map entries

Toke Høiland-Jørgensen <toke@...hat.com> writes:

>> It would also be great if this scenario in general could be placed
>> under the Documentation/RCU/whatisRCU.rst as an example, so we could
>> refer to the official doc on this, too, if Paul is good with this.
>
> I'll take a look and see if I can find a way to fit it in there...

OK, I poked around in Documentation/RCU and decided that the most
natural place to put this was in checklist.rst which already talks about
local_bh_disable(), but a bit differently. Fixing that up to correspond
to what we've been discussing in this thread, and adding a mention of
XDP as a usage example, results in the patch below.

Paul, WDYT?

-Toke



diff --git a/Documentation/RCU/checklist.rst b/Documentation/RCU/checklist.rst
index 1030119294d0..e5bc93e8f9f5 100644
--- a/Documentation/RCU/checklist.rst
+++ b/Documentation/RCU/checklist.rst
@@ -226,12 +226,16 @@ over a rather long period of time, but improvements are always welcome!
 	broken kernels, and has even resulted in an exploitable security
 	issue.
 
-	One exception to this rule: rcu_read_lock() and rcu_read_unlock()
-	may be substituted for rcu_read_lock_bh() and rcu_read_unlock_bh()
-	in cases where local bottom halves are already known to be
-	disabled, for example, in irq or softirq context.  Commenting
-	such cases is a must, of course!  And the jury is still out on
-	whether the increased speed is worth it.
+	One exception to this rule: a pair of local_bh_disable() /
+	local_bh_enable() calls function like one big RCU read-side critical
+	section, so separate rcu_read_lock()s can be omitted in cases where
+	local bottom halves are already known to be disabled, for example, in
+	irq or softirq context. Commenting such cases is a must, of course!
+	One notable example of this usage is the XDP feature in networking,
+	which calls BPF programs from network-driver NAPI (softirq) context.
+	BPF relies heavily on RCU protection for its data structures, but
+	because the BPF program invocation happens entirely within a single
+	local_bh_disable() section in a NAPI poll cycle, this usage is safe.
 
 8.	Although synchronize_rcu() is slower than is call_rcu(), it
 	usually results in simpler code.  So, unless update performance is