Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753612AbdHOSGt (ORCPT ); Tue, 15 Aug 2017 14:06:49 -0400 Received: from dispatch1-us1.ppe-hosted.com ([67.231.154.164]:55725 "EHLO dispatch1-us1.ppe-hosted.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751460AbdHOSGr (ORCPT ); Tue, 15 Aug 2017 14:06:47 -0400 Subject: Re: [PATCH v2 net-next] bpf/verifier: track liveness for pruning To: Daniel Borkmann , , "Alexei Starovoitov" , Alexei Starovoitov References: <3c245ef4-3161-0e5a-3708-6b9b47db01cd@solarflare.com> <5993226A.9010704@iogearbox.net> CC: , , iovisor-dev From: Edward Cree Message-ID: Date: Tue, 15 Aug 2017 19:06:34 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.1.0 MIME-Version: 1.0 In-Reply-To: <5993226A.9010704@iogearbox.net> Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: 7bit X-Originating-IP: [10.17.20.45] X-ClientProxiedBy: ocex03.SolarFlarecom.com (10.20.40.36) To ukex01.SolarFlarecom.com (10.17.10.4) X-TM-AS-Product-Ver: SMEX-11.0.0.1191-8.100.1062-23258.003 X-TM-AS-Result: No--11.375500-0.000000-31 X-TM-AS-User-Approved-Sender: Yes X-TM-AS-User-Blocked-Sender: No X-MDID: 1502820405-4lgzxhvH6MhE Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 1938 Lines: 50 On 15/08/17 17:33, Daniel Borkmann wrote: > On 08/15/2017 03:53 PM, Edward Cree wrote: >> State of a register doesn't matter if it wasn't read in reaching an exit; >> a write screens off all reads downstream of it from all explored_states >> upstream of it. >> This allows us to prune many more branches; here are some processed insn >> counts for some Cilium programs: >> Program before after >> bpf_lb_opt_-DLB_L3.o 6515 3361 >> bpf_lb_opt_-DLB_L4.o 8976 5176 >> bpf_lb_opt_-DUNKNOWN.o 2960 1137 >> bpf_lxc_opt_-DDROP_ALL.o 95412 48537 >> bpf_lxc_opt_-DUNKNOWN.o 141706 78718 >> bpf_netdev.o 24251 17995 >> bpf_overlay.o 10999 9385 >> >> The runtime is also improved; here are 'time' results in ms: >> Program before after >> bpf_lb_opt_-DLB_L3.o 24 6 >> bpf_lb_opt_-DLB_L4.o 26 11 >> bpf_lb_opt_-DUNKNOWN.o 11 2 >> bpf_lxc_opt_-DDROP_ALL.o 1288 139 >> bpf_lxc_opt_-DUNKNOWN.o 1768 234 >> bpf_netdev.o 62 31 >> bpf_overlay.o 15 13 >> >> Signed-off-by: Edward Cree > [...] >> @@ -1639,10 +1675,13 @@ static int check_call(struct bpf_verifier_env *env, int func_id, int insn_idx) >> } >> >> /* reset caller saved regs */ >> - for (i = 0; i < CALLER_SAVED_REGS; i++) >> + for (i = 0; i < CALLER_SAVED_REGS; i++) { >> mark_reg_not_init(regs, caller_saved[i]); >> + check_reg_arg(env, i, DST_OP_NO_MARK); > > Ah, I oversaw that earlier, this needs to be: s/i/caller_saved[i]/ So it does. Of course testing didn't spot this, because caller_saved[i] == i currently. >> + } >> >> /* update return register */ >> + check_reg_arg(env, BPF_REG_0, DST_OP_NO_MARK); > > We could leave this for clarity, but ... Yeah, I'll replace it with a comment, like the other one. Thanks for review :) -Ed