From mboxrd@z Thu Jan 1 00:00:00 1970 Authentication-Results: mail.toke.dk; spf=pass (mailfrom) smtp.mailfrom=linux.dev (client-ip=2001:41d0:1004:224b::2e; helo=out-46.mta0.migadu.com; envelope-from=martin.lau@linux.dev; receiver=) Received: from out-46.mta0.migadu.com (out-46.mta0.migadu.com [IPv6:2001:41d0:1004:224b::2e]) by mail.toke.dk (Postfix) with ESMTPS id 764699CD2D0 for ; Thu, 8 Dec 2022 23:53:47 +0100 (CET) Message-ID: Date: Thu, 8 Dec 2022 14:53:37 -0800 MIME-Version: 1.0 Content-Language: en-US To: Stanislav Fomichev References: <20221206024554.3826186-1-sdf@google.com> <20221206024554.3826186-4-sdf@google.com> <391b9abf-c53a-623c-055f-60768c716baa@linux.dev> X-Report-Abuse: Please report any abuse attempt to abuse@migadu.com and include these headers. From: Martin KaFai Lau In-Reply-To: Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit Message-ID-Hash: FEZA7PBNM5OW3QDJLBD762MFB4SZKBGC X-Message-ID-Hash: FEZA7PBNM5OW3QDJLBD762MFB4SZKBGC X-MailFrom: martin.lau@linux.dev X-Mailman-Rule-Misses: dmarc-mitigation; no-senders; approved; emergency; loop; banned-address; member-moderation; nonmember-moderation; administrivia; implicit-dest; max-recipients; max-size; news-moderation; no-subject; digests; suspicious-header CC: ast@kernel.org, daniel@iogearbox.net, andrii@kernel.org, song@kernel.org, yhs@fb.com, john.fastabend@gmail.com, kpsingh@kernel.org, haoluo@google.com, jolsa@kernel.org, David Ahern , Jakub Kicinski , Willem de Bruijn , Jesper Dangaard Brouer , Anatoly Burakov , Alexander Lobakin , Magnus Karlsson , Maryam Tahhan , xdp-hints@xdp-project.net, netdev@vger.kernel.org, bpf@vger.kernel.org X-Mailman-Version: 3.3.7 Precedence: list Subject: [xdp-hints] Re: [PATCH bpf-next v3 03/12] bpf: XDP metadata RX kfuncs List-Id: XDP hardware hints design discussion Archived-At: List-Archive: List-Help: List-Owner: List-Post: List-Subscribe: List-Unsubscribe: On 12/8/22 11:07 AM, Stanislav Fomichev wrote: >>> @@ -102,11 +112,25 @@ int bpf_prog_offload_init(struct bpf_prog *prog, union bpf_attr *attr) >>> if (err) >>> goto err_maybe_put; >>> >>> + prog->aux->offload_requested = !(attr->prog_flags & BPF_F_XDP_HAS_METADATA); >>> + >> >> If I read the set correctly, bpf prog can either use metadata kfunc or offload >> but not both. It is fine to start with only supporting metadata kfunc when there >> is no offload but will be useful to understand the reason. I assume an offloaded >> bpf prog should still be able to call the bpf helpers like adjust_head/tail and >> the same should go for any kfunc? > > Yes, I'm assuming there should be some work on the offloaded device > drivers to support metadata kfuncs. > Offloaded kfuncs, in general, seem hard (how do we call kernel func > from the device-offloaded prog?); so refusing kfuncs early for the > offloaded case seems fair for now? Ah, ok. I was actually thinking the HW offloaded prog can just use the software ndo_* kfunc (like other bpf-helpers). From skimming some bpf_prog_offload_ops:prepare implementation, I think you are right and it seems BPF_PSEUDO_KFUNC_CALL has not been recognized yet. [ ... ] >>> @@ -226,10 +263,17 @@ static void __bpf_prog_offload_destroy(struct bpf_prog *prog) >>> >>> void bpf_prog_offload_destroy(struct bpf_prog *prog) >>> { >>> + struct net_device *netdev = NULL; >>> + >>> down_write(&bpf_devs_lock); >>> - if (prog->aux->offload) >>> + if (prog->aux->offload) { >>> + netdev = prog->aux->offload->netdev; >>> __bpf_prog_offload_destroy(prog); >>> + } >>> up_write(&bpf_devs_lock); >>> + >>> + if (netdev) >> >> May be I have missed a refcnt or lock somewhere. Is it possible that netdev may >> have been freed? > > Yeah, with the offload framework, there are no refcnts. We put an > "offloaded" device into a separate hashtable (protected by > rtnl/semaphore). > maybe_remove_bound_netdev will re-grab the locks (due to ordering: > rtnl->bpf_devs_lock) and remove the device from the hashtable if it's > still there. > At least this is how, I think, it should work; LMK if something is > still fishy here... > > Or is the concern here that somebody might allocate new netdev reusing > the same address? I think I have enough checks in > maybe_remove_bound_netdev to guard against that. Or, at least, to make > it safe :-) Race is ok because ondev needs to be removed anyway when '!ondev->offdev && list_empty(&ondev->progs)'? hmmm... tricky, please add a comment. :) Why it cannot be done together in the bpf_devs_lock above? The above cannot take an extra rtnl_lock before bpf_devs_lock?