netdev - Re: [PATCH bpf-next V1] bpf: devmap dynamic map-value area based on BTF

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: <20200604174806.29130b81@carbon>
Date:   Thu, 4 Jun 2020 17:48:06 +0200
From:   Jesper Dangaard Brouer <brouer@...hat.com>
To:     Alexei Starovoitov <alexei.starovoitov@...il.com>
Cc:     David Ahern <dsahern@...il.com>, bpf@...r.kernel.org,
        netdev@...r.kernel.org, Daniel Borkmann <borkmann@...earbox.net>,
        Andrii Nakryiko <andrii.nakryiko@...il.com>,
        Lorenzo Bianconi <lorenzo@...nel.org>, brouer@...hat.com,
        David Miller <davem@...emloft.net>
Subject: Re: [PATCH bpf-next V1] bpf: devmap dynamic map-value area based on
 BTF

On Wed, 3 Jun 2020 09:22:57 -0700
Alexei Starovoitov <alexei.starovoitov@...il.com> wrote:

> On Wed, Jun 03, 2020 at 05:44:43PM +0200, Jesper Dangaard Brouer wrote:
> > The recent commit fbee97feed9b ("bpf: Add support to attach bpf program to a
> > devmap entry"), introduced ability to attach (and run) a separate XDP
> > bpf_prog for each devmap entry. A bpf_prog is added via a file-descriptor,
> > thus not using the feature requires using value minus-1. The UAPI is
> > extended via tail-extending struct bpf_devmap_val and using map->value_size
> > to determine the feature set.
> > 
> > There is a specific problem with dev_map_can_have_prog() check, which is
> > called from net/core/dev.c in generic_xdp_install() to refuse usage of
> > devmap's from generic-XDP that support these bpf_prog's. The check is size
> > based. This means that all newer features will be blocked from being use by
> > generic-XDP.
> > 
> > This patch allows userspace to skip handling of 'bpf_prog' on map-inserts.
> > The feature can be skipped, via not including the member 'bpf_prog' in the
> > map-value struct, which is propagated/described via BTF.
> > 
> > Fixes: fbee97feed9b ("bpf: Add support to attach bpf program to a devmap entry")
> > Signed-off-by: Jesper Dangaard Brouer <brouer@...hat.com  
> 
> The patch makes no sense to me.

Hmm, that is not a very constructive answer, and it doesn't help me
to improve and move forward with the code.  I interpret that you think
my approach is completely wrong, but it would have been nice to
understand why.  I will give up on this approach, also given bpf-next
is closed now.


> please expose 'struct struct bpf_devmap_val' in uapi/bpf.h
> That's what it is whether you want to acknowledge that or not.

I will NOT send a patch that expose this in uapi/bpf.h.  As I explained
before, this caused the issues for my userspace application, that
automatically picked-up struct bpf_devmap_val, and started to fail
(with no code changes), because it needed minus-1 as input.  I fear
that this will cause more work for me later, when I have to helpout and
support end-users on e.g. xdp-newbies list, as it will not be obvious
to end-users why their programs map-insert start to fail.  I have given
up, so I will not NACK anyone sending such a patch.

Why is it we need to support file-descriptor zero as a valid
file-descriptor for a bpf-prog?

-- 
Best regards,
  Jesper Dangaard Brouer
  MSc.CS, Principal Kernel Engineer at Red Hat
  LinkedIn: http://www.linkedin.com/in/brouer