* [Intel-wired-lan] [PATCH net-next 20/24] net: intel: Use nested-BH locking for XDP redirect. [not found] <20231215171020.687342-1-bigeasy@linutronix.de> @ 2023-12-15 17:07 ` Sebastian Andrzej Siewior 2023-12-16 4:53 ` kernel test robot 0 siblings, 1 reply; 4+ messages in thread From: Sebastian Andrzej Siewior @ 2023-12-15 17:07 UTC (permalink / raw) To: linux-kernel, netdev Cc: Jesper Dangaard Brouer, Daniel Borkmann, Peter Zijlstra, intel-wired-lan, Frederic Weisbecker, bpf, John Fastabend, Alexei Starovoitov, David S. Miller, Jesse Brandeburg, Eric Dumazet, Ingo Molnar, Tony Nguyen, Waiman Long, Jakub Kicinski, Thomas Gleixner, Paolo Abeni, Will Deacon, Boqun Feng, Sebastian Andrzej Siewior The per-CPU variables used during bpf_prog_run_xdp() invocation and later during xdp_do_redirect() rely on disabled BH for their protection. Without locking in local_bh_disable() on PREEMPT_RT these data structure require explicit locking. This is a follow-up on the previous change which introduced bpf_run_lock.redirect_lock and uses it now within drivers. The simple way is to acquire the lock before bpf_prog_run_xdp() is invoked and hold it until the end of function. This does not always work because some drivers (cpsw, atlantic) invoke xdp_do_flush() in the same context. Acquiring the lock in bpf_prog_run_xdp() and dropping in xdp_do_redirect() (without touching drivers) does not work because not all driver, which use bpf_prog_run_xdp(), do support XDP_REDIRECT (and invoke xdp_do_redirect()). Ideally the minimal locking scope would be bpf_prog_run_xdp() + xdp_do_redirect() and everything else (error recovery, DMA unmapping, free/ alloc of memory, …) would happen outside of the locked section. Cc: Alexei Starovoitov <ast@kernel.org> Cc: Jesper Dangaard Brouer <hawk@kernel.org> Cc: Jesse Brandeburg <jesse.brandeburg@intel.com> Cc: John Fastabend <john.fastabend@gmail.com> Cc: Tony Nguyen <anthony.l.nguyen@intel.com> Cc: bpf@vger.kernel.org (open list:XDP Cc: intel-wired-lan@lists.osuosl.org Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de> --- drivers/net/ethernet/intel/i40e/i40e_txrx.c | 1 + drivers/net/ethernet/intel/i40e/i40e_xsk.c | 22 +++++++++-------- drivers/net/ethernet/intel/ice/ice_txrx.c | 1 + drivers/net/ethernet/intel/ice/ice_xsk.c | 21 ++++++++-------- drivers/net/ethernet/intel/igb/igb_main.c | 1 + drivers/net/ethernet/intel/igc/igc_main.c | 5 +++- drivers/net/ethernet/intel/ixgbe/ixgbe_main.c | 1 + drivers/net/ethernet/intel/ixgbe/ixgbe_xsk.c | 24 ++++++++++--------- .../net/ethernet/intel/ixgbevf/ixgbevf_main.c | 3 ++- 9 files changed, 46 insertions(+), 33 deletions(-) diff --git a/drivers/net/ethernet/intel/i40e/i40e_txrx.c b/drivers/net/ethernet/intel/i40e/i40e_txrx.c index dd410b15000f7..76e069ae2183a 100644 --- a/drivers/net/ethernet/intel/i40e/i40e_txrx.c +++ b/drivers/net/ethernet/intel/i40e/i40e_txrx.c @@ -2326,6 +2326,7 @@ static int i40e_run_xdp(struct i40e_ring *rx_ring, struct xdp_buff *xdp, struct prefetchw(xdp->data_hard_start); /* xdp_frame write */ + guard(local_lock_nested_bh)(&bpf_run_lock.redirect_lock); act = bpf_prog_run_xdp(xdp_prog, xdp); switch (act) { case XDP_PASS: diff --git a/drivers/net/ethernet/intel/i40e/i40e_xsk.c b/drivers/net/ethernet/intel/i40e/i40e_xsk.c index e99fa854d17f1..2b0c0c1f3ddc8 100644 --- a/drivers/net/ethernet/intel/i40e/i40e_xsk.c +++ b/drivers/net/ethernet/intel/i40e/i40e_xsk.c @@ -201,17 +201,19 @@ static int i40e_run_xdp_zc(struct i40e_ring *rx_ring, struct xdp_buff *xdp, struct i40e_ring *xdp_ring; u32 act; - act = bpf_prog_run_xdp(xdp_prog, xdp); + scoped_guard(local_lock_nested_bh, &bpf_run_lock.redirect_lock) { + act = bpf_prog_run_xdp(xdp_prog, xdp); - if (likely(act == XDP_REDIRECT)) { - err = xdp_do_redirect(rx_ring->netdev, xdp, xdp_prog); - if (!err) - return I40E_XDP_REDIR; - if (xsk_uses_need_wakeup(rx_ring->xsk_pool) && err == -ENOBUFS) - result = I40E_XDP_EXIT; - else - result = I40E_XDP_CONSUMED; - goto out_failure; + if (likely(act == XDP_REDIRECT)) { + err = xdp_do_redirect(rx_ring->netdev, xdp, xdp_prog); + if (!err) + return I40E_XDP_REDIR; + if (xsk_uses_need_wakeup(rx_ring->xsk_pool) && err == -ENOBUFS) + result = I40E_XDP_EXIT; + else + result = I40E_XDP_CONSUMED; + goto out_failure; + } } switch (act) { diff --git a/drivers/net/ethernet/intel/ice/ice_txrx.c b/drivers/net/ethernet/intel/ice/ice_txrx.c index 9e97ea8630686..5d4cfa3455b37 100644 --- a/drivers/net/ethernet/intel/ice/ice_txrx.c +++ b/drivers/net/ethernet/intel/ice/ice_txrx.c @@ -571,6 +571,7 @@ ice_run_xdp(struct ice_rx_ring *rx_ring, struct xdp_buff *xdp, if (!xdp_prog) goto exit; + guard(local_lock_nested_bh)(&bpf_run_lock.redirect_lock); act = bpf_prog_run_xdp(xdp_prog, xdp); switch (act) { case XDP_PASS: diff --git a/drivers/net/ethernet/intel/ice/ice_xsk.c b/drivers/net/ethernet/intel/ice/ice_xsk.c index 99954508184f9..02f89c22d19e3 100644 --- a/drivers/net/ethernet/intel/ice/ice_xsk.c +++ b/drivers/net/ethernet/intel/ice/ice_xsk.c @@ -762,17 +762,18 @@ ice_run_xdp_zc(struct ice_rx_ring *rx_ring, struct xdp_buff *xdp, int err, result = ICE_XDP_PASS; u32 act; + scoped_guard(local_lock_nested_bh, &bpf_run_lock.redirect_lock) { act = bpf_prog_run_xdp(xdp_prog, xdp); - - if (likely(act == XDP_REDIRECT)) { - err = xdp_do_redirect(rx_ring->netdev, xdp, xdp_prog); - if (!err) - return ICE_XDP_REDIR; - if (xsk_uses_need_wakeup(rx_ring->xsk_pool) && err == -ENOBUFS) - result = ICE_XDP_EXIT; - else - result = ICE_XDP_CONSUMED; - goto out_failure; + if (likely(act == XDP_REDIRECT)) { + err = xdp_do_redirect(rx_ring->netdev, xdp, xdp_prog); + if (!err) + return ICE_XDP_REDIR; + if (xsk_uses_need_wakeup(rx_ring->xsk_pool) && err == -ENOBUFS) + result = ICE_XDP_EXIT; + else + result = ICE_XDP_CONSUMED; + goto out_failure; + } } switch (act) { diff --git a/drivers/net/ethernet/intel/igb/igb_main.c b/drivers/net/ethernet/intel/igb/igb_main.c index b2295caa2f0ab..e01be809d030e 100644 --- a/drivers/net/ethernet/intel/igb/igb_main.c +++ b/drivers/net/ethernet/intel/igb/igb_main.c @@ -8621,6 +8621,7 @@ static struct sk_buff *igb_run_xdp(struct igb_adapter *adapter, prefetchw(xdp->data_hard_start); /* xdp_frame write */ + guard(local_lock_nested_bh)(&bpf_run_lock.redirect_lock); act = bpf_prog_run_xdp(xdp_prog, xdp); switch (act) { case XDP_PASS: diff --git a/drivers/net/ethernet/intel/igc/igc_main.c b/drivers/net/ethernet/intel/igc/igc_main.c index e9bb403bbacf9..8321419b3a307 100644 --- a/drivers/net/ethernet/intel/igc/igc_main.c +++ b/drivers/net/ethernet/intel/igc/igc_main.c @@ -2485,7 +2485,10 @@ static int __igc_xdp_run_prog(struct igc_adapter *adapter, struct bpf_prog *prog, struct xdp_buff *xdp) { - u32 act = bpf_prog_run_xdp(prog, xdp); + u32 act; + + guard(local_lock_nested_bh)(&bpf_run_lock.redirect_lock); + act = bpf_prog_run_xdp(prog, xdp); switch (act) { case XDP_PASS: diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c index 94bde2cad0f47..de564e8b83be2 100644 --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c @@ -2203,6 +2203,7 @@ static struct sk_buff *ixgbe_run_xdp(struct ixgbe_adapter *adapter, prefetchw(xdp->data_hard_start); /* xdp_frame write */ + guard(local_lock_nested_bh)(&bpf_run_lock.redirect_lock); act = bpf_prog_run_xdp(xdp_prog, xdp); switch (act) { case XDP_PASS: diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_xsk.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_xsk.c index 59798bc33298f..b988f758aad49 100644 --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_xsk.c +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_xsk.c @@ -104,18 +104,20 @@ static int ixgbe_run_xdp_zc(struct ixgbe_adapter *adapter, struct xdp_frame *xdpf; u32 act; - xdp_prog = READ_ONCE(rx_ring->xdp_prog); - act = bpf_prog_run_xdp(xdp_prog, xdp); + scoped_guard(local_lock_nested_bh, &bpf_run_lock.redirect_lock) { + 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); - if (!err) - return IXGBE_XDP_REDIR; - if (xsk_uses_need_wakeup(rx_ring->xsk_pool) && err == -ENOBUFS) - result = IXGBE_XDP_EXIT; - else - result = IXGBE_XDP_CONSUMED; - goto out_failure; + if (likely(act == XDP_REDIRECT)) { + err = xdp_do_redirect(rx_ring->netdev, xdp, xdp_prog); + if (!err) + return IXGBE_XDP_REDIR; + if (xsk_uses_need_wakeup(rx_ring->xsk_pool) && err == -ENOBUFS) + result = IXGBE_XDP_EXIT; + else + result = IXGBE_XDP_CONSUMED; + goto out_failure; + } } switch (act) { diff --git a/drivers/net/ethernet/intel/ixgbevf/ixgbevf_main.c b/drivers/net/ethernet/intel/ixgbevf/ixgbevf_main.c index a44e4bd561421..1c58c08aa15ff 100644 --- a/drivers/net/ethernet/intel/ixgbevf/ixgbevf_main.c +++ b/drivers/net/ethernet/intel/ixgbevf/ixgbevf_main.c @@ -1059,7 +1059,8 @@ static struct sk_buff *ixgbevf_run_xdp(struct ixgbevf_adapter *adapter, if (!xdp_prog) goto xdp_out; - act = bpf_prog_run_xdp(xdp_prog, xdp); + scoped_guard(local_lock_nested_bh, &bpf_run_lock.redirect_lock) + act = bpf_prog_run_xdp(xdp_prog, xdp); switch (act) { case XDP_PASS: break; -- 2.43.0 _______________________________________________ Intel-wired-lan mailing list Intel-wired-lan@osuosl.org https://lists.osuosl.org/mailman/listinfo/intel-wired-lan ^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [Intel-wired-lan] [PATCH net-next 20/24] net: intel: Use nested-BH locking for XDP redirect. 2023-12-15 17:07 ` [Intel-wired-lan] [PATCH net-next 20/24] net: intel: Use nested-BH locking for XDP redirect Sebastian Andrzej Siewior @ 2023-12-16 4:53 ` kernel test robot 2023-12-19 0:01 ` Nathan Chancellor 0 siblings, 1 reply; 4+ messages in thread From: kernel test robot @ 2023-12-16 4:53 UTC (permalink / raw) To: Sebastian Andrzej Siewior, linux-kernel, netdev Cc: Jesper Dangaard Brouer, Daniel Borkmann, Peter Zijlstra, intel-wired-lan, Frederic Weisbecker, llvm, John Fastabend, Alexei Starovoitov, bpf, Jesse Brandeburg, Eric Dumazet, Ingo Molnar, Tony Nguyen, Waiman Long, oe-kbuild-all, Jakub Kicinski, Thomas Gleixner, Paolo Abeni, Will Deacon, Boqun Feng, Sebastian Andrzej Siewior Hi Sebastian, kernel test robot noticed the following build errors: [auto build test ERROR on net-next/main] url: https://github.com/intel-lab-lkp/linux/commits/Sebastian-Andrzej-Siewior/locking-local_lock-Introduce-guard-definition-for-local_lock/20231216-011911 base: net-next/main patch link: https://lore.kernel.org/r/20231215171020.687342-21-bigeasy%40linutronix.de patch subject: [PATCH net-next 20/24] net: intel: Use nested-BH locking for XDP redirect. config: arm-defconfig (https://download.01.org/0day-ci/archive/20231216/202312161212.D5tju5i6-lkp@intel.com/config) compiler: clang version 14.0.6 (https://github.com/llvm/llvm-project.git f28c006a5895fc0e329fe15fead81e37457cb1d1) reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20231216/202312161212.D5tju5i6-lkp@intel.com/reproduce) If you fix the issue in a separate patch/commit (i.e. not just a new version of the same patch/commit), kindly add following tags | Reported-by: kernel test robot <lkp@intel.com> | Closes: https://lore.kernel.org/oe-kbuild-all/202312161212.D5tju5i6-lkp@intel.com/ All errors (new ones prefixed by >>): >> drivers/net/ethernet/intel/igb/igb_main.c:8620:3: error: cannot jump from this goto statement to its label goto xdp_out; ^ drivers/net/ethernet/intel/igb/igb_main.c:8624:2: note: jump bypasses initialization of variable with __attribute__((cleanup)) guard(local_lock_nested_bh)(&bpf_run_lock.redirect_lock); ^ include/linux/cleanup.h:142:15: note: expanded from macro 'guard' CLASS(_name, __UNIQUE_ID(guard)) ^ include/linux/compiler.h:180:29: note: expanded from macro '__UNIQUE_ID' #define __UNIQUE_ID(prefix) __PASTE(__PASTE(__UNIQUE_ID_, prefix), __COUNTER__) ^ include/linux/compiler_types.h:84:22: note: expanded from macro '__PASTE' #define __PASTE(a,b) ___PASTE(a,b) ^ include/linux/compiler_types.h:83:23: note: expanded from macro '___PASTE' #define ___PASTE(a,b) a##b ^ <scratch space>:52:1: note: expanded from here __UNIQUE_ID_guard753 ^ 1 error generated. vim +8620 drivers/net/ethernet/intel/igb/igb_main.c b1bb2eb0a0deb0 Alexander Duyck 2017-02-06 8608 9cbc948b5a20c9 Sven Auhagen 2020-09-02 8609 static struct sk_buff *igb_run_xdp(struct igb_adapter *adapter, 9cbc948b5a20c9 Sven Auhagen 2020-09-02 8610 struct igb_ring *rx_ring, 9cbc948b5a20c9 Sven Auhagen 2020-09-02 8611 struct xdp_buff *xdp) 9cbc948b5a20c9 Sven Auhagen 2020-09-02 8612 { 9cbc948b5a20c9 Sven Auhagen 2020-09-02 8613 int err, result = IGB_XDP_PASS; 9cbc948b5a20c9 Sven Auhagen 2020-09-02 8614 struct bpf_prog *xdp_prog; 9cbc948b5a20c9 Sven Auhagen 2020-09-02 8615 u32 act; 9cbc948b5a20c9 Sven Auhagen 2020-09-02 8616 9cbc948b5a20c9 Sven Auhagen 2020-09-02 8617 xdp_prog = READ_ONCE(rx_ring->xdp_prog); 9cbc948b5a20c9 Sven Auhagen 2020-09-02 8618 9cbc948b5a20c9 Sven Auhagen 2020-09-02 8619 if (!xdp_prog) 9cbc948b5a20c9 Sven Auhagen 2020-09-02 @8620 goto xdp_out; 9cbc948b5a20c9 Sven Auhagen 2020-09-02 8621 9cbc948b5a20c9 Sven Auhagen 2020-09-02 8622 prefetchw(xdp->data_hard_start); /* xdp_frame write */ 9cbc948b5a20c9 Sven Auhagen 2020-09-02 8623 d568b111738dbb Sebastian Andrzej Siewior 2023-12-15 8624 guard(local_lock_nested_bh)(&bpf_run_lock.redirect_lock); 9cbc948b5a20c9 Sven Auhagen 2020-09-02 8625 act = bpf_prog_run_xdp(xdp_prog, xdp); 9cbc948b5a20c9 Sven Auhagen 2020-09-02 8626 switch (act) { 9cbc948b5a20c9 Sven Auhagen 2020-09-02 8627 case XDP_PASS: 9cbc948b5a20c9 Sven Auhagen 2020-09-02 8628 break; 9cbc948b5a20c9 Sven Auhagen 2020-09-02 8629 case XDP_TX: 9cbc948b5a20c9 Sven Auhagen 2020-09-02 8630 result = igb_xdp_xmit_back(adapter, xdp); 74431c40b9c5fa Magnus Karlsson 2021-05-10 8631 if (result == IGB_XDP_CONSUMED) 74431c40b9c5fa Magnus Karlsson 2021-05-10 8632 goto out_failure; 9cbc948b5a20c9 Sven Auhagen 2020-09-02 8633 break; 9cbc948b5a20c9 Sven Auhagen 2020-09-02 8634 case XDP_REDIRECT: 9cbc948b5a20c9 Sven Auhagen 2020-09-02 8635 err = xdp_do_redirect(adapter->netdev, xdp, xdp_prog); 74431c40b9c5fa Magnus Karlsson 2021-05-10 8636 if (err) 74431c40b9c5fa Magnus Karlsson 2021-05-10 8637 goto out_failure; 9cbc948b5a20c9 Sven Auhagen 2020-09-02 8638 result = IGB_XDP_REDIR; 9cbc948b5a20c9 Sven Auhagen 2020-09-02 8639 break; 9cbc948b5a20c9 Sven Auhagen 2020-09-02 8640 default: c8064e5b4adac5 Paolo Abeni 2021-11-30 8641 bpf_warn_invalid_xdp_action(adapter->netdev, xdp_prog, act); 9cbc948b5a20c9 Sven Auhagen 2020-09-02 8642 fallthrough; 9cbc948b5a20c9 Sven Auhagen 2020-09-02 8643 case XDP_ABORTED: 74431c40b9c5fa Magnus Karlsson 2021-05-10 8644 out_failure: 9cbc948b5a20c9 Sven Auhagen 2020-09-02 8645 trace_xdp_exception(rx_ring->netdev, xdp_prog, act); 9cbc948b5a20c9 Sven Auhagen 2020-09-02 8646 fallthrough; 9cbc948b5a20c9 Sven Auhagen 2020-09-02 8647 case XDP_DROP: 9cbc948b5a20c9 Sven Auhagen 2020-09-02 8648 result = IGB_XDP_CONSUMED; 9cbc948b5a20c9 Sven Auhagen 2020-09-02 8649 break; 9cbc948b5a20c9 Sven Auhagen 2020-09-02 8650 } 9cbc948b5a20c9 Sven Auhagen 2020-09-02 8651 xdp_out: 9cbc948b5a20c9 Sven Auhagen 2020-09-02 8652 return ERR_PTR(-result); 9cbc948b5a20c9 Sven Auhagen 2020-09-02 8653 } 9cbc948b5a20c9 Sven Auhagen 2020-09-02 8654 -- 0-DAY CI Kernel Test Service https://github.com/intel/lkp-tests/wiki _______________________________________________ Intel-wired-lan mailing list Intel-wired-lan@osuosl.org https://lists.osuosl.org/mailman/listinfo/intel-wired-lan ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [Intel-wired-lan] [PATCH net-next 20/24] net: intel: Use nested-BH locking for XDP redirect. 2023-12-16 4:53 ` kernel test robot @ 2023-12-19 0:01 ` Nathan Chancellor 2023-12-19 16:55 ` Nick Desaulniers 0 siblings, 1 reply; 4+ messages in thread From: Nathan Chancellor @ 2023-12-19 0:01 UTC (permalink / raw) To: kernel test robot Cc: Peter Zijlstra, Sebastian Andrzej Siewior, llvm, Alexei Starovoitov, Eric Dumazet, Tony Nguyen, Will Deacon, Daniel Borkmann, John Fastabend, Jesse Brandeburg, Ingo Molnar, intel-wired-lan, Waiman Long, Paolo Abeni, Boqun Feng, Jesper Dangaard Brouer, Frederic Weisbecker, Jakub Kicinski, oe-kbuild-all, Thomas Gleixner, netdev, linux-kernel, bpf On Sat, Dec 16, 2023 at 12:53:43PM +0800, kernel test robot wrote: > Hi Sebastian, > > kernel test robot noticed the following build errors: > > [auto build test ERROR on net-next/main] > > url: https://github.com/intel-lab-lkp/linux/commits/Sebastian-Andrzej-Siewior/locking-local_lock-Introduce-guard-definition-for-local_lock/20231216-011911 > base: net-next/main > patch link: https://lore.kernel.org/r/20231215171020.687342-21-bigeasy%40linutronix.de > patch subject: [PATCH net-next 20/24] net: intel: Use nested-BH locking for XDP redirect. > config: arm-defconfig (https://download.01.org/0day-ci/archive/20231216/202312161212.D5tju5i6-lkp@intel.com/config) > compiler: clang version 14.0.6 (https://github.com/llvm/llvm-project.git f28c006a5895fc0e329fe15fead81e37457cb1d1) > reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20231216/202312161212.D5tju5i6-lkp@intel.com/reproduce) > > If you fix the issue in a separate patch/commit (i.e. not just a new version of > the same patch/commit), kindly add following tags > | Reported-by: kernel test robot <lkp@intel.com> > | Closes: https://lore.kernel.org/oe-kbuild-all/202312161212.D5tju5i6-lkp@intel.com/ > > All errors (new ones prefixed by >>): > > >> drivers/net/ethernet/intel/igb/igb_main.c:8620:3: error: cannot jump from this goto statement to its label > goto xdp_out; > ^ > drivers/net/ethernet/intel/igb/igb_main.c:8624:2: note: jump bypasses initialization of variable with __attribute__((cleanup)) > guard(local_lock_nested_bh)(&bpf_run_lock.redirect_lock); > ^ > include/linux/cleanup.h:142:15: note: expanded from macro 'guard' > CLASS(_name, __UNIQUE_ID(guard)) > ^ > include/linux/compiler.h:180:29: note: expanded from macro '__UNIQUE_ID' > #define __UNIQUE_ID(prefix) __PASTE(__PASTE(__UNIQUE_ID_, prefix), __COUNTER__) > ^ > include/linux/compiler_types.h:84:22: note: expanded from macro '__PASTE' > #define __PASTE(a,b) ___PASTE(a,b) > ^ > include/linux/compiler_types.h:83:23: note: expanded from macro '___PASTE' > #define ___PASTE(a,b) a##b > ^ > <scratch space>:52:1: note: expanded from here > __UNIQUE_ID_guard753 > ^ > 1 error generated. I initially thought that this may have been https://github.com/ClangBuiltLinux/linux/issues/1886 but asm goto is not involved here. This error occurs because jumping over the initialization of a variable declared with __attribute__((__cleanup__(...))) does not prevent the clean up function from running as one may expect it to, but could instead result in the clean up function getting run on uninitialized memory. A contrived example (see the bottom of the "Output" tabs for the execution output): https://godbolt.org/z/9bvGboxvc While there is a warning from GCC in that example, I don't see one in the kernel's case. I see there is an open GCC issue around this problem: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=91951 While it is possible that there may not actually be a problem with how the kernel uses __attribute__((__cleanup__(...))) and gotos, I think clang's behavior is reasonable given the potential footguns that this construct has. Cheers, Nathan _______________________________________________ Intel-wired-lan mailing list Intel-wired-lan@osuosl.org https://lists.osuosl.org/mailman/listinfo/intel-wired-lan ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [Intel-wired-lan] [PATCH net-next 20/24] net: intel: Use nested-BH locking for XDP redirect. 2023-12-19 0:01 ` Nathan Chancellor @ 2023-12-19 16:55 ` Nick Desaulniers 0 siblings, 0 replies; 4+ messages in thread From: Nick Desaulniers @ 2023-12-19 16:55 UTC (permalink / raw) To: Nathan Chancellor, Sebastian Andrzej Siewior Cc: Peter Zijlstra, llvm, Alexei Starovoitov, Eric Dumazet, Tony Nguyen, Will Deacon, Daniel Borkmann, John Fastabend, Jesse Brandeburg, Ingo Molnar, intel-wired-lan, Waiman Long, Paolo Abeni, Frederic Weisbecker, Jesper Dangaard Brouer, Boqun Feng, Jakub Kicinski, oe-kbuild-all, Thomas Gleixner, netdev, linux-kernel, bpf On Mon, Dec 18, 2023 at 4:01 PM Nathan Chancellor <nathan@kernel.org> wrote: > > On Sat, Dec 16, 2023 at 12:53:43PM +0800, kernel test robot wrote: > > Hi Sebastian, > > > > kernel test robot noticed the following build errors: > > > > [auto build test ERROR on net-next/main] > > > > url: https://github.com/intel-lab-lkp/linux/commits/Sebastian-Andrzej-Siewior/locking-local_lock-Introduce-guard-definition-for-local_lock/20231216-011911 > > base: net-next/main > > patch link: https://lore.kernel.org/r/20231215171020.687342-21-bigeasy%40linutronix.de > > patch subject: [PATCH net-next 20/24] net: intel: Use nested-BH locking for XDP redirect. > > config: arm-defconfig (https://download.01.org/0day-ci/archive/20231216/202312161212.D5tju5i6-lkp@intel.com/config) > > compiler: clang version 14.0.6 (https://github.com/llvm/llvm-project.git f28c006a5895fc0e329fe15fead81e37457cb1d1) > > reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20231216/202312161212.D5tju5i6-lkp@intel.com/reproduce) > > > > If you fix the issue in a separate patch/commit (i.e. not just a new version of > > the same patch/commit), kindly add following tags > > | Reported-by: kernel test robot <lkp@intel.com> > > | Closes: https://lore.kernel.org/oe-kbuild-all/202312161212.D5tju5i6-lkp@intel.com/ > > > > All errors (new ones prefixed by >>): > > > > >> drivers/net/ethernet/intel/igb/igb_main.c:8620:3: error: cannot jump from this goto statement to its label > > goto xdp_out; > > ^ ^ The problematic goto should be replaced with an early return. (and perhaps a comment that you can't jump over __cleanup variable initialization). Otherwise the compiler cannot put the cleanup in the destination basic block; it would have to split the edges and have all the happy paths go to a synthesized basic block that runs the cleanup, then jumps to the original destination. > > drivers/net/ethernet/intel/igb/igb_main.c:8624:2: note: jump bypasses initialization of variable with __attribute__((cleanup)) > > guard(local_lock_nested_bh)(&bpf_run_lock.redirect_lock); > > ^ > > include/linux/cleanup.h:142:15: note: expanded from macro 'guard' > > CLASS(_name, __UNIQUE_ID(guard)) > > ^ > > include/linux/compiler.h:180:29: note: expanded from macro '__UNIQUE_ID' > > #define __UNIQUE_ID(prefix) __PASTE(__PASTE(__UNIQUE_ID_, prefix), __COUNTER__) > > ^ > > include/linux/compiler_types.h:84:22: note: expanded from macro '__PASTE' > > #define __PASTE(a,b) ___PASTE(a,b) > > ^ > > include/linux/compiler_types.h:83:23: note: expanded from macro '___PASTE' > > #define ___PASTE(a,b) a##b > > ^ > > <scratch space>:52:1: note: expanded from here > > __UNIQUE_ID_guard753 > > ^ > > 1 error generated. > > I initially thought that this may have been > https://github.com/ClangBuiltLinux/linux/issues/1886 but asm goto is not > involved here. > > This error occurs because jumping over the initialization of a variable > declared with __attribute__((__cleanup__(...))) does not prevent the > clean up function from running as one may expect it to, but could > instead result in the clean up function getting run on uninitialized > memory. A contrived example (see the bottom of the "Output" tabs for the > execution output): > > https://godbolt.org/z/9bvGboxvc > > While there is a warning from GCC in that example, I don't see one in > the kernel's case. I see there is an open GCC issue around this problem: > > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=91951 > > While it is possible that there may not actually be a problem with how > the kernel uses __attribute__((__cleanup__(...))) and gotos, I think > clang's behavior is reasonable given the potential footguns that this > construct has. > > Cheers, > Nathan > -- Thanks, ~Nick Desaulniers _______________________________________________ Intel-wired-lan mailing list Intel-wired-lan@osuosl.org https://lists.osuosl.org/mailman/listinfo/intel-wired-lan ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2023-12-19 16:55 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <20231215171020.687342-1-bigeasy@linutronix.de>
2023-12-15 17:07 ` [Intel-wired-lan] [PATCH net-next 20/24] net: intel: Use nested-BH locking for XDP redirect Sebastian Andrzej Siewior
2023-12-16 4:53 ` kernel test robot
2023-12-19 0:01 ` Nathan Chancellor
2023-12-19 16:55 ` Nick Desaulniers
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox