Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
MIME-Version: 1.0
References: <20190920185843.4096-1-matthew.cover@stackpath.com> <20190922080326-mutt-send-email-mst@kernel.org>
In-Reply-To: <20190922080326-mutt-send-email-mst@kernel.org>
From:   Matt Cover <werekraken@gmail.com>
Date:   Sun, 22 Sep 2019 10:43:19 -0700
Message-ID: <CAGyo_hqGbFdt1PoDrmo=S5iTO8TwbrbtOJtbvGT1WrFFMLwk-Q@mail.gmail.com>
Subject: Re: [PATCH net-next] tuntap: Fallback to automq on TUNSETSTEERINGEBPF
 prog negative return
To:     "Michael S. Tsirkin" <mst@redhat.com>
Cc:     davem@davemloft.net, ast@kernel.org, daniel@iogearbox.net,
        kafai@fb.com, songliubraving@fb.com, yhs@fb.com,
        Jason Wang <jasowang@redhat.com>,
        Eric Dumazet <edumazet@google.com>,
        Stanislav Fomichev <sdf@google.com>,
        Matthew Cover <matthew.cover@stackpath.com>,
        mail@timurcelik.de, pabeni@redhat.com,
        Nicolas Dichtel <nicolas.dichtel@6wind.com>,
        wangli39@baidu.com, lifei.shirley@bytedance.com,
        tglx@linutronix.de, netdev@vger.kernel.org,
        linux-kernel@vger.kernel.org, bpf@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Sun, Sep 22, 2019 at 5:37 AM Michael S. Tsirkin <mst@redhat.com> wrote:
>
> On Fri, Sep 20, 2019 at 11:58:43AM -0700, Matthew Cover wrote:
> > Treat a negative return from a TUNSETSTEERINGEBPF bpf prog as a signal
> > to fallback to tun_automq_select_queue() for tx queue selection.
> >
> > Compilation of this exact patch was tested.
> >
> > For functional testing 3 additional printk()s were added.
> >
> > Functional testing results (on 2 txq tap device):
> >
> >   [Fri Sep 20 18:33:27 2019] ========== tun no prog ==========
> >   [Fri Sep 20 18:33:27 2019] tuntap: tun_ebpf_select_queue() returned '-1'
> >   [Fri Sep 20 18:33:27 2019] tuntap: tun_automq_select_queue() ran
> >   [Fri Sep 20 18:33:27 2019] ========== tun prog -1 ==========
> >   [Fri Sep 20 18:33:27 2019] tuntap: bpf_prog_run_clear_cb() returned '-1'
> >   [Fri Sep 20 18:33:27 2019] tuntap: tun_ebpf_select_queue() returned '-1'
> >   [Fri Sep 20 18:33:27 2019] tuntap: tun_automq_select_queue() ran
> >   [Fri Sep 20 18:33:27 2019] ========== tun prog 0 ==========
> >   [Fri Sep 20 18:33:27 2019] tuntap: bpf_prog_run_clear_cb() returned '0'
> >   [Fri Sep 20 18:33:27 2019] tuntap: tun_ebpf_select_queue() returned '0'
> >   [Fri Sep 20 18:33:27 2019] ========== tun prog 1 ==========
> >   [Fri Sep 20 18:33:27 2019] tuntap: bpf_prog_run_clear_cb() returned '1'
> >   [Fri Sep 20 18:33:27 2019] tuntap: tun_ebpf_select_queue() returned '1'
> >   [Fri Sep 20 18:33:27 2019] ========== tun prog 2 ==========
> >   [Fri Sep 20 18:33:27 2019] tuntap: bpf_prog_run_clear_cb() returned '2'
> >   [Fri Sep 20 18:33:27 2019] tuntap: tun_ebpf_select_queue() returned '0'
> >
> > Signed-off-by: Matthew Cover <matthew.cover@stackpath.com>
>
>
> Could you add a bit more motivation data here?

Thank you for these questions Michael.

I'll plan on adding the below information to the
commit message and submitting a v2 of this patch
when net-next reopens. In the meantime, it would
be very helpful to know if these answers address
some of your concerns.

> 1. why is this a good idea

This change allows TUNSETSTEERINGEBPF progs to
do any of the following.
 1. implement queue selection for a subset of
    traffic (e.g. special queue selection logic
    for ipv4, but return negative and use the
    default automq logic for ipv6)
 2. determine there isn't sufficient information
    to do proper queue selection; return
    negative and use the default automq logic
    for the unknown
 3. implement a noop prog (e.g. do
    bpf_trace_printk() then return negative and
    use the default automq logic for everything)

> 2. how do we know existing userspace does not rely on existing behaviour

Prior to this change a negative return from a
TUNSETSTEERINGEBPF prog would have been cast
into a u16 and traversed netdev_cap_txqueue().

In most cases netdev_cap_txqueue() would have
found this value to exceed real_num_tx_queues
and queue_index would be updated to 0.

It is possible that a TUNSETSTEERINGEBPF prog
return a negative value which when cast into a
u16 results in a positive queue_index less than
real_num_tx_queues. For example, on x86_64, a
return value of -65535 results in a queue_index
of 1; which is a valid queue for any multiqueue
device.

It seems unlikely, however as stated above is
unfortunately possible, that existing
TUNSETSTEERINGEBPF programs would choose to
return a negative value rather than return the
positive value which holds the same meaning.

It seems more likely that future
TUNSETSTEERINGEBPF programs would leverage a
negative return and potentially be loaded into
a kernel with the old behavior.

> 3. why doesn't userspace need a way to figure out whether it runs on a kernel with and
>    without this patch

There may be some value in exposing this fact
to the ebpf prog loader. What is the standard
practice here, a define?

>
>
> thanks,
> MST
>
> > ---
> >  drivers/net/tun.c | 20 +++++++++++---------
> >  1 file changed, 11 insertions(+), 9 deletions(-)
> >
> > diff --git a/drivers/net/tun.c b/drivers/net/tun.c
> > index aab0be4..173d159 100644
> > --- a/drivers/net/tun.c
> > +++ b/drivers/net/tun.c
> > @@ -583,35 +583,37 @@ static u16 tun_automq_select_queue(struct tun_struct *tun, struct sk_buff *skb)
> >       return txq;
> >  }
> >
> > -static u16 tun_ebpf_select_queue(struct tun_struct *tun, struct sk_buff *skb)
> > +static int tun_ebpf_select_queue(struct tun_struct *tun, struct sk_buff *skb)
> >  {
> >       struct tun_prog *prog;
> >       u32 numqueues;
> > -     u16 ret = 0;
> > +     int ret = -1;
> >
> >       numqueues = READ_ONCE(tun->numqueues);
> >       if (!numqueues)
> >               return 0;
> >
> > +     rcu_read_lock();
> >       prog = rcu_dereference(tun->steering_prog);
> >       if (prog)
> >               ret = bpf_prog_run_clear_cb(prog->prog, skb);
> > +     rcu_read_unlock();
> >
> > -     return ret % numqueues;
> > +     if (ret >= 0)
> > +             ret %= numqueues;
> > +
> > +     return ret;
> >  }
> >
> >  static u16 tun_select_queue(struct net_device *dev, struct sk_buff *skb,
> >                           struct net_device *sb_dev)
> >  {
> >       struct tun_struct *tun = netdev_priv(dev);
> > -     u16 ret;
> > +     int ret;
> >
> > -     rcu_read_lock();
> > -     if (rcu_dereference(tun->steering_prog))
> > -             ret = tun_ebpf_select_queue(tun, skb);
> > -     else
> > +     ret = tun_ebpf_select_queue(tun, skb);
> > +     if (ret < 0)
> >               ret = tun_automq_select_queue(tun, skb);
> > -     rcu_read_unlock();
> >
> >       return ret;
> >  }
> > --
> > 1.8.3.1