Re: [PATCH bpf-next v6 1/4] bpf: sockmap, refactor sockmap routines to work with hashmap

[John Fastabend <john.fastabend@gmail.com>]

On 05/15/2018 12:19 PM, Daniel Borkmann wrote:
> On 05/14/2018 07:00 PM, John Fastabend wrote:
> [...]


[...]

> 
> As you say in the comment above the function wrt locking notes that the
> __sock_map_ctx_update_elem() can be called concurrently.
> 
>   All operations operate on sock_map using cmpxchg and xchg operations to ensure we
>   do not get stale references. Any reads into the map must be done with READ_ONCE()
>   because of this.
> 
> You initially use the READ_ONCE() on the verdict/parse/tx_msg, but later on when
> grabbing the reference you use again progs->bpf_verdict/bpf_parse/bpf_tx_msg which
> would potentially refetch it, but if updates would happen concurrently e.g. to the
> three progs, they could be NULL in the mean-time, no? bpf_prog_inc_not_zero() would
> then crash. Why are not the ones used that you fetched previously via READ_ONCE()
> for taking the ref?

Nice catch. We should use the reference fetched by READ_ONCE in all cases.

> 
> The second question I had is that verdict/parse/tx_msg are updated independently
> from each other and each could be NULL or non-NULL. What if, say, parse is NULL
> and verdict as well as tx_msg is non-NULL and the bpf_prog_inc_not_zero() on the
> tx_msg prog fails. Doesn't this cause a use-after-free since a ref on verdict wasn't
> taken earlier but the bpf_prog_put() will cause accidental misbalance/free of the
> progs?

Also good catch. I'll send patches for both now. Thanks.

> 
> It would probably help to clarify the locking comment a bit more if indeed the
> above should be okay as is.
> 
> Thanks,
> Daniel
>