On 08/17/2016 08:10 PM, Alexei Starovoitov wrote: > On Wed, Aug 17, 2016 at 09:16:02AM -0700, Eric Dumazet wrote: >> On Wed, 2016-08-17 at 16:00 +0200, Daniel Mack wrote: >> >>> + progp = is_ingress ? &cgrp->bpf_ingress : &cgrp->bpf_egress; >>> + >>> + rcu_read_lock(); >>> + old_prog = rcu_dereference(*progp); >>> + rcu_assign_pointer(*progp, prog); >>> + >>> + if (old_prog) >>> + bpf_prog_put(old_prog); >>> + >>> + rcu_read_unlock(); >> >> >> This is a bogus locking strategy. > > yep. this rcu_lock/unlock won't solve the race between parallel > bpf_prog_attach calls. > please use xchg() similar to bpf_fd_array_map_update_elem() > Then prog pointer will be swapped automically and bpf_prog_put() > will free it via call_rcu. > The reader side in sk_filter_cgroup_bpf() looks correct.
Thanks! I reworked all the bits I got comments on, and fixed some other details as well. I'll wait some days to see what else shakes out of this thread, and then post again. FWIW, the current code can be found here: https://github.com/zonque/linux/commits/cg-bpf-syscall Thanks, Daniel
