From mboxrd@z Thu Jan 1 00:00:00 1970 From: John Fastabend Subject: Re: [PATCH bpf-next v6 1/4] bpf: sockmap, refactor sockmap routines to work with hashmap Date: Wed, 16 May 2018 14:46:33 -0700 Message-ID: References: <1526317219-7752-1-git-send-email-john.fastabend@gmail.com> <1526317219-7752-2-git-send-email-john.fastabend@gmail.com> <954e3b7f-9001-e528-c6ab-f8f69c84cfd8@iogearbox.net> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit Cc: netdev@vger.kernel.org, davem@davemloft.net To: Daniel Borkmann , ast@kernel.org Return-path: Received: from mail-it0-f68.google.com ([209.85.214.68]:36468 "EHLO mail-it0-f68.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751028AbeEPVqp (ORCPT ); Wed, 16 May 2018 17:46:45 -0400 Received: by mail-it0-f68.google.com with SMTP id e20-v6so5958404itc.1 for ; Wed, 16 May 2018 14:46:44 -0700 (PDT) In-Reply-To: <954e3b7f-9001-e528-c6ab-f8f69c84cfd8@iogearbox.net> Content-Language: en-US Sender: netdev-owner@vger.kernel.org List-ID: 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 >