From mboxrd@z Thu Jan 1 00:00:00 1970 From: John Fastabend Subject: Re: [bpf PATCH 2/2] bpf: parse and verdict prog attach may race with bpf map update Date: Thu, 17 May 2018 14:06:22 -0700 Message-ID: References: <20180516214651.6664.62408.stgit@john-Precision-Tower-5810> <20180516214656.6664.34077.stgit@john-Precision-Tower-5810> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit Cc: netdev@vger.kernel.org To: Daniel Borkmann , ast@kernel.org Return-path: Received: from mail-it0-f68.google.com ([209.85.214.68]:40375 "EHLO mail-it0-f68.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751280AbeEQVGm (ORCPT ); Thu, 17 May 2018 17:06:42 -0400 Received: by mail-it0-f68.google.com with SMTP id j186-v6so10606953ita.5 for ; Thu, 17 May 2018 14:06:41 -0700 (PDT) In-Reply-To: Content-Language: en-US Sender: netdev-owner@vger.kernel.org List-ID: 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 >> Signed-off-by: John Fastabend >> --- [...] > 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.