From mboxrd@z Thu Jan 1 00:00:00 1970 From: Daniel Borkmann Subject: Re: [PATCHv6 net-next 04/15] bpf: don't (ab)use instructions to store state Date: Mon, 19 Sep 2016 23:44:50 +0200 Message-ID: <57E05C52.4060208@iogearbox.net> References: <1474211365-20088-1-git-send-email-jakub.kicinski@netronome.com> <1474211365-20088-5-git-send-email-jakub.kicinski@netronome.com> <57E05295.4010904@iogearbox.net> <20160919223621.363c8ac5@jkicinski-Precision-T1700> Mime-Version: 1.0 Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit Cc: netdev@vger.kernel.org, ast@kernel.org, kubakici@wp.pl To: Jakub Kicinski Return-path: Received: from www62.your-server.de ([213.133.104.62]:45936 "EHLO www62.your-server.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S932354AbcISVox (ORCPT ); Mon, 19 Sep 2016 17:44:53 -0400 In-Reply-To: <20160919223621.363c8ac5@jkicinski-Precision-T1700> Sender: netdev-owner@vger.kernel.org List-ID: On 09/19/2016 11:36 PM, Jakub Kicinski wrote: > On Mon, 19 Sep 2016 23:03:17 +0200, Daniel Borkmann wrote: >> On 09/18/2016 05:09 PM, Jakub Kicinski wrote: >>> Storing state in reserved fields of instructions makes >>> it impossible to run verifier on programs already >>> marked as read-only. Allocate and use an array of >>> per-instruction state instead. >>> >>> While touching the error path rename and move existing >>> jump target. >>> >>> Suggested-by: Alexei Starovoitov >>> Signed-off-by: Jakub Kicinski >>> Acked-by: Alexei Starovoitov >>> Acked-by: Daniel Borkmann >> >> I believe there's still an issue here. Could you please double check >> and confirm? >> >> I rebased my locally pending stuff on top of your set and suddenly my >> test case breaks. So I did a bisect and it pointed me to this commit >> eventually. >> >> [...] >>> @@ -2697,11 +2706,8 @@ static int convert_ctx_accesses(struct verifier_env *env) >>> else >>> continue; >>> >>> - if (insn->imm != PTR_TO_CTX) { >>> - /* clear internal mark */ >>> - insn->imm = 0; >>> + if (env->insn_aux_data[i].ptr_type != PTR_TO_CTX) >>> continue; >>> - } >>> >>> cnt = env->prog->aux->ops-> >>> convert_ctx_access(type, insn->dst_reg, insn->src_reg, >> >> Looking at the code, I believe the issue is in above snippet. In the >> convert_ctx_accesses() rewrite loop, each time we bpf_patch_insn_single() >> a program, the program can grow in size (due to __sk_buff access rewrite, >> for example). After rewrite, we do 'i += insn_delta' for adjustment to >> process next insn. >> >> However, env->insn_aux_data is alloced under the assumption that the >> very initial, pre-verification prog->len doesn't change, right? So in >> the above conversion access to env->insn_aux_data[i].ptr_type is off, >> since after rewrites, corresponding mappings to ptr_type might not be >> related anymore. >> >> I noticed this with direct packet access where suddenly the data vs >> data_end test failed and contained some "semi-random" value always >> bailing out for me. > > You are correct. Should I respin or would you like to post your set? :) Heh, if you don't mind I would go ahead tonight, the conflict at two spots when exposing verifier is really minor turns out. Are you okay with this? What's the plan wrt env->insn_aux_data? Realloc plus rewrite of the array, or do you see a more straight forward solution?