netdev - Re: [PATCH v2 bpf-next 1/5] devmap: Formalize map value as a named struct

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAEf4BzZPbndT3daoSt_z6cH=CPJSdH5yMhpR2gDbmeQUacWAyg@mail.gmail.com>
Date:   Thu, 28 May 2020 00:01:35 -0700
From:   Andrii Nakryiko <andrii.nakryiko@...il.com>
To:     David Ahern <dsahern@...nel.org>
Cc:     Networking <netdev@...r.kernel.org>,
        "David S. Miller" <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>,
        Jesper Dangaard Brouer <brouer@...hat.com>,
        Toke Høiland-Jørgensen <toke@...hat.com>,
        Daniel Borkmann <daniel@...earbox.net>,
        john fastabend <john.fastabend@...il.com>,
        Alexei Starovoitov <ast@...nel.org>, Martin Lau <kafai@...com>,
        Song Liu <songliubraving@...com>, Yonghong Song <yhs@...com>,
        Andrii Nakryiko <andriin@...com>,
        David Ahern <dsahern@...il.com>
Subject: Re: [PATCH v2 bpf-next 1/5] devmap: Formalize map value as a named struct

On Wed, May 27, 2020 at 5:15 PM David Ahern <dsahern@...nel.org> wrote:
>
> From: David Ahern <dsahern@...il.com>
>
> Add 'struct devmap_val' to the bpf uapi to formalize the
> expected values that can be passed in for a DEVMAP.
> Update devmap code to use the struct.
>
> Signed-off-by: David Ahern <dsahern@...il.com>
> ---
>  include/uapi/linux/bpf.h       |  5 ++++
>  kernel/bpf/devmap.c            | 43 ++++++++++++++++++++--------------
>  tools/include/uapi/linux/bpf.h |  5 ++++
>  3 files changed, 35 insertions(+), 18 deletions(-)
>
> diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
> index 54b93f8b49b8..d27302ecaa9c 100644
> --- a/include/uapi/linux/bpf.h
> +++ b/include/uapi/linux/bpf.h
> @@ -3625,6 +3625,11 @@ struct xdp_md {
>         __u32 rx_queue_index;  /* rxq->queue_index  */
>  };
>
> +/* DEVMAP values */
> +struct devmap_val {
> +       __u32 ifindex;   /* device index */
> +};
> +

can DEVMAP be used outside of BPF ecosystem? If not, shouldn't this be
`struct bpf_devmap_val`, to be consistent with the rest of the type
names?

>  enum sk_action {
>         SK_DROP = 0,
>         SK_PASS,
> diff --git a/kernel/bpf/devmap.c b/kernel/bpf/devmap.c
> index a51d9fb7a359..069a50113e26 100644
> --- a/kernel/bpf/devmap.c
> +++ b/kernel/bpf/devmap.c
> @@ -66,6 +66,7 @@ struct bpf_dtab_netdev {
>         struct bpf_dtab *dtab;
>         struct rcu_head rcu;
>         unsigned int idx;
> +       struct devmap_val val;
>  };
>
>  struct bpf_dtab {
> @@ -110,7 +111,8 @@ static int dev_map_init_map(struct bpf_dtab *dtab, union bpf_attr *attr)
>
>         /* check sanity of attributes */
>         if (attr->max_entries == 0 || attr->key_size != 4 ||
> -           attr->value_size != 4 || attr->map_flags & ~DEV_CREATE_FLAG_MASK)
> +           attr->value_size > sizeof(struct devmap_val) ||

So is 0, 1, 2, 3, and after next patch 5, 6, and 7 all allowed as
well? Isn't that a bit too permissive?

> +           attr->map_flags & ~DEV_CREATE_FLAG_MASK)
>                 return -EINVAL;
>

[...]