netdev - Re: [PATCH bpf-next v4 05/14] xsk: move queue_id, dev and need

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: <CAJ8uoz1mg-NJ-gNDVrikVp93DC5bU6QN8gzEPpyw36URRtBYxg@mail.gmail.com>
Date:   Wed, 29 Jul 2020 15:20:08 +0200
From:   Magnus Karlsson <magnus.karlsson@...il.com>
To:     Björn Töpel <bjorn.topel@...el.com>
Cc:     Magnus Karlsson <magnus.karlsson@...el.com>,
        Alexei Starovoitov <ast@...nel.org>,
        Daniel Borkmann <daniel@...earbox.net>,
        Network Development <netdev@...r.kernel.org>,
        Jonathan Lemon <jonathan.lemon@...il.com>,
        Maxim Mikityanskiy <maximmi@...lanox.com>,
        bpf <bpf@...r.kernel.org>, jeffrey.t.kirsher@...el.com,
        anthony.l.nguyen@...el.com,
        "Fijalkowski, Maciej" <maciej.fijalkowski@...el.com>,
        Maciej Fijalkowski <maciejromanfijalkowski@...il.com>,
        cristian.dumitrescu@...el.com
Subject: Re: [PATCH bpf-next v4 05/14] xsk: move queue_id, dev and need_wakeup
 to buffer pool

On Tue, Jul 28, 2020 at 9:10 AM Björn Töpel <bjorn.topel@...el.com> wrote:
>
>
>
> On 2020-07-21 07:03, Magnus Karlsson wrote:
> > Move queue_id, dev, and need_wakeup from the umem to the
> > buffer pool. This so that we in a later commit can share the umem
> > between multiple HW queues. There is one buffer pool per dev and
> > queue id, so these variables should belong to the buffer pool, not
> > the umem. Need_wakeup is also something that is set on a per napi
> > level, so there is usually one per device and queue id. So move
> > this to the buffer pool too.
> >
> > Signed-off-by: Magnus Karlsson <magnus.karlsson@...el.com>
> > ---
> >   include/net/xdp_sock.h      |  3 ---
> >   include/net/xsk_buff_pool.h |  4 ++++
> >   net/xdp/xdp_umem.c          | 19 +------------------
> >   net/xdp/xdp_umem.h          |  4 ----
> >   net/xdp/xsk.c               | 40 +++++++++++++++-------------------------
> >   net/xdp/xsk_buff_pool.c     | 39 ++++++++++++++++++++++-----------------
> >   net/xdp/xsk_diag.c          |  4 ++--
> >   7 files changed, 44 insertions(+), 69 deletions(-)
> >
> [...]
> >               }
> > diff --git a/net/xdp/xsk_buff_pool.c b/net/xdp/xsk_buff_pool.c
> > index 36287d2..436648a 100644
> > --- a/net/xdp/xsk_buff_pool.c
> > +++ b/net/xdp/xsk_buff_pool.c
> > @@ -95,10 +95,9 @@ void xp_set_rxq_info(struct xsk_buff_pool *pool, struct xdp_rxq_info *rxq)
> >   }
> >   EXPORT_SYMBOL(xp_set_rxq_info);
> >
> > -int xp_assign_dev(struct xsk_buff_pool *pool, struct net_device *dev,
> > +int xp_assign_dev(struct xsk_buff_pool *pool, struct net_device *netdev,
> >                 u16 queue_id, u16 flags)
> >   {
> > -     struct xdp_umem *umem = pool->umem;
> >       bool force_zc, force_copy;
> >       struct netdev_bpf bpf;
> >       int err = 0;
> > @@ -111,27 +110,30 @@ int xp_assign_dev(struct xsk_buff_pool *pool, struct net_device *dev,
> >       if (force_zc && force_copy)
> >               return -EINVAL;
> >
> > -     if (xsk_get_pool_from_qid(dev, queue_id))
> > +     if (xsk_get_pool_from_qid(netdev, queue_id))
> >               return -EBUSY;
> >
> > -     err = xsk_reg_pool_at_qid(dev, pool, queue_id);
> > +     err = xsk_reg_pool_at_qid(netdev, pool, queue_id);
> >       if (err)
> >               return err;
> >
> >       if (flags & XDP_USE_NEED_WAKEUP) {
> > -             umem->flags |= XDP_UMEM_USES_NEED_WAKEUP;
> > +             pool->uses_need_wakeup = true;
> >               /* Tx needs to be explicitly woken up the first time.
> >                * Also for supporting drivers that do not implement this
> >                * feature. They will always have to call sendto().
> >                */
> > -             umem->need_wakeup = XDP_WAKEUP_TX;
> > +             pool->cached_need_wakeup = XDP_WAKEUP_TX;
> >       }
> >
> > +     dev_hold(netdev);
> > +
>
> You have a reference leak here for the error case.

Thanks. Will fix.

/Magnus

>
> Björn