Re: [bpf PATCH 2/2] bpf: parse and verdict prog attach may race with bpf map update

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

On 05/17/2018 01:31 PM, Daniel Borkmann wrote:
> On 05/16/2018 11:46 PM, John Fastabend wrote:
>> In the sockmap design BPF programs (SK_SKB_STREAM_PARSER and
>> SK_SKB_STREAM_VERDICT) are attached to the sockmap map type and when
>> a sock is added to the map the programs are used by the socket.
>> However, sockmap updates from both userspace and BPF programs can
>> happen concurrently with the attach and detach of these programs.
>>
>> To resolve this we use the bpf_prog_inc_not_zero and a READ_ONCE()
>> primitive to ensure the program pointer is not refeched and
>> possibly NULL'd before the refcnt increment. This happens inside
>> a RCU critical section so although the pointer reference in the map
>> object may be NULL (by a concurrent detach operation) the reference
>> from READ_ONCE will not be free'd until after grace period. This
>> ensures the object returned by READ_ONCE() is valid through the
>> RCU criticl section and safe to use as long as we "know" it may
>> be free'd shortly.
>>
>> Daniel spotted a case in the sock update API where instead of using
>> the READ_ONCE() program reference we used the pointer from the
>> original map, stab->bpf_{verdict|parse}. The problem with this is
>> the logic checks the object returned from the READ_ONCE() is not
>> NULL and then tries to reference the object again but using the
>> above map pointer, which may have already been NULL'd by a parallel
>> detach operation. If this happened bpf_porg_inc_not_zero could
>> dereference a NULL pointer.
>>
>> Fix this by using variable returned by READ_ONCE() that is checked
>> for NULL.
>>
>> Fixes: 2f857d04601a ("bpf: sockmap, remove STRPARSER map_flags and add multi-map support")
>> Reported-by: Daniel Borkmann <daniel@iogearbox.net>
>> Signed-off-by: John Fastabend <john.fastabend@gmail.com>
>> ---

[...]

> Isn't the same sort of behavior also possible with the bpf_prog_inc_not_zero(stab->bpf_tx_msg)?
> Meaning, we now have verdict and parse covered with the patch, but the original tx_msg we
> fetched earlier via READ_ONCE() where same would apply not (yet)?
> 

Yes, will send a v2 and fix both cases in one shot.