From mboxrd@z Thu Jan 1 00:00:00 1970 From: Jiong Wang Subject: Re: [oss-drivers] Re: [PATCH bpf] bpf: verifier: make sure callees don't prune with caller differences Date: Thu, 13 Dec 2018 10:52:34 +0000 Message-ID: <87k1kdu2a5.fsf@netronome.com> References: <20181210193512.20463-1-jakub.kicinski@netronome.com> <64af6c24-d21a-d149-af48-3b7eaf6cdb25@solarflare.com> <20181212160210.45b2b1ff@cakuba.netronome.com> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8bit Cc: Edward Cree , alexei.starovoitov@gmail.com, daniel@iogearbox.net, oss-drivers@netronome.com, netdev@vger.kernel.org To: Jakub Kicinski Return-path: Received: from mail-wr1-f66.google.com ([209.85.221.66]:39047 "EHLO mail-wr1-f66.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1727455AbeLMKwj (ORCPT ); Thu, 13 Dec 2018 05:52:39 -0500 Received: by mail-wr1-f66.google.com with SMTP id t27so1500666wra.6 for ; Thu, 13 Dec 2018 02:52:37 -0800 (PST) In-reply-to: <20181212160210.45b2b1ff@cakuba.netronome.com> Sender: netdev-owner@vger.kernel.org List-ID: Jakub Kicinski writes: >> Is that just an optimisation that we can do because we know caller's >> r1-r5 were scratched by the call? (resent, got a bounce back from netdev mailing list for previous email) I had been pondering whether it is really necessary to mark caller saved registers as scratched. I kind of feel the following code inside check_func_call might cause valid program rejected: /* after the call registers r0 - r5 were scratched */ for (i = 0; i < CALLER_SAVED_REGS; i++) { mark_reg_not_init(env, caller->regs, caller_saved[i]); check_reg_arg(env, caller_saved[i], DST_OP_NO_MARK); } Because there is inter-procedure register allocation support in LLVM (-enable-ipra), which could effectively eliminate register save/restore for one caller-saved register across function call if the compiler can prove callee or any other childs on the callgraph doesn't use/clobber this particular caller-saved register. Then the later sequence in caller after the call site could just safely read the caller-saved without restoring it from stack etc. But we are marking all caller-saved as NOT_INIT, such read will be treated as reading from uninitialized value, so the program will be rejected. -enable-ipra is disabled at default at the moment. Regards, Jiong > r0 - r5 of the caller will not be pushed to the stack by JITs and > therefore those are really dead. It is an optimization. We could've > connected them but they are already marked as WRITTEN in > check_func_call(). > >> It looks like, without that, the change is "every reg in all frames >> needs to be parented to the new-state", which is somewhat easier to >> understand (though I did have to think for quite a long time before >> it made sense to me why even that was necessary ;-) > > Ack, that was my initial patch actually, but Jiong pointed out it's > unnecessary and I didn't argue :) r1 - r5 are marked as WRITTEN and > UNINIT anyway, and r0 gets its parent from the callees frame > (prepare_func_exit() links callers r0 parentage chain to previous > states in the callee).