Received: by 2002:a05:6a10:a0d1:0:0:0:0 with SMTP id j17csp1204626pxa; Thu, 20 Aug 2020 05:35:20 -0700 (PDT) X-Google-Smtp-Source: ABdhPJzInxQB/Gu/Em4SARuK5xeUU94DSziNYKd3pV65nfMNPKKiHVf5u18OIMA9PRpcM/mEpnam X-Received: by 2002:a05:6402:17a1:: with SMTP id j1mr2569017edy.99.1597926920187; Thu, 20 Aug 2020 05:35:20 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1597926920; cv=none; d=google.com; s=arc-20160816; b=rVVC9tkPfKPoWE2hDVrq+rIdrBvdoEXFTAwQf/qW3o7HM/uBs0C3+5oA+qMlS3t/Om pqKIKfcV/u8jXmbhi/2QqbzjD+43oqZ7CqBL5BptO95RzjqolWCuJpucWWDHkO7Zu93h HUImJpzFIytXbCR53YNMC7fgzbvy/CqN97MJ07SrqSzb8LOUHJ2UYxUsQDmRbSY9AXS8 y9wxc8KTPlUma86V9eh9EjOJ9q9dDqVQFcjDFHCRXtBm78TTrJF1HBNQWT7CNtKoM5NI 45IyfYPGepn1boAFmVHY8/RaEcvqxm0ZBobGcxwsXmiyNKo4FKm98nSBU2J6le08LxCw nv7Q== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:sender:cc:to:subject:message-id:date:from :in-reply-to:references:mime-version:dkim-signature; bh=BIs6d/jQFwGlnkZ5L4RtC3BBouLuQNHRbQqNyXKW5UA=; b=bi1m4shiOcq9+KLYillMsxLmMVrW8niseDDql98N+kct+am+BUb0uiDBvSQ5YrRnLw YchrDXRVcqt1b0BcvmvRQvvs4AYv344gdAh16tLJtfVfB003PTRD2B/UaGl97dvw7d/4 lvBzA1nUHjzWUirNOPDFSf1yQFYp3iEHscipnXk7bz57FPRZAMCDMgtKb+gyBYxKDjsg B+3w7k43PKcMxYcAZjM7x2+g4sXJbL85/LJV7nQ3FhFIIwPlxILBt8ZyEjW0Q5WjALaX Ymj/KFS7J1t7IxhWY6bsDoQN23GnCPVedZfibJsBWIMtFIl2DtS7pfq7PM5ujarO94T1 fzmQ== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@cloudflare.com header.s=google header.b=XPae7+kr; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=REJECT sp=REJECT dis=NONE) header.from=cloudflare.com Return-Path: Received: from vger.kernel.org (vger.kernel.org. [23.128.96.18]) by mx.google.com with ESMTP id f12si1551022edx.142.2020.08.20.05.34.56; Thu, 20 Aug 2020 05:35:20 -0700 (PDT) Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18; Authentication-Results: mx.google.com; dkim=pass header.i=@cloudflare.com header.s=google header.b=XPae7+kr; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=REJECT sp=REJECT dis=NONE) header.from=cloudflare.com Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1727836AbgHTMeY (ORCPT + 99 others); Thu, 20 Aug 2020 08:34:24 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:57320 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1730105AbgHTMeK (ORCPT ); Thu, 20 Aug 2020 08:34:10 -0400 Received: from mail-ot1-x341.google.com (mail-ot1-x341.google.com [IPv6:2607:f8b0:4864:20::341]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id A0491C061387 for ; Thu, 20 Aug 2020 05:34:09 -0700 (PDT) Received: by mail-ot1-x341.google.com with SMTP id h16so1298417oti.7 for ; Thu, 20 Aug 2020 05:34:09 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=cloudflare.com; s=google; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=BIs6d/jQFwGlnkZ5L4RtC3BBouLuQNHRbQqNyXKW5UA=; b=XPae7+krQuaIKLfqBwHPdeeqh4BCT2yAY1Kx/KY4xCqMP8r+sRNyl0bTAEA1mCUV/6 W6JulKNVZzjnr/6wCrVOicF//Ednj9hvSSv9Y2pP3FVoJpWIRv3zJ9+vZCTYlPkLOo0W BtfK4tT9nkXPhvzon3pOhchSSBcJxqBNfiHZ0= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=BIs6d/jQFwGlnkZ5L4RtC3BBouLuQNHRbQqNyXKW5UA=; b=r5CCibQMiC/XFwqUrIk4x29w1IMxNjHh9aWdZNeUZGo4E1VFLU5h0ReRIQMtdLrnKt /7BfIFRLn4+49nf/Qgm+6BgitpJjm/RodYYg0qTgCKOGh1gbdBdnBVKrZWY6p/epVlHn A+Ii1h+YQaSQwPQSxGpyt8En2uIVegEp/D3ZVAl1kQ6JtqOTuNU7wG/EjlLaT8Iuf/GR rJEveGmzIO8rFIxT4zkNXkZ+1km9ue3qoCkewDekpAhZeeu0UB9+gaqCmlACTldbjSp4 JmZzda/y++RMhkEivF5zAX0eoHbzlFXOVO/2k1eQL9tJ318NGdIiMn7YjfMO694PTciG Ox6g== X-Gm-Message-State: AOAM5300I0yvZgkoiTVSow2WftuU6KPVBxOAXx1pcGp0ut72ntdbx1IV qNmKpCHepatIU5rjdyOwiv59JeLdnItwuyXkM+jNqw== X-Received: by 2002:a9d:6e18:: with SMTP id e24mr1890875otr.132.1597926848493; Thu, 20 Aug 2020 05:34:08 -0700 (PDT) MIME-Version: 1.0 References: <20200819092436.58232-1-lmb@cloudflare.com> <20200819092436.58232-5-lmb@cloudflare.com> <5d64158b-35ed-d28d-9857-6ee725d287f2@fb.com> In-Reply-To: <5d64158b-35ed-d28d-9857-6ee725d287f2@fb.com> From: Lorenz Bauer Date: Thu, 20 Aug 2020 13:33:57 +0100 Message-ID: Subject: Re: [PATCH bpf-next 4/6] bpf: override the meaning of ARG_PTR_TO_MAP_VALUE for sockmap and sockhash To: Yonghong Song Cc: Jakub Sitnicki , John Fastabend , Alexei Starovoitov , Daniel Borkmann , kernel-team , Networking , bpf , LKML Content-Type: text/plain; charset="UTF-8" Sender: linux-kernel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Wed, 19 Aug 2020 at 21:13, Yonghong Song wrote: > > > > On 8/19/20 2:24 AM, Lorenz Bauer wrote: > > The verifier assumes that map values are simple blobs of memory, and > > therefore treats ARG_PTR_TO_MAP_VALUE, etc. as such. However, there are > > map types where this isn't true. For example, sockmap and sockhash store > > sockets. In general this isn't a big problem: we can just > > write helpers that explicitly requests PTR_TO_SOCKET instead of > > ARG_PTR_TO_MAP_VALUE. > > > > The one exception are the standard map helpers like map_update_elem, > > map_lookup_elem, etc. Here it would be nice we could overload the > > function prototype for different kinds of maps. Unfortunately, this > > isn't entirely straight forward: > > We only know the type of the map once we have resolved meta->map_ptr > > in check_func_arg. This means we can't swap out the prototype > > in check_helper_call until we're half way through the function. > > > > Instead, modify check_func_arg to treat ARG_PTR_TO_MAP_VALUE* to > > mean "the native type for the map" instead of "pointer to memory" > > for sockmap and sockhash. This means we don't have to modify the > > function prototype at all > > > > Signed-off-by: Lorenz Bauer > > --- > > kernel/bpf/verifier.c | 40 ++++++++++++++++++++++++++++++++++++++++ > > 1 file changed, 40 insertions(+) > > > > diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c > > index b6ccfce3bf4c..47f9b94bb9d4 100644 > > --- a/kernel/bpf/verifier.c > > +++ b/kernel/bpf/verifier.c > > @@ -3872,6 +3872,38 @@ static int int_ptr_type_to_size(enum bpf_arg_type type) > > return -EINVAL; > > } > > > > +static int override_map_arg_type(struct bpf_verifier_env *env, > > + const struct bpf_call_arg_meta *meta, > > + enum bpf_arg_type *arg_type) > > +{ > > + if (!meta->map_ptr) { > > + /* kernel subsystem misconfigured verifier */ > > + verbose(env, "invalid map_ptr to access map->type\n"); > > + return -EACCES; > > + } > > + > > + switch (meta->map_ptr->map_type) { > > + case BPF_MAP_TYPE_SOCKMAP: > > + case BPF_MAP_TYPE_SOCKHASH: > > + switch (*arg_type) { > > + case ARG_PTR_TO_MAP_VALUE: > > + *arg_type = ARG_PTR_TO_SOCKET; > > + break; > > + case ARG_PTR_TO_MAP_VALUE_OR_NULL: > > + *arg_type = ARG_PTR_TO_SOCKET_OR_NULL; > > + break; > > + default: > > + verbose(env, "invalid arg_type for sockmap/sockhash\n"); > > + return -EINVAL; > > + } > > + break; > > + > > + default: > > + break; > > + } > > + return 0; > > +} > > + > > static int check_func_arg(struct bpf_verifier_env *env, u32 arg, > > struct bpf_call_arg_meta *meta, > > const struct bpf_func_proto *fn) > > @@ -3904,6 +3936,14 @@ static int check_func_arg(struct bpf_verifier_env *env, u32 arg, > > return -EACCES; > > } > > > > + if (arg_type == ARG_PTR_TO_MAP_VALUE || > > + arg_type == ARG_PTR_TO_UNINIT_MAP_VALUE || > > + arg_type == ARG_PTR_TO_MAP_VALUE_OR_NULL) { > > We probably do not need ARG_PTR_TO_UNINIT_MAP_VALUE here. > > Do we need ARG_PTR_TO_MAP_VALUE_OR_NULL? bpf_map_update_elem arg type > is ARG_PTR_TO_MAP_VALUE. I did this to be consistent: in a single function definition you either get the map specific types or the regular semantics. You don't get to mix and match them. For the same reason I included ARG_PTR_TO_UNINIT_MAP_VALUE: the semantics don't make sense for sockmap, so a function using this doesn't make sense either. > > > + err = override_map_arg_type(env, meta, &arg_type); > > + if (err) > > + return err; > > + } > > + > > if (arg_type == ARG_PTR_TO_MAP_KEY || > > arg_type == ARG_PTR_TO_MAP_VALUE || > > arg_type == ARG_PTR_TO_UNINIT_MAP_VALUE || > > -- Lorenz Bauer | Systems Engineer 6th Floor, County Hall/The Riverside Building, SE1 7PB, UK www.cloudflare.com