| 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
| ||
|
Message-ID: <0f4541d9-a405-6185-7e54-112dc9188146@redhat.com>
Date: Mon, 23 Sep 2019 08:46:04 +0800
From: Jason Wang <jasowang@...hat.com>
To: Matt Cover <werekraken@...il.com>,
"Michael S. Tsirkin" <mst@...hat.com>
Cc: davem@...emloft.net, ast@...nel.org, daniel@...earbox.net,
kafai@...com, songliubraving@...com, yhs@...com,
Eric Dumazet <edumazet@...gle.com>,
Stanislav Fomichev <sdf@...gle.com>,
Matthew Cover <matthew.cover@...ckpath.com>,
mail@...urcelik.de, pabeni@...hat.com,
Nicolas Dichtel <nicolas.dichtel@...nd.com>,
wangli39@...du.com, lifei.shirley@...edance.com,
tglx@...utronix.de, netdev@...r.kernel.org,
linux-kernel@...r.kernel.org, bpf@...r.kernel.org
Subject: Re: [PATCH net-next] tuntap: Fallback to automq on TUNSETSTEERINGEBPF
prog negative return
On 2019/9/23 上午1:43, Matt Cover wrote:
> On Sun, Sep 22, 2019 at 5:37 AM Michael S. Tsirkin <mst@...hat.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@...ckpath.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)
Well, using ebpf means it need to take care of all the cases. E.g you
can easily implement the fallback through eBPF as well.
> 2. determine there isn't sufficient information
> to do proper queue selection; return
> negative and use the default automq logic
> for the unknown
Same as above.
> 3. implement a noop prog (e.g. do
> bpf_trace_printk() then return negative and
> use the default automq logic for everything)
ditto.
>
>> 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.
Yes, eBPF can return probably wrong value, but what kernel did is just
to make sure it doesn't harm anything.
I would rather just drop the packet in this case.
Thanks
>
>> 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
Powered by blists - more mailing lists