netdev - Re: [PATCHv2 bpf-next 2/5] bpf: Prefetch established socket destinations

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, 26 Mar 2020 14:11:52 -0700
From:   Alexei Starovoitov <alexei.starovoitov@...il.com>
To:     Joe Stringer <joe@...d.net.nz>
Cc:     bpf@...r.kernel.org, netdev@...r.kernel.org, daniel@...earbox.net,
        ast@...nel.org, eric.dumazet@...il.com, lmb@...udflare.com,
        kafai@...com
Subject: Re: [PATCHv2 bpf-next 2/5] bpf: Prefetch established socket
 destinations

On Tue, Mar 24, 2020 at 10:57:42PM -0700, Joe Stringer wrote:
> Enhance the sk_assign logic to temporarily store the socket
> receive destination, to save the route lookup later on. The dst
> reference is kept alive by the caller's socket reference.
> 
> Suggested-by: Daniel Borkmann <daniel@...earbox.net>
> Signed-off-by: Joe Stringer <joe@...d.net.nz>
> ---
> v2: Provide cookie to dst_check() for IPv6 case
> v1: Initial version
> ---
>  net/core/filter.c | 15 +++++++++++++++
>  1 file changed, 15 insertions(+)
> 
> diff --git a/net/core/filter.c b/net/core/filter.c
> index f7f9b6631f75..0fada7fe9b75 100644
> --- a/net/core/filter.c
> +++ b/net/core/filter.c
> @@ -5876,6 +5876,21 @@ BPF_CALL_3(bpf_sk_assign, struct sk_buff *, skb, struct sock *, sk, u64, flags)
>  	skb_orphan(skb);
>  	skb->sk = sk;
>  	skb->destructor = sock_pfree;
> +	if (sk_fullsock(sk)) {
> +		struct dst_entry *dst = READ_ONCE(sk->sk_rx_dst);
> +		u32 cookie = 0;
> +
> +#if IS_ENABLED(CONFIG_IPV6)
> +		if (sk->sk_family == AF_INET6)
> +			cookie = inet6_sk(sk)->rx_dst_cookie;
> +#endif
> +		if (dst)
> +			dst = dst_check(dst, cookie);
> +		if (dst) {
> +			skb_dst_drop(skb);
> +			skb_dst_set_noref(skb, dst);
> +		}

I think the rest of the feedback for the patches can be addressed quickly and
overall the set is imo ready to land within this cycle. My only concern is
above dst_set().
Since it's an optimization may be drop this patch? we can land
the rest and this one can be introduced in the next cycle?
I'm happy to be convinced otherwise, but would like a better explanation
why it's safe to do so in this context.