| 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
| ||
|
Message-ID: <20200817235621.ulkqw6mqd2uu647t@kafai-mbp.dhcp.thefacebook.com>
Date: Mon, 17 Aug 2020 16:56:21 -0700
From: Martin KaFai Lau <kafai@...com>
To: KP Singh <kpsingh@...omium.org>
CC: <linux-kernel@...r.kernel.org>, <bpf@...r.kernel.org>,
<linux-security-module@...r.kernel.org>,
Alexei Starovoitov <ast@...nel.org>,
Daniel Borkmann <daniel@...earbox.net>,
Paul Turner <pjt@...gle.com>, Jann Horn <jannh@...gle.com>,
Florent Revest <revest@...omium.org>
Subject: Re: [PATCH bpf-next v8 1/7] A purely mechanical change to split the
renaming from the actual generalization.
On Mon, Aug 03, 2020 at 06:46:49PM +0200, KP Singh wrote:
> From: KP Singh <kpsingh@...gle.com>
>
> Flags/consts:
>
> SK_STORAGE_CREATE_FLAG_MASK BPF_LOCAL_STORAGE_CREATE_FLAG_MASK
> BPF_SK_STORAGE_CACHE_SIZE BPF_LOCAL_STORAGE_CACHE_SIZE
> MAX_VALUE_SIZE BPF_LOCAL_STORAGE_MAX_VALUE_SIZE
>
> Structs:
>
> bucket bpf_local_storage_map_bucket
> bpf_sk_storage_map bpf_local_storage_map
> bpf_sk_storage_data bpf_local_storage_data
> bpf_sk_storage_elem bpf_local_storage_elem
> bpf_sk_storage bpf_local_storage
>
> The "sk" member in bpf_local_storage is also updated to "owner"
> in preparation for changing the type to void * in a subsequent patch.
>
> Functions:
>
> selem_linked_to_sk selem_linked_to_storage
> selem_alloc bpf_selem_alloc
> __selem_unlink_sk bpf_selem_unlink_storage
> __selem_link_sk bpf_selem_link_storage
> selem_unlink_sk __bpf_selem_unlink_storage
> sk_storage_update bpf_local_storage_update
> __sk_storage_lookup bpf_local_storage_lookup
> bpf_sk_storage_map_free bpf_local_storage_map_free
> bpf_sk_storage_map_alloc bpf_local_storage_map_alloc
> bpf_sk_storage_map_alloc_check bpf_local_storage_map_alloc_check
> bpf_sk_storage_map_check_btf bpf_local_storage_map_check_btf
>
[ ... ]
> @@ -147,85 +148,86 @@ static struct bpf_sk_storage_elem *selem_alloc(struct bpf_sk_storage_map *smap,
> * The caller must ensure selem->smap is still valid to be
> * dereferenced for its smap->elem_size and smap->cache_idx.
> */
> -static bool __selem_unlink_sk(struct bpf_sk_storage *sk_storage,
> - struct bpf_sk_storage_elem *selem,
> - bool uncharge_omem)
> +static bool bpf_selem_unlink_storage(struct bpf_local_storage *local_storage,
> + struct bpf_local_storage_elem *selem,
> + bool uncharge_omem)
Please add a "_nolock()" suffix, just to be clear that the unlink_map()
counter part is locked. It could be a follow up later.
> {
> - struct bpf_sk_storage_map *smap;
> - bool free_sk_storage;
> + struct bpf_local_storage_map *smap;
> + bool free_local_storage;
> struct sock *sk;
>
> smap = rcu_dereference(SDATA(selem)->smap);
> - sk = sk_storage->sk;
> + sk = local_storage->owner;
>
> /* All uncharging on sk->sk_omem_alloc must be done first.
> - * sk may be freed once the last selem is unlinked from sk_storage.
> + * sk may be freed once the last selem is unlinked from local_storage.
> */
> if (uncharge_omem)
> atomic_sub(smap->elem_size, &sk->sk_omem_alloc);
>
> - free_sk_storage = hlist_is_singular_node(&selem->snode,
> - &sk_storage->list);
> - if (free_sk_storage) {
> - atomic_sub(sizeof(struct bpf_sk_storage), &sk->sk_omem_alloc);
> - sk_storage->sk = NULL;
> + free_local_storage = hlist_is_singular_node(&selem->snode,
> + &local_storage->list);
> + if (free_local_storage) {
> + atomic_sub(sizeof(struct bpf_local_storage), &sk->sk_omem_alloc);
> + local_storage->owner = NULL;
> /* After this RCU_INIT, sk may be freed and cannot be used */
> RCU_INIT_POINTER(sk->sk_bpf_storage, NULL);
>
> - /* sk_storage is not freed now. sk_storage->lock is
> - * still held and raw_spin_unlock_bh(&sk_storage->lock)
> + /* local_storage is not freed now. local_storage->lock is
> + * still held and raw_spin_unlock_bh(&local_storage->lock)
> * will be done by the caller.
> *
> * Although the unlock will be done under
> * rcu_read_lock(), it is more intutivie to
> - * read if kfree_rcu(sk_storage, rcu) is done
> - * after the raw_spin_unlock_bh(&sk_storage->lock).
> + * read if kfree_rcu(local_storage, rcu) is done
> + * after the raw_spin_unlock_bh(&local_storage->lock).
> *
> - * Hence, a "bool free_sk_storage" is returned
> + * Hence, a "bool free_local_storage" is returned
> * to the caller which then calls the kfree_rcu()
> * after unlock.
> */
> }
> hlist_del_init_rcu(&selem->snode);
> - if (rcu_access_pointer(sk_storage->cache[smap->cache_idx]) ==
> + if (rcu_access_pointer(local_storage->cache[smap->cache_idx]) ==
> SDATA(selem))
> - RCU_INIT_POINTER(sk_storage->cache[smap->cache_idx], NULL);
> + RCU_INIT_POINTER(local_storage->cache[smap->cache_idx], NULL);
>
> kfree_rcu(selem, rcu);
>
> - return free_sk_storage;
> + return free_local_storage;
> }
>
> -static void selem_unlink_sk(struct bpf_sk_storage_elem *selem)
> +static void __bpf_selem_unlink_storage(struct bpf_local_storage_elem *selem)
> {
> - struct bpf_sk_storage *sk_storage;
> - bool free_sk_storage = false;
> + struct bpf_local_storage *local_storage;
> + bool free_local_storage = false;
>
> - if (unlikely(!selem_linked_to_sk(selem)))
> + if (unlikely(!selem_linked_to_storage(selem)))
> /* selem has already been unlinked from sk */
> return;
>
> - sk_storage = rcu_dereference(selem->sk_storage);
> - raw_spin_lock_bh(&sk_storage->lock);
> - if (likely(selem_linked_to_sk(selem)))
> - free_sk_storage = __selem_unlink_sk(sk_storage, selem, true);
> - raw_spin_unlock_bh(&sk_storage->lock);
> + local_storage = rcu_dereference(selem->local_storage);
> + raw_spin_lock_bh(&local_storage->lock);
> + if (likely(selem_linked_to_storage(selem)))
> + free_local_storage =
> + bpf_selem_unlink_storage(local_storage, selem, true);
> + raw_spin_unlock_bh(&local_storage->lock);
>
> - if (free_sk_storage)
> - kfree_rcu(sk_storage, rcu);
> + if (free_local_storage)
> + kfree_rcu(local_storage, rcu);
> }
>
> -static void __selem_link_sk(struct bpf_sk_storage *sk_storage,
> - struct bpf_sk_storage_elem *selem)
> +static void bpf_selem_link_storage(struct bpf_local_storage *local_storage,
> + struct bpf_local_storage_elem *selem)
Same here. bpf_selem_link_storage"_nolock"().
Please tag the Subject line with "bpf:".
Acked-by: Martin KaFai Lau <kafai@...com>
Powered by blists - more mailing lists