On Thu, May 16, 2024 at 4:45 PM Martin KaFai Lau <martin.lau@xxxxxxxxx> wrote: > > 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. > Got it. I will improve the clarity of the commit message. In addition, I will also remove "struct_ops_ref_acquire_dup_ref.c" as whether duplicate references can be acquired through kfunc is out of scope (should be taken care of by struct_ops implementer). Actually, this testcase should load the and it does load... As for the name, do you have any thoughts? Thanks, Amery > > > > > [0] https://lore.kernel.org/netdev/2d31261b245828d09d2f80e0953e911a9c38573a.1705432850.git.amery.hung@xxxxxxxxxxxxx/ >
