From: Martin KaFai Lau <martin.lau@linux.dev>
To: Stanislav Fomichev <sdf@google.com>
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,
"Toke Høiland-Jørgensen" <toke@redhat.com>,
bpf@vger.kernel.org
Subject: Re: [PATCH bpf-next v5 08/17] bpf: Support consuming XDP HW metadata from fext programs
Date: Thu, 22 Dec 2022 16:37:25 -0800 [thread overview]
Message-ID: <5983e0f0-e1ee-5843-33ea-64d139e2e849@linux.dev> (raw)
In-Reply-To: <20221220222043.3348718-9-sdf@google.com>
On 12/20/22 2:20 PM, Stanislav Fomichev wrote:
> diff --git a/kernel/bpf/offload.c b/kernel/bpf/offload.c
> index 0e3fc743e0a8..60978a1f9baa 100644
> --- a/kernel/bpf/offload.c
> +++ b/kernel/bpf/offload.c
> @@ -187,17 +187,13 @@ static void __bpf_offload_dev_netdev_unregister(struct bpf_offload_dev *offdev,
> kfree(ondev);
> }
>
> -int bpf_prog_dev_bound_init(struct bpf_prog *prog, union bpf_attr *attr)
> +static int __bpf_prog_dev_bound_init(struct bpf_prog *prog, struct net_device *netdev)
> {
> struct bpf_offload_netdev *ondev;
> struct bpf_prog_offload *offload;
> int err;
>
> - if (attr->prog_type != BPF_PROG_TYPE_SCHED_CLS &&
> - attr->prog_type != BPF_PROG_TYPE_XDP)
> - return -EINVAL;
> -
> - if (attr->prog_flags & ~BPF_F_XDP_DEV_BOUND_ONLY)
> + if (!netdev)
Is this !netdev test needed?
> return -EINVAL;
>
> offload = kzalloc(sizeof(*offload), GFP_USER);
> @@ -205,21 +201,13 @@ int bpf_prog_dev_bound_init(struct bpf_prog *prog, union bpf_attr *attr)
> return -ENOMEM;
>
> offload->prog = prog;
> + offload->netdev = netdev;
>
> - offload->netdev = dev_get_by_index(current->nsproxy->net_ns,
> - attr->prog_ifindex);
> - err = bpf_dev_offload_check(offload->netdev);
> - if (err)
> - goto err_maybe_put;
> -
> - prog->aux->offload_requested = !(attr->prog_flags & BPF_F_XDP_DEV_BOUND_ONLY);
> -
> - down_write(&bpf_devs_lock);
> ondev = bpf_offload_find_netdev(offload->netdev);
> if (!ondev) {
> if (bpf_prog_is_offloaded(prog->aux)) {
> err = -EINVAL;
> - goto err_unlock;
> + goto err_free;
> }
>
> /* When only binding to the device, explicitly
> @@ -227,25 +215,80 @@ int bpf_prog_dev_bound_init(struct bpf_prog *prog, union bpf_attr *attr)
> */
> err = __bpf_offload_dev_netdev_register(NULL, offload->netdev);
> if (err)
> - goto err_unlock;
> + goto err_free;
> ondev = bpf_offload_find_netdev(offload->netdev);
> }
> offload->offdev = ondev->offdev;
> prog->aux->offload = offload;
> list_add_tail(&offload->offloads, &ondev->progs);
> - dev_put(offload->netdev);
> - up_write(&bpf_devs_lock);
>
> return 0;
> -err_unlock:
> - up_write(&bpf_devs_lock);
> -err_maybe_put:
> - if (offload->netdev)
> - dev_put(offload->netdev);
> +err_free:
> kfree(offload);
> return err;
> }
>
> +int bpf_prog_dev_bound_init(struct bpf_prog *prog, union bpf_attr *attr)
> +{
> + struct net_device *netdev;
> + int err;
> +
> + if (attr->prog_type != BPF_PROG_TYPE_SCHED_CLS &&
> + attr->prog_type != BPF_PROG_TYPE_XDP)
> + return -EINVAL;
> +
> + if (attr->prog_flags & ~BPF_F_XDP_DEV_BOUND_ONLY)
> + return -EINVAL;
> +
> + netdev = dev_get_by_index(current->nsproxy->net_ns, attr->prog_ifindex);
> + if (!netdev)
> + return -EINVAL;
> +
> + down_write(&bpf_devs_lock);
> + err = bpf_dev_offload_check(netdev);
> + if (err)
> + goto out;
> +
> + prog->aux->offload_requested = !(attr->prog_flags & BPF_F_XDP_DEV_BOUND_ONLY);
nit. move the bpf_dev_offload_check() and offload_requested assignment out. I
don't think they need lock protection so that it is clear what the lock is
protecting in the future reading. It seems the original code have them outside
also.
> +
> + err = __bpf_prog_dev_bound_init(prog, netdev);
> + if (err)
> + goto out;
nit. goto can be saved.
> +
> +out:
> + dev_put(netdev);
> + up_write(&bpf_devs_lock);
> + return err;
> +}
> +
> +int bpf_prog_dev_bound_inherit(struct bpf_prog *new_prog, struct bpf_prog *old_prog)
> +{
> + int err;
> +
> + if (!bpf_prog_is_dev_bound(old_prog->aux))
> + return 0;
> +
> + if (bpf_prog_is_offloaded(old_prog->aux))
> + return -EINVAL;
> +
> + down_write(&bpf_devs_lock);
> + if (!old_prog->aux->offload) {
> + err = -EINVAL;
> + goto out;
> + }
> +
> + new_prog->aux->dev_bound = old_prog->aux->dev_bound;
> + new_prog->aux->offload_requested = old_prog->aux->offload_requested;
nit. Same here, I think the initialization can be moved outside of the lock.
> +
> + err = __bpf_prog_dev_bound_init(new_prog, old_prog->aux->offload->netdev);
> + if (err)
> + goto out;
goto can be saved.
> +
> +out:
> + up_write(&bpf_devs_lock);
> + return err;
> +}
> +
> int bpf_prog_offload_verifier_prep(struct bpf_prog *prog)
> {
> struct bpf_prog_offload *offload;
> @@ -687,6 +730,22 @@ bool bpf_offload_dev_match(struct bpf_prog *prog, struct net_device *netdev)
> }
> EXPORT_SYMBOL_GPL(bpf_offload_dev_match);
>
> +bool bpf_prog_dev_bound_match(struct bpf_prog *lhs, struct bpf_prog *rhs)
> +{
> + bool ret;
> +
> + if (bpf_prog_is_offloaded(lhs->aux) != bpf_prog_is_offloaded(rhs->aux))
> + return false;
> +
> + down_read(&bpf_devs_lock);
> + ret = lhs->aux->offload && rhs->aux->offload &&
> + lhs->aux->offload->netdev &&
> + lhs->aux->offload->netdev == rhs->aux->offload->netdev;
> + up_read(&bpf_devs_lock);
> +
> + return ret;
> +}
> +
> bool bpf_offload_prog_map_match(struct bpf_prog *prog, struct bpf_map *map)
> {
> struct bpf_offloaded_map *offmap;
> diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
> index 11c558be4992..64a68e8fb072 100644
> --- a/kernel/bpf/syscall.c
> +++ b/kernel/bpf/syscall.c
> @@ -2605,6 +2605,12 @@ static int bpf_prog_load(union bpf_attr *attr, bpfptr_t uattr)
> goto free_prog_sec;
> }
>
> + if (type == BPF_PROG_TYPE_EXT && dst_prog) {
Does it also need to test the bpf_prog_is_dev_bound(dst_prog->aux)? Otherwise,
the bpf_prog_dev_bound_inherit() below will fail on everything for !CONFIG_NET.
> + err = bpf_prog_dev_bound_inherit(prog, dst_prog);
> + if (err)
> + goto free_prog_sec;
> + }
> +
> /* find program type: socket_filter vs tracing_filter */
> err = find_prog_type(type, prog);
> if (err < 0)
> @@ -3021,6 +3027,12 @@ static int bpf_tracing_prog_attach(struct bpf_prog *prog,
> goto out_put_prog;
> }
>
> + if (bpf_prog_is_dev_bound(prog->aux) &&
Like here.
> + !bpf_prog_dev_bound_match(prog, tgt_prog)) {
> + err = -EINVAL;
> + goto out_put_prog;
> + }
> +
> key = bpf_trampoline_compute_key(tgt_prog, NULL, btf_id);
> }
>
> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index 320451a0be3e..64f4d2b5824f 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -16537,11 +16537,6 @@ int bpf_check_attach_target(struct bpf_verifier_log *log,
> if (tgt_prog) {
> struct bpf_prog_aux *aux = tgt_prog->aux;
>
> - if (bpf_prog_is_dev_bound(tgt_prog->aux)) {
> - bpf_log(log, "Replacing device-bound programs not supported\n");
> - return -EINVAL;
> - }
> -
> for (i = 0; i < aux->func_info_cnt; i++)
> if (aux->func_info[i].type_id == btf_id) {
> subprog = i;
next prev parent reply other threads:[~2022-12-23 0:37 UTC|newest]
Thread overview: 39+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-12-20 22:20 [PATCH bpf-next v5 00/17] xdp: hints via kfuncs Stanislav Fomichev
2022-12-20 22:20 ` [PATCH bpf-next v5 01/17] bpf: Document XDP RX metadata Stanislav Fomichev
2022-12-28 17:25 ` David Vernet
2023-01-03 22:23 ` Stanislav Fomichev
2023-01-04 16:02 ` David Vernet
2022-12-20 22:20 ` [PATCH bpf-next v5 02/17] bpf: Rename bpf_{prog,map}_is_dev_bound to is_offloaded Stanislav Fomichev
2022-12-20 22:20 ` [PATCH bpf-next v5 03/17] bpf: Move offload initialization into late_initcall Stanislav Fomichev
2022-12-20 22:20 ` [PATCH bpf-next v5 04/17] bpf: Reshuffle some parts of bpf/offload.c Stanislav Fomichev
2022-12-20 22:20 ` [PATCH bpf-next v5 05/17] bpf: Introduce device-bound XDP programs Stanislav Fomichev
2022-12-23 0:19 ` Martin KaFai Lau
2022-12-23 4:06 ` Stanislav Fomichev
2022-12-20 22:20 ` [PATCH bpf-next v5 06/17] selftests/bpf: Update expected test_offload.py messages Stanislav Fomichev
2022-12-20 22:20 ` [PATCH bpf-next v5 07/17] bpf: XDP metadata RX kfuncs Stanislav Fomichev
2022-12-21 5:43 ` kernel test robot
2022-12-21 17:49 ` Stanislav Fomichev
2022-12-23 0:31 ` Martin KaFai Lau
2022-12-23 4:06 ` Stanislav Fomichev
2022-12-27 20:33 ` David Vernet
2023-01-03 22:23 ` Stanislav Fomichev
2023-01-03 22:35 ` David Vernet
2022-12-20 22:20 ` [PATCH bpf-next v5 08/17] bpf: Support consuming XDP HW metadata from fext programs Stanislav Fomichev
2022-12-23 0:37 ` Martin KaFai Lau [this message]
2022-12-23 4:06 ` Stanislav Fomichev
2023-01-04 1:51 ` Martin KaFai Lau
2023-01-04 3:59 ` Stanislav Fomichev
2022-12-20 22:20 ` [PATCH bpf-next v5 09/17] veth: Introduce veth_xdp_buff wrapper for xdp_buff Stanislav Fomichev
2022-12-20 22:20 ` [PATCH bpf-next v5 10/17] veth: Support RX XDP metadata Stanislav Fomichev
2022-12-20 22:20 ` [PATCH bpf-next v5 11/17] selftests/bpf: Verify xdp_metadata xdp->af_xdp path Stanislav Fomichev
2022-12-23 0:40 ` Martin KaFai Lau
2022-12-23 4:06 ` Stanislav Fomichev
2023-01-04 2:05 ` Martin KaFai Lau
2022-12-20 22:20 ` [PATCH bpf-next v5 12/17] net/mlx4_en: Introduce wrapper for xdp_buff Stanislav Fomichev
2022-12-20 22:20 ` [PATCH bpf-next v5 13/17] net/mlx4_en: Support RX XDP metadata Stanislav Fomichev
2022-12-20 22:20 ` [PATCH bpf-next v5 14/17] xsk: Add cb area to struct xdp_buff_xsk Stanislav Fomichev
2022-12-20 22:20 ` [PATCH bpf-next v5 15/17] net/mlx5e: Introduce wrapper for xdp_buff Stanislav Fomichev
2022-12-20 22:20 ` [PATCH bpf-next v5 16/17] net/mlx5e: Support RX XDP metadata Stanislav Fomichev
2022-12-20 22:20 ` [PATCH bpf-next v5 17/17] selftests/bpf: Simple program to dump XDP RX metadata Stanislav Fomichev
2022-12-23 0:53 ` Martin KaFai Lau
2022-12-23 4:07 ` Stanislav Fomichev
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=5983e0f0-e1ee-5843-33ea-64d139e2e849@linux.dev \
--to=martin.lau@linux.dev \
--cc=andrii@kernel.org \
--cc=ast@kernel.org \
--cc=bpf@vger.kernel.org \
--cc=daniel@iogearbox.net \
--cc=haoluo@google.com \
--cc=john.fastabend@gmail.com \
--cc=jolsa@kernel.org \
--cc=kpsingh@kernel.org \
--cc=sdf@google.com \
--cc=song@kernel.org \
--cc=toke@redhat.com \
--cc=yhs@fb.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.