Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754295AbdHUVAX (ORCPT ); Mon, 21 Aug 2017 17:00:23 -0400 Received: from www62.your-server.de ([213.133.104.62]:46435 "EHLO www62.your-server.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753648AbdHUVAU (ORCPT ); Mon, 21 Aug 2017 17:00:20 -0400 Message-ID: <599B49DB.6010008@iogearbox.net> Date: Mon, 21 Aug 2017 23:00:11 +0200 From: Daniel Borkmann User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.7.0 MIME-Version: 1.0 To: Edward Cree , Alexei Starovoitov , davem@davemloft.net, Alexei Starovoitov CC: netdev@vger.kernel.org, linux-kernel@vger.kernel.org, iovisor-dev Subject: Re: [PATCH v3 net-next] bpf/verifier: track liveness for pruning References: <89ff34f7-84ee-0e0a-3766-5b4d046189bf@fb.com> <5d4e12aa-6861-a176-a8cf-a766bbca0a7a@fb.com> <599B4231.3080405@iogearbox.net> <128a84cd-234d-f505-95e2-7561db974981@solarflare.com> In-Reply-To: <128a84cd-234d-f505-95e2-7561db974981@solarflare.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit X-Authenticated-Sender: daniel@iogearbox.net Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 2260 Lines: 41 On 08/21/2017 10:44 PM, Edward Cree wrote: > On 21/08/17 21:27, Daniel Borkmann wrote: >> On 08/21/2017 08:36 PM, Edward Cree wrote: >>> On 19/08/17 00:37, Alexei Starovoitov wrote: >> [...] >>> I'm tempted to just rip out env->varlen_map_value_access and always check >>> the whole thing, because honestly I don't know what it was meant to do >>> originally or how it can ever do any useful pruning. While drastic, it >>> does cause your test case to pass. >> >> Original intention from 484611357c19 ("bpf: allow access into map >> value arrays") was that it wouldn't potentially make pruning worse >> if PTR_TO_MAP_VALUE_ADJ was not used, meaning that we wouldn't need >> to take reg state's min_value and max_value into account for state >> checking; this was basically due to min_value / max_value is being >> adjusted/tracked on every alu/jmp ops for involved regs (e.g. >> adjust_reg_min_max_vals() and others that mangle them) even if we >> have the case that no actual dynamic map access is used throughout >> the program. To give an example on net tree, the bpf_lxc.o prog's >> section increases from 36,386 to 68,226 when env->varlen_map_value_access >> is always true, so it does have an effect. Did you do some checks >> on this on net-next? > I tested with the cilium progs and saw no change in insn count. I > suspect that for the normal case I already killed this optimisation > when I did my unification patch, it was previously about ignoring > min/max values on all regs (including scalars), whereas on net-next > it only ignores them on map_value pointers; in practice this is > useless because we tend to still have the offset scalar sitting in > a register somewhere. (Come to think of it, this may have been > behind a large chunk of the #insn increase that my patches caused.) Yeah, this would seem plausible. > Since we use umax_value in find_good_pkt_pointers() now (to check > against MAX_PACKET_OFF and ensure our reg->range is really ok), we > can't just stop caring about all min/max values just because we > haven't done any variable map accesses. > I don't see a way around this. Agree, was thinking the same. If there's not really a regression in terms of complexity, then lets kill the flag.