netdev - Re: [PATCH bpf-next v3] bpf: using rcu_read_lock for bpf_sk_storage

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:   Wed, 16 Sep 2020 10:55:53 -0700
From:   Martin KaFai Lau <kafai@...com>
To:     Yonghong Song <yhs@...com>
CC:     <bpf@...r.kernel.org>, <netdev@...r.kernel.org>,
        Alexei Starovoitov <ast@...nel.org>,
        Daniel Borkmann <daniel@...earbox.net>, <kernel-team@...com>,
        Song Liu <songliubraving@...com>
subject: Re: [PATCH bpf-next v3] bpf: using rcu_read_lock for bpf_sk_storage_map iterator

On Tue, Sep 15, 2020 at 11:16:49PM -0700, Yonghong Song wrote:
[ ... ]

> diff --git a/net/core/bpf_sk_storage.c b/net/core/bpf_sk_storage.c
> index 4a86ea34f29e..d43c3d6d0693 100644
> --- a/net/core/bpf_sk_storage.c
> +++ b/net/core/bpf_sk_storage.c
> @@ -678,6 +678,7 @@ struct bpf_iter_seq_sk_storage_map_info {
>  static struct bpf_local_storage_elem *
>  bpf_sk_storage_map_seq_find_next(struct bpf_iter_seq_sk_storage_map_info *info,
>  				 struct bpf_local_storage_elem *prev_selem)
> +	__acquires(RCU) __releases(RCU)
>  {
>  	struct bpf_local_storage *sk_storage;
>  	struct bpf_local_storage_elem *selem;
In the while loop earlier in this function, if I read it correctly,
it is sort of continuing the earlier hlist_for_each_entry_rcu() for the
same bucket, so the hlist_entry_safe() needs to be changed also.
Something like this (uncompiled code):

        while (selem) {
-               selem = hlist_entry_safe(selem->map_node.next,
+               selem = hlist_entry_safe(rcu_dereference(hlist_next_rcu(&selem->map_node)),
                                         struct bpf_local_storage_elem, map_node);
                if (!selem) {
                        /* not found, unlock and go to the next bucket */

> @@ -701,11 +702,11 @@ bpf_sk_storage_map_seq_find_next(struct bpf_iter_seq_sk_storage_map_info *info,
>  		if (!selem) {
>  			/* not found, unlock and go to the next bucket */
>  			b = &smap->buckets[bucket_id++];
> -			raw_spin_unlock_bh(&b->lock);
> +			rcu_read_unlock();
>  			skip_elems = 0;
>  			break;
>  		}
> -		sk_storage = rcu_dereference_raw(selem->local_storage);
> +		sk_storage = rcu_dereference(selem->local_storage);
>  		if (sk_storage) {
>  			info->skip_elems = skip_elems + count;
>  			return selem;