Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1755298AbaGBF5x (ORCPT ); Wed, 2 Jul 2014 01:57:53 -0400 Received: from mail-wg0-f41.google.com ([74.125.82.41]:46711 "EHLO mail-wg0-f41.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752461AbaGBF5u (ORCPT ); Wed, 2 Jul 2014 01:57:50 -0400 MIME-Version: 1.0 In-Reply-To: <87y4wc4aff.fsf@sejong.aot.lge.com> References: <1403913966-4927-1-git-send-email-ast@plumgrid.com> <1403913966-4927-9-git-send-email-ast@plumgrid.com> <87y4wc4aff.fsf@sejong.aot.lge.com> Date: Tue, 1 Jul 2014 22:57:49 -0700 Message-ID: Subject: Re: [PATCH RFC net-next 08/14] bpf: add eBPF verifier From: Alexei Starovoitov To: Namhyung Kim Cc: "David S. Miller" , Ingo Molnar , Linus Torvalds , Steven Rostedt , Daniel Borkmann , Chema Gonzalez , Eric Dumazet , Peter Zijlstra , Arnaldo Carvalho de Melo , Jiri Olsa , Thomas Gleixner , "H. Peter Anvin" , Andrew Morton , Kees Cook , Linux API , Network Development , LKML Content-Type: text/plain; charset=UTF-8 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Tue, Jul 1, 2014 at 10:05 PM, Namhyung Kim wrote: > Mostly questions and few nitpicks.. :) great questions. Thank you for review! Answers below: > On Fri, 27 Jun 2014 17:06:00 -0700, Alexei Starovoitov wrote: >> +/* types of values: >> + * - stored in an eBPF register >> + * - passed into helper functions as an argument >> + * - returned from helper functions >> + */ >> +enum bpf_reg_type { >> + INVALID_PTR, /* reg doesn't contain a valid pointer */ > > I don't think it's a good name. The INVALID_PTR can be read as it > contains a "pointer" which is invalid. Maybe INTEGER, NUMBER or > something different can be used. And I think the struct reg_state->ptr > should be renamed also. ok. I agree that 'invalid' part of the name is too negative. May be 'unknown_value' ? >> + PTR_TO_CTX, /* reg points to bpf_context */ >> + PTR_TO_MAP, /* reg points to map element value */ >> + PTR_TO_MAP_CONDITIONAL, /* points to map element value or NULL */ >> + PTR_TO_STACK, /* reg == frame_pointer */ >> + PTR_TO_STACK_IMM, /* reg == frame_pointer + imm */ >> + PTR_TO_STACK_IMM_MAP_KEY, /* pointer to stack used as map key */ >> + PTR_TO_STACK_IMM_MAP_VALUE, /* pointer to stack used as map elem */ > > So, these PTR_TO_STACK_IMM[_*] types are only for function argument, > right? I guessed it could be used to access memory in general too, but > then I thought it'd make verification complicated.. > > And I also agree that it'd better splitting reg types and function > argument constraints. Ok. Will split this enum into three. >> + >> +/* check read/write into map element returned by bpf_table_lookup() */ >> +static int check_table_access(struct verifier_env *env, int regno, int off, >> + int size) > > I guess the "table" is an old name of the "map"? oops :) Yes. I've been calling them 'bpf tables' initially, but it created too strong of a correlation to 'hash table', so I've changed the name to 'map' to stress that this is a generic key/value and not just hash table. >> + } else if (state->regs[regno].ptr == PTR_TO_STACK) { >> + if (off >= 0 || off < -MAX_BPF_STACK) { >> + verbose("invalid stack off=%d size=%d\n", off, size); >> + return -EACCES; >> + } > > So memory (stack) access is only allowed for a stack base regsiter and a > constant offset, right? Correct. In other words it allows instructions: BPF_STX_MEM(BPF_W, BPF_REG_10, BPF_REG_xx, -stack_offset); Verifier makes no attempt to track pointer arithmetic and just marks the result as 'invalid_ptr'. For non-root programs it will reject programs that are trying to do arithmetic on pointers (it's not part of this patch yet). >> + /* check args */ >> + _(check_func_arg(env, BPF_REG_1, fn->arg1_type, &map_id, &map)); >> + _(check_func_arg(env, BPF_REG_2, fn->arg2_type, &map_id, &map)); >> + _(check_func_arg(env, BPF_REG_3, fn->arg3_type, &map_id, &map)); >> + _(check_func_arg(env, BPF_REG_4, fn->arg4_type, &map_id, &map)); > > Missing BPF_REG_5? yes. good catch. I guess this shows that we didn't have a use case for function with 5 args :) Will fix this. >> +#define PEAK_INT() \ > > s/PEAK/PEEK/ ? aren't these the same? ;)) Will fix. Thanks! -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/