linux-kernel - Re: [PATCH bpf-next v2 4/9] bpf: Enable cpumasks to be queried and used as kptrs

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, 24 Jan 2023 20:36:02 -0800
From:   Alexei Starovoitov <alexei.starovoitov@...il.com>
To:     David Vernet <void@...ifault.com>
Cc:     bpf@...r.kernel.org, ast@...nel.org, daniel@...earbox.net,
        andrii@...nel.org, martin.lau@...ux.dev, song@...nel.org,
        yhs@...a.com, john.fastabend@...il.com, kpsingh@...nel.org,
        sdf@...gle.com, haoluo@...gle.com, jolsa@...nel.org,
        linux-kernel@...r.kernel.org, kernel-team@...a.com, tj@...nel.org,
        memxor@...il.com
Subject: Re: [PATCH bpf-next v2 4/9] bpf: Enable cpumasks to be queried and
 used as kptrs

On Fri, Jan 20, 2023 at 01:25:18PM -0600, David Vernet wrote:
> +
> +/**
> + * struct bpf_cpumask - refcounted BPF cpumask wrapper structure
> + * @cpumask:	The actual cpumask embedded in the struct.
> + * @usage:	Object reference counter. When the refcount goes to 0, the
> + *		memory is released back to the BPF allocator, which provides
> + *		RCU safety.
> + *
> + * Note that we explicitly embed a cpumask_t rather than a cpumask_var_t.  This
> + * is done to avoid confusing the verifier due to the typedef of cpumask_var_t
> + * changing depending on whether CONFIG_CPUMASK_OFFSTACK is defined or not. See
> + * the details in <linux/cpumask.h>. The consequence is that this structure is
> + * likely a bit larger than it needs to be when CONFIG_CPUMASK_OFFSTACK is
> + * defined due to embedding the whole NR_CPUS-size bitmap, but the extra memory
> + * overhead is minimal. For the more typical case of CONFIG_CPUMASK_OFFSTACK
> + * not being defined, the structure is the same size regardless.
> + */
> +struct bpf_cpumask {
> +	cpumask_t cpumask;
> +	refcount_t usage;
> +};
> +
> +static struct bpf_mem_alloc bpf_cpumask_ma;
> +
> +static bool cpu_valid(u32 cpu)
> +{
> +	return cpu < nr_cpu_ids;
> +}
> +
> +__diag_push();
> +__diag_ignore_all("-Wmissing-prototypes",
> +		  "Global kfuncs as their definitions will be in BTF");
> +
> +struct bpf_cpumask *bpf_cpumask_create(void)
> +{
> +	struct bpf_cpumask *cpumask;
> +
> +	cpumask = bpf_mem_alloc(&bpf_cpumask_ma, sizeof(*cpumask));
> +	if (!cpumask)
> +		return NULL;
> +
> +	memset(cpumask, 0, sizeof(*cpumask));
> +	refcount_set(&cpumask->usage, 1);
> +
> +	return cpumask;
> +}

Applied patches 1 and 2. Patch 3 doesn't apply anymore. Pls rebase.

I'm fine with existing bpf_cpumask proposal, but can we do better?
This is so close to be a bitmap template.

Can we generalize it as
struct bpf_bitmap {
	refcount_t refcnt;
        int num_bits;
        u64 bits[];
};

struct bpf_bitmap *bpf_bitmap_create(int bits)
{
  bitmap = bpf_mem_alloc(&bpf_cpumask_ma, sizeof(*bitmap) + BITS_TO_LONGS(bits) * sizeof(u64));
  bitmap->num_bits = bits;
}

and special case few custom kfuncs in the verifier that allow
type cast from bpf_bitmap with to 'struct cpumask *' ? Like
struct cpumask *bpf_bitmap_cast_to_cpumask(struct bpf_bitmap *bitmap)
{
  if (bitmap->num_bits == nr_cpu_ids)
     return bitmap->bits;
  return NULL;
}
BTF_ID_FLAGS(func, bpf_bitmap_cast_to_cpumask, KF_TRUSTED_ARGS | KF_RET_NULL)

The UX will be a bit worse, since bpf prog would need to do !=NULL check
but with future bpf_assert() we may get rid of !=NULL check.

We can keep direct cpumask accessors as kfuncs:

u32 bpf_cpumask_first(const struct cpumask *cpumask);
u32 bpf_cpumask_first_zero(const struct cpumask *cpumask);

and add bpf_find_first_bit() and the rest of bit manipulations.

Since all of the bpf_cpumask do run-time cpu_valid() check we're not
sacrificing performance.

Feels more generic with wider applicability at the expense of little bit worse UX.
I haven't thought about acq/rel consequences.

wdyt?