Re: [PATCH bpf-next v5 1/8] bpf: Add support for forcing kfunc args to be referenced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Fri, Jun 24, 2022 at 12:56:30AM +0530, Kumar Kartikeya Dwivedi wrote:
> Similar to how we detect mem, size pairs in kfunc, teach verifier to
> treat __ref suffix on argument name to imply that it must be a
> referenced pointer when passed to kfunc. This is required to ensure that
> kfunc that operate on some object only work on acquired pointers and not
> normal PTR_TO_BTF_ID with same type which can be obtained by pointer
> walking. Release functions need not specify such suffix on release
> arguments as they are already expected to receive one referenced
> argument.
> 
> Note that we use strict type matching when a __ref suffix is present on
> the argument.
... 
> +		/* Check if argument must be a referenced pointer, args + i has
> +		 * been verified to be a pointer (after skipping modifiers).
> +		 */
> +		arg_ref = is_kfunc_arg_ref(btf, args + i);
> +		if (is_kfunc && arg_ref && !reg->ref_obj_id) {
> +			bpf_log(log, "R%d must be referenced\n", regno);
> +			return -EINVAL;
> +		}
> +

imo this suffix will be confusing to use.
If I understand the intent the __ref should only be used
in acquire (and other) kfuncs that also do release.
Adding __ref to actual release kfunc will be a nop.
It will be checked, but it's not necessary.

At the end
+struct nf_conn *bpf_ct_insert_entry(struct nf_conn___init *nfct__ref)
will behave like kptr_xchg with exception that kptr_xchg takes any btf_id
while here it's fixed.

The code:
 if (rel && reg->ref_obj_id)
        arg_type |= OBJ_RELEASE;
should probably be updated with '|| arg_ref'
to make sure reg->off == 0 ?
That looks like a small bug.

But stepping back... why __ref is needed ?
We can add bpf_ct_insert_entry to acq and rel sets and it should work?
I'm assuming you're doing the orthogonal cleanup of resolve_btfid,
so we will have a single kfunc set where bpf_ct_insert_entry will
have both acq and rel flags.
I'm surely missing something.



[Index of Archives]     [Linux Samsung SoC]     [Linux Rockchip SoC]     [Linux Actions SoC]     [Linux for Synopsys ARC Processors]     [Linux NFS]     [Linux NILFS]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]


  Powered by Linux