netdev - Re: [PATCH bpf-next 2/2] xsk: fix bug when trying to use both copy and zero-copy on one queue id

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: <CAJ8uoz27a-+OYxHGnXOT6pY2v0hNxvrmCW8Eb5y5JkWLG9K3hg@mail.gmail.com>
Date:   Wed, 19 Sep 2018 09:18:02 +0200
From:   Magnus Karlsson <magnus.karlsson@...il.com>
To:     jakub.kicinski@...ronome.com
Cc:     ys114321@...il.com, "Karlsson, Magnus" <magnus.karlsson@...el.com>,
        Björn Töpel <bjorn.topel@...el.com>,
        ast@...nel.org, Daniel Borkmann <daniel@...earbox.net>,
        Network Development <netdev@...r.kernel.org>
Subject: Re: [PATCH bpf-next 2/2] xsk: fix bug when trying to use both copy
 and zero-copy on one queue id

On Wed, Sep 19, 2018 at 3:58 AM Jakub Kicinski
<jakub.kicinski@...ronome.com> wrote:
>
> On Tue, 18 Sep 2018 10:22:11 -0700, Y Song wrote:
> > > +/* The umem is stored both in the _rx struct and the _tx struct as we do
> > > + * not know if the device has more tx queues than rx, or the opposite.
> > > + * This might also change during run time.
> > > + */
> > > +static void xdp_reg_umem_at_qid(struct net_device *dev, struct xdp_umem *umem,
> > > +                               u16 queue_id)
> > > +{
> > > +       if (queue_id < dev->real_num_rx_queues)
> > > +               dev->_rx[queue_id].umem = umem;
> > > +       if (queue_id < dev->real_num_tx_queues)
> > > +               dev->_tx[queue_id].umem = umem;
> > > +}
> > > +
> > > +static struct xdp_umem *xdp_get_umem_from_qid(struct net_device *dev,
> > > +                                             u16 queue_id)
> > > +{
> > > +       if (queue_id < dev->real_num_rx_queues)
> > > +               return dev->_rx[queue_id].umem;
> > > +       if (queue_id < dev->real_num_tx_queues)
> > > +               return dev->_tx[queue_id].umem;
> > > +
> > > +       return NULL;
> > > +}
> > > +
> > > +static void xdp_clear_umem_at_qid(struct net_device *dev, u16 queue_id)
> > > +{
> > > +       /* Zero out the entry independent on how many queues are configured
> > > +        * at this point in time, as it might be used in the future.
> > > +        */
> > > +       if (queue_id < dev->num_rx_queues)
> > > +               dev->_rx[queue_id].umem = NULL;
> > > +       if (queue_id < dev->num_tx_queues)
> > > +               dev->_tx[queue_id].umem = NULL;
> > > +}
> > > +
> >
> > I am sure whether the following scenario can happen or not.
> > Could you clarify?
> >    1. suppose initially we have num_rx_queues = num_tx_queues = 10
> >        xdp_reg_umem_at_qid() set umem1 to queue_id = 8
> >    2. num_tx_queues is changed to 5
> >    3. xdp_clear_umem_at_qid() is called for queue_id = 8,
> >        and dev->_rx[8].umum = 0.
> >    4. xdp_reg_umem_at_qid() is called gain to set for queue_id = 8
> >        dev->_rx[8].umem = umem2
> >    5. num_tx_queues is changed to 10
> >   Now dev->_rx[8].umem != dev->_tx[8].umem, is this possible and is it
> > a problem?
>
> Plus IIRC the check of qid vs real_num_[rt]x_queues in xsk_bind() is
> not under rtnl_lock so it doesn't count for much.  Why not do all the
> checks against num_[rt]x_queues here, instead of real_..?

You are correct, two separate rtnl_lock regions is broken. Will spin a
v2 tomorrow when I am back in the office.

Thanks Jakub for catching this. I really appreciate you reviewing my code.

/Magnus