netdev - Re: [PATCH bpf-next v1 06/13] bpf: net_sched: Add basic bpf qdisc kfuncs

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]

Message-ID: <CAMB2axPioMoEwLAH4y-nPjYngq_+uv5PiaO=708rS98=t8dEvg@mail.gmail.com>
Date: Wed, 18 Dec 2024 09:11:32 -0800
From: Amery Hung <ameryhung@...il.com>
To: Amery Hung <amery.hung@...edance.com>
Cc: netdev@...r.kernel.org, bpf@...r.kernel.org, daniel@...earbox.net, 
	andrii@...nel.org, alexei.starovoitov@...il.com, martin.lau@...nel.org, 
	sinquersw@...il.com, toke@...hat.com, jhs@...atatu.com, jiri@...nulli.us, 
	stfomichev@...il.com, ekarani.silvestre@....ufcg.edu.br, 
	yangpeihao@...u.edu.cn, xiyou.wangcong@...il.com, yepeilin.cs@...il.com
Subject: Re: [PATCH bpf-next v1 06/13] bpf: net_sched: Add basic bpf qdisc kfuncs

On Fri, Dec 13, 2024 at 3:30 PM Amery Hung <amery.hung@...edance.com> wrote:
>
> Add basic kfuncs for working on skb in qdisc.
>
> Both bpf_qdisc_skb_drop() and bpf_kfree_skb() can be used to release
> a reference to an skb. However, bpf_qdisc_skb_drop() can only be called
> in .enqueue where a to_free skb list is available from kernel to defer
> the release. bpf_kfree_skb() should be used elsewhere. It is also used
> in bpf_obj_free_fields() when cleaning up skb in maps and collections.
>
> bpf_skb_get_hash() returns the flow hash of an skb, which can be used
> to build flow-based queueing algorithms.
>
> Finally, allow users to create read-only dynptr via bpf_dynptr_from_skb().
>
> Signed-off-by: Amery Hung <amery.hung@...edance.com>
> ---
>  net/sched/bpf_qdisc.c | 77 ++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 76 insertions(+), 1 deletion(-)
>
> diff --git a/net/sched/bpf_qdisc.c b/net/sched/bpf_qdisc.c
> index a2e2db29e5fc..28959424eab0 100644
> --- a/net/sched/bpf_qdisc.c
> +++ b/net/sched/bpf_qdisc.c
> @@ -106,6 +106,67 @@ static int bpf_qdisc_btf_struct_access(struct bpf_verifier_log *log,
>         return 0;
>  }
>
> +__bpf_kfunc_start_defs();
> +
> +/* bpf_skb_get_hash - Get the flow hash of an skb.
> + * @skb: The skb to get the flow hash from.
> + */
> +__bpf_kfunc u32 bpf_skb_get_hash(struct sk_buff *skb)
> +{
> +       return skb_get_hash(skb);
> +}
> +
> +/* bpf_kfree_skb - Release an skb's reference and drop it immediately.
> + * @skb: The skb whose reference to be released and dropped.
> + */
> +__bpf_kfunc void bpf_kfree_skb(struct sk_buff *skb)
> +{
> +       kfree_skb(skb);
> +}
> +
> +/* bpf_qdisc_skb_drop - Drop an skb by adding it to a deferred free list.
> + * @skb: The skb whose reference to be released and dropped.
> + * @to_free_list: The list of skbs to be dropped.
> + */
> +__bpf_kfunc void bpf_qdisc_skb_drop(struct sk_buff *skb,
> +                                   struct bpf_sk_buff_ptr *to_free_list)
> +{
> +       __qdisc_drop(skb, (struct sk_buff **)to_free_list);
> +}
> +
> +__bpf_kfunc_end_defs();
> +
> +#define BPF_QDISC_KFUNC_xxx \
> +       BPF_QDISC_KFUNC(bpf_skb_get_hash, KF_TRUSTED_ARGS) \
> +       BPF_QDISC_KFUNC(bpf_kfree_skb, KF_RELEASE) \
> +       BPF_QDISC_KFUNC(bpf_qdisc_skb_drop, KF_RELEASE) \
> +
> +BTF_KFUNCS_START(bpf_qdisc_kfunc_ids)
> +#define BPF_QDISC_KFUNC(name, flag) BTF_ID_FLAGS(func, name, flag)
> +BPF_QDISC_KFUNC_xxx
> +#undef BPF_QDISC_KFUNC
> +BTF_ID_FLAGS(func, bpf_dynptr_from_skb, KF_TRUSTED_ARGS)
> +BTF_KFUNCS_END(bpf_qdisc_kfunc_ids)
> +
> +#define BPF_QDISC_KFUNC(name, _) BTF_ID_LIST_SINGLE(name##_ids, func, name)
> +BPF_QDISC_KFUNC_xxx
> +#undef BPF_QDISC_KFUNC
> +
> +static int bpf_qdisc_kfunc_filter(const struct bpf_prog *prog, u32 kfunc_id)
> +{

Here is a null pointer dereference since prog->aux->attach_func_name
is not populated yet during check_cfg(). I will add:

        if (!btf_id_set8_contains(&bpf_qdisc_kfunc_ids, kfunc_id) ||
            !prog->aux->attach_func_name)
                return 0;

> +       if (kfunc_id == bpf_qdisc_skb_drop_ids[0])
> +               if (strcmp(prog->aux->attach_func_name, "enqueue"))
> +                       return -EACCES;
> +
> +       return 0;
> +}
> +
> +static const struct btf_kfunc_id_set bpf_qdisc_kfunc_set = {
> +       .owner = THIS_MODULE,
> +       .set   = &bpf_qdisc_kfunc_ids,
> +       .filter = bpf_qdisc_kfunc_filter,
> +};
> +
>  static const struct bpf_verifier_ops bpf_qdisc_verifier_ops = {
>         .get_func_proto         = bpf_qdisc_get_func_proto,
>         .is_valid_access        = bpf_qdisc_is_valid_access,
> @@ -209,6 +270,20 @@ static struct bpf_struct_ops bpf_Qdisc_ops = {
>
>  static int __init bpf_qdisc_kfunc_init(void)
>  {
> -       return register_bpf_struct_ops(&bpf_Qdisc_ops, Qdisc_ops);
> +       int ret;
> +       const struct btf_id_dtor_kfunc skb_kfunc_dtors[] = {
> +               {
> +                       .btf_id       = bpf_sk_buff_ids[0],
> +                       .kfunc_btf_id = bpf_kfree_skb_ids[0]
> +               },
> +       };
> +
> +       ret = register_btf_kfunc_id_set(BPF_PROG_TYPE_STRUCT_OPS, &bpf_qdisc_kfunc_set);
> +       ret = ret ?: register_btf_id_dtor_kfuncs(skb_kfunc_dtors,
> +                                                ARRAY_SIZE(skb_kfunc_dtors),
> +                                                THIS_MODULE);
> +       ret = ret ?: register_bpf_struct_ops(&bpf_Qdisc_ops, Qdisc_ops);
> +
> +       return ret;
>  }
>  late_initcall(bpf_qdisc_kfunc_init);
> --
> 2.20.1
>