From: Stanislav Fomichev <sdf@google.com>
To: Martin KaFai Lau <martin.lau@linux.dev>
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 20:06:36 -0800 [thread overview]
Message-ID: <CAKH8qBtCrAqxTzSECyG2VjO7rx27mdSEKMwXadrvVOvDaf5rBg@mail.gmail.com> (raw)
In-Reply-To: <5983e0f0-e1ee-5843-33ea-64d139e2e849@linux.dev>
On Thu, Dec 22, 2022 at 4:37 PM Martin KaFai Lau <martin.lau@linux.dev> wrote:
>
> 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?
Seems safe to drop. _inherit has a 'old_prog->aux->offload' check and
_init has a check after dev_get_by_index.
> > 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.
Sure.
> > +
> > + err = __bpf_prog_dev_bound_init(prog, netdev);
> > + if (err)
> > + goto out;
>
> nit. goto can be saved.
Ack, will drop here; although, will still keep the goto above for
dev_put(netdev).
> > +
> > +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.
Agreed. Seems like this will cause bpf_prog_dev_bound_destroy to be
called when we return with an error below; but seems safe since we're
also doing an 'aux->offload' check in there.
> > +
> > + err = __bpf_prog_dev_bound_init(new_prog, old_prog->aux->offload->netdev);
> > + if (err)
> > + goto out;
>
> goto can be saved.
Thx.
> > +
> > +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.
We do the following in bpf_prog_dev_bound_inherit which should be enough?
if (!bpf_prog_is_dev_bound(old_prog->aux))
return 0;
Or am I missing something?
> > + 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 4:07 UTC|newest]
Thread overview: 38+ 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
[not found] ` <202212211311.e2ZWQLue-lkp@intel.com>
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
2022-12-23 4:06 ` Stanislav Fomichev [this message]
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=CAKH8qBtCrAqxTzSECyG2VjO7rx27mdSEKMwXadrvVOvDaf5rBg@mail.gmail.com \
--to=sdf@google.com \
--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=martin.lau@linux.dev \
--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 a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).