From mboxrd@z Thu Jan 1 00:00:00 1970 From: Edward Cree Subject: Re: [RFC PATCH v3 bpf-next 2/5] bpf/verifier: rewrite subprog boundary detection Date: Tue, 1 May 2018 21:40:16 +0100 Message-ID: <2a368af7-4b2c-545f-0e85-826c93d4f58b@solarflare.com> References: <99e70dfe-66a1-911a-6616-60eae4ddc689@solarflare.com> <20180417234826.egydr2sg2rewzvyu@ast-mbp> Mime-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: 8bit Cc: Daniel Borkmann , To: Alexei Starovoitov Return-path: Received: from dispatch1-us1.ppe-hosted.com ([148.163.129.52]:44676 "EHLO dispatch1-us1.ppe-hosted.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750755AbeEAUkW (ORCPT ); Tue, 1 May 2018 16:40:22 -0400 In-Reply-To: <20180417234826.egydr2sg2rewzvyu@ast-mbp> Content-Language: en-GB Sender: netdev-owner@vger.kernel.org List-ID: On 18/04/18 00:48, Alexei Starovoitov wrote: > as I was saying before this is no go. > subprogno is meaningless in the hierarchy of: prog -> func -> bb -> insn > Soon bpf will have libraries and this field would need to become > a pointer back to bb or func structure creating unnecessary circular dependency. I'm afraid I don't follow.  Why can't func numbers (and later, bb numbers)  be per-prog?  When verifier is linking multiple progs together it will  necessarily have the subprog-info for each prog, and when making cross-  prog calls it'll have to already know which prog it's calling into; I  don't see any reason why the index into a prog's subprog_info array  should become "meaningless" in such a setup. Besides, subprogno is how the rest of the verifier currently identifies a  func, and in the absence of any indication of how anything different  will be implemented, that's what an incremental patch has to work with. If you're worried about the SPOT violation from having both  aux->subprogno and subprog_info->start... well, we could actually get  rid of the latter!  Uses of it are: * carving up insn arrays in jit_subprogs().  Could be done based on   aux->subprogno instead (v1 of this series did that) * checking CALL destination is at start of function.  That could be done   by putting a flag in the aux_data to mark "this insn is at the start of   its subprog".  Doesn't even need to increase memory usage: it could be   done by ORing a flag (0x8000, say) into aux->subprogno; or we could   replace 'bool seen;' with 'u8 flags;' again with no extra memory used. * a few verbose() messages. That would have another nice consequence, in that adjust_subprog_starts()  could go away - another code simplification resulting from use of the  right data structures. Btw, sorry for delay in responding; got bogged down in some sfc driver  work. -Ed