From mboxrd@z Thu Jan  1 00:00:00 1970
From: Jakub Kicinski <jakub.kicinski@netronome.com>
Subject: Re: [PATCH bpf-next 2/2] xsk: fix bug when trying to use both copy
 and zero-copy on one queue id
Date: Tue, 18 Sep 2018 18:55:57 -0700
Message-ID: <20180918185557.4da5a463@cakuba.netronome.com>
References: <1537265538-5882-1-git-send-email-magnus.karlsson@intel.com>
        <1537265538-5882-3-git-send-email-magnus.karlsson@intel.com>
        <CAH3MdRU8jKBdpNK6Uq3NRyeb2ih6tgWJOAKW_pQFm0QQ0kuvpw@mail.gmail.com>
Mime-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit
Cc: magnus.karlsson@intel.com,
        =?UTF-8?B?QmrDtnJuIFTDtnBlbA==?= <bjorn.topel@intel.com>,
        Alexei Starovoitov <ast@kernel.org>,
        Daniel Borkmann <daniel@iogearbox.net>,
        netdev <netdev@vger.kernel.org>
To: Y Song <ys114321@gmail.com>
Return-path: <netdev-owner@vger.kernel.org>
Received: from mail-qk1-f196.google.com ([209.85.222.196]:40924 "EHLO
        mail-qk1-f196.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org
        with ESMTP id S1728226AbeISHb0 (ORCPT
        <rfc822;netdev@vger.kernel.org>); Wed, 19 Sep 2018 03:31:26 -0400
Received: by mail-qk1-f196.google.com with SMTP id c126-v6so2184670qkd.7
        for <netdev@vger.kernel.org>; Tue, 18 Sep 2018 18:56:02 -0700 (PDT)
In-Reply-To: <CAH3MdRU8jKBdpNK6Uq3NRyeb2ih6tgWJOAKW_pQFm0QQ0kuvpw@mail.gmail.com>
Sender: netdev-owner@vger.kernel.org
List-ID: <netdev.vger.kernel.org>

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_..?