From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-3.1 required=3.0 tests=DKIM_SIGNED,DKIM_VALID, DKIM_VALID_AU,FREEMAIL_FORGED_FROMDOMAIN,FREEMAIL_FROM, HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI,SPF_PASS,USER_AGENT_NEOMUTT autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id A6D3BC43387 for ; Thu, 17 Jan 2019 01:16:40 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id 72D07205C9 for ; Thu, 17 Jan 2019 01:16:40 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (2048-bit key) header.d=gmail.com header.i=@gmail.com header.b="F5wj4coH" Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1727449AbfAQBQj (ORCPT ); Wed, 16 Jan 2019 20:16:39 -0500 Received: from mail-pf1-f193.google.com ([209.85.210.193]:44643 "EHLO mail-pf1-f193.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1725864AbfAQBQf (ORCPT ); Wed, 16 Jan 2019 20:16:35 -0500 Received: by mail-pf1-f193.google.com with SMTP id u6so3930692pfh.11 for ; Wed, 16 Jan 2019 17:16:34 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to:user-agent; bh=AvTZp0E8XqJJEC1ohxdoEdQGUtss8GQho4Qz83CuwPE=; b=F5wj4coHw1Kx9NhK2jnyw5a06kmnjbBzxDOBhBcFF0bWQJgMHTt6ZpsbPh6ZgbK57j SYhb/8QwCHxrHE7pvosUjl8jLvPEUCoS7Mg7JdoXkoDusjagmQDH0uosVX2tS5P7WGDT K/H9jvwcuwm9SWz8zBGIVdIn+3paz1yHCpTs5P8Q9f1i15Lx8YzWbhhetmXItTt/eQk6 FaDCezBhjK51dBnAAVx+SZ1NDA/YMd7PCgL5oWITRzsgSB8+8FLJdzrtHEjwCm9bny1D d5la72fs/27dMbaTP7ADiTUOt/ERXEIifzEC2ub71Z+H+1d0l3vnzJpAMmUfCxN28D34 PdNA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to:user-agent; bh=AvTZp0E8XqJJEC1ohxdoEdQGUtss8GQho4Qz83CuwPE=; b=fqRzaK0jY48aFXgqcr0CSVk7mdfVArvWYpY6wHu/hSEEwoJRcF/0GXTIdtI9/kNEMp bH4a75FPUsMizHJPGULY/Z1HyIqKIX7unNynF9+AcwgTXwkQaPRXhOfA9VqArL0Y15GY xYHU4JVBUdxHjk1Ls9zZIOJEYjBKEWywv1cAqvEn1nOJq/RdtWj6tYYd1qN+BX56LRVe XPLtWLI2j/CPR//W5cXube5fEg+QzEW0aCEz2AC8OzoKJcocefETXFBQi7bema6qLB4z WFQpc9AfDTDQFG/5JkP1xyQ4ohcZuXJa7d/4zSe6s2Uiy18YneCwUlhKQw2B1JiOw9Qw iyDQ== X-Gm-Message-State: AJcUukdwxXW46g4d7Wq5cPFeIs290tvJKMxB/pJdIsp8hfehUw1IF4ns eJRIbhxjZdDIZQgIIGN9u3c= X-Google-Smtp-Source: ALg8bN4RNyOL6ze8OXnCIes31zvhMOFSvEo4w6GE5FI3PpkvKyf1LNlYSuxrmK8mEnehDfuRE7+G2g== X-Received: by 2002:a62:68c5:: with SMTP id d188mr13204537pfc.194.1547687793910; Wed, 16 Jan 2019 17:16:33 -0800 (PST) Received: from ast-mbp ([2620:10d:c090:200::6:30ae]) by smtp.gmail.com with ESMTPSA id u126sm60555pgb.2.2019.01.16.17.16.32 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Wed, 16 Jan 2019 17:16:32 -0800 (PST) Date: Wed, 16 Jan 2019 17:16:31 -0800 From: Alexei Starovoitov To: Daniel Borkmann Cc: Alexei Starovoitov , davem@davemloft.net, jakub.kicinski@netronome.com, netdev@vger.kernel.org, kernel-team@fb.com Subject: Re: [PATCH bpf-next 1/9] bpf: introduce bpf_spin_lock Message-ID: <20190117011629.efxp7abj4bpf5yco@ast-mbp> References: <20190116050830.1881316-1-ast@kernel.org> <20190116050830.1881316-2-ast@kernel.org> <20190116233029.xdaddrfehfjk4hrx@ast-mbp> <3367abf6-c6a3-2c2e-9fde-7de923f898d2@iogearbox.net> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <3367abf6-c6a3-2c2e-9fde-7de923f898d2@iogearbox.net> User-Agent: NeoMutt/20180223 Sender: netdev-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: netdev@vger.kernel.org On Thu, Jan 17, 2019 at 01:21:32AM +0100, Daniel Borkmann wrote: > On 01/17/2019 12:30 AM, Alexei Starovoitov wrote: > > On Thu, Jan 17, 2019 at 12:16:44AM +0100, Daniel Borkmann wrote: > >> On 01/16/2019 11:48 PM, Daniel Borkmann wrote: > >>> On 01/16/2019 06:08 AM, Alexei Starovoitov wrote: > >> [...] > >>>> @@ -6096,6 +6226,11 @@ static int do_check(struct bpf_verifier_env *env) > >>>> return -EINVAL; > >>>> } > >>>> > >>>> + if (env->cur_state->active_spin_lock) { > >>>> + verbose(env, "bpf_spin_unlock is missing\n"); > >>>> + return -EINVAL; > >>>> + } > >>>> + > >>>> if (state->curframe) { > >>>> /* exit from nested function */ > >>>> env->prev_insn_idx = env->insn_idx; > >>> > >>> I think if I'm not mistaken there should still be a possibility for causing a > >>> deadlock, namely if in the middle of the critical section I'm using an LD_ABS > >>> or LD_IND instruction with oob index such that I cause an implicit return 0 > >>> while lock is held. At least I don't see this being caught, probably also for > >>> such case a test_verifier snippet would be good. > >>> > >>> Wouldn't we also need to mark queued spinlock functions as notrace such that > >>> e.g. from kprobe one cannot attach to these causing a deadlock? > >> > >> I think there may be another problem: haven't verified, but it might be possible > >> at least from reading the code that I have two programs which share a common > >> array/hash with spin_lock in BTF provided. Program A is properly using spin_lock > >> as in one of your examples. Program B is using map in map with inner map being > >> that same map using spin_lock. When we return that fake inner_map_meta as > >> reg->map_ptr then we can bypass any read/write restrictions into spin_lock area > >> which is normally prevented by verifier. Meaning, map in map needs to be made > >> aware of spin_lock case as well. > > > > 2nd great catch. thanks! > > Indeed inner_map_meta doesn't preserve all the fields from struct bpf_map. > > It seems long term we'll be able to support spin_lock in inner map too, > > but for now I'll disable it. > > There's also one more potential issue in pruning I _think_. In regsafe() we > make the basic assumption that for PTR_TO_MAP_VALUE id has been zeroed which > is true up to here, and as such we prune state not taking id into account. > The only other case we have is PTR_TO_SOCKET{,_OR_NULL} which only allows > for exact matches. Potentially there could be a case where you have two map > pointers from different branches but with same basic map properties read/ > writing map data, and in first run for PTR_TO_MAP_VALUE w/o spin_lock path > it was considered safe such that we would get a match in regsafe() as well > and could potentially prune the access? I guess definitely worth adding such > test case to test_verifier to make sure. Hmm. Something to think about for sure. I belive if (old->active_spin_lock != cur->active_spin_lock) check protects from all cases where spin_lock-ed paths are mixed with non-spin. Like going through non-locked ld/st of map_value in the first pass through the prog and then jumping half way into that pass after taking spin_lock to trigger regsafe(). I cannot quite see how to construct such test without triggering old->active_spin_lock != cur->active_spin_lock before reaching regsafe(). But I will keep thinking. If you have more concrete description for the test please suggest.