During the discussion[0] of Hangbin's multicast patch series, Martin pointed out
that the lifetime of the RCU-protected map entries used by XDP_REDIRECT is by
no means obvious. I promised to look into cleaning this up, and Paul helpfully
provided some hints and a new unrcu_pointer() helper to aid in this.
It seems[1] that back in the early days of XDP, local_bh_disable() did not
provide RCU protection, which is why the rcu_read_lock() calls were added
to drivers in the first place. But according to Paul[2], in recent kernels
a local_bh_disable()/local_bh_enable() pair functions as one big RCU
read-side section, so no further protection is needed. This even applies to
-rt kernels, which has an explicit rcu_read_lock() in place as part of the
local_bh_disable()[3].
This patch series is mostly a documentation exercise, cleaning up the
description of the lifetime expectations and adding __rcu annotations so
sparse and lockdep can help verify it.
Patches 1 and 2 are preparatory: Patch 1 adds Paul's unrcu_pointer()
helper (which has already been added to his tree), which we need for some
of the operations in devmap, and patch 2 adds bh context as a valid
condition for map lookups. Patch 3 is the main bit that adds the __rcu
annotations and updates documentation comments, and the rest are patches
updating the drivers, with one patch per distinct maintainer.
Unfortunately I don't have any hardware to test any of the driver patches;
Jesper helpfully verified that it doesn't break anything on i40e, but the rest
of the driver patches are only compile-tested.
[0] https://lore.kernel.org/bpf/20210415173551.7ma4slcbqeyiba2r@kafai-mbp.dhcp.thefacebook.com/
[1] https://lore.kernel.org/bpf/c5192ab3-1c05-8679-79f2-59d98299095b@iogearbox.net/
[2] https://lore.kernel.org/bpf/20210417002301.GO4212@paulmck-ThinkPad-P17-Gen-1/
[3] https://lore.kernel.org/bpf/20210419165837.GA975577@paulmck-ThinkPad-P17-Gen-1/
Changelog:
v3:
- Remove one other unnecessary change to hlist_for_each_entry_rcu()
- Carry forward another ACK
v2:
- Add a comment about RCU protection to the drivers where rcu_read_lock()
is removed
- Drop unnecessary patch 3 which changed dev_get_by_index_rcu()
- Add some more text with the history to cover letter
- Fix a few places where the wrong RCU checks were used in cpumap and
xskmap code
- Carry forward ACKs
Paul E. McKenney (1):
rcu: Create an unrcu_pointer() to remove __rcu from a pointer
Toke Høiland-Jørgensen (15):
bpf: allow RCU-protected lookups to happen from bh context
xdp: add proper __rcu annotations to redirect map entries
ena: remove rcu_read_lock() around XDP program invocation
bnxt: remove rcu_read_lock() around XDP program invocation
thunderx: remove rcu_read_lock() around XDP program invocation
freescale: remove rcu_read_lock() around XDP program invocation
net: intel: remove rcu_read_lock() around XDP program invocation
marvell: remove rcu_read_lock() around XDP program invocation
mlx4: remove rcu_read_lock() around XDP program invocation
nfp: remove rcu_read_lock() around XDP program invocation
qede: remove rcu_read_lock() around XDP program invocation
sfc: remove rcu_read_lock() around XDP program invocation
netsec: remove rcu_read_lock() around XDP program invocation
stmmac: remove rcu_read_lock() around XDP program invocation
net: ti: remove rcu_read_lock() around XDP program invocation
drivers/net/ethernet/amazon/ena/ena_netdev.c | 6 +--
drivers/net/ethernet/broadcom/bnxt/bnxt_xdp.c | 5 +-
.../net/ethernet/cavium/thunder/nicvf_main.c | 5 +-
.../net/ethernet/freescale/dpaa/dpaa_eth.c | 11 ++---
.../net/ethernet/freescale/dpaa2/dpaa2-eth.c | 6 +--
drivers/net/ethernet/intel/i40e/i40e_txrx.c | 5 +-
drivers/net/ethernet/intel/i40e/i40e_xsk.c | 11 ++---
drivers/net/ethernet/intel/ice/ice_txrx.c | 6 +--
drivers/net/ethernet/intel/ice/ice_xsk.c | 6 +--
drivers/net/ethernet/intel/igb/igb_main.c | 5 +-
drivers/net/ethernet/intel/igc/igc_main.c | 10 ++--
drivers/net/ethernet/intel/ixgbe/ixgbe_main.c | 5 +-
drivers/net/ethernet/intel/ixgbe/ixgbe_xsk.c | 9 ++--
.../net/ethernet/intel/ixgbevf/ixgbevf_main.c | 5 +-
drivers/net/ethernet/marvell/mvneta.c | 6 ++-
.../net/ethernet/marvell/mvpp2/mvpp2_main.c | 8 +--
drivers/net/ethernet/mellanox/mlx4/en_rx.c | 8 +--
.../ethernet/netronome/nfp/nfp_net_common.c | 6 ++-
drivers/net/ethernet/qlogic/qede/qede_fp.c | 7 +--
drivers/net/ethernet/sfc/rx.c | 12 ++---
drivers/net/ethernet/socionext/netsec.c | 7 +--
.../net/ethernet/stmicro/stmmac/stmmac_main.c | 13 ++---
drivers/net/ethernet/ti/cpsw_priv.c | 13 ++---
include/linux/rcupdate.h | 14 ++++++
include/net/xdp_sock.h | 2 +-
kernel/bpf/cpumap.c | 13 +++--
kernel/bpf/devmap.c | 49 ++++++++-----------
kernel/bpf/hashtab.c | 21 +++++---
kernel/bpf/helpers.c | 6 +--
kernel/bpf/lpm_trie.c | 6 ++-
net/core/filter.c | 28 +++++++++++
net/xdp/xsk.c | 4 +-
net/xdp/xsk.h | 4 +-
net/xdp/xskmap.c | 29 ++++++-----
34 files changed, 194 insertions(+), 157 deletions(-)
--
2.32.0
XDP programs are called from a NAPI poll context, which means the RCU
reference liveness is ensured by local_bh_disable(). Add
rcu_read_lock_bh_held() as a condition to the RCU checks for map lookups so
lockdep understands that the dereferences are safe from inside *either* an
rcu_read_lock() section *or* a local_bh_disable() section. While both
bh_disabled and rcu_read_lock() provide RCU protection, they are
semantically distinct, so we need both conditions to prevent lockdep
complaints.
This change is done in preparation for removing the redundant
rcu_read_lock()s from drivers.
Acked-by: Martin KaFai Lau <redacted>
Signed-off-by: Toke Høiland-Jørgensen <redacted>
---
kernel/bpf/hashtab.c | 21 ++++++++++++++-------
kernel/bpf/helpers.c | 6 +++---
kernel/bpf/lpm_trie.c | 6 ++++--
3 files changed, 21 insertions(+), 12 deletions(-)
From: "Paul E. McKenney" <paulmck@kernel.org>
The xchg() and cmpxchg() functions are sometimes used to carry out RCU
updates. Unfortunately, this can result in sparse warnings for both
the old-value and new-value arguments, as well as for the return value.
The arguments can be dealt with using RCU_INITIALIZER():
old_p = xchg(&p, RCU_INITIALIZER(new_p));
But a sparse warning still remains due to assigning the __rcu pointer
returned from xchg to the (most likely) non-__rcu pointer old_p.
This commit therefore provides an unrcu_pointer() macro that strips
the __rcu. This macro can be used as follows:
old_p = unrcu_pointer(xchg(&p, RCU_INITIALIZER(new_p)));
Reported-by: Toke Høiland-Jørgensen <redacted>
Signed-off-by: Paul E. McKenney <paulmck@kernel.org>
Signed-off-by: Toke Høiland-Jørgensen <redacted>
---
include/linux/rcupdate.h | 14 ++++++++++++++
1 file changed, 14 insertions(+)
The ena driver has rcu_read_lock()/rcu_read_unlock() pairs around XDP
program invocations. However, the actual lifetime of the objects referred
by the XDP program invocation is longer, all the way through to the call to
xdp_do_flush(), making the scope of the rcu_read_lock() too small. This
turns out to be harmless because it all happens in a single NAPI poll
cycle (and thus under local_bh_disable()), but it makes the rcu_read_lock()
misleading.
Rather than extend the scope of the rcu_read_lock(), just get rid of it
entirely. With the addition of RCU annotations to the XDP_REDIRECT map
types that take bh execution into account, lockdep even understands this to
be safe, so there's really no reason to keep it around.
Cc: Guy Tzalik <redacted>
Cc: Saeed Bishara <redacted>
Signed-off-by: Toke Høiland-Jørgensen <redacted>
---
drivers/net/ethernet/amazon/ena/ena_netdev.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
@@ -385,7 +385,9 @@ static int ena_xdp_execute(struct ena_ring *rx_ring, struct xdp_buff *xdp)u64*xdp_stat;intqid;-rcu_read_lock();+/* This code is invoked within a single NAPI poll cycle and thus under+*local_bh_disable(),whichprovidestheneededRCUprotection.+*/xdp_prog=READ_ONCE(rx_ring->xdp_bpf_prog);if(!xdp_prog)
The Intel drivers all have rcu_read_lock()/rcu_read_unlock() pairs around
XDP program invocations. However, the actual lifetime of the objects
referred by the XDP program invocation is longer, all the way through to
the call to xdp_do_flush(), making the scope of the rcu_read_lock() too
small. This turns out to be harmless because it all happens in a single
NAPI poll cycle (and thus under local_bh_disable()), but it makes the
rcu_read_lock() misleading.
Rather than extend the scope of the rcu_read_lock(), just get rid of it
entirely. With the addition of RCU annotations to the XDP_REDIRECT map
types that take bh execution into account, lockdep even understands this to
be safe, so there's really no reason to keep it around.
Cc: Jesse Brandeburg <redacted>
Cc: Tony Nguyen <anthony.l.nguyen@intel.com>
Cc: intel-wired-lan@lists.osuosl.org
Tested-by: Jesper Dangaard Brouer <redacted> # i40e
Signed-off-by: Toke Høiland-Jørgensen <redacted>
---
drivers/net/ethernet/intel/i40e/i40e_txrx.c | 5 +++--
drivers/net/ethernet/intel/i40e/i40e_xsk.c | 11 +++++------
drivers/net/ethernet/intel/ice/ice_txrx.c | 6 +-----
drivers/net/ethernet/intel/ice/ice_xsk.c | 6 +-----
drivers/net/ethernet/intel/igb/igb_main.c | 5 +++--
drivers/net/ethernet/intel/igc/igc_main.c | 10 +++++-----
drivers/net/ethernet/intel/ixgbe/ixgbe_main.c | 5 +++--
drivers/net/ethernet/intel/ixgbe/ixgbe_xsk.c | 9 ++++-----
drivers/net/ethernet/intel/ixgbevf/ixgbevf_main.c | 5 +++--
9 files changed, 28 insertions(+), 34 deletions(-)
@@ -2306,6 +2305,9 @@ static int i40e_run_xdp(struct i40e_ring *rx_ring, struct xdp_buff *xdp)prefetchw(xdp->data_hard_start);/* xdp_frame write */+/* This code is invoked within a single NAPI poll cycle and thus under+*local_bh_disable(),whichprovidestheneededRCUprotection.+*/act=bpf_prog_run_xdp(xdp_prog,xdp);switch(act){caseXDP_PASS:
@@ -153,8 +153,10 @@ static int i40e_run_xdp_zc(struct i40e_ring *rx_ring, struct xdp_buff *xdp)structbpf_prog*xdp_prog;u32act;-rcu_read_lock();-/* NB! xdp_prog will always be !NULL, due to the fact that+/* This code is invoked within a single NAPI poll cycle and thus under+*local_bh_disable(),whichprovidestheneededRCUprotection.+*+*NB!xdp_progwillalwaysbe!NULL,duetothefactthat*thispathisenabledbysettinganXDPprogram.*/xdp_prog=READ_ONCE(rx_ring->xdp_prog);
@@ -1129,15 +1129,11 @@ int ice_clean_rx_irq(struct ice_ring *rx_ring, int budget)xdp.frame_sz=ice_rx_frame_truesize(rx_ring,size);#endif-rcu_read_lock();xdp_prog=READ_ONCE(rx_ring->xdp_prog);-if(!xdp_prog){-rcu_read_unlock();+if(!xdp_prog)gotoconstruct_skb;-}xdp_res=ice_run_xdp(rx_ring,&xdp,xdp_prog);-rcu_read_unlock();if(!xdp_res)gotoconstruct_skb;if(xdp_res&(ICE_XDP_TX|ICE_XDP_REDIR)){
@@ -463,7 +463,6 @@ ice_run_xdp_zc(struct ice_ring *rx_ring, struct xdp_buff *xdp)structice_ring*xdp_ring;u32act;-rcu_read_lock();/* ZC patch is enabled only when XDP program is set,*sohereitcannotbeNULL*/
@@ -8395,6 +8394,9 @@ static struct sk_buff *igb_run_xdp(struct igb_adapter *adapter,prefetchw(xdp->data_hard_start);/* xdp_frame write */+/* This code is invoked within a single NAPI poll cycle and thus under+*local_bh_disable(),whichprovidestheneededRCUprotection.+*/act=bpf_prog_run_xdp(xdp_prog,xdp);switch(act){caseXDP_PASS:
@@ -2175,18 +2175,18 @@ static struct sk_buff *igc_xdp_run_prog(struct igc_adapter *adapter,structbpf_prog*prog;intres;-rcu_read_lock();-prog=READ_ONCE(adapter->xdp_prog);if(!prog){res=IGC_XDP_PASS;-gotounlock;+gotoout;}+/* This code is invoked within a single NAPI poll cycle and thus under+*local_bh_disable(),whichprovidestheneededRCUprotection.+*/res=__igc_xdp_run_prog(adapter,prog,xdp);-unlock:-rcu_read_unlock();+out:returnERR_PTR(-res);}
@@ -2207,6 +2206,9 @@ static struct sk_buff *ixgbe_run_xdp(struct ixgbe_adapter *adapter,prefetchw(xdp->data_hard_start);/* xdp_frame write */+/* This code is invoked within a single NAPI poll cycle and thus under+*local_bh_disable(),whichprovidestheneededRCUprotection.+*/act=bpf_prog_run_xdp(xdp_prog,xdp);switch(act){caseXDP_PASS:
@@ -100,15 +100,15 @@ static int ixgbe_run_xdp_zc(struct ixgbe_adapter *adapter,structxdp_frame*xdpf;u32act;-rcu_read_lock();+/* This code is invoked within a single NAPI poll cycle and thus under+*local_bh_disable(),whichprovidestheneededRCUprotection.+*/xdp_prog=READ_ONCE(rx_ring->xdp_prog);act=bpf_prog_run_xdp(xdp_prog,xdp);if(likely(act==XDP_REDIRECT)){err=xdp_do_redirect(rx_ring->netdev,xdp,xdp_prog);-result=!err?IXGBE_XDP_REDIR:IXGBE_XDP_CONSUMED;-rcu_read_unlock();-returnresult;+return!err?IXGBE_XDP_REDIR:IXGBE_XDP_CONSUMED;}switch(act){
@@ -132,7 +132,6 @@ static int ixgbe_run_xdp_zc(struct ixgbe_adapter *adapter,result=IXGBE_XDP_CONSUMED;break;}-rcu_read_unlock();returnresult;}
@@ -1054,12 +1054,14 @@ static struct sk_buff *ixgbevf_run_xdp(struct ixgbevf_adapter *adapter,structbpf_prog*xdp_prog;u32act;-rcu_read_lock();xdp_prog=READ_ONCE(rx_ring->xdp_prog);if(!xdp_prog)gotoxdp_out;+/* This code is invoked within a single NAPI poll cycle and thus under+*local_bh_disable(),whichprovidestheneededRCUprotection.+*/act=bpf_prog_run_xdp(xdp_prog,xdp);switch(act){caseXDP_PASS:
The stmmac driver has rcu_read_lock()/rcu_read_unlock() pairs around XDP
program invocations. However, the actual lifetime of the objects referred
by the XDP program invocation is longer, all the way through to the call to
xdp_do_flush(), making the scope of the rcu_read_lock() too small. This
turns out to be harmless because it all happens in a single NAPI poll
cycle (and thus under local_bh_disable()), but it makes the rcu_read_lock()
misleading.
Rather than extend the scope of the rcu_read_lock(), just get rid of it
entirely. With the addition of RCU annotations to the XDP_REDIRECT map
types that take bh execution into account, lockdep even understands this to
be safe, so there's really no reason to keep it around.
Cc: Giuseppe Cavallaro <redacted>
Cc: Alexandre Torgue <alexandre.torgue@foss.st.com>
Cc: Jose Abreu <redacted>
Signed-off-by: Toke Høiland-Jørgensen <redacted>
---
drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 13 +++++--------
1 file changed, 5 insertions(+), 8 deletions(-)
@@ -4654,7 +4654,6 @@ static int stmmac_xdp_xmit_back(struct stmmac_priv *priv,returnres;}-/* This function assumes rcu_read_lock() is held by the caller. */staticint__stmmac_xdp_run_prog(structstmmac_priv*priv,structbpf_prog*prog,structxdp_buff*xdp)
@@ -4662,6 +4661,9 @@ static int __stmmac_xdp_run_prog(struct stmmac_priv *priv,u32act;intres;+/* This code is invoked within a single NAPI poll cycle and thus under+*local_bh_disable(),whichprovidestheneededRCUprotection.+*/act=bpf_prog_run_xdp(prog,xdp);switch(act){caseXDP_PASS:
XDP_REDIRECT works by a three-step process: the bpf_redirect() and
bpf_redirect_map() helpers will lookup the target of the redirect and store
it (along with some other metadata) in a per-CPU struct bpf_redirect_info.
Next, when the program returns the XDP_REDIRECT return code, the driver
will call xdp_do_redirect() which will use the information thus stored to
actually enqueue the frame into a bulk queue structure (that differs
slightly by map type, but shares the same principle). Finally, before
exiting its NAPI poll loop, the driver will call xdp_do_flush(), which will
flush all the different bulk queues, thus completing the redirect.
Pointers to the map entries will be kept around for this whole sequence of
steps, protected by RCU. However, there is no top-level rcu_read_lock() in
the core code; instead drivers add their own rcu_read_lock() around the XDP
portions of the code, but somewhat inconsistently as Martin discovered[0].
However, things still work because everything happens inside a single NAPI
poll sequence, which means it's between a pair of calls to
local_bh_disable()/local_bh_enable(). So Paul suggested[1] that we could
document this intention by using rcu_dereference_check() with
rcu_read_lock_bh_held() as a second parameter, thus allowing sparse and
lockdep to verify that everything is done correctly.
This patch does just that: we add an __rcu annotation to the map entry
pointers and remove the various comments explaining the NAPI poll assurance
strewn through devmap.c in favour of a longer explanation in filter.c. The
goal is to have one coherent documentation of the entire flow, and rely on
the RCU annotations as a "standard" way of communicating the flow in the
map code (which can additionally be understood by sparse and lockdep).
The RCU annotation replacements result in a fairly straight-forward
replacement where READ_ONCE() becomes rcu_dereference_check(), WRITE_ONCE()
becomes rcu_assign_pointer() and xchg() and cmpxchg() gets wrapped in the
proper constructs to cast the pointer back and forth between __rcu and
__kernel address space (for the benefit of sparse). The one complication is
that xskmap has a few constructions where double-pointers are passed back
and forth; these simply all gain __rcu annotations, and only the final
reference/dereference to the inner-most pointer gets changed.
With this, everything can be run through sparse without eliciting
complaints, and lockdep can verify correctness even without the use of
rcu_read_lock() in the drivers. Subsequent patches will clean these up from
the drivers.
[0] https://lore.kernel.org/bpf/20210415173551.7ma4slcbqeyiba2r@kafai-mbp.dhcp.thefacebook.com/
[1] https://lore.kernel.org/bpf/20210419165837.GA975577@paulmck-ThinkPad-P17-Gen-1/
Signed-off-by: Toke Høiland-Jørgensen <redacted>
---
include/net/xdp_sock.h | 2 +-
kernel/bpf/cpumap.c | 13 +++++++----
kernel/bpf/devmap.c | 49 ++++++++++++++++++------------------------
net/core/filter.c | 28 ++++++++++++++++++++++++
net/xdp/xsk.c | 4 ++--
net/xdp/xsk.h | 4 ++--
net/xdp/xskmap.c | 29 ++++++++++++++-----------
7 files changed, 80 insertions(+), 49 deletions(-)
@@ -74,7 +74,7 @@ struct bpf_cpu_map_entry {structbpf_cpu_map{structbpf_mapmap;/* Below members specific for map type */-structbpf_cpu_map_entry**cpu_map;+structbpf_cpu_map_entry__rcu**cpu_map;};staticDEFINE_PER_CPU(structlist_head,cpu_map_flush_list);
@@ -562,6 +562,10 @@ static void cpu_map_free(struct bpf_map *map)kfree(cmap);}+/* Elements are kept alive by RCU; either by rcu_read_lock() (from syscall) or+*bylocal_bh_disable()(fromXDPcallsinsideNAPI).The+*rcu_read_lock_bh_held()belowmakeslockdepacceptboth.+*/staticvoid*__cpu_map_lookup_elem(structbpf_map*map,u32key){structbpf_cpu_map*cmap=container_of(map,structbpf_cpu_map,map);
@@ -73,7 +73,7 @@ struct bpf_dtab_netdev {structbpf_dtab{structbpf_mapmap;-structbpf_dtab_netdev**netdev_map;/* DEVMAP type only */+structbpf_dtab_netdev__rcu**netdev_map;/* DEVMAP type only */structlist_headlist;/* these are only used for DEVMAP_HASH type maps */
@@ -259,6 +259,10 @@ static int dev_map_get_next_key(struct bpf_map *map, void *key, void *next_key)return0;}+/* Elements are kept alive by RCU; either by rcu_read_lock() (from syscall) or+*bylocal_bh_disable()(fromXDPcallsinsideNAPI).The+*rcu_read_lock_bh_held()belowmakeslockdepacceptboth.+*/staticvoid*__dev_map_hash_lookup_elem(structbpf_map*map,u32key){structbpf_dtab*dtab=container_of(map,structbpf_dtab,map);
@@ -410,15 +414,9 @@ static void bq_xmit_all(struct xdp_dev_bulk_queue *bq, u32 flags)trace_xdp_devmap_xmit(bq->dev_rx,dev,sent,cnt-sent,err);}-/* __dev_flush is called from xdp_do_flush() which _must_ be signaled-*fromthedriverbeforereturningfromitsnapi->poll()routine.Thepoll()-*routineiscalledeitherfrombusy_pollcontextornet_rx_actionsignaled-*fromNET_RX_SOFTIRQ.Eitherwaythepollroutinemustcompletebeforethe-*netdevicecanbetorndown.Ondevmapteardownweensuretheflushlist-*isemptybeforecompletingtoensureallflushoperationshavecompleted.-*Whendriversupdatethebpfprogramtheymayneedtoensureanyflushops-*arealsocomplete.Usingsynchronize_rcuorcall_rcuwillsufficeforthis-*becausebothwaitfornapicontexttoexit.+/* __dev_flush is called from xdp_do_flush() which _must_ be signalled from the+*driverbeforereturningfromitsnapi->poll()routine.Seethecommentabove+*xdp_do_flush()infilter.c.*/void__dev_flush(void){
@@ -433,9 +431,9 @@ void __dev_flush(void)}}-/* rcu_read_lock (from syscall and BPF contexts) ensures that if a delete and/or-*updatehappensinparallelhereadev_putwon'thappenuntilafterreading-*theifindex.+/* Elements are kept alive by RCU; either by rcu_read_lock() (from syscall) or+*bylocal_bh_disable()(fromXDPcallsinsideNAPI).The+*rcu_read_lock_bh_held()belowmakeslockdepacceptboth.*/staticvoid*__dev_map_lookup_elem(structbpf_map*map,u32key){
@@ -445,12 +443,14 @@ static void *__dev_map_lookup_elem(struct bpf_map *map, u32 key)if(key>=map->max_entries)returnNULL;-obj=READ_ONCE(dtab->netdev_map[key]);+obj=rcu_dereference_check(dtab->netdev_map[key],+rcu_read_lock_bh_held());returnobj;}-/* Runs under RCU-read-side, plus in softirq under NAPI protection.-*Thus,safepercpuvariableaccess.+/* Runs in NAPI, i.e., softirq under local_bh_disable(). Thus, safe percpu+*variableaccess,andmapelementsstickaround.Seecommentabove+*xdp_do_flush()infilter.c.*/staticvoidbq_enqueue(structnet_device*dev,structxdp_frame*xdpf,structnet_device*dev_rx,structbpf_prog*xdp_prog)
@@ -735,14 +735,7 @@ static int dev_map_delete_elem(struct bpf_map *map, void *key)if(k>=map->max_entries)return-EINVAL;-/* Use call_rcu() here to ensure any rcu critical sections have-*completedaswellasanyflushoperationsbecausecall_rcu-*willwaitforpreempt-disableregiontocomplete,NAPIinthis-*context.Andadditionally,thedriverteardownensuresall-*softirqsarecompletebeforeremovingthenetdeviceinthe-*caseofdev_putequalszero.-*/-old_dev=xchg(&dtab->netdev_map[k],NULL);+old_dev=unrcu_pointer(xchg(&dtab->netdev_map[k],NULL));if(old_dev)call_rcu(&old_dev->rcu,__dev_map_entry_free);return0;
@@ -851,7 +844,7 @@ static int __dev_map_update_elem(struct net *net, struct bpf_map *map,*Rememberingthedriversideflushoperationwillhappenbeforethe*netdeviceisremoved.*/-old_dev=xchg(&dtab->netdev_map[i],dev);+old_dev=unrcu_pointer(xchg(&dtab->netdev_map[i],RCU_INITIALIZER(dev)));if(old_dev)call_rcu(&old_dev->rcu,__dev_map_entry_free);
@@ -1031,10 +1024,10 @@ static int dev_map_notification(struct notifier_block *notifier,for(i=0;i<dtab->map.max_entries;i++){structbpf_dtab_netdev*dev,*odev;-dev=READ_ONCE(dtab->netdev_map[i]);+dev=rcu_dereference(dtab->netdev_map[i]);if(!dev||netdev!=dev->dev)continue;-odev=cmpxchg(&dtab->netdev_map[i],dev,NULL);+odev=unrcu_pointer(cmpxchg(&dtab->netdev_map[i],RCU_INITIALIZER(dev),NULL));if(dev==odev)call_rcu(&dev->rcu,__dev_map_entry_free);
@@ -3922,6 +3922,34 @@ static const struct bpf_func_proto bpf_xdp_adjust_meta_proto = {.arg2_type=ARG_ANYTHING,};+/* XDP_REDIRECT works by a three-step process, implemented in the functions+*below:+*+*1.Thebpf_redirect()andbpf_redirect_map()helperswilllookupthetarget+*oftheredirectandstoreit(alongwithsomeothermetadata)inaper-CPU+*structbpf_redirect_info.+*+*2.WhentheprogramreturnstheXDP_REDIRECTreturncode,thedriverwill+*callxdp_do_redirect()whichwillusetheinformationinstruct+*bpf_redirect_infotoactuallyenqueuetheframeintoamaptype-specific+*bulkqueuestructure.+*+*3.BeforeexitingitsNAPIpollloop,thedriverwillcallxdp_do_flush(),+*whichwillflushallthedifferentbulkqueues,thuscompletingthe+*redirect.+*+*Pointerstothemapentrieswillbekeptaroundforthiswholesequenceof+*steps,protectedbyRCU.However,thereisnotop-levelrcu_read_lock()in+*thecorecode;instead,theRCUprotectionreliesoneverythinghappening+*insideasingleNAPIpollsequence,whichmeansit'sbetweenapairofcalls+*tolocal_bh_disable()/local_bh_enable().+*+*Themapentriesaremarkedas__rcuandthemapcodemakessureto+*dereferencethosepointerswithrcu_dereference_check()inawaythatworks+*forbothsectionsthattoholdanrcu_read_lock()andsectionsthatare+*calledfromNAPIwithoutaseparatercu_read_lock().Thecodebelowdoesnot+*useRCUannotations,butreliesonthoseinthemapcode.+*/voidxdp_do_flush(void){__dev_flush();
@@ -124,6 +124,10 @@ static int xsk_map_gen_lookup(struct bpf_map *map, struct bpf_insn *insn_buf)returninsn-insn_buf;}+/* Elements are kept alive by RCU; either by rcu_read_lock() (from syscall) or+*bylocal_bh_disable()(fromXDPcallsinsideNAPI).The+*rcu_read_lock_bh_held()belowmakeslockdepacceptboth.+*/staticvoid*__xsk_map_lookup_elem(structbpf_map*map,u32key){structxsk_map*m=container_of(map,structxsk_map,map);
The cpsw driver has rcu_read_lock()/rcu_read_unlock() pairs around XDP
program invocations. However, the actual lifetime of the objects referred
by the XDP program invocation is longer, all the way through to the call to
xdp_do_flush(), making the scope of the rcu_read_lock() too small. This
turns out to be harmless because it all happens in a single NAPI poll
cycle (and thus under local_bh_disable()), but it makes the rcu_read_lock()
misleading.
Rather than extend the scope of the rcu_read_lock(), just get rid of it
entirely. With the addition of RCU annotations to the XDP_REDIRECT map
types that take bh execution into account, lockdep even understands this to
be safe, so there's really no reason to keep it around.
Cc: Grygorii Strashko <grygorii.strashko@ti.com>
Cc: linux-omap@vger.kernel.org
Tested-by: Grygorii Strashko <grygorii.strashko@ti.com>
Reviewed-by: Grygorii Strashko <grygorii.strashko@ti.com>
Signed-off-by: Toke Høiland-Jørgensen <redacted>
---
drivers/net/ethernet/ti/cpsw_priv.c | 13 +++++--------
1 file changed, 5 insertions(+), 8 deletions(-)
@@ -1328,14 +1328,13 @@ int cpsw_run_xdp(struct cpsw_priv *priv, int ch, struct xdp_buff *xdp,structbpf_prog*prog;u32act;-rcu_read_lock();-prog=READ_ONCE(priv->xdp_prog);-if(!prog){-ret=CPSW_XDP_PASS;-gotoout;-}+if(!prog)+returnCPSW_XDP_PASS;+/* This code is invoked within a single NAPI poll cycle and thus under+*local_bh_disable(),whichprovidestheneededRCUprotection.+*/act=bpf_prog_run_xdp(prog,xdp);/* XDP prog might have changed packet data and boundaries */*len=xdp->data_end-xdp->data;
@@ -1378,10 +1377,8 @@ int cpsw_run_xdp(struct cpsw_priv *priv, int ch, struct xdp_buff *xdp,ndev->stats.rx_bytes+=*len;ndev->stats.rx_packets++;out:-rcu_read_unlock();returnret;drop:-rcu_read_unlock();page_pool_recycle_direct(cpsw->page_pool[ch],page);returnret;}
The netsec driver has a rcu_read_lock()/rcu_read_unlock() pair around the
full RX loop, covering everything up to and including xdp_do_flush(). This
is actually the correct behaviour, but because it all happens in a single
NAPI poll cycle (and thus under local_bh_disable()), it is also technically
redundant.
With the addition of RCU annotations to the XDP_REDIRECT map types that
take bh execution into account, lockdep even understands this to be safe,
so there's really no reason to keep the rcu_read_lock() around anymore, so
let's just remove it.
Cc: Jassi Brar <redacted>
Cc: Ilias Apalodimas <ilias.apalodimas@linaro.org>
Acked-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
Signed-off-by: Toke Høiland-Jørgensen <redacted>
---
drivers/net/ethernet/socionext/netsec.c | 7 ++++---
1 file changed, 4 insertions(+), 3 deletions(-)
@@ -958,7 +958,6 @@ static int netsec_process_rx(struct netsec_priv *priv, int budget)xdp_init_buff(&xdp,PAGE_SIZE,&dring->xdp_rxq);-rcu_read_lock();xdp_prog=READ_ONCE(priv->xdp_prog);dma_dir=page_pool_get_dma_dir(dring->page_pool);
@@ -1019,6 +1018,10 @@ static int netsec_process_rx(struct netsec_priv *priv, int budget)pkt_len,false);if(xdp_prog){+/* This code is invoked within a single NAPI poll cycle+*andthusunderlocal_bh_disable(),whichprovidesthe+*neededRCUprotection.+*/xdp_result=netsec_run_xdp(priv,xdp_prog,&xdp);if(xdp_result!=NETSEC_XDP_PASS){xdp_act|=xdp_result;
@@ -1069,8 +1072,6 @@ static int netsec_process_rx(struct netsec_priv *priv, int budget)}netsec_finalize_xdp_rx(priv,xdp_act,xdp_xmit);-rcu_read_unlock();-returndone;}
The qede driver has rcu_read_lock()/rcu_read_unlock() pairs around XDP
program invocations. However, the actual lifetime of the objects referred
by the XDP program invocation is longer, all the way through to the call to
xdp_do_flush(), making the scope of the rcu_read_lock() too small. This
turns out to be harmless because it all happens in a single NAPI poll
cycle (and thus under local_bh_disable()), but it makes the rcu_read_lock()
misleading.
Rather than extend the scope of the rcu_read_lock(), just get rid of it
entirely. With the addition of RCU annotations to the XDP_REDIRECT map
types that take bh execution into account, lockdep even understands this to
be safe, so there's really no reason to keep it around.
Cc: Ariel Elior <redacted>
Cc: GR-everest-linux-l2@marvell.com
Signed-off-by: Toke Høiland-Jørgensen <redacted>
---
drivers/net/ethernet/qlogic/qede/qede_fp.c | 7 ++-----
1 file changed, 2 insertions(+), 5 deletions(-)
@@ -1089,13 +1089,10 @@ static bool qede_rx_xdp(struct qede_dev *edev,xdp_prepare_buff(&xdp,page_address(bd->data),*data_offset,*len,false);-/* Queues always have a full reset currently, so for the time-*beinguntilthere'satomicprogramreplacejustmarkread-*sideformaphelpers.+/* This code is invoked within a single NAPI poll cycle and thus under+*local_bh_disable(),whichprovidestheneededRCUprotection.*/-rcu_read_lock();act=bpf_prog_run_xdp(prog,&xdp);-rcu_read_unlock();/* Recalculate, as XDP might have changed the headers */*data_offset=xdp.data-xdp.data_hard_start;
The nfp driver has rcu_read_lock()/rcu_read_unlock() pairs around XDP
program invocations. However, the actual lifetime of the objects referred
by the XDP program invocation is longer, all the way through to the call to
xdp_do_flush(), making the scope of the rcu_read_lock() too small.
While this is not actually an issue for the nfp driver because it doesn't
support XDP_REDIRECT (and thus doesn't call xdp_do_flush()), the
rcu_read_lock() is still unneeded. And With the addition of RCU annotations
to the XDP_REDIRECT map types that take bh execution into account, lockdep
even understands this to be safe, so there's really no reason to keep it
around.
Cc: Simon Horman <redacted>
Cc: oss-drivers@netronome.com
Reviewed-by: Simon Horman <redacted>
Signed-off-by: Toke Høiland-Jørgensen <redacted>
---
drivers/net/ethernet/netronome/nfp/nfp_net_common.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
@@ -1819,7 +1819,6 @@ static int nfp_net_rx(struct nfp_net_rx_ring *rx_ring, int budget)structxdp_buffxdp;intidx;-rcu_read_lock();xdp_prog=READ_ONCE(dp->xdp_prog);true_bufsz=xdp_prog?PAGE_SIZE:dp->fl_bufsz;xdp_init_buff(&xdp,PAGE_SIZE-NFP_NET_RX_BUF_HEADROOM,
@@ -1919,6 +1918,10 @@ static int nfp_net_rx(struct nfp_net_rx_ring *rx_ring, int budget)pkt_off-NFP_NET_RX_BUF_HEADROOM,pkt_len,true);+/* This code is invoked within a single NAPI poll cycle+*andthusunderlocal_bh_disable(),whichprovidesthe+*neededRCUprotection.+*/act=bpf_prog_run_xdp(xdp_prog,&xdp);pkt_len=xdp.data_end-xdp.data;
@@ -2036,7 +2039,6 @@ static int nfp_net_rx(struct nfp_net_rx_ring *rx_ring, int budget)if(!nfp_net_xdp_complete(tx_ring))pkts_polled=budget;}-rcu_read_unlock();returnpkts_polled;}
The sfc driver has rcu_read_lock()/rcu_read_unlock() pairs around XDP
program invocations. However, the actual lifetime of the objects referred
by the XDP program invocation is longer, all the way through to the call to
xdp_do_flush(), making the scope of the rcu_read_lock() too small. This
turns out to be harmless because it all happens in a single NAPI poll
cycle (and thus under local_bh_disable()), but it makes the rcu_read_lock()
misleading.
Rather than extend the scope of the rcu_read_lock(), just get rid of it
entirely. With the addition of RCU annotations to the XDP_REDIRECT map
types that take bh execution into account, lockdep even understands this to
be safe, so there's really no reason to keep it around.
Cc: Edward Cree <ecree.xilinx@gmail.com>
Cc: Martin Habets <redacted>
Acked-by: Edward Cree <ecree.xilinx@gmail.com>
Signed-off-by: Toke Høiland-Jørgensen <redacted>
---
drivers/net/ethernet/sfc/rx.c | 12 +++++-------
1 file changed, 5 insertions(+), 7 deletions(-)
@@ -260,18 +260,14 @@ static bool efx_do_xdp(struct efx_nic *efx, struct efx_channel *channel,s16offset;interr;-rcu_read_lock();-xdp_prog=rcu_dereference(efx->xdp_prog);-if(!xdp_prog){-rcu_read_unlock();+xdp_prog=rcu_dereference_bh(efx->xdp_prog);+if(!xdp_prog)returntrue;-}rx_queue=efx_channel_get_rx_queue(channel);if(unlikely(channel->rx_pkt_n_frags>1)){/* We can't do XDP on fragmented packets - drop. */-rcu_read_unlock();efx_free_rx_buffers(rx_queue,rx_buf,channel->rx_pkt_n_frags);if(net_ratelimit())
@@ -295,8 +291,10 @@ static bool efx_do_xdp(struct efx_nic *efx, struct efx_channel *channel,xdp_prepare_buff(&xdp,*ehp-EFX_XDP_HEADROOM,EFX_XDP_HEADROOM,rx_buf->len,false);+/* This code is invoked within a single NAPI poll cycle and thus under+*local_bh_disable(),whichprovidestheneededRCUprotection.+*/xdp_act=bpf_prog_run_xdp(xdp_prog,&xdp);-rcu_read_unlock();offset=(u8*)xdp.data-*ehp;
The mlx4 driver has rcu_read_lock()/rcu_read_unlock() pairs around XDP
program invocations. However, the actual lifetime of the objects referred
by the XDP program invocation is longer, all the way through to the call to
xdp_do_flush(), making the scope of the rcu_read_lock() too small. This
turns out to be harmless because it all happens in a single NAPI poll
cycle (and thus under local_bh_disable()), but it makes the rcu_read_lock()
misleading.
Rather than extend the scope of the rcu_read_lock(), just get rid of it
entirely. With the addition of RCU annotations to the XDP_REDIRECT map
types that take bh execution into account, lockdep even understands this to
be safe, so there's really no reason to keep it around. Also switch the RCU
dereferences in the driver loop itself to the _bh variants.
Cc: Tariq Toukan <tariqt@nvidia.com>
Reviewed-by: Tariq Toukan <tariqt@nvidia.com>
Signed-off-by: Toke Høiland-Jørgensen <redacted>
---
drivers/net/ethernet/mellanox/mlx4/en_rx.c | 8 ++------
1 file changed, 2 insertions(+), 6 deletions(-)
@@ -679,9 +679,7 @@ int mlx4_en_process_rx_cq(struct net_device *dev, struct mlx4_en_cq *cq, int budring=priv->rx_ring[cq_ring];-/* Protect accesses to: ring->xdp_prog, priv->mac_hash list */-rcu_read_lock();-xdp_prog=rcu_dereference(ring->xdp_prog);+xdp_prog=rcu_dereference_bh(ring->xdp_prog);xdp_init_buff(&xdp,priv->frag_info[0].frag_stride,&ring->xdp_rxq);doorbell_pending=false;
@@ -744,7 +742,7 @@ int mlx4_en_process_rx_cq(struct net_device *dev, struct mlx4_en_cq *cq, int bud/* Drop the packet, since HW loopback-ed it */mac_hash=ethh->h_source[MLX4_EN_MAC_HASH_IDX];bucket=&priv->mac_hash[mac_hash];-hlist_for_each_entry_rcu(entry,bucket,hlist){+hlist_for_each_entry_rcu_bh(entry,bucket,hlist){if(ether_addr_equal_64bits(entry->mac,ethh->h_source))gotonext;
@@ -899,8 +897,6 @@ int mlx4_en_process_rx_cq(struct net_device *dev, struct mlx4_en_cq *cq, int budbreak;}-rcu_read_unlock();-if(likely(polled)){if(doorbell_pending){priv->tx_cq[TX_XDP][cq_ring]->xdp_busy=true;
The bnxt driver has rcu_read_lock()/rcu_read_unlock() pairs around XDP
program invocations. However, the actual lifetime of the objects referred
by the XDP program invocation is longer, all the way through to the call to
xdp_do_flush(), making the scope of the rcu_read_lock() too small. This
turns out to be harmless because it all happens in a single NAPI poll
cycle (and thus under local_bh_disable()), but it makes the rcu_read_lock()
misleading.
Rather than extend the scope of the rcu_read_lock(), just get rid of it
entirely. With the addition of RCU annotations to the XDP_REDIRECT map
types that take bh execution into account, lockdep even understands this to
be safe, so there's really no reason to keep it around.
Cc: Michael Chan <michael.chan@broadcom.com>
Signed-off-by: Toke Høiland-Jørgensen <redacted>
---
drivers/net/ethernet/broadcom/bnxt/bnxt_xdp.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
@@ -138,9 +138,10 @@ bool bnxt_rx_xdp(struct bnxt *bp, struct bnxt_rx_ring_info *rxr, u16 cons,xdp_prepare_buff(&xdp,*data_ptr-offset,offset,*len,false);orig_data=xdp.data;-rcu_read_lock();+/* This code is invoked within a single NAPI poll cycle and thus under+*local_bh_disable(),whichprovidestheneededRCUprotection.+*/act=bpf_prog_run_xdp(xdp_prog,&xdp);-rcu_read_unlock();tx_avail=bnxt_tx_avail(bp,txr);/* If the tx ring is not full, we must not update the rx producer yet
The mvneta and mvpp2 drivers have rcu_read_lock()/rcu_read_unlock() pairs
around XDP program invocations. However, the actual lifetime of the objects
referred by the XDP program invocation is longer, all the way through to
the call to xdp_do_flush(), making the scope of the rcu_read_lock() too
small. This turns out to be harmless because it all happens in a single
NAPI poll cycle (and thus under local_bh_disable()), but it makes the
rcu_read_lock() misleading.
Rather than extend the scope of the rcu_read_lock(), just get rid of it
entirely. With the addition of RCU annotations to the XDP_REDIRECT map
types that take bh execution into account, lockdep even understands this to
be safe, so there's really no reason to keep it around.
Cc: Thomas Petazzoni <thomas.petazzoni@bootlin.com>
Cc: Marcin Wojtas <redacted>
Cc: Russell King <linux@armlinux.org.uk>
Signed-off-by: Toke Høiland-Jørgensen <redacted>
---
drivers/net/ethernet/marvell/mvneta.c | 6 ++++--
drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c | 8 ++++----
2 files changed, 8 insertions(+), 6 deletions(-)
@@ -2370,7 +2370,6 @@ static int mvneta_rx_swbm(struct napi_struct *napi,/* Get number of received packets */rx_todo=mvneta_rxq_busy_desc_num_get(pp,rxq);-rcu_read_lock();xdp_prog=READ_ONCE(pp->xdp_prog);/* Fairness NAPI loop */
@@ -2421,6 +2420,10 @@ static int mvneta_rx_swbm(struct napi_struct *napi,gotonext;}+/* This code is invoked within a single NAPI poll cycle and thus+*underlocal_bh_disable(),whichprovidestheneededRCU+*protection.+*/if(xdp_prog&&mvneta_run_xdp(pp,rxq,xdp_prog,&xdp_buf,frame_sz,&ps))gotonext;
@@ -2448,7 +2451,6 @@ static int mvneta_rx_swbm(struct napi_struct *napi,xdp_buf.data_hard_start=NULL;sinfo.nr_frags=0;}-rcu_read_unlock();if(xdp_buf.data_hard_start)mvneta_xdp_put_buff(pp,rxq,&xdp_buf,&sinfo,-1);
@@ -3852,8 +3852,6 @@ static int mvpp2_rx(struct mvpp2_port *port, struct napi_struct *napi,intrx_done=0;u32xdp_ret=0;-rcu_read_lock();-xdp_prog=READ_ONCE(port->xdp_prog);/* Get number of received packets and clamp the to-do */
@@ -3925,6 +3923,10 @@ static int mvpp2_rx(struct mvpp2_port *port, struct napi_struct *napi,MVPP2_MH_SIZE+MVPP2_SKB_HEADROOM,rx_bytes,false);+/* This code is invoked within a single NAPI poll cycle+*andthusunderlocal_bh_disable(),whichprovidesthe+*neededRCUprotection.+*/ret=mvpp2_run_xdp(port,xdp_prog,&xdp,pp,&ps);if(ret){
The thunderx driver has rcu_read_lock()/rcu_read_unlock() pairs around XDP
program invocations. However, the actual lifetime of the objects referred
by the XDP program invocation is longer, all the way through to the call to
xdp_do_flush(), making the scope of the rcu_read_lock() too small. This
turns out to be harmless because it all happens in a single NAPI poll
cycle (and thus under local_bh_disable()), but it makes the rcu_read_lock()
misleading.
Rather than extend the scope of the rcu_read_lock(), just get rid of it
entirely. With the addition of RCU annotations to the XDP_REDIRECT map
types that take bh execution into account, lockdep even understands this to
be safe, so there's really no reason to keep it around.
Cc: Sunil Goutham <sgoutham@marvell.com>
Cc: linux-arm-kernel@lists.infradead.org
Signed-off-by: Toke Høiland-Jørgensen <redacted>
---
drivers/net/ethernet/cavium/thunder/nicvf_main.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
@@ -555,9 +555,10 @@ static inline bool nicvf_xdp_rx(struct nicvf *nic, struct bpf_prog *prog,xdp_prepare_buff(&xdp,hard_start,data-hard_start,len,false);orig_data=xdp.data;-rcu_read_lock();+/* This code is invoked within a single NAPI poll cycle and thus under+*local_bh_disable(),whichprovidestheneededRCUprotection.+*/action=bpf_prog_run_xdp(prog,&xdp);-rcu_read_unlock();len=xdp.data_end-xdp.data;/* Check if XDP program has changed headers */
The dpaa and dpaa2 drivers have rcu_read_lock()/rcu_read_unlock() pairs
around XDP program invocations. However, the actual lifetime of the objects
referred by the XDP program invocation is longer, all the way through to
the call to xdp_do_flush(), making the scope of the rcu_read_lock() too
small. This turns out to be harmless because it all happens in a single
NAPI poll cycle (and thus under local_bh_disable()), but it makes the
rcu_read_lock() misleading.
Rather than extend the scope of the rcu_read_lock(), just get rid of it
entirely. With the addition of RCU annotations to the XDP_REDIRECT map
types that take bh execution into account, lockdep even understands this to
be safe, so there's really no reason to keep it around.
Cc: Madalin Bucur <madalin.bucur@nxp.com>
Cc: Ioana Ciornei <ioana.ciornei@nxp.com>
Cc: Ioana Radulescu <redacted>
Reviewed-by: Camelia Groza <redacted>
Signed-off-by: Toke Høiland-Jørgensen <redacted>
---
drivers/net/ethernet/freescale/dpaa/dpaa_eth.c | 11 ++++-------
drivers/net/ethernet/freescale/dpaa2/dpaa2-eth.c | 6 +++---
2 files changed, 7 insertions(+), 10 deletions(-)
@@ -2585,6 +2581,9 @@ static u32 dpaa_run_xdp(struct dpaa_priv *priv, struct qm_fd *fd, void *vaddr,}#endif+/* This code is invoked within a single NAPI poll cycle and thus under+*local_bh_disable(),whichprovidestheneededRCUprotection.+*/xdp_act=bpf_prog_run_xdp(xdp_prog,&xdp);/* Update the length and the offset of the FD */
@@ -363,6 +361,9 @@ static u32 dpaa2_eth_run_xdp(struct dpaa2_eth_priv *priv,xdp_prepare_buff(&xdp,vaddr+offset,XDP_PACKET_HEADROOM,dpaa2_fd_get_len(fd),false);+/* This code is invoked within a single NAPI poll cycle and thus under+*local_bh_disable(),whichprovidestheneededRCUprotection.+*/xdp_act=bpf_prog_run_xdp(xdp_prog,&xdp);/* xdp.data pointer may have changed */
From: Martin KaFai Lau <hidden> Date: 2021-06-18 04:58:16
On Thu, Jun 17, 2021 at 11:27:35PM +0200, Toke Høiland-Jørgensen wrote:
XDP_REDIRECT works by a three-step process: the bpf_redirect() and
bpf_redirect_map() helpers will lookup the target of the redirect and store
it (along with some other metadata) in a per-CPU struct bpf_redirect_info.
Next, when the program returns the XDP_REDIRECT return code, the driver
will call xdp_do_redirect() which will use the information thus stored to
actually enqueue the frame into a bulk queue structure (that differs
slightly by map type, but shares the same principle). Finally, before
exiting its NAPI poll loop, the driver will call xdp_do_flush(), which will
flush all the different bulk queues, thus completing the redirect.
Pointers to the map entries will be kept around for this whole sequence of
steps, protected by RCU. However, there is no top-level rcu_read_lock() in
the core code; instead drivers add their own rcu_read_lock() around the XDP
portions of the code, but somewhat inconsistently as Martin discovered[0].
However, things still work because everything happens inside a single NAPI
poll sequence, which means it's between a pair of calls to
local_bh_disable()/local_bh_enable(). So Paul suggested[1] that we could
document this intention by using rcu_dereference_check() with
rcu_read_lock_bh_held() as a second parameter, thus allowing sparse and
lockdep to verify that everything is done correctly.
This patch does just that: we add an __rcu annotation to the map entry
pointers and remove the various comments explaining the NAPI poll assurance
strewn through devmap.c in favour of a longer explanation in filter.c. The
goal is to have one coherent documentation of the entire flow, and rely on
the RCU annotations as a "standard" way of communicating the flow in the
map code (which can additionally be understood by sparse and lockdep).
The RCU annotation replacements result in a fairly straight-forward
replacement where READ_ONCE() becomes rcu_dereference_check(), WRITE_ONCE()
becomes rcu_assign_pointer() and xchg() and cmpxchg() gets wrapped in the
proper constructs to cast the pointer back and forth between __rcu and
__kernel address space (for the benefit of sparse). The one complication is
that xskmap has a few constructions where double-pointers are passed back
and forth; these simply all gain __rcu annotations, and only the final
reference/dereference to the inner-most pointer gets changed.
With this, everything can be run through sparse without eliciting
complaints, and lockdep can verify correctness even without the use of
rcu_read_lock() in the drivers. Subsequent patches will clean these up from
the drivers.
[0] https://lore.kernel.org/bpf/20210415173551.7ma4slcbqeyiba2r@kafai-mbp.dhcp.thefacebook.com/
[1] https://lore.kernel.org/bpf/20210419165837.GA975577@paulmck-ThinkPad-P17-Gen-1/
Signed-off-by: Toke Høiland-Jørgensen <redacted>
From: Wong Vee Khee <hidden> Date: 2021-06-18 09:47:44
On Thu, Jun 17, 2021 at 11:27:47PM +0200, Toke Høiland-Jørgensen wrote:
The stmmac driver has rcu_read_lock()/rcu_read_unlock() pairs around XDP
program invocations. However, the actual lifetime of the objects referred
by the XDP program invocation is longer, all the way through to the call to
xdp_do_flush(), making the scope of the rcu_read_lock() too small. This
turns out to be harmless because it all happens in a single NAPI poll
cycle (and thus under local_bh_disable()), but it makes the rcu_read_lock()
misleading.
Rather than extend the scope of the rcu_read_lock(), just get rid of it
entirely. With the addition of RCU annotations to the XDP_REDIRECT map
types that take bh execution into account, lockdep even understands this to
be safe, so there's really no reason to keep it around.
Cc: Giuseppe Cavallaro <redacted>
Cc: Alexandre Torgue <alexandre.torgue@foss.st.com>
Cc: Jose Abreu <redacted>
Signed-off-by: Toke Høiland-Jørgensen <redacted>
Acked-by: Wong Vee Khee <redacted>
Tested-by: Song, Yoong Siang <redacted>
From: Daniel Borkmann <daniel@iogearbox.net> Date: 2021-06-18 23:27:32
On 6/17/21 11:27 PM, Toke Høiland-Jørgensen wrote:
XDP_REDIRECT works by a three-step process: the bpf_redirect() and
bpf_redirect_map() helpers will lookup the target of the redirect and store
it (along with some other metadata) in a per-CPU struct bpf_redirect_info.
Next, when the program returns the XDP_REDIRECT return code, the driver
will call xdp_do_redirect() which will use the information thus stored to
actually enqueue the frame into a bulk queue structure (that differs
slightly by map type, but shares the same principle). Finally, before
exiting its NAPI poll loop, the driver will call xdp_do_flush(), which will
flush all the different bulk queues, thus completing the redirect.
Pointers to the map entries will be kept around for this whole sequence of
steps, protected by RCU. However, there is no top-level rcu_read_lock() in
the core code; instead drivers add their own rcu_read_lock() around the XDP
portions of the code, but somewhat inconsistently as Martin discovered[0].
However, things still work because everything happens inside a single NAPI
poll sequence, which means it's between a pair of calls to
local_bh_disable()/local_bh_enable(). So Paul suggested[1] that we could
document this intention by using rcu_dereference_check() with
rcu_read_lock_bh_held() as a second parameter, thus allowing sparse and
lockdep to verify that everything is done correctly.
This patch does just that: we add an __rcu annotation to the map entry
pointers and remove the various comments explaining the NAPI poll assurance
strewn through devmap.c in favour of a longer explanation in filter.c. The
goal is to have one coherent documentation of the entire flow, and rely on
the RCU annotations as a "standard" way of communicating the flow in the
map code (which can additionally be understood by sparse and lockdep).
The RCU annotation replacements result in a fairly straight-forward
replacement where READ_ONCE() becomes rcu_dereference_check(), WRITE_ONCE()
becomes rcu_assign_pointer() and xchg() and cmpxchg() gets wrapped in the
proper constructs to cast the pointer back and forth between __rcu and
__kernel address space (for the benefit of sparse). The one complication is
that xskmap has a few constructions where double-pointers are passed back
and forth; these simply all gain __rcu annotations, and only the final
reference/dereference to the inner-most pointer gets changed.
With this, everything can be run through sparse without eliciting
complaints, and lockdep can verify correctness even without the use of
rcu_read_lock() in the drivers. Subsequent patches will clean these up from
the drivers.
[0] https://lore.kernel.org/bpf/20210415173551.7ma4slcbqeyiba2r@kafai-mbp.dhcp.thefacebook.com/
[1] https://lore.kernel.org/bpf/20210419165837.GA975577@paulmck-ThinkPad-P17-Gen-1/
Signed-off-by: Toke Høiland-Jørgensen <redacted>
---
include/net/xdp_sock.h | 2 +-
kernel/bpf/cpumap.c | 13 +++++++----
kernel/bpf/devmap.c | 49 ++++++++++++++++++------------------------
net/core/filter.c | 28 ++++++++++++++++++++++++
net/xdp/xsk.c | 4 ++--
net/xdp/xsk.h | 4 ++--
net/xdp/xskmap.c | 29 ++++++++++++++-----------
7 files changed, 80 insertions(+), 49 deletions(-)
@@ -3922,6 +3922,34 @@ static const struct bpf_func_proto bpf_xdp_adjust_meta_proto = {.arg2_type=ARG_ANYTHING,};+/* XDP_REDIRECT works by a three-step process, implemented in the functions+*below:+*+*1.Thebpf_redirect()andbpf_redirect_map()helperswilllookupthetarget+*oftheredirectandstoreit(alongwithsomeothermetadata)inaper-CPU+*structbpf_redirect_info.+*+*2.WhentheprogramreturnstheXDP_REDIRECTreturncode,thedriverwill+*callxdp_do_redirect()whichwillusetheinformationinstruct+*bpf_redirect_infotoactuallyenqueuetheframeintoamaptype-specific+*bulkqueuestructure.+*+*3.BeforeexitingitsNAPIpollloop,thedriverwillcallxdp_do_flush(),+*whichwillflushallthedifferentbulkqueues,thuscompletingthe+*redirect.+*+*Pointerstothemapentrieswillbekeptaroundforthiswholesequenceof+*steps,protectedbyRCU.However,thereisnotop-levelrcu_read_lock()in+*thecorecode;instead,theRCUprotectionreliesoneverythinghappening+*insideasingleNAPIpollsequence,whichmeansit'sbetweenapairofcalls+*tolocal_bh_disable()/local_bh_enable().+*+*Themapentriesaremarkedas__rcuandthemapcodemakessureto+*dereferencethosepointerswithrcu_dereference_check()inawaythatworks+*forbothsectionsthattoholdanrcu_read_lock()andsectionsthatare+*calledfromNAPIwithoutaseparatercu_read_lock().Thecodebelowdoesnot+*useRCUannotations,butreliesonthoseinthemapcode.
One more follow-up question related to tc BPF: given we do use rcu_read_lock_bh()
in case of sch_handle_egress(), could we also remove the rcu_read_lock() pair
from cls_bpf_classify() then?
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.
Could you also update the RCU comment in bpf_prog_run_xdp()? Or alternatively move all
the below driver comments in there as a single location?
/* This code is invoked within a single NAPI poll cycle and thus under
* local_bh_disable(), which provides the needed RCU protection.
*/
Thanks,
Daniel
On 6/17/21 11:27 PM, Toke Høiland-Jørgensen wrote:
quoted
XDP_REDIRECT works by a three-step process: the bpf_redirect() and
bpf_redirect_map() helpers will lookup the target of the redirect and store
it (along with some other metadata) in a per-CPU struct bpf_redirect_info.
Next, when the program returns the XDP_REDIRECT return code, the driver
will call xdp_do_redirect() which will use the information thus stored to
actually enqueue the frame into a bulk queue structure (that differs
slightly by map type, but shares the same principle). Finally, before
exiting its NAPI poll loop, the driver will call xdp_do_flush(), which will
flush all the different bulk queues, thus completing the redirect.
Pointers to the map entries will be kept around for this whole sequence of
steps, protected by RCU. However, there is no top-level rcu_read_lock() in
the core code; instead drivers add their own rcu_read_lock() around the XDP
portions of the code, but somewhat inconsistently as Martin discovered[0].
However, things still work because everything happens inside a single NAPI
poll sequence, which means it's between a pair of calls to
local_bh_disable()/local_bh_enable(). So Paul suggested[1] that we could
document this intention by using rcu_dereference_check() with
rcu_read_lock_bh_held() as a second parameter, thus allowing sparse and
lockdep to verify that everything is done correctly.
This patch does just that: we add an __rcu annotation to the map entry
pointers and remove the various comments explaining the NAPI poll assurance
strewn through devmap.c in favour of a longer explanation in filter.c. The
goal is to have one coherent documentation of the entire flow, and rely on
the RCU annotations as a "standard" way of communicating the flow in the
map code (which can additionally be understood by sparse and lockdep).
The RCU annotation replacements result in a fairly straight-forward
replacement where READ_ONCE() becomes rcu_dereference_check(), WRITE_ONCE()
becomes rcu_assign_pointer() and xchg() and cmpxchg() gets wrapped in the
proper constructs to cast the pointer back and forth between __rcu and
__kernel address space (for the benefit of sparse). The one complication is
that xskmap has a few constructions where double-pointers are passed back
and forth; these simply all gain __rcu annotations, and only the final
reference/dereference to the inner-most pointer gets changed.
With this, everything can be run through sparse without eliciting
complaints, and lockdep can verify correctness even without the use of
rcu_read_lock() in the drivers. Subsequent patches will clean these up from
the drivers.
[0] https://lore.kernel.org/bpf/20210415173551.7ma4slcbqeyiba2r@kafai-mbp.dhcp.thefacebook.com/
[1] https://lore.kernel.org/bpf/20210419165837.GA975577@paulmck-ThinkPad-P17-Gen-1/
Signed-off-by: Toke Høiland-Jørgensen <redacted>
---
include/net/xdp_sock.h | 2 +-
kernel/bpf/cpumap.c | 13 +++++++----
kernel/bpf/devmap.c | 49 ++++++++++++++++++------------------------
net/core/filter.c | 28 ++++++++++++++++++++++++
net/xdp/xsk.c | 4 ++--
net/xdp/xsk.h | 4 ++--
net/xdp/xskmap.c | 29 ++++++++++++++-----------
7 files changed, 80 insertions(+), 49 deletions(-)
@@ -3922,6 +3922,34 @@ static const struct bpf_func_proto bpf_xdp_adjust_meta_proto = {.arg2_type=ARG_ANYTHING,};+/* XDP_REDIRECT works by a three-step process, implemented in the functions+*below:+*+*1.Thebpf_redirect()andbpf_redirect_map()helperswilllookupthetarget+*oftheredirectandstoreit(alongwithsomeothermetadata)inaper-CPU+*structbpf_redirect_info.+*+*2.WhentheprogramreturnstheXDP_REDIRECTreturncode,thedriverwill+*callxdp_do_redirect()whichwillusetheinformationinstruct+*bpf_redirect_infotoactuallyenqueuetheframeintoamaptype-specific+*bulkqueuestructure.+*+*3.BeforeexitingitsNAPIpollloop,thedriverwillcallxdp_do_flush(),+*whichwillflushallthedifferentbulkqueues,thuscompletingthe+*redirect.+*+*Pointerstothemapentrieswillbekeptaroundforthiswholesequenceof+*steps,protectedbyRCU.However,thereisnotop-levelrcu_read_lock()in+*thecorecode;instead,theRCUprotectionreliesoneverythinghappening+*insideasingleNAPIpollsequence,whichmeansit'sbetweenapairofcalls+*tolocal_bh_disable()/local_bh_enable().+*+*Themapentriesaremarkedas__rcuandthemapcodemakessureto+*dereferencethosepointerswithrcu_dereference_check()inawaythatworks+*forbothsectionsthattoholdanrcu_read_lock()andsectionsthatare+*calledfromNAPIwithoutaseparatercu_read_lock().Thecodebelowdoesnot+*useRCUannotations,butreliesonthoseinthemapcode.
One more follow-up question related to tc BPF: given we do use rcu_read_lock_bh()
in case of sch_handle_egress(), could we also remove the rcu_read_lock() pair
from cls_bpf_classify() then?
I believe so, yeah. Patch 2 in this series should even make lockdep stop
complaining about it :)
I can add a patch removing the rcu_read_lock() from cls_bpf in the next
version.
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...
Could you also update the RCU comment in bpf_prog_run_xdp()? Or
alternatively move all the below driver comments in there as a single
location?
/* This code is invoked within a single NAPI poll cycle and thus under
* local_bh_disable(), which provides the needed RCU protection.
*/
Sure, can do. And yeah, I do agree that moving the comment in there
makes more sense than scattering it over all the drivers, even if that
means I have to go back and edit all the drivers again :P
-Toke
From: Daniel Borkmann <daniel@iogearbox.net> Date: 2021-06-21 22:15:27
On 6/21/21 11:39 PM, Toke Høiland-Jørgensen wrote:
Daniel Borkmann [off-list ref] writes:
quoted
On 6/17/21 11:27 PM, Toke Høiland-Jørgensen wrote:
quoted
XDP_REDIRECT works by a three-step process: the bpf_redirect() and
bpf_redirect_map() helpers will lookup the target of the redirect and store
it (along with some other metadata) in a per-CPU struct bpf_redirect_info.
Next, when the program returns the XDP_REDIRECT return code, the driver
will call xdp_do_redirect() which will use the information thus stored to
actually enqueue the frame into a bulk queue structure (that differs
slightly by map type, but shares the same principle). Finally, before
exiting its NAPI poll loop, the driver will call xdp_do_flush(), which will
flush all the different bulk queues, thus completing the redirect.
Pointers to the map entries will be kept around for this whole sequence of
steps, protected by RCU. However, there is no top-level rcu_read_lock() in
the core code; instead drivers add their own rcu_read_lock() around the XDP
portions of the code, but somewhat inconsistently as Martin discovered[0].
However, things still work because everything happens inside a single NAPI
poll sequence, which means it's between a pair of calls to
local_bh_disable()/local_bh_enable(). So Paul suggested[1] that we could
document this intention by using rcu_dereference_check() with
rcu_read_lock_bh_held() as a second parameter, thus allowing sparse and
lockdep to verify that everything is done correctly.
This patch does just that: we add an __rcu annotation to the map entry
pointers and remove the various comments explaining the NAPI poll assurance
strewn through devmap.c in favour of a longer explanation in filter.c. The
goal is to have one coherent documentation of the entire flow, and rely on
the RCU annotations as a "standard" way of communicating the flow in the
map code (which can additionally be understood by sparse and lockdep).
The RCU annotation replacements result in a fairly straight-forward
replacement where READ_ONCE() becomes rcu_dereference_check(), WRITE_ONCE()
becomes rcu_assign_pointer() and xchg() and cmpxchg() gets wrapped in the
proper constructs to cast the pointer back and forth between __rcu and
__kernel address space (for the benefit of sparse). The one complication is
that xskmap has a few constructions where double-pointers are passed back
and forth; these simply all gain __rcu annotations, and only the final
reference/dereference to the inner-most pointer gets changed.
With this, everything can be run through sparse without eliciting
complaints, and lockdep can verify correctness even without the use of
rcu_read_lock() in the drivers. Subsequent patches will clean these up from
the drivers.
[0] https://lore.kernel.org/bpf/20210415173551.7ma4slcbqeyiba2r@kafai-mbp.dhcp.thefacebook.com/
[1] https://lore.kernel.org/bpf/20210419165837.GA975577@paulmck-ThinkPad-P17-Gen-1/
Signed-off-by: Toke Høiland-Jørgensen <redacted>
---
include/net/xdp_sock.h | 2 +-
kernel/bpf/cpumap.c | 13 +++++++----
kernel/bpf/devmap.c | 49 ++++++++++++++++++------------------------
net/core/filter.c | 28 ++++++++++++++++++++++++
net/xdp/xsk.c | 4 ++--
net/xdp/xsk.h | 4 ++--
net/xdp/xskmap.c | 29 ++++++++++++++-----------
7 files changed, 80 insertions(+), 49 deletions(-)
@@ -3922,6 +3922,34 @@ static const struct bpf_func_proto bpf_xdp_adjust_meta_proto = {.arg2_type=ARG_ANYTHING,};+/* XDP_REDIRECT works by a three-step process, implemented in the functions+*below:+*+*1.Thebpf_redirect()andbpf_redirect_map()helperswilllookupthetarget+*oftheredirectandstoreit(alongwithsomeothermetadata)inaper-CPU+*structbpf_redirect_info.+*+*2.WhentheprogramreturnstheXDP_REDIRECTreturncode,thedriverwill+*callxdp_do_redirect()whichwillusetheinformationinstruct+*bpf_redirect_infotoactuallyenqueuetheframeintoamaptype-specific+*bulkqueuestructure.+*+*3.BeforeexitingitsNAPIpollloop,thedriverwillcallxdp_do_flush(),+*whichwillflushallthedifferentbulkqueues,thuscompletingthe+*redirect.+*+*Pointerstothemapentrieswillbekeptaroundforthiswholesequenceof+*steps,protectedbyRCU.However,thereisnotop-levelrcu_read_lock()in+*thecorecode;instead,theRCUprotectionreliesoneverythinghappening+*insideasingleNAPIpollsequence,whichmeansit'sbetweenapairofcalls+*tolocal_bh_disable()/local_bh_enable().+*+*Themapentriesaremarkedas__rcuandthemapcodemakessureto+*dereferencethosepointerswithrcu_dereference_check()inawaythatworks+*forbothsectionsthattoholdanrcu_read_lock()andsectionsthatare+*calledfromNAPIwithoutaseparatercu_read_lock().Thecodebelowdoesnot+*useRCUannotations,butreliesonthoseinthemapcode.
One more follow-up question related to tc BPF: given we do use rcu_read_lock_bh()
in case of sch_handle_egress(), could we also remove the rcu_read_lock() pair
from cls_bpf_classify() then?
I believe so, yeah. Patch 2 in this series should even make lockdep stop
complaining about it :)
Btw, I was wondering whether we should just get rid of all the WARN_ON_ONCE()s
from those map helpers given in most situations these are not triggered anyway
due to retpoline avoidance where verifier rewrites the calls to jump to the map
backend implementation directly. One alternative could be to have an extension
to the bpf prologue generation under CONFIG_DEBUG_LOCK_ALLOC and call the lockdep
checks from there, but it's probably not worth the effort. (In the trampoline
case we have those __bpf_prog_enter()/__bpf_prog_enter_sleepable() where the
latter in particular has asserts like might_fault(), fwiw.)
I can add a patch removing the rcu_read_lock() from cls_bpf in the next
version.
quoted
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...
quoted
Could you also update the RCU comment in bpf_prog_run_xdp()? Or
alternatively move all the below driver comments in there as a single
location?
/* This code is invoked within a single NAPI poll cycle and thus under
* local_bh_disable(), which provides the needed RCU protection.
*/
Sure, can do. And yeah, I do agree that moving the comment in there
makes more sense than scattering it over all the drivers, even if that
means I have to go back and edit all the drivers again :P
On 6/21/21 11:39 PM, Toke Høiland-Jørgensen wrote:
quoted
Daniel Borkmann [off-list ref] writes:
quoted
On 6/17/21 11:27 PM, Toke Høiland-Jørgensen wrote:
quoted
XDP_REDIRECT works by a three-step process: the bpf_redirect() and
bpf_redirect_map() helpers will lookup the target of the redirect and store
it (along with some other metadata) in a per-CPU struct bpf_redirect_info.
Next, when the program returns the XDP_REDIRECT return code, the driver
will call xdp_do_redirect() which will use the information thus stored to
actually enqueue the frame into a bulk queue structure (that differs
slightly by map type, but shares the same principle). Finally, before
exiting its NAPI poll loop, the driver will call xdp_do_flush(), which will
flush all the different bulk queues, thus completing the redirect.
Pointers to the map entries will be kept around for this whole sequence of
steps, protected by RCU. However, there is no top-level rcu_read_lock() in
the core code; instead drivers add their own rcu_read_lock() around the XDP
portions of the code, but somewhat inconsistently as Martin discovered[0].
However, things still work because everything happens inside a single NAPI
poll sequence, which means it's between a pair of calls to
local_bh_disable()/local_bh_enable(). So Paul suggested[1] that we could
document this intention by using rcu_dereference_check() with
rcu_read_lock_bh_held() as a second parameter, thus allowing sparse and
lockdep to verify that everything is done correctly.
This patch does just that: we add an __rcu annotation to the map entry
pointers and remove the various comments explaining the NAPI poll assurance
strewn through devmap.c in favour of a longer explanation in filter.c. The
goal is to have one coherent documentation of the entire flow, and rely on
the RCU annotations as a "standard" way of communicating the flow in the
map code (which can additionally be understood by sparse and lockdep).
The RCU annotation replacements result in a fairly straight-forward
replacement where READ_ONCE() becomes rcu_dereference_check(), WRITE_ONCE()
becomes rcu_assign_pointer() and xchg() and cmpxchg() gets wrapped in the
proper constructs to cast the pointer back and forth between __rcu and
__kernel address space (for the benefit of sparse). The one complication is
that xskmap has a few constructions where double-pointers are passed back
and forth; these simply all gain __rcu annotations, and only the final
reference/dereference to the inner-most pointer gets changed.
With this, everything can be run through sparse without eliciting
complaints, and lockdep can verify correctness even without the use of
rcu_read_lock() in the drivers. Subsequent patches will clean these up from
the drivers.
[0] https://lore.kernel.org/bpf/20210415173551.7ma4slcbqeyiba2r@kafai-mbp.dhcp.thefacebook.com/
[1] https://lore.kernel.org/bpf/20210419165837.GA975577@paulmck-ThinkPad-P17-Gen-1/
Signed-off-by: Toke Høiland-Jørgensen <redacted>
---
include/net/xdp_sock.h | 2 +-
kernel/bpf/cpumap.c | 13 +++++++----
kernel/bpf/devmap.c | 49 ++++++++++++++++++------------------------
net/core/filter.c | 28 ++++++++++++++++++++++++
net/xdp/xsk.c | 4 ++--
net/xdp/xsk.h | 4 ++--
net/xdp/xskmap.c | 29 ++++++++++++++-----------
7 files changed, 80 insertions(+), 49 deletions(-)
@@ -3922,6 +3922,34 @@ static const struct bpf_func_proto bpf_xdp_adjust_meta_proto = {.arg2_type=ARG_ANYTHING,};+/* XDP_REDIRECT works by a three-step process, implemented in the functions+*below:+*+*1.Thebpf_redirect()andbpf_redirect_map()helperswilllookupthetarget+*oftheredirectandstoreit(alongwithsomeothermetadata)inaper-CPU+*structbpf_redirect_info.+*+*2.WhentheprogramreturnstheXDP_REDIRECTreturncode,thedriverwill+*callxdp_do_redirect()whichwillusetheinformationinstruct+*bpf_redirect_infotoactuallyenqueuetheframeintoamaptype-specific+*bulkqueuestructure.+*+*3.BeforeexitingitsNAPIpollloop,thedriverwillcallxdp_do_flush(),+*whichwillflushallthedifferentbulkqueues,thuscompletingthe+*redirect.+*+*Pointerstothemapentrieswillbekeptaroundforthiswholesequenceof+*steps,protectedbyRCU.However,thereisnotop-levelrcu_read_lock()in+*thecorecode;instead,theRCUprotectionreliesoneverythinghappening+*insideasingleNAPIpollsequence,whichmeansit'sbetweenapairofcalls+*tolocal_bh_disable()/local_bh_enable().+*+*Themapentriesaremarkedas__rcuandthemapcodemakessureto+*dereferencethosepointerswithrcu_dereference_check()inawaythatworks+*forbothsectionsthattoholdanrcu_read_lock()andsectionsthatare+*calledfromNAPIwithoutaseparatercu_read_lock().Thecodebelowdoesnot+*useRCUannotations,butreliesonthoseinthemapcode.
One more follow-up question related to tc BPF: given we do use rcu_read_lock_bh()
in case of sch_handle_egress(), could we also remove the rcu_read_lock() pair
from cls_bpf_classify() then?
I believe so, yeah. Patch 2 in this series should even make lockdep stop
complaining about it :)
Btw, I was wondering whether we should just get rid of all the WARN_ON_ONCE()s
from those map helpers given in most situations these are not triggered anyway
due to retpoline avoidance where verifier rewrites the calls to jump to the map
backend implementation directly. One alternative could be to have an extension
to the bpf prologue generation under CONFIG_DEBUG_LOCK_ALLOC and call the lockdep
checks from there, but it's probably not worth the effort. (In the trampoline
case we have those __bpf_prog_enter()/__bpf_prog_enter_sleepable() where the
latter in particular has asserts like might_fault(), fwiw.)
I agree that it's probably overkill to amend the prologue. No strong
opinion on whether removing the checks entirely is a good idea; I guess
they at least serve as documentation even if they're not actually called
that often?
quoted
I can add a patch removing the rcu_read_lock() from cls_bpf in the next
version.
quoted
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...
quoted
Could you also update the RCU comment in bpf_prog_run_xdp()? Or
alternatively move all the below driver comments in there as a single
location?
/* This code is invoked within a single NAPI poll cycle and thus under
* local_bh_disable(), which provides the needed RCU protection.
*/
Sure, can do. And yeah, I do agree that moving the comment in there
makes more sense than scattering it over all the drivers, even if that
means I have to go back and edit all the drivers again :P
From: Daniel Borkmann <daniel@iogearbox.net> Date: 2021-06-22 08:51:36
On 6/22/21 12:35 AM, Toke Høiland-Jørgensen wrote:
Daniel Borkmann [off-list ref] writes:
quoted
On 6/21/21 11:39 PM, Toke Høiland-Jørgensen wrote:
quoted
Daniel Borkmann [off-list ref] writes:
quoted
On 6/17/21 11:27 PM, Toke Høiland-Jørgensen wrote:
quoted
XDP_REDIRECT works by a three-step process: the bpf_redirect() and
bpf_redirect_map() helpers will lookup the target of the redirect and store
it (along with some other metadata) in a per-CPU struct bpf_redirect_info.
Next, when the program returns the XDP_REDIRECT return code, the driver
will call xdp_do_redirect() which will use the information thus stored to
actually enqueue the frame into a bulk queue structure (that differs
slightly by map type, but shares the same principle). Finally, before
exiting its NAPI poll loop, the driver will call xdp_do_flush(), which will
flush all the different bulk queues, thus completing the redirect.
Pointers to the map entries will be kept around for this whole sequence of
steps, protected by RCU. However, there is no top-level rcu_read_lock() in
the core code; instead drivers add their own rcu_read_lock() around the XDP
portions of the code, but somewhat inconsistently as Martin discovered[0].
However, things still work because everything happens inside a single NAPI
poll sequence, which means it's between a pair of calls to
local_bh_disable()/local_bh_enable(). So Paul suggested[1] that we could
document this intention by using rcu_dereference_check() with
rcu_read_lock_bh_held() as a second parameter, thus allowing sparse and
lockdep to verify that everything is done correctly.
This patch does just that: we add an __rcu annotation to the map entry
pointers and remove the various comments explaining the NAPI poll assurance
strewn through devmap.c in favour of a longer explanation in filter.c. The
goal is to have one coherent documentation of the entire flow, and rely on
the RCU annotations as a "standard" way of communicating the flow in the
map code (which can additionally be understood by sparse and lockdep).
The RCU annotation replacements result in a fairly straight-forward
replacement where READ_ONCE() becomes rcu_dereference_check(), WRITE_ONCE()
becomes rcu_assign_pointer() and xchg() and cmpxchg() gets wrapped in the
proper constructs to cast the pointer back and forth between __rcu and
__kernel address space (for the benefit of sparse). The one complication is
that xskmap has a few constructions where double-pointers are passed back
and forth; these simply all gain __rcu annotations, and only the final
reference/dereference to the inner-most pointer gets changed.
With this, everything can be run through sparse without eliciting
complaints, and lockdep can verify correctness even without the use of
rcu_read_lock() in the drivers. Subsequent patches will clean these up from
the drivers.
[0] https://lore.kernel.org/bpf/20210415173551.7ma4slcbqeyiba2r@kafai-mbp.dhcp.thefacebook.com/
[1] https://lore.kernel.org/bpf/20210419165837.GA975577@paulmck-ThinkPad-P17-Gen-1/
Signed-off-by: Toke Høiland-Jørgensen <redacted>
---
include/net/xdp_sock.h | 2 +-
kernel/bpf/cpumap.c | 13 +++++++----
kernel/bpf/devmap.c | 49 ++++++++++++++++++------------------------
net/core/filter.c | 28 ++++++++++++++++++++++++
net/xdp/xsk.c | 4 ++--
net/xdp/xsk.h | 4 ++--
net/xdp/xskmap.c | 29 ++++++++++++++-----------
7 files changed, 80 insertions(+), 49 deletions(-)
@@ -3922,6 +3922,34 @@ static const struct bpf_func_proto bpf_xdp_adjust_meta_proto = {.arg2_type=ARG_ANYTHING,};+/* XDP_REDIRECT works by a three-step process, implemented in the functions+*below:+*+*1.Thebpf_redirect()andbpf_redirect_map()helperswilllookupthetarget+*oftheredirectandstoreit(alongwithsomeothermetadata)inaper-CPU+*structbpf_redirect_info.+*+*2.WhentheprogramreturnstheXDP_REDIRECTreturncode,thedriverwill+*callxdp_do_redirect()whichwillusetheinformationinstruct+*bpf_redirect_infotoactuallyenqueuetheframeintoamaptype-specific+*bulkqueuestructure.+*+*3.BeforeexitingitsNAPIpollloop,thedriverwillcallxdp_do_flush(),+*whichwillflushallthedifferentbulkqueues,thuscompletingthe+*redirect.+*+*Pointerstothemapentrieswillbekeptaroundforthiswholesequenceof+*steps,protectedbyRCU.However,thereisnotop-levelrcu_read_lock()in+*thecorecode;instead,theRCUprotectionreliesoneverythinghappening+*insideasingleNAPIpollsequence,whichmeansit'sbetweenapairofcalls+*tolocal_bh_disable()/local_bh_enable().+*+*Themapentriesaremarkedas__rcuandthemapcodemakessureto+*dereferencethosepointerswithrcu_dereference_check()inawaythatworks+*forbothsectionsthattoholdanrcu_read_lock()andsectionsthatare+*calledfromNAPIwithoutaseparatercu_read_lock().Thecodebelowdoesnot+*useRCUannotations,butreliesonthoseinthemapcode.
One more follow-up question related to tc BPF: given we do use rcu_read_lock_bh()
in case of sch_handle_egress(), could we also remove the rcu_read_lock() pair
from cls_bpf_classify() then?
I believe so, yeah. Patch 2 in this series should even make lockdep stop
complaining about it :)
Btw, I was wondering whether we should just get rid of all the WARN_ON_ONCE()s
from those map helpers given in most situations these are not triggered anyway
due to retpoline avoidance where verifier rewrites the calls to jump to the map
backend implementation directly. One alternative could be to have an extension
to the bpf prologue generation under CONFIG_DEBUG_LOCK_ALLOC and call the lockdep
checks from there, but it's probably not worth the effort. (In the trampoline
case we have those __bpf_prog_enter()/__bpf_prog_enter_sleepable() where the
latter in particular has asserts like might_fault(), fwiw.)
I agree that it's probably overkill to amend the prologue. No strong
opinion on whether removing the checks entirely is a good idea; I guess
they at least serve as documentation even if they're not actually called
that often?
Ack, that's okay with me, and if we find a better solution, we can always change it
later on.
Thanks,
Daniel
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
@@ -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
From: "Paul E. McKenney" <paulmck@kernel.org> Date: 2021-06-22 20:26:07
On Tue, Jun 22, 2021 at 03:55:25PM +0200, Toke Høiland-Jørgensen wrote:
Toke Høiland-Jørgensen [off-list ref] writes:
quoted
quoted
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?
I think that my original paragraph needed to have been updated back
when v4.20 came out. And again when RCU Tasks Trace came out. ;-)
So I did that updating, then approximated your patch on top of it,
as shown below. Does this work for you?
Thanx, Paul
------------------------------------------------------------------------
commit c6ef58907d22f4f327f1e9a637b50a5899aac450
Author: Toke Høiland-Jørgensen [off-list ref]
Date: Tue Jun 22 11:54:34 2021 -0700
doc: Give XDP as example of non-obvious RCU reader/updater pairing
This commit gives an example of non-obvious RCU reader/updater pairing
in the guise of the XDP feature in networking, which calls BPF programs
from network-driver NAPI (softirq) context.
Signed-off-by: Toke Høiland-Jørgensen [off-list ref]
Signed-off-by: Paul E. McKenney [off-list ref]
@@ -236,8 +236,15 @@ over a rather long period of time, but improvements are always welcome! Mixing things up will result in confusion and broken kernels, and has even resulted in an exploitable security issue. Therefore,- when using non-obvious pairs of primitives, commenting is of- course a must.+ when using non-obvious pairs of primitives, commenting is+ of course a must. One example of non-obvious pairing 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. The reason+ that this usage is safe is that readers can use anything that+ disables BH when updaters use call_rcu() or synchronize_rcu(). 8. Although synchronize_rcu() is slower than is call_rcu(), it usually results in simpler code. So, unless update performance is
On Tue, Jun 22, 2021 at 03:55:25PM +0200, Toke Høiland-Jørgensen wrote:
quoted
Toke Høiland-Jørgensen [off-list ref] writes:
quoted
quoted
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?
I think that my original paragraph needed to have been updated back
when v4.20 came out. And again when RCU Tasks Trace came out. ;-)
So I did that updating, then approximated your patch on top of it,
as shown below. Does this work for you?
Yup, LGTM, thanks! Shall I just fold that version into the next version
of my series, or do you want to take it through your tree (I suppose
it's independent of the rest, so either way is fine by me)?
-Toke
From: "Paul E. McKenney" <paulmck@kernel.org> Date: 2021-06-22 23:19:51
On Tue, Jun 22, 2021 at 11:48:26PM +0200, Toke Høiland-Jørgensen wrote:
"Paul E. McKenney" [off-list ref] writes:
quoted
On Tue, Jun 22, 2021 at 03:55:25PM +0200, Toke Høiland-Jørgensen wrote:
quoted
Toke Høiland-Jørgensen [off-list ref] writes:
quoted
quoted
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?
I think that my original paragraph needed to have been updated back
when v4.20 came out. And again when RCU Tasks Trace came out. ;-)
So I did that updating, then approximated your patch on top of it,
as shown below. Does this work for you?
Yup, LGTM, thanks! Shall I just fold that version into the next version
of my series, or do you want to take it through your tree (I suppose
it's independent of the rest, so either way is fine by me)?
I currently have the two here in -rcu, most likely for v5.15 (as in
the merge window after the upcoming one):
2b7cb9d95ba4 ("doc: Clarify and expand RCU updaters and corresponding readers")
c6ef58907d22 ("doc: Give XDP as example of non-obvious RCU reader/updater pairing")
I am happy taking it, but if you really would like to add it to your
series, please do take both. ;-)
Thanx, Paul
On Tue, Jun 22, 2021 at 11:48:26PM +0200, Toke Høiland-Jørgensen wrote:
quoted
"Paul E. McKenney" [off-list ref] writes:
quoted
On Tue, Jun 22, 2021 at 03:55:25PM +0200, Toke Høiland-Jørgensen wrote:
quoted
Toke Høiland-Jørgensen [off-list ref] writes:
quoted
quoted
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?
I think that my original paragraph needed to have been updated back
when v4.20 came out. And again when RCU Tasks Trace came out. ;-)
So I did that updating, then approximated your patch on top of it,
as shown below. Does this work for you?
Yup, LGTM, thanks! Shall I just fold that version into the next version
of my series, or do you want to take it through your tree (I suppose
it's independent of the rest, so either way is fine by me)?
I currently have the two here in -rcu, most likely for v5.15 (as in
the merge window after the upcoming one):
2b7cb9d95ba4 ("doc: Clarify and expand RCU updaters and corresponding readers")
c6ef58907d22 ("doc: Give XDP as example of non-obvious RCU reader/updater pairing")
I am happy taking it, but if you really would like to add it to your
series, please do take both. ;-)