| 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
| ||
|
Date: Thu, 8 Aug 2019 00:18:59 +0000
From: Yonghong Song <yhs@...com>
To: Stanislav Fomichev <sdf@...ichev.me>
CC: Stanislav Fomichev <sdf@...gle.com>,
"netdev@...r.kernel.org" <netdev@...r.kernel.org>,
"bpf@...r.kernel.org" <bpf@...r.kernel.org>,
"davem@...emloft.net" <davem@...emloft.net>,
"ast@...nel.org" <ast@...nel.org>,
"daniel@...earbox.net" <daniel@...earbox.net>,
Martin Lau <kafai@...com>
Subject: Re: [PATCH bpf-next 1/3] bpf: support cloning sk storage on accept()
On 8/7/19 5:05 PM, Stanislav Fomichev wrote:
> On 08/07, Yonghong Song wrote:
>>
>>
>> On 8/7/19 8:47 AM, Stanislav Fomichev wrote:
>>> Add new helper bpf_sk_storage_clone which optionally clones sk storage
>>> and call it from bpf_sk_storage_clone. Reuse the gap in
>>> bpf_sk_storage_elem to store clone/non-clone flag.
>>>
>>> Cc: Martin KaFai Lau <kafai@...com>
>>> Signed-off-by: Stanislav Fomichev <sdf@...gle.com>
>>
>> I tried to see whether I can find any missing race conditions in
>> the code but I failed. So except a minor comments below,
> Thanks for a review!
>
>> Acked-by: Yonghong Song <yhs@...com>
>>
>>> ---
>>> include/net/bpf_sk_storage.h | 10 ++++
>>> include/uapi/linux/bpf.h | 1 +
>>> net/core/bpf_sk_storage.c | 102 +++++++++++++++++++++++++++++++++--
>>> net/core/sock.c | 9 ++--
>>> 4 files changed, 115 insertions(+), 7 deletions(-)
>>>
>>> diff --git a/include/net/bpf_sk_storage.h b/include/net/bpf_sk_storage.h
>>> index b9dcb02e756b..8e4f831d2e52 100644
>>> --- a/include/net/bpf_sk_storage.h
>>> +++ b/include/net/bpf_sk_storage.h
>>> @@ -10,4 +10,14 @@ void bpf_sk_storage_free(struct sock *sk);
>>> extern const struct bpf_func_proto bpf_sk_storage_get_proto;
>>> extern const struct bpf_func_proto bpf_sk_storage_delete_proto;
>>>
>>> +#ifdef CONFIG_BPF_SYSCALL
>>> +int bpf_sk_storage_clone(const struct sock *sk, struct sock *newsk);
>>> +#else
>>> +static inline int bpf_sk_storage_clone(const struct sock *sk,
>>> + struct sock *newsk)
>>> +{
>>> + return 0;
>>> +}
>>> +#endif
>>> +
>>> #endif /* _BPF_SK_STORAGE_H */
>>> diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
>>> index 4393bd4b2419..00459ca4c8cf 100644
>>> --- a/include/uapi/linux/bpf.h
>>> +++ b/include/uapi/linux/bpf.h
>>> @@ -2931,6 +2931,7 @@ enum bpf_func_id {
>>>
>>> /* BPF_FUNC_sk_storage_get flags */
>>> #define BPF_SK_STORAGE_GET_F_CREATE (1ULL << 0)
>>> +#define BPF_SK_STORAGE_GET_F_CLONE (1ULL << 1)
>>>
>>> /* Mode for BPF_FUNC_skb_adjust_room helper. */
>>> enum bpf_adj_room_mode {
>>> diff --git a/net/core/bpf_sk_storage.c b/net/core/bpf_sk_storage.c
>>> index 94c7f77ecb6b..b6dea67965bc 100644
>>> --- a/net/core/bpf_sk_storage.c
>>> +++ b/net/core/bpf_sk_storage.c
>>> @@ -12,6 +12,9 @@
>>>
>>> static atomic_t cache_idx;
>>>
>>> +#define BPF_SK_STORAGE_GET_F_MASK (BPF_SK_STORAGE_GET_F_CREATE | \
>>> + BPF_SK_STORAGE_GET_F_CLONE)
>>> +
>>> struct bucket {
>>> struct hlist_head list;
>>> raw_spinlock_t lock;
>>> @@ -66,7 +69,8 @@ struct bpf_sk_storage_elem {
>>> struct hlist_node snode; /* Linked to bpf_sk_storage */
>>> struct bpf_sk_storage __rcu *sk_storage;
>>> struct rcu_head rcu;
>>> - /* 8 bytes hole */
>>> + u8 clone:1;
>>> + /* 7 bytes hole */
>>> /* The data is stored in aother cacheline to minimize
>>> * the number of cachelines access during a cache hit.
>>> */
>>> @@ -509,7 +513,7 @@ static int sk_storage_delete(struct sock *sk, struct bpf_map *map)
>>> return 0;
>>> }
>>>
>>> -/* Called by __sk_destruct() */
>>> +/* Called by __sk_destruct() & bpf_sk_storage_clone() */
>>> void bpf_sk_storage_free(struct sock *sk)
>>> {
>>> struct bpf_sk_storage_elem *selem;
>>> @@ -739,19 +743,106 @@ static int bpf_fd_sk_storage_delete_elem(struct bpf_map *map, void *key)
>>> return err;
>>> }
>>>
>>> +static struct bpf_sk_storage_elem *
>>> +bpf_sk_storage_clone_elem(struct sock *newsk,
>>> + struct bpf_sk_storage_map *smap,
>>> + struct bpf_sk_storage_elem *selem)
>>> +{
>>> + struct bpf_sk_storage_elem *copy_selem;
>>> +
>>> + copy_selem = selem_alloc(smap, newsk, NULL, true);
>>> + if (!copy_selem)
>>> + return ERR_PTR(-ENOMEM);
>>> +
>>> + if (map_value_has_spin_lock(&smap->map))
>>> + copy_map_value_locked(&smap->map, SDATA(copy_selem)->data,
>>> + SDATA(selem)->data, true);
>>> + else
>>> + copy_map_value(&smap->map, SDATA(copy_selem)->data,
>>> + SDATA(selem)->data);
>>> +
>>> + return copy_selem;
>>> +}
>>> +
>>> +int bpf_sk_storage_clone(const struct sock *sk, struct sock *newsk)
>>> +{
>>> + struct bpf_sk_storage *new_sk_storage = NULL;
>>> + struct bpf_sk_storage *sk_storage;
>>> + struct bpf_sk_storage_elem *selem;
>>> + int ret;
>>> +
>>> + RCU_INIT_POINTER(newsk->sk_bpf_storage, NULL);
>>> +
>>> + rcu_read_lock();
>>> + sk_storage = rcu_dereference(sk->sk_bpf_storage);
>>> +
>>> + if (!sk_storage || hlist_empty(&sk_storage->list))
>>> + goto out;
>>> +
>>> + hlist_for_each_entry_rcu(selem, &sk_storage->list, snode) {
>>> + struct bpf_sk_storage_map *smap;
>>> + struct bpf_sk_storage_elem *copy_selem;
>>> +
>>> + if (!selem->clone)
>>> + continue;
>>> +
>>> + smap = rcu_dereference(SDATA(selem)->smap);
>>> + if (!smap)
>>> + continue;
>>> +
>>> + copy_selem = bpf_sk_storage_clone_elem(newsk, smap, selem);
>>> + if (IS_ERR(copy_selem)) {
>>> + ret = PTR_ERR(copy_selem);
>>> + goto err;
>>> + }
>>> +
>>> + if (!new_sk_storage) {
>>> + ret = sk_storage_alloc(newsk, smap, copy_selem);
>>> + if (ret) {
>>> + kfree(copy_selem);
>>> + atomic_sub(smap->elem_size,
>>> + &newsk->sk_omem_alloc);
>>> + goto err;
>>> + }
>>> +
>>> + new_sk_storage = rcu_dereference(copy_selem->sk_storage);
>>> + continue;
>>> + }
>>> +
>>> + raw_spin_lock_bh(&new_sk_storage->lock);
>>> + selem_link_map(smap, copy_selem);
>>> + __selem_link_sk(new_sk_storage, copy_selem);
>>> + raw_spin_unlock_bh(&new_sk_storage->lock);
>>
>> Considering in this particular case, new socket is not visible to
>> outside world yet (both kernel and user space), map_delete/map_update
>> operations are not applicable in this situation, so
>> the above raw_spin_lock_bh() probably not needed.
> I agree, it's doing nothing, but __selem_link_sk has the following comment:
> /* sk_storage->lock must be held and sk_storage->list cannot be empty */
>
> Just wanted to keep that invariant for this call site as well (in case
> we add some lockdep enforcement or smth else). WDYT?
Agree. Let us keep the locking to be consistent with other uses in
the same file. This is not the critical path.
Powered by blists - more mailing lists