On Tue, Dec 13, 2022 at 5:54 PM Martin KaFai Lau <martin.lau@xxxxxxxxx> wrote:
>
> On 12/12/22 6:35 PM, Stanislav Fomichev wrote:
> > diff --git a/include/linux/bpf.h b/include/linux/bpf.h
> > index ca22e8b8bd82..de6279725f41 100644
> > --- a/include/linux/bpf.h
> > +++ b/include/linux/bpf.h
> > @@ -2477,6 +2477,8 @@ void bpf_offload_dev_netdev_unregister(struct bpf_offload_dev *offdev,
> > struct net_device *netdev);
> > bool bpf_offload_dev_match(struct bpf_prog *prog, struct net_device *netdev);
> >
> > +void *bpf_dev_bound_resolve_kfunc(struct bpf_prog *prog, u32 func_id);
> > +
>
> This probably requires an inline version for !CONFIG_NET.
Yeah, not sure why my confings didn't catch this :-(
> > diff --git a/kernel/bpf/core.c b/kernel/bpf/core.c
> > index d434a994ee04..c3e501e3e39c 100644
> > --- a/kernel/bpf/core.c
> > +++ b/kernel/bpf/core.c
> > @@ -2097,6 +2097,13 @@ bool bpf_prog_map_compatible(struct bpf_map *map,
> > if (fp->kprobe_override)
> > return false;
> >
> > + /* When tail-calling from a non-dev-bound program to a dev-bound one,
> > + * XDP metadata helpers should be disabled. Until it's implemented,
> > + * prohibit adding dev-bound programs to tail-call maps.
> > + */
> > + if (bpf_prog_is_dev_bound(fp->aux))
> > + return false;
> > +
> > spin_lock(&map->owner.lock);
> > if (!map->owner.type) {
> > /* There's no owner yet where we could check for
> > diff --git a/kernel/bpf/offload.c b/kernel/bpf/offload.c
> > index f714c941f8ea..3b6c9023f24d 100644
> > --- a/kernel/bpf/offload.c
> > +++ b/kernel/bpf/offload.c
> > @@ -757,6 +757,29 @@ void bpf_dev_bound_netdev_unregister(struct net_device *dev)
> > up_write(&bpf_devs_lock);
> > }
> >
> > +void *bpf_dev_bound_resolve_kfunc(struct bpf_prog *prog, u32 func_id)
> > +{
> > + const struct xdp_metadata_ops *ops;
> > + void *p = NULL;
> > +
> > + down_read(&bpf_devs_lock);
> > + if (!prog->aux->offload || !prog->aux->offload->netdev)
>
> This happens when netdev is unregistered in the middle of bpf_prog_load and the
> bpf_offload_dev_match() will eventually fail during dev_xdp_attach()? A comment
> will be useful.
Right, that's the expectation - we load/verify the prog but it's
essentially un-attach-able.
Will try to clarify here.
> > + goto out;
> > +
> > + ops = prog->aux->offload->netdev->xdp_metadata_ops;
> > + if (!ops)
> > + goto out;
> > +
> > + if (func_id == xdp_metadata_kfunc_id(XDP_METADATA_KFUNC_RX_TIMESTAMP))
> > + p = ops->xmo_rx_timestamp;
> > + else if (func_id == xdp_metadata_kfunc_id(XDP_METADATA_KFUNC_RX_HASH))
> > + p = ops->xmo_rx_hash;
> > +out:
> > + up_read(&bpf_devs_lock);
> > +
> > + return p;
> > +}
> > +
> > static int __init bpf_offload_init(void)
> > {
> > int err;
> > diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> > index 203d8cfeda70..e61fe0472b9b 100644
> > --- a/kernel/bpf/verifier.c
> > +++ b/kernel/bpf/verifier.c
> > @@ -15479,12 +15479,35 @@ static int fixup_kfunc_call(struct bpf_verifier_env *env, struct bpf_insn *insn,
> > struct bpf_insn *insn_buf, int insn_idx, int *cnt)
> > {
> > const struct bpf_kfunc_desc *desc;
> > + void *xdp_kfunc;
> >
> > if (!insn->imm) {
> > verbose(env, "invalid kernel function call not eliminated in verifier pass\n");
> > return -EINVAL;
> > }
> >
> > + *cnt = 0;
> > +
> > + if (xdp_is_metadata_kfunc_id(insn->imm)) {
> > + if (!bpf_prog_is_dev_bound(env->prog->aux)) {
>
> The "xdp_is_metadata_kfunc_id() && (!bpf_prog_is_dev_bound() ||
> bpf_prog_is_offloaded())" test should have been done much earlier in
> add_kfunc_call(). Then the later stage of the verifier does not have to keep
> worrying about it like here.
>
> nit. may be rename xdp_is_metadata_kfunc_id() to bpf_dev_bound_kfunc_id() and
> hide the "!bpf_prog_is_dev_bound() || bpf_prog_is_offloaded()" test into
> bpf_dev_bound_kfunc_check(&env->log, env->prog).
>
> The change in fixup_kfunc_call could then become:
>
> if (bpf_dev_bound_kfunc_id(insn->imm)) {
> xdp_kfunc = bpf_dev_bound_resolve_kfunc(env->prog, insn->imm);
> /* ... */
> }
Makes sense, ty!
> > + verbose(env, "metadata kfuncs require device-bound program\n");
> > + return -EINVAL;
> > + }
> > +
> > + if (bpf_prog_is_offloaded(env->prog->aux)) {
> > + verbose(env, "metadata kfuncs can't be offloaded\n");
> > + return -EINVAL;
> > + }
> > +
> > + xdp_kfunc = bpf_dev_bound_resolve_kfunc(env->prog, insn->imm);
> > + if (xdp_kfunc) {
> > + insn->imm = BPF_CALL_IMM(xdp_kfunc);
> > + return 0;
> > + }
> > +
> > + /* fallback to default kfunc when not supported by netdev */
> > + }
> > +
>
>
