| Message ID | 20220623192637.3866852-2-memxor@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | BPF |
| Headers | show |
| Series | New nf_conntrack kfuncs for insertion, changing timeout, status | expand |
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.
On Wed, 29 Jun 2022 at 08:53, Alexei Starovoitov <alexei.starovoitov@gmail.com> wrote: > > 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. > Indeed, I missed that. Thanks for catching it. > 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. It is needed to prevent the case where someone might do: ct = bpf_xdp_ct_alloc(...); bpf_ct_set_timeout(ct->master, ...); Or just obtain PTR_TO_BTF_ID by pointer walking and try to pass it in to bpf_ct_set_timeout. __ref allows an argument on a non-release kfunc to have checks like a release argument, i.e. refcounted, reg->off == 0 (var_off is already checked to be 0), so use the original pointer that was obtained from an acquire kfunc. As you noted, it isn't strictly needed on release kfunc (like bpf_ct_insert_entry) because the same checks happen for it anyway. But both timeout and status helpers should use it if they "operate" on the acquired ct (from alloc, insert, or lookup).
On Sun, 3 Jul 2022 at 10:54, Kumar Kartikeya Dwivedi <memxor@gmail.com> wrote: > > On Wed, 29 Jun 2022 at 08:53, Alexei Starovoitov > <alexei.starovoitov@gmail.com> wrote: > > > > 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. > > > > Indeed, I missed that. Thanks for catching it. > > > 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. > > It is needed to prevent the case where someone might do: > ct = bpf_xdp_ct_alloc(...); > bpf_ct_set_timeout(ct->master, ...); > A better illustration is probably bpf_xdp_ct_lookup and bpf_ct_change_timeout, since here the type for ct->master won't match with bpf_ct_set_timeout, but the point is the same. > Or just obtain PTR_TO_BTF_ID by pointer walking and try to pass it in > to bpf_ct_set_timeout. > > __ref allows an argument on a non-release kfunc to have checks like a > release argument, i.e. refcounted, reg->off == 0 (var_off is already > checked to be 0), so use the original pointer that was obtained from > an acquire kfunc. As you noted, it isn't strictly needed on release > kfunc (like bpf_ct_insert_entry) because the same checks happen for it > anyway. But both timeout and status helpers should use it if they > "operate" on the acquired ct (from alloc, insert, or lookup).
On Sun, Jul 03, 2022 at 11:04:22AM +0530, Kumar Kartikeya Dwivedi wrote: > On Sun, 3 Jul 2022 at 10:54, Kumar Kartikeya Dwivedi <memxor@gmail.com> wrote: > > > > On Wed, 29 Jun 2022 at 08:53, Alexei Starovoitov > > <alexei.starovoitov@gmail.com> wrote: > > > > > > 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. > > > > > > > Indeed, I missed that. Thanks for catching it. > > > > > 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. > > > > It is needed to prevent the case where someone might do: > > ct = bpf_xdp_ct_alloc(...); > > bpf_ct_set_timeout(ct->master, ...); > > > > A better illustration is probably bpf_xdp_ct_lookup and > bpf_ct_change_timeout, since here the type for ct->master won't match > with bpf_ct_set_timeout, but the point is the same. Sorry, I'm still not following. Didn't we make pointer walking 'untrusted' so ct->master cannot be passed into any kfunc? > > Or just obtain PTR_TO_BTF_ID by pointer walking and try to pass it in > > to bpf_ct_set_timeout. > > > > __ref allows an argument on a non-release kfunc to have checks like a > > release argument, i.e. refcounted, reg->off == 0 (var_off is already > > checked to be 0), so use the original pointer that was obtained from > > an acquire kfunc. As you noted, it isn't strictly needed on release > > kfunc (like bpf_ct_insert_entry) because the same checks happen for it > > anyway. But both timeout and status helpers should use it if they > > "operate" on the acquired ct (from alloc, insert, or lookup).
On Thu, 7 Jul 2022 at 00:14, Alexei Starovoitov <alexei.starovoitov@gmail.com> wrote: > > On Sun, Jul 03, 2022 at 11:04:22AM +0530, Kumar Kartikeya Dwivedi wrote: > > On Sun, 3 Jul 2022 at 10:54, Kumar Kartikeya Dwivedi <memxor@gmail.com> wrote: > > > > > > On Wed, 29 Jun 2022 at 08:53, Alexei Starovoitov > > > <alexei.starovoitov@gmail.com> wrote: > > > > > > > > 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. > > > > > > > > > > Indeed, I missed that. Thanks for catching it. > > > > > > > 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. > > > > > > It is needed to prevent the case where someone might do: > > > ct = bpf_xdp_ct_alloc(...); > > > bpf_ct_set_timeout(ct->master, ...); > > > > > > > A better illustration is probably bpf_xdp_ct_lookup and > > bpf_ct_change_timeout, since here the type for ct->master won't match > > with bpf_ct_set_timeout, but the point is the same. > > Sorry, I'm still not following. > Didn't we make pointer walking 'untrusted' so ct->master cannot be > passed into any kfunc? > I don't believe that is the case, it is only true for kptrs loaded from BPF maps (that too those with BPF_LDX, not the ones with kptr_xchg). There we had a chance to do things differently. For normal PTR_TO_BTF_ID obtained from kfuncs/BPF helpers, there is no untrusted flag set on them, nor is it set when walking them. I also think we discussed switching to this mode, by making many cases untrusted by default, and using annotation to allow cases, making pointers trusted at one level (like args for tracing/lsm progs, but next deref becomes untrusted), but admittedly it may not cover enough ground, and you didn't like it much either, so I stopped pursuing it. > > > Or just obtain PTR_TO_BTF_ID by pointer walking and try to pass it in > > > to bpf_ct_set_timeout. > > > > > > __ref allows an argument on a non-release kfunc to have checks like a > > > release argument, i.e. refcounted, reg->off == 0 (var_off is already > > > checked to be 0), so use the original pointer that was obtained from > > > an acquire kfunc. As you noted, it isn't strictly needed on release > > > kfunc (like bpf_ct_insert_entry) because the same checks happen for it > > > anyway. But both timeout and status helpers should use it if they > > > "operate" on the acquired ct (from alloc, insert, or lookup).
On Thu, Jul 07, 2022 at 12:51:15AM +0530, Kumar Kartikeya Dwivedi wrote: > On Thu, 7 Jul 2022 at 00:14, Alexei Starovoitov > <alexei.starovoitov@gmail.com> wrote: > > > > On Sun, Jul 03, 2022 at 11:04:22AM +0530, Kumar Kartikeya Dwivedi wrote: > > > On Sun, 3 Jul 2022 at 10:54, Kumar Kartikeya Dwivedi <memxor@gmail.com> wrote: > > > > > > > > On Wed, 29 Jun 2022 at 08:53, Alexei Starovoitov > > > > <alexei.starovoitov@gmail.com> wrote: > > > > > > > > > > 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. > > > > > > > > > > > > > Indeed, I missed that. Thanks for catching it. > > > > > > > > > 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. > > > > > > > > It is needed to prevent the case where someone might do: > > > > ct = bpf_xdp_ct_alloc(...); > > > > bpf_ct_set_timeout(ct->master, ...); > > > > > > > > > > A better illustration is probably bpf_xdp_ct_lookup and > > > bpf_ct_change_timeout, since here the type for ct->master won't match > > > with bpf_ct_set_timeout, but the point is the same. > > > > Sorry, I'm still not following. > > Didn't we make pointer walking 'untrusted' so ct->master cannot be > > passed into any kfunc? > > > > I don't believe that is the case, it is only true for kptrs loaded > from BPF maps (that too those with BPF_LDX, not the ones with > kptr_xchg). There we had a chance to do things differently. For normal > PTR_TO_BTF_ID obtained from kfuncs/BPF helpers, there is no untrusted > flag set on them, nor is it set when walking them. > > I also think we discussed switching to this mode, by making many cases > untrusted by default, and using annotation to allow cases, making > pointers trusted at one level (like args for tracing/lsm progs, but > next deref becomes untrusted), but admittedly it may not cover enough > ground, and you didn't like it much either, so I stopped pursuing it. Ahh. Now I remember. Thanks for reminding :) Could you please summarize this thread and add all of it as a big comment in the source code next to __ref handling to explain the motivation and an example on when and how this __ref suffix should be used. Otherwise somebody, like me, will forget the context soon. I was thinking of better name than __ref, but couldn't come up with one. __ref fits this use case the best.
On Wed, Jul 6, 2022 at 2:29 PM Alexei Starovoitov <alexei.starovoitov@gmail.com> wrote: > > On Thu, Jul 07, 2022 at 12:51:15AM +0530, Kumar Kartikeya Dwivedi wrote: > > On Thu, 7 Jul 2022 at 00:14, Alexei Starovoitov > > <alexei.starovoitov@gmail.com> wrote: > > > > > > On Sun, Jul 03, 2022 at 11:04:22AM +0530, Kumar Kartikeya Dwivedi wrote: > > > > On Sun, 3 Jul 2022 at 10:54, Kumar Kartikeya Dwivedi <memxor@gmail.com> wrote: > > > > > > > > > > On Wed, 29 Jun 2022 at 08:53, Alexei Starovoitov > > > > > <alexei.starovoitov@gmail.com> wrote: > > > > > > > > > > > > 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. > > > > > > > > > > > > > > > > Indeed, I missed that. Thanks for catching it. > > > > > > > > > > > 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. > > > > > > > > > > It is needed to prevent the case where someone might do: > > > > > ct = bpf_xdp_ct_alloc(...); > > > > > bpf_ct_set_timeout(ct->master, ...); > > > > > > > > > > > > > A better illustration is probably bpf_xdp_ct_lookup and > > > > bpf_ct_change_timeout, since here the type for ct->master won't match > > > > with bpf_ct_set_timeout, but the point is the same. > > > > > > Sorry, I'm still not following. > > > Didn't we make pointer walking 'untrusted' so ct->master cannot be > > > passed into any kfunc? > > > > > > > I don't believe that is the case, it is only true for kptrs loaded > > from BPF maps (that too those with BPF_LDX, not the ones with > > kptr_xchg). There we had a chance to do things differently. For normal > > PTR_TO_BTF_ID obtained from kfuncs/BPF helpers, there is no untrusted > > flag set on them, nor is it set when walking them. > > > > I also think we discussed switching to this mode, by making many cases > > untrusted by default, and using annotation to allow cases, making > > pointers trusted at one level (like args for tracing/lsm progs, but > > next deref becomes untrusted), but admittedly it may not cover enough > > ground, and you didn't like it much either, so I stopped pursuing it. > > Ahh. Now I remember. Thanks for reminding :) > Could you please summarize this thread and add all of it as a big comment > in the source code next to __ref handling to explain the motivation > and an example on when and how this __ref suffix should be used. > Otherwise somebody, like me, will forget the context soon. > > I was thinking of better name than __ref, but couldn't come up with one. > __ref fits this use case the best. Actually, maybe a kfunc flag will be better? Like REF_ARGS that would apply to all arguments of the kfunc (not only those with __ref suffix). We have three types of ptr_btf_id: - ref counted - untrusted - old legacy that we cannot be break due to backward compat In the future we'll probably be adding new kfuncs where we'd want every argument to be trusted. In our naming convention these are the refcounted ptr_to_btf_id that come from lookup-like kfuncs. To consume them in the release kfunc they have to be refcounted, but non-release kfunc (like set_timeout) also want a trusted ptr. So the simple way of describe the intent would be: BTF_ID(func, bpf_ct_release, RELEASE) BTF_ID(func, bpf_ct_set_timeout, REF_ARGS) or maybe TRUSTED_ARGS would be a better flag name. wdyt?
On Thu, 7 Jul 2022 at 00:04, Alexei Starovoitov <alexei.starovoitov@gmail.com> wrote: > > On Wed, Jul 6, 2022 at 2:29 PM Alexei Starovoitov > <alexei.starovoitov@gmail.com> wrote: > > > > On Thu, Jul 07, 2022 at 12:51:15AM +0530, Kumar Kartikeya Dwivedi wrote: > > > On Thu, 7 Jul 2022 at 00:14, Alexei Starovoitov > > > <alexei.starovoitov@gmail.com> wrote: > > > > > > > > On Sun, Jul 03, 2022 at 11:04:22AM +0530, Kumar Kartikeya Dwivedi wrote: > > > > > On Sun, 3 Jul 2022 at 10:54, Kumar Kartikeya Dwivedi <memxor@gmail.com> wrote: > > > > > > > > > > > > On Wed, 29 Jun 2022 at 08:53, Alexei Starovoitov > > > > > > <alexei.starovoitov@gmail.com> wrote: > > > > > > > > > > > > > > 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. > > > > > > > > > > > > > > > > > > > Indeed, I missed that. Thanks for catching it. > > > > > > > > > > > > > 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. > > > > > > > > > > > > It is needed to prevent the case where someone might do: > > > > > > ct = bpf_xdp_ct_alloc(...); > > > > > > bpf_ct_set_timeout(ct->master, ...); > > > > > > > > > > > > > > > > A better illustration is probably bpf_xdp_ct_lookup and > > > > > bpf_ct_change_timeout, since here the type for ct->master won't match > > > > > with bpf_ct_set_timeout, but the point is the same. > > > > > > > > Sorry, I'm still not following. > > > > Didn't we make pointer walking 'untrusted' so ct->master cannot be > > > > passed into any kfunc? > > > > > > > > > > I don't believe that is the case, it is only true for kptrs loaded > > > from BPF maps (that too those with BPF_LDX, not the ones with > > > kptr_xchg). There we had a chance to do things differently. For normal > > > PTR_TO_BTF_ID obtained from kfuncs/BPF helpers, there is no untrusted > > > flag set on them, nor is it set when walking them. > > > > > > I also think we discussed switching to this mode, by making many cases > > > untrusted by default, and using annotation to allow cases, making > > > pointers trusted at one level (like args for tracing/lsm progs, but > > > next deref becomes untrusted), but admittedly it may not cover enough > > > ground, and you didn't like it much either, so I stopped pursuing it. > > > > Ahh. Now I remember. Thanks for reminding :) > > Could you please summarize this thread and add all of it as a big comment > > in the source code next to __ref handling to explain the motivation > > and an example on when and how this __ref suffix should be used. > > Otherwise somebody, like me, will forget the context soon. > > > > I was thinking of better name than __ref, but couldn't come up with one. > > __ref fits this use case the best. > > Actually, maybe a kfunc flag will be better? > Like REF_ARGS > that would apply to all arguments of the kfunc > (not only those with __ref suffix). > > We have three types of ptr_btf_id: > - ref counted > - untrusted > - old legacy that we cannot be break due to backward compat > > In the future we'll probably be adding new kfuncs where we'd want > every argument to be trusted. In our naming convention these are > the refcounted ptr_to_btf_id that come from lookup-like kfuncs. > To consume them in the release kfunc they have to be refcounted, > but non-release kfunc (like set_timeout) also want a trusted ptr. > So the simple way of describe the intent would be: > BTF_ID(func, bpf_ct_release, RELEASE) > BTF_ID(func, bpf_ct_set_timeout, REF_ARGS) > > or maybe TRUSTED_ARGS would be a better flag name. > wdyt? Ok, I've implemented the kfunc flags and kept TRUSTED_ARGS as the name. Just need to do a little bit of testing and will post it together with this. Just to confirm, should I still keep __ref or drop it? I think TRUSTED_ARGS has its use but it may be too coarse. I already have the patch so if you like we can add both ways now.
On Wed, Jul 13, 2022 at 5:13 AM Kumar Kartikeya Dwivedi <memxor@gmail.com> wrote: > > > Ahh. Now I remember. Thanks for reminding :) > > > Could you please summarize this thread and add all of it as a big comment > > > in the source code next to __ref handling to explain the motivation > > > and an example on when and how this __ref suffix should be used. > > > Otherwise somebody, like me, will forget the context soon. > > > > > > I was thinking of better name than __ref, but couldn't come up with one. > > > __ref fits this use case the best. > > > > Actually, maybe a kfunc flag will be better? > > Like REF_ARGS > > that would apply to all arguments of the kfunc > > (not only those with __ref suffix). > > > > We have three types of ptr_btf_id: > > - ref counted > > - untrusted > > - old legacy that we cannot be break due to backward compat > > > > In the future we'll probably be adding new kfuncs where we'd want > > every argument to be trusted. In our naming convention these are > > the refcounted ptr_to_btf_id that come from lookup-like kfuncs. > > To consume them in the release kfunc they have to be refcounted, > > but non-release kfunc (like set_timeout) also want a trusted ptr. > > So the simple way of describe the intent would be: > > BTF_ID(func, bpf_ct_release, RELEASE) > > BTF_ID(func, bpf_ct_set_timeout, REF_ARGS) > > > > or maybe TRUSTED_ARGS would be a better flag name. > > wdyt? > > Ok, I've implemented the kfunc flags and kept TRUSTED_ARGS as the > name. Just need to do a little bit of testing and will post it > together with this. Awesome! > Just to confirm, should I still keep __ref or drop it? I think > TRUSTED_ARGS has its use but it may be too coarse. I already have the > patch so if you like we can add both ways now. TRUSTED_ARGS may become too coarse, but let's cross that bridge when there is actual need. If we land __ref support right now there won't be any users and the code will start to bit rot. So let's delay it. Pls post that patch as an extra RFC patch anyway, so it won't get lost.
diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c index f08037c31dd7..7b4128e3359a 100644 --- a/kernel/bpf/btf.c +++ b/kernel/bpf/btf.c @@ -6136,18 +6136,13 @@ static bool __btf_type_is_scalar_struct(struct bpf_verifier_log *log, return true; } -static bool is_kfunc_arg_mem_size(const struct btf *btf, - const struct btf_param *arg, - const struct bpf_reg_state *reg) +static bool btf_param_match_suffix(const struct btf *btf, + const struct btf_param *arg, + const char *suffix) { - int len, sfx_len = sizeof("__sz") - 1; - const struct btf_type *t; + int len, sfx_len = strlen(suffix); const char *param_name; - t = btf_type_skip_modifiers(btf, arg->type, NULL); - if (!btf_type_is_scalar(t) || reg->type != SCALAR_VALUE) - return false; - /* In the future, this can be ported to use BTF tagging */ param_name = btf_name_by_offset(btf, arg->name_off); if (str_is_empty(param_name)) @@ -6156,22 +6151,41 @@ static bool is_kfunc_arg_mem_size(const struct btf *btf, if (len < sfx_len) return false; param_name += len - sfx_len; - if (strncmp(param_name, "__sz", sfx_len)) + if (strncmp(param_name, suffix, sfx_len)) return false; return true; } +static bool is_kfunc_arg_ref(const struct btf *btf, + const struct btf_param *arg) +{ + return btf_param_match_suffix(btf, arg, "__ref"); +} + +static bool is_kfunc_arg_mem_size(const struct btf *btf, + const struct btf_param *arg, + const struct bpf_reg_state *reg) +{ + const struct btf_type *t; + + t = btf_type_skip_modifiers(btf, arg->type, NULL); + if (!btf_type_is_scalar(t) || reg->type != SCALAR_VALUE) + return false; + + return btf_param_match_suffix(btf, arg, "__sz"); +} + static int btf_check_func_arg_match(struct bpf_verifier_env *env, const struct btf *btf, u32 func_id, struct bpf_reg_state *regs, bool ptr_to_mem_ok) { enum bpf_prog_type prog_type = resolve_prog_type(env->prog); + bool rel = false, kptr_get = false, arg_ref = false; struct bpf_verifier_log *log = &env->log; u32 i, nargs, ref_id, ref_obj_id = 0; bool is_kfunc = btf_is_kernel(btf); - bool rel = false, kptr_get = false; const char *func_name, *ref_tname; const struct btf_type *t, *ref_t; const struct btf_param *args; @@ -6231,6 +6245,15 @@ static int btf_check_func_arg_match(struct bpf_verifier_env *env, return -EINVAL; } + /* 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; + } + ref_t = btf_type_skip_modifiers(btf, t->type, &ref_id); ref_tname = btf_name_by_offset(btf, ref_t->name_off); @@ -6332,7 +6355,8 @@ static int btf_check_func_arg_match(struct bpf_verifier_env *env, reg_ref_tname = btf_name_by_offset(reg_btf, reg_ref_t->name_off); if (!btf_struct_ids_match(log, reg_btf, reg_ref_id, - reg->off, btf, ref_id, rel && reg->ref_obj_id)) { + reg->off, btf, ref_id, + arg_ref || (rel && reg->ref_obj_id))) { bpf_log(log, "kernel function %s args#%d expected pointer to %s %s but R%d has a pointer to %s %s\n", func_name, i, btf_type_str(ref_t), ref_tname, diff --git a/net/bpf/test_run.c b/net/bpf/test_run.c index 2ca96acbc50a..4314b8172b52 100644 --- a/net/bpf/test_run.c +++ b/net/bpf/test_run.c @@ -691,6 +691,10 @@ noinline void bpf_kfunc_call_test_mem_len_fail2(u64 *mem, int len) { } +noinline void bpf_kfunc_call_test_ref(struct prog_test_ref_kfunc *p__ref) +{ +} + __diag_pop(); ALLOW_ERROR_INJECTION(bpf_modify_return_test, ERRNO); @@ -714,6 +718,7 @@ BTF_ID(func, bpf_kfunc_call_test_fail3) BTF_ID(func, bpf_kfunc_call_test_mem_len_pass1) BTF_ID(func, bpf_kfunc_call_test_mem_len_fail1) BTF_ID(func, bpf_kfunc_call_test_mem_len_fail2) +BTF_ID(func, bpf_kfunc_call_test_ref) BTF_SET_END(test_sk_check_kfunc_ids) BTF_SET_START(test_sk_acquire_kfunc_ids)