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]

Date:   Tue, 18 Sep 2018 18:55:57 -0700
From:   Jakub Kicinski <jakub.kicinski@...ronome.com>
To:     Y Song <ys114321@...il.com>
Cc:     magnus.karlsson@...el.com,
        Björn Töpel <bjorn.topel@...el.com>,
        Alexei Starovoitov <ast@...nel.org>,
        Daniel Borkmann <daniel@...earbox.net>,
        netdev <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 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_..?