netdev - Re: [RFC PATCH v8 18/20] selftests: Add a bpf fq qdisc to selftest

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] [day] [month] [year] [list]

Date: Mon, 27 May 2024 12:09:00 +0200
From: Toke Høiland-Jørgensen <toke@...hat.com>
To: Martin KaFai Lau <martin.lau@...ux.dev>, Amery Hung <ameryhung@...il.com>
Cc: netdev@...r.kernel.org, bpf@...r.kernel.org, yangpeihao@...u.edu.cn,
 daniel@...earbox.net, andrii@...nel.org, martin.lau@...nel.org,
 sinquersw@...il.com, jhs@...atatu.com, jiri@...nulli.us, sdf@...gle.com,
 xiyou.wangcong@...il.com, yepeilin.cs@...il.com
Subject: Re: [RFC PATCH v8 18/20] selftests: Add a bpf fq qdisc to selftest

Martin KaFai Lau <martin.lau@...ux.dev> writes:

> On 5/24/24 12:40 AM, Toke Høiland-Jørgensen wrote:
>> I think behaviour like this is potentially quite interesting and will
>> allow some neat optimisations (skipping a redirect to a different
>> interface and just directly enqueueing it to a different place comes to
>
> hmm... I am not sure it is a good/safe optimization. From looking at
> skb_do_redirect, there are quite a few things bypassed from
> __dev_queue_xmit upto the final dequeue of the redirected dev. I don't
> know if all of them is not dev dependent.

There are certainly footguns, but as long as they are of the "break the
data path" variety and not the "immediately crash the kernel" variety
that may be OK. After all, you can already do plenty of convoluted
things with BPF that will break things. And glancing through the
redirect code, nothing immediately jumps out as something that will
definitely crash, AFAICT.

However, it does feel a bit risky, so I am also totally fine with
disallowing this until someone comes up with a concrete use case where
it would be beneficial :)

>> mind). However, as you point out it may lead to weird things like a
>> mismatched skb->dev, so if we allow this we should make sure that the
>> kernel will disallow (or fix) such behaviour.
>
> Have been thinking about the skb->dev "fix" but the thought is originally for 
> the bpf_skb_set_dev() use case in patch 14.
>
> Note that the struct_ops ".dequeue" is actually realized by a fentry trampoline 
> (call it fentry ".dequeue"). May be using an extra fexit ".dequeue" here. The 
> fexit ".dequeue" will be called after the fentry ".dequeue". The fexit 
> ".dequeue" has the function arguments (sch here that has the correct dev) and 
> the return value (skb) from the fentry ".dequeue". This will be an extra call 
> (to the fexit ".dequeue") and very specific to this use case but may be the less 
> evil solution I can think of now...

That's an interesting idea, certainly! Relying on fexit functions
to do specific sanity checks/fixups after a BPF program has run
(enforcing/checking post-conditions, basically) does not seem totally
crazy to me, and may have other applications :)

-Toke