netdev - Re: [bpf PATCH v4 3/4] bpf: sockhash fix omitted bucket lock in sock

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:   Fri, 29 Jun 2018 15:45:15 +0200
From:   Daniel Borkmann <daniel@...earbox.net>
To:     John Fastabend <john.fastabend@...il.com>, ast@...nel.org,
        kafai@...com
Cc:     netdev@...r.kernel.org
Subject: Re: [bpf PATCH v4 3/4] bpf: sockhash fix omitted bucket lock in
 sock_close

On 06/25/2018 05:34 PM, John Fastabend wrote:
[...]
> @@ -2302,9 +2347,12 @@ static int sock_hash_ctx_update_elem(struct bpf_sock_ops_kern *skops,
>  		goto bucket_err;
>  	}
>  
> -	e->hash_link = l_new;
> -	e->htab = container_of(map, struct bpf_htab, map);
> +	rcu_assign_pointer(e->hash_link, l_new);
> +	rcu_assign_pointer(e->htab,
> +			   container_of(map, struct bpf_htab, map));
> +	write_lock_bh(&sock->sk_callback_lock);
>  	list_add_tail(&e->list, &psock->maps);
> +	write_unlock_bh(&sock->sk_callback_lock);
>  
>  	/* add new element to the head of the list, so that
>  	 * concurrent search will find it before old elem
> @@ -2314,8 +2362,10 @@ static int sock_hash_ctx_update_elem(struct bpf_sock_ops_kern *skops,
>  		psock = smap_psock_sk(l_old->sk);
>  
>  		hlist_del_rcu(&l_old->hash_node);
> +		write_lock_bh(&l_old->sk->sk_callback_lock);
>  		smap_list_hash_remove(psock, l_old);
>  		smap_release_sock(psock, l_old->sk);
> +		write_unlock_bh(&l_old->sk->sk_callback_lock);
>  		free_htab_elem(htab, l_old);
>  	}
>  	raw_spin_unlock_bh(&b->lock);

Rather a question for clarification that came up while staring at this part
in sock_hash_ctx_update_elem():

In the 'err' label, wouldn't we also need the sk_callback_lock given we access
a potential psock, and modify its state (refcnt) would this be needed as well,
or are we good here since we're under RCU context anyway? Hmm, if latter is
indeed the case, then I'm wondering why we would need the above /sock/'s
sk_callback_lock for modifying list handling inside the psock and couldn't use
some locking that is only in scope of the actual struct smap_psock?

My other question is, if someone calls the update helper with BPF_NOEXIST which
we seem to support with bailing out via -EEXIST, and if there's currently a
psock attached already to the sock, then we drop this one off from the socket?
Is that correct / expected?

Thanks,
Daniel