Re: [PATCH net-next] tuntap: Fallback to automq on TUNSETSTEERINGEBPF prog negative return

Jason Wang
2019-09-23T05:16:09Z

On 2019/9/23 上午11:18, Matt Cover wrote:
> On Sun, Sep 22, 2019 at 7:34 PM Jason Wang <jasowang@redhat.com> wrote:
>>
>> On 2019/9/23 上午9:15, Matt Cover wrote:
>>> On Sun, Sep 22, 2019 at 5:51 PM Jason Wang <jasowang@redhat.com> wrote:
>>>> On 2019/9/23 上午6:30, Matt Cover wrote:
>>>>> On Sun, Sep 22, 2019 at 1:36 PM Michael S. Tsirkin <mst@redhat.com> wrote:
>>>>>> On Sun, Sep 22, 2019 at 10:43:19AM -0700, Matt Cover wrote:
>>>>>>> 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.
>>>>>> OK if we are returning a special
>>>>>> value, shouldn't we limit it? How about a special
>>>>>> value with this meaning?
>>>>>> If we are changing an ABI let's at least make it
>>>>>> extensible.
>>>>>>
>>>>> A special value with this meaning sounds
>>>>> good to me. I'll plan on adding a define
>>>>> set to -1 to cause the fallback to automq.
>>>> Can it really return -1?
>>>>
>>>> I see:
>>>>
>>>> static inline u32 bpf_prog_run_clear_cb(const struct bpf_prog *prog,
>>>> struct sk_buff *skb)
>>>> ...
>>>>
>>>>
>>>>> The way I was initially viewing the old
>>>>> behavior was that returning negative was
>>>>> undefined; it happened to have the
>>>>> outcomes I walked through, but not
>>>>> necessarily by design.
>>>> Having such fallback may bring extra troubles, it requires the eBPF
>>>> program know the existence of the behavior which is not a part of kernel
>>>> ABI actually. And then some eBPF program may start to rely on that which
>>>> is pretty dangerous. Note, one important consideration is to have
>>>> macvtap support where does not have any stuffs like automq.
>>>>
>>>> Thanks
>>>>
>>> How about we call this TUN_SSE_ABORT
>>> instead of TUN_SSE_DO_AUTOMQ?
>>>
>>> TUN_SSE_ABORT could be documented as
>>> falling back to the default queue
>>> selection method in either space
>>> (presumably macvtap has some queue
>>> selection method when there is no prog).
>>
>> This looks like a more complex API, we don't want userspace to differ
>> macvtap from tap too much.
>>
>> Thanks
>>
> This is barely more complex and provides
> similar to what is done in many places.
> For xdp, an XDP_PASS enacts what the
> kernel would do if there was no bpf prog.
> For tc cls in da mode, TC_ACT_OK enacts
> what the kernel would do if there was
> no bpf prog. For xt_bpf, false enacts
> what the kernel would do if there was
> no bpf prog (as long as negation
> isn't in play in the rule, I believe).

I think this is simply because you can't implement e.g
XDP_PASS/TC_ACT_OK through eBPF itself which is not the case of steering
prog here.

>
> I know that this is somewhat of an
> oversimplification and that each of
> these also means something else in
> the respective hookpoint, but I standby
> seeing value in this change.
>
> macvtap must have some default (i.e the
> action which it takes when no prog is
> loaded), even if that is just use queue
> 0. We can provide the same TUN_SSE_ABORT
> in userspace which does the same thing;
> enacts the default when returned. Any
> differences left between tap and macvtap
> would be in what the default is, not in
> these changes. And that difference already
> exists today.

I think it's better to safe to just drop the packet instead of trying to
workaround it.

Thanks

>
>>>>> In order to keep the new behavior
>>>>> extensible, how should we state that a
>>>>> negative return other than -1 is
>>>>> undefined and therefore subject to
>>>>> change. Is something like this
>>>>> sufficient?
>>>>>
>>>>> Documentation/networking/tc-actions-env-rules.txt
>>>>>
>>>>> Additionally, what should the new
>>>>> behavior implement when a negative other
>>>>> than -1 is returned? I would like to have
>>>>> it do the same thing as -1 for now, but
>>>>> with the understanding that this behavior
>>>>> is undefined. Does this sound reasonable?
>>>>>
>>>>>>>> 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?
>>>>>> We'll need something at runtime - people move binaries between kernels
>>>>>> without rebuilding then. An ioctl is one option.
>>>>>> A sysfs attribute is another, an ethtool flag yet another.
>>>>>> A combination of these is possible.
>>>>>>
>>>>>> And if we are doing this anyway, maybe let userspace select
>>>>>> the new behaviour? This way we can stay compatible with old
>>>>>> userspace...
>>>>>>
>>>>> Understood. I'll look into adding an
>>>>> ioctl to activate the new behavior. And
>>>>> perhaps a method of checking which is
>>>>> behavior is currently active (in case we
>>>>> ever want to change the default, say
>>>>> after some suitably long transition
>>>>> period).
>>>>>
>>>>>>>> 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