netdev - Re: [PATCH bpf-next v4 05/19] xdp: add proper __rcu annotations to redirect map entries

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

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

Date:   Thu, 24 Jun 2021 15:12:37 +0200
From:   Daniel Borkmann <daniel@...earbox.net>
To:     Toke Høiland-Jørgensen <toke@...hat.com>,
        bpf@...r.kernel.org, netdev@...r.kernel.org
Cc:     Martin KaFai Lau <kafai@...com>,
        Hangbin Liu <liuhangbin@...il.com>,
        Jesper Dangaard Brouer <brouer@...hat.com>,
        Magnus Karlsson <magnus.karlsson@...il.com>,
        "Paul E . McKenney" <paulmck@...nel.org>,
        Jakub Kicinski <kuba@...nel.org>
Subject: Re: [PATCH bpf-next v4 05/19] xdp: add proper __rcu annotations to
 redirect map entries

On 6/23/21 1:07 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.
[...]
> 
> Signed-off-by: Toke Høiland-Jørgensen <toke@...hat.com>
[...]
> diff --git a/include/linux/filter.h b/include/linux/filter.h
> index c5ad7df029ed..b01e266dad9e 100644
> --- a/include/linux/filter.h
> +++ b/include/linux/filter.h
> @@ -762,12 +762,10 @@ DECLARE_BPF_DISPATCHER(xdp)
>   
>   static __always_inline u32 bpf_prog_run_xdp(const struct bpf_prog *prog,
>   					    struct xdp_buff *xdp)
> -{
> -	/* Caller needs to hold rcu_read_lock() (!), otherwise program
> -	 * can be released while still running, or map elements could be
> -	 * freed early while still having concurrent users. XDP fastpath
> -	 * already takes rcu_read_lock() when fetching the program, so
> -	 * it's not necessary here anymore.
> +
> +	/* Driver XDP hooks are invoked within a single NAPI poll cycle and thus
> +	 * under local_bh_disable(), which provides the needed RCU protection
> +	 * for accessing map entries.
>   	 */
>   	return __BPF_PROG_RUN(prog, xdp, BPF_DISPATCHER_FUNC(xdp));
>   }

I just went over the series to manually fix up merge conflicts in the driver
patches since they didn't apply cleanly against bpf-next.

But as it turned out that extra work was needless, since you didn't even compile
test the series before submission, sigh.

Please fix (and only submit compile- & runtime-tested code in future).

Thanks,
Daniel