On 07/03/2018 07:40 AM, Daniel Borkmann wrote:
> On 06/30/2018 03:51 PM, John Fastabend wrote:
>> The current code, in the error path of sock_hash_ctx_update_elem,
>> checks if the sock has a psock in the user data and if so decrements
>> the reference count of the psock. However, if the error happens early
>> in the error path we may have never incremented the psock reference
>> count and if the psock exists because the sock is in another map then
>> we may inadvertently decrement the reference count.
>>
>> Fix this by making the error path only call smap_release_sock if the
>> error happens after the increment.
>>
>> Reported-by: [email protected]
>> Fixes: 81110384441a ("bpf: sockmap, add hash map support")
>> Signed-off-by: John Fastabend <[email protected]>
>> ---
[...]
>> @@ -2324,7 +2324,12 @@ static int sock_hash_ctx_update_elem(struct
>> bpf_sock_ops_kern *skops,
>> if (err)
>> goto err;
>>
>> - /* bpf_map_update_elem() can be called in_irq() */
>> + psock = smap_psock_sk(sock);
>> + if (unlikely(!psock)) {
>> + err = -EINVAL;
>> + goto err;
>> + }
>
> Is an error even possible at this point? If __sock_map_ctx_update_elem()
> succeeds,
> we either allocated and linked a new psock to the sock or we inc'ed the
> existing
> one's refcount. From my reading it seems we should always succeed the
> subsequent
> smap_psock_sk(). If we would have failed here in between it would mean we'd
> have
> a refcount imbalance somewhere?
>
Its not possible will replace with a comment. Thanks.
