netdev - Re: [RFC bpf-next 3/7] bpf: implement devtx hook points

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date: Tue, 13 Jun 2023 12:00:30 -0700
From: Stanislav Fomichev <sdf@...gle.com>
To: Willem de Bruijn <willemdebruijn.kernel@...il.com>
Cc: bpf@...r.kernel.org, ast@...nel.org, daniel@...earbox.net, 
	andrii@...nel.org, martin.lau@...ux.dev, song@...nel.org, yhs@...com, 
	john.fastabend@...il.com, kpsingh@...nel.org, haoluo@...gle.com, 
	jolsa@...nel.org, netdev@...r.kernel.org
Subject: Re: [RFC bpf-next 3/7] bpf: implement devtx hook points

On Tue, Jun 13, 2023 at 7:55 AM Willem de Bruijn
<willemdebruijn.kernel@...il.com> wrote:
>
> On Mon, Jun 12, 2023 at 7:24 PM Stanislav Fomichev <sdf@...gle.com> wrote:
> >
> > devtx is a lightweight set of hooks before and after packet transmission.
> > The hook is supposed to work for both skb and xdp paths by exposing
> > a light-weight packet wrapper via devtx_frame (header portion + frags).
> >
> > devtx is implemented as a tracing program which has access to the
> > XDP-metadata-like kfuncs. The initial set of kfuncs is implemented
> > in the next patch, but the idea is similar to XDP metadata:
> > the kfuncs have netdev-specific implementation, but common
> > interface. Upon loading, the kfuncs are resolved to direct
> > calls against per-netdev implementation. This can be achieved
> > by marking devtx-tracing programs as dev-bound (largely
> > reusing xdp-dev-bound program infrastructure).
> >
> > Attachment and detachment is implemented via syscall BPF program
> > by calling bpf_devtx_sb_attach (attach to tx-submission)
> > or bpf_devtx_cp_attach (attach to tx completion). Right now,
> > the attachment does not return a link and doesn't support
> > multiple programs. I plan to switch to Daniel's bpf_mprog infra
> > once it's available.
> >
> > Cc: netdev@...r.kernel.org
> > Signed-off-by: Stanislav Fomichev <sdf@...gle.com>
>
>
> > @@ -2238,6 +2238,8 @@ struct net_device {
> >         unsigned int            real_num_rx_queues;
> >
> >         struct bpf_prog __rcu   *xdp_prog;
> > +       struct bpf_prog __rcu   *devtx_sb;
> > +       struct bpf_prog __rcu   *devtx_cp;
>
> nit/subjective: non-obvious two letter acronyms are nr. How about tx
> and txc (or txcomp)

devtx and devtxc? I was using devtxs vs devtxc initially, but that
seems confusing. I can probably spell them out here:
devtx_submit
devtx_complete

Should probably be better?

> > +static int __bpf_devtx_attach(struct net_device *netdev, int prog_fd,
> > +                             const char *attach_func_name, struct bpf_prog **pprog)
> > +{
> > +       struct bpf_prog *prog;
> > +       int ret = 0;
> > +
> > +       if (prog_fd < 0)
> > +               return __bpf_devtx_detach(netdev, pprog);
> > +
> > +       if (*pprog)
> > +               return -EBUSY;
> > +
> > +       prog = bpf_prog_get(prog_fd);
> > +       if (IS_ERR(prog))
> > +               return PTR_ERR(prog);
> > +
> > +       if (prog->type != BPF_PROG_TYPE_TRACING ||
> > +           prog->expected_attach_type != BPF_TRACE_FENTRY ||
> > +           !bpf_prog_is_dev_bound(prog->aux) ||
> > +           !bpf_offload_dev_match(prog, netdev) ||
> > +           strcmp(prog->aux->attach_func_name, attach_func_name)) {
> > +               bpf_prog_put(prog);
> > +               return -EINVAL;
> > +       }
> > +
> > +       *pprog = prog;
> > +       static_branch_inc(&devtx_enabled);
> > +
> > +       return ret;
>
> nit: just return 0, no variable needed

Ack.

> > +}
> > +
> > +__diag_push();
> > +__diag_ignore_all("-Wmissing-prototypes",
> > +                 "Global functions as their definitions will be in vmlinux BTF");
> > +
> > +/**
> > + * bpf_devtx_sb_attach - Attach devtx 'packet submit' program
> > + * @ifindex: netdev interface index.
> > + * @prog_fd: BPF program file descriptor.
> > + *
> > + * Return:
> > + * * Returns 0 on success or ``-errno`` on error.
> > + */
> > +__bpf_kfunc int bpf_devtx_sb_attach(int ifindex, int prog_fd)
> > +{
> > +       struct net_device *netdev;
> > +       int ret;
> > +
> > +       netdev = dev_get_by_index(current->nsproxy->net_ns, ifindex);
> > +       if (!netdev)
> > +               return -EINVAL;
> > +
> > +       mutex_lock(&devtx_attach_lock);
> > +       ret = __bpf_devtx_attach(netdev, prog_fd, "devtx_sb", &netdev->devtx_sb);
> > +       mutex_unlock(&devtx_attach_lock);
> > +
> > +       dev_put(netdev);
> > +
> > +       return ret;
> > +}
> > +
> > +/**
> > + * bpf_devtx_cp_attach - Attach devtx 'packet complete' program
> > + * @ifindex: netdev interface index.
> > + * @prog_fd: BPF program file descriptor.
> > + *
> > + * Return:
> > + * * Returns 0 on success or ``-errno`` on error.
> > + */
> > +__bpf_kfunc int bpf_devtx_cp_attach(int ifindex, int prog_fd)
> > +{
> > +       struct net_device *netdev;
> > +       int ret;
> > +
> > +       netdev = dev_get_by_index(current->nsproxy->net_ns, ifindex);
> > +       if (!netdev)
> > +               return -EINVAL;
> > +
> > +       mutex_lock(&devtx_attach_lock);
> > +       ret = __bpf_devtx_attach(netdev, prog_fd, "devtx_cp", &netdev->devtx_cp);
> > +       mutex_unlock(&devtx_attach_lock);
> > +
> > +       dev_put(netdev);
> > +
> > +       return ret;
> > +}
>
> These two functions are near duplicates, aside from the arguments to
> their inner call to __bpf_devtx_attach. Can be dedup-ed further?

I've deduped most of the stuff via __bpf_devtx_attach; can probaby
dedup the rest with a bool argument, let me try...