netdev - Re: [PATCH bpf-next 7/8] xdp: remove map_to

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: <CAJ+HfNjg5kRhBuWQ0F1jM+YL8CYW2okes0jbFy6MQw9umT_dcA@mail.gmail.com>
Date:   Wed, 18 Dec 2019 14:09:23 +0100
From:   Björn Töpel <bjorn.topel@...il.com>
To:     Jesper Dangaard Brouer <jbrouer@...hat.com>
Cc:     Netdev <netdev@...r.kernel.org>,
        Alexei Starovoitov <ast@...nel.org>,
        Daniel Borkmann <daniel@...earbox.net>,
        Björn Töpel <bjorn.topel@...el.com>,
        bpf <bpf@...r.kernel.org>, David Miller <davem@...emloft.net>,
        Jakub Kicinski <jakub.kicinski@...ronome.com>,
        Jesper Dangaard Brouer <hawk@...nel.org>,
        John Fastabend <john.fastabend@...il.com>,
        "Karlsson, Magnus" <magnus.karlsson@...el.com>,
        Jonathan Lemon <jonathan.lemon@...il.com>
Subject: Re: [PATCH bpf-next 7/8] xdp: remove map_to_flush and map swap detection

On Wed, 18 Dec 2019 at 14:03, Jesper Dangaard Brouer <jbrouer@...hat.com> wrote:
>
> On Wed, 18 Dec 2019 11:53:59 +0100
> Björn Töpel <bjorn.topel@...il.com> wrote:
>
> > From: Björn Töpel <bjorn.topel@...el.com>
> >
> > Now that all XDP maps that can be used with bpf_redirect_map() tracks
> > entries to be flushed in a global fashion, there is not need to track
> > that the map has changed and flush from xdp_do_generic_map()
> > anymore. All entries will be flushed in xdp_do_flush_map().
> >
> > This means that the map_to_flush can be removed, and the corresponding
> > checks. Moving the flush logic to one place, xdp_do_flush_map(), give
> > a bulking behavior and performance boost.
> >
> > Signed-off-by: Björn Töpel <bjorn.topel@...el.com>
> > ---
> >  include/linux/filter.h |  1 -
> >  net/core/filter.c      | 27 +++------------------------
> >  2 files changed, 3 insertions(+), 25 deletions(-)
> >
> > diff --git a/include/linux/filter.h b/include/linux/filter.h
> > index 37ac7025031d..69d6706fc889 100644
> > --- a/include/linux/filter.h
> > +++ b/include/linux/filter.h
> > @@ -592,7 +592,6 @@ struct bpf_redirect_info {
> >       u32 tgt_index;
> >       void *tgt_value;
> >       struct bpf_map *map;
> > -     struct bpf_map *map_to_flush;
> >       u32 kern_flags;
> >  };
> >
> > diff --git a/net/core/filter.c b/net/core/filter.c
> > index c706325b3e66..d9caa3e57ea1 100644
> > --- a/net/core/filter.c
> > +++ b/net/core/filter.c
> > @@ -3547,26 +3547,9 @@ static int __bpf_tx_xdp_map(struct net_device *dev_rx, void *fwd,
> >
> >  void xdp_do_flush_map(void)
> >  {
> > -     struct bpf_redirect_info *ri = this_cpu_ptr(&bpf_redirect_info);
> > -     struct bpf_map *map = ri->map_to_flush;
> > -
> > -     ri->map_to_flush = NULL;
> > -     if (map) {
> > -             switch (map->map_type) {
> > -             case BPF_MAP_TYPE_DEVMAP:
> > -             case BPF_MAP_TYPE_DEVMAP_HASH:
> > -                     __dev_map_flush();
> > -                     break;
> > -             case BPF_MAP_TYPE_CPUMAP:
> > -                     __cpu_map_flush();
> > -                     break;
> > -             case BPF_MAP_TYPE_XSKMAP:
> > -                     __xsk_map_flush();
> > -                     break;
> > -             default:
> > -                     break;
> > -             }
> > -     }
> > +     __dev_map_flush();
> > +     __cpu_map_flush();
> > +     __xsk_map_flush();
> >  }
> >  EXPORT_SYMBOL_GPL(xdp_do_flush_map);
> >
> > @@ -3615,14 +3598,10 @@ static int xdp_do_redirect_map(struct net_device *dev, struct xdp_buff *xdp,
> >       ri->tgt_value = NULL;
> >       WRITE_ONCE(ri->map, NULL);
> >
> > -     if (ri->map_to_flush && unlikely(ri->map_to_flush != map))
> > -             xdp_do_flush_map();
> > -
>
> I guess, I need to read the other patches to understand why this is valid.
>

Please do; Review would be very much appreciated!

> The idea here is to detect if the BPF-prog are using several different
> redirect maps, and do the flush if the program uses another map.  I
> assume you handle this?
>

Yes, the highlevel idea is that since the maps are dealing partly with
this already (but swaps map internal), let the map code deal with map
swaps as well.