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 >
