| Message ID | 20210615145455.564037-8-toke@redhat.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | BPF |
| Headers | show |
| Series | Clean up and document RCU-based object protection for XDP_REDIRECT | expand |
| Context | Check | Description |
|---|---|---|
| netdev/cover_letter | success | Link |
| netdev/fixes_present | success | Link |
| netdev/patch_count | fail | Series longer than 15 patches |
| netdev/tree_selection | success | Clearly marked for bpf-next |
| netdev/subject_prefix | success | Link |
| netdev/cc_maintainers | warning | 9 maintainers not CCed: yhs@fb.com kpsingh@kernel.org daniel@iogearbox.net andrii@kernel.org hawk@kernel.org ast@kernel.org john.fastabend@gmail.com songliubraving@fb.com davem@davemloft.net |
| netdev/source_inline | success | Was 0 now: 0 |
| netdev/verify_signedoff | success | Link |
| netdev/module_param | success | Was 0 now: 0 |
| netdev/build_32bit | success | Errors and warnings before: 0 this patch: 0 |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/verify_fixes | success | Link |
| netdev/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 55 lines checked |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 0 this patch: 0 |
| netdev/header_inline | success | Link |
> -----Original Message----- > From: Toke Høiland-Jørgensen <toke@redhat.com> > Sent: Tuesday, June 15, 2021 17:55 > To: bpf@vger.kernel.org; netdev@vger.kernel.org > Cc: Martin KaFai Lau <kafai@fb.com>; Hangbin Liu <liuhangbin@gmail.com>; > Jesper Dangaard Brouer <brouer@redhat.com>; Magnus Karlsson > <magnus.karlsson@gmail.com>; Paul E . McKenney <paulmck@kernel.org>; > Jakub Kicinski <kuba@kernel.org>; Toke Høiland-Jørgensen > <toke@redhat.com>; Madalin Bucur <madalin.bucur@nxp.com>; Ioana > Ciornei <ioana.ciornei@nxp.com>; Ioana Ciocoi Radulescu > <ruxandra.radulescu@nxp.com> > Subject: [PATCH bpf-next v2 07/16] freescale: remove rcu_read_lock() > around XDP program invocation > > 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 <ruxandra.radulescu@nxp.com> > Signed-off-by: Toke Høiland-Jørgensen <toke@redhat.com> > --- Reviewed-by: Camelia Groza <camelia.groza@nxp.com> Thanks! > 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(-) > > diff --git a/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c > b/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c > index 177c020bf34a..98fdcbde687a 100644 > --- a/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c > +++ b/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c > @@ -2558,13 +2558,9 @@ static u32 dpaa_run_xdp(struct dpaa_priv *priv, > struct qm_fd *fd, void *vaddr, > u32 xdp_act; > int err; > > - rcu_read_lock(); > - > xdp_prog = READ_ONCE(priv->xdp_prog); > - if (!xdp_prog) { > - rcu_read_unlock(); > + if (!xdp_prog) > return XDP_PASS; > - } > > xdp_init_buff(&xdp, DPAA_BP_RAW_SIZE - > DPAA_TX_PRIV_DATA_SIZE, > &dpaa_fq->xdp_rxq); > @@ -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(), which provides the needed RCU protection. > + */ > xdp_act = bpf_prog_run_xdp(xdp_prog, &xdp); > > /* Update the length and the offset of the FD */ > @@ -2638,8 +2637,6 @@ static u32 dpaa_run_xdp(struct dpaa_priv *priv, > struct qm_fd *fd, void *vaddr, > break; > } > > - rcu_read_unlock(); > - > return xdp_act; > } > > diff --git a/drivers/net/ethernet/freescale/dpaa2/dpaa2-eth.c > b/drivers/net/ethernet/freescale/dpaa2/dpaa2-eth.c > index 8433aa730c42..964d85c9e37d 100644 > --- a/drivers/net/ethernet/freescale/dpaa2/dpaa2-eth.c > +++ b/drivers/net/ethernet/freescale/dpaa2/dpaa2-eth.c > @@ -352,8 +352,6 @@ static u32 dpaa2_eth_run_xdp(struct dpaa2_eth_priv > *priv, > u32 xdp_act = XDP_PASS; > int err, offset; > > - rcu_read_lock(); > - > xdp_prog = READ_ONCE(ch->xdp.prog); > if (!xdp_prog) > goto out; > @@ -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(), which provides the needed RCU protection. > + */ > xdp_act = bpf_prog_run_xdp(xdp_prog, &xdp); > > /* xdp.data pointer may have changed */ > @@ -414,7 +415,6 @@ static u32 dpaa2_eth_run_xdp(struct dpaa2_eth_priv > *priv, > > ch->xdp.res |= xdp_act; > out: > - rcu_read_unlock(); > return xdp_act; > } > > -- > 2.31.1
diff --git a/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c b/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c index 177c020bf34a..98fdcbde687a 100644 --- a/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c +++ b/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c @@ -2558,13 +2558,9 @@ static u32 dpaa_run_xdp(struct dpaa_priv *priv, struct qm_fd *fd, void *vaddr, u32 xdp_act; int err; - rcu_read_lock(); - xdp_prog = READ_ONCE(priv->xdp_prog); - if (!xdp_prog) { - rcu_read_unlock(); + if (!xdp_prog) return XDP_PASS; - } xdp_init_buff(&xdp, DPAA_BP_RAW_SIZE - DPAA_TX_PRIV_DATA_SIZE, &dpaa_fq->xdp_rxq); @@ -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(), which provides the needed RCU protection. + */ xdp_act = bpf_prog_run_xdp(xdp_prog, &xdp); /* Update the length and the offset of the FD */ @@ -2638,8 +2637,6 @@ static u32 dpaa_run_xdp(struct dpaa_priv *priv, struct qm_fd *fd, void *vaddr, break; } - rcu_read_unlock(); - return xdp_act; } diff --git a/drivers/net/ethernet/freescale/dpaa2/dpaa2-eth.c b/drivers/net/ethernet/freescale/dpaa2/dpaa2-eth.c index 8433aa730c42..964d85c9e37d 100644 --- a/drivers/net/ethernet/freescale/dpaa2/dpaa2-eth.c +++ b/drivers/net/ethernet/freescale/dpaa2/dpaa2-eth.c @@ -352,8 +352,6 @@ static u32 dpaa2_eth_run_xdp(struct dpaa2_eth_priv *priv, u32 xdp_act = XDP_PASS; int err, offset; - rcu_read_lock(); - xdp_prog = READ_ONCE(ch->xdp.prog); if (!xdp_prog) goto out; @@ -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(), which provides the needed RCU protection. + */ xdp_act = bpf_prog_run_xdp(xdp_prog, &xdp); /* xdp.data pointer may have changed */ @@ -414,7 +415,6 @@ static u32 dpaa2_eth_run_xdp(struct dpaa2_eth_priv *priv, ch->xdp.res |= xdp_act; out: - rcu_read_unlock(); return xdp_act; }
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 <ruxandra.radulescu@nxp.com> Signed-off-by: Toke Høiland-Jørgensen <toke@redhat.com> --- 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(-)