Message-ID: <184079b1-1ad0-414d-b8ff-179b5525c439@linux.dev>
Date: Thu, 16 May 2024 16:43:49 -0700
From: Martin KaFai Lau <martin.lau@...ux.dev>
To: Amery Hung <ameryhung@...il.com>, Kui-Feng Lee <sinquersw@...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,
 toke@...hat.com, jhs@...atatu.com, jiri@...nulli.us, sdf@...gle.com,
 xiyou.wangcong@...il.com, yepeilin.cs@...il.com
Subject: Re: [RFC PATCH v8 02/20] selftests/bpf: Test referenced kptr
 arguments of struct_ops programs

On 5/16/24 4:14 PM, Amery Hung wrote:
> I thought about patch 1-4 a bit more after the discussion in LSFMMBPF and
> I think we should keep what "ref_acquried" does, but maybe rename it to
> "ref_moved".
> 
> We discussed the lifecycle of skb in qdisc and changes to struct_ops and
> bpf semantics. In short, At the beginning of .enqueue, the kernel passes
> the ownership of an skb to a qdisc. We do not increase the reference count
> of skb since this is an ownership transfer, not kernel and qdisc both
> holding references to the skb. (The counterexample can be found in RFC v7.
> See how weird skb release kfuncs look[0]). The skb should be either
> enqueued or dropped. Then, in .dequeue, an skb will be removed from the
> queue and the ownership will be returned to the kernel.
> 
> Referenced kptr in bpf already carries the semantic of ownership. Thus,
> what we need here is to enable struct_ops programs to get a referenced
> kptr from the argument and returning referenced kptr (achieved via patch
> 1-4).
> 
> Proper handling of referenced objects is important for safety reasons.
> In the case of bpf qdisc, there are three problematic situations as listed
> below, and referenced kptr has taken care of (1) and (2).
> 
> (1) .enqueue not enqueueing nor dropping the skb, causing reference leak
> 
> (2) .dequeue making up an invalid skb ptr and returning to kernel
> 
> (3) If bpf qdisc operators can duplicate skb references, multiple
>      references to the same skb can be present. If we enqueue these
>      references to a collection and dequeue one, since skb->dev will be
>      restored after the skb is removed from the collection, other skb in
>      the collection will then have invalid skb->rbnode as "dev" and "rbnode"
>      share the same memory.
> 
> A discussion point was about introducing and enforcing a unique reference
> semantic (PTR_UNIQUE) to mitigate (3). After giving it more thoughts, I
> think we should keep "ref_acquired", and be careful about kernel-side
> implementation that could return referenced kptr. Taking a step back, (3)
> is only problematic because I made an assumption that the kfunc only
> increases the reference count of skb (i.e., skb_get()). It could have been
> done safely using skb_copy() or maybe pskb_copy(). In other words, it is a
> kernel implementation issue, and not a verifier issue. Besides, the
> verifier has no knowledge about what a kfunc with KF_ACQUIRE does
> internally whatsoever.
> 
> In v8, we try to do this safely by only allowing reading "ref_acquired"-
> annotated argument once. Since the argument passed to struct_ops never
> changes when during a single invocation, it will always be referencing the
> same kernel object. Therefore, reading more than once and returning
> mulitple references shouldn't be allowed. Maybe "ref_moved" is a more
> precise annotation label, hinting that the ownership is transferred.

The part that no skb acquire kfunc should be available to the qdisc struct_ops 
prog is understood. I think it just needs to clarify the commit message and 
remove the "It must be released and cannot be acquired more than once" part.


> 
> [0] https://lore.kernel.org/netdev/2d31261b245828d09d2f80e0953e911a9c38573a.1705432850.git.amery.hung@bytedance.com/

Powered by blists - more mailing lists