| Message ID | 20200922070422.1917351-1-kafai@fb.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v3,bpf-next,01/11] bpf: Move the PTR_TO_BTF_ID check to check_reg_type() | expand |
On Tue, 22 Sep 2020 at 08:04, Martin KaFai Lau <kafai@fb.com> wrote: > > There is a constant need to add more fields into the bpf_tcp_sock > for the bpf programs running at tc, sock_ops...etc. > > A current workaround could be to use bpf_probe_read_kernel(). However, > other than making another helper call for reading each field and missing > CO-RE, it is also not as intuitive to use as directly reading > "tp->lsndtime" for example. While already having perfmon cap to do > bpf_probe_read_kernel(), it will be much easier if the bpf prog can > directly read from the tcp_sock. > > This patch tries to do that by using the existing casting-helpers > bpf_skc_to_*() whose func_proto returns a btf_id. For example, the > func_proto of bpf_skc_to_tcp_sock returns the btf_id of the > kernel "struct tcp_sock". > > These helpers are also added to is_ptr_cast_function(). > It ensures the returning reg (BPF_REF_0) will also carries the ref_obj_id. > That will keep the ref-tracking works properly. > > The bpf_skc_to_* helpers are made available to most of the bpf prog > types in filter.c. They are limited by perfmon cap. > > This patch adds a ARG_PTR_TO_BTF_ID_SOCK_COMMON. The helper accepting > this arg can accept a btf-id-ptr (PTR_TO_BTF_ID + &btf_sock_ids[BTF_SOCK_TYPE_SOCK_COMMON]) > or a legacy-ctx-convert-skc-ptr (PTR_TO_SOCK_COMMON). The bpf_skc_to_*() > helpers are changed to take ARG_PTR_TO_BTF_ID_SOCK_COMMON such that > they will accept pointer obtained from skb->sk. > > PTR_TO_*_OR_NULL is not accepted as an ARG_PTR_TO_BTF_ID_SOCK_COMMON > at verification time. All PTR_TO_*_OR_NULL reg has to do a NULL check > first before passing into the helper or else the bpf prog will be > rejected by the verifier. > > [ ARG_PTR_TO_SOCK_COMMON_OR_NULL was attempted earlier. The _OR_NULL was > needed because the PTR_TO_BTF_ID could be NULL but note that a could be NULL > PTR_TO_BTF_ID is not a scalar NULL to the verifier. "_OR_NULL" implicitly > gives an expectation that the helper can take a scalar NULL which does > not make sense in most (except one) helpers. Passing scalar NULL > should be rejected at the verification time. What is the benefit of requiring a !sk check from the user if all of the helpers know how to deal with a NULL pointer? > > Thus, this patch uses ARG_PTR_TO_BTF_ID_SOCK_COMMON to specify that the > helper can take both the btf-id ptr or the legacy PTR_TO_SOCK_COMMON but > not scalar NULL. It requires the func_proto to explicitly specify the > arg_btf_id such that there is a very clear expectation that the helper > can handle a NULL PTR_TO_BTF_ID. ] I think ARG_PTR_TO_BTF_ID_SOCK_COMMON is actually a misnomer, since nothing enforces that arg_btf_id is actually an ID for sock common. This is where ARG_PTR_TO_SOCK_COMMON_OR_NULL is much easier to understand, even though it's more permissive than it has to be. It communicates very clearly what values the argument can take. If you're set on ARG_PTR_TO_BTF_ID_SOCK_COMMON I'd suggest forcing the btf_id in struct bpf_reg_types. This avoids the weird case where the btf_id doesn't actually point at sock_common, and it also makes my life easier for sockmap update from iter, as mentioned in the other email. -- Lorenz Bauer | Systems Engineer 6th Floor, County Hall/The Riverside Building, SE1 7PB, UK www.cloudflare.com
On Tue, Sep 22, 2020 at 10:46:41AM +0100, Lorenz Bauer wrote: > On Tue, 22 Sep 2020 at 08:04, Martin KaFai Lau <kafai@fb.com> wrote: > > > > There is a constant need to add more fields into the bpf_tcp_sock > > for the bpf programs running at tc, sock_ops...etc. > > > > A current workaround could be to use bpf_probe_read_kernel(). However, > > other than making another helper call for reading each field and missing > > CO-RE, it is also not as intuitive to use as directly reading > > "tp->lsndtime" for example. While already having perfmon cap to do > > bpf_probe_read_kernel(), it will be much easier if the bpf prog can > > directly read from the tcp_sock. > > > > This patch tries to do that by using the existing casting-helpers > > bpf_skc_to_*() whose func_proto returns a btf_id. For example, the > > func_proto of bpf_skc_to_tcp_sock returns the btf_id of the > > kernel "struct tcp_sock". > > > > These helpers are also added to is_ptr_cast_function(). > > It ensures the returning reg (BPF_REF_0) will also carries the ref_obj_id. > > That will keep the ref-tracking works properly. > > > > The bpf_skc_to_* helpers are made available to most of the bpf prog > > types in filter.c. They are limited by perfmon cap. > > > > This patch adds a ARG_PTR_TO_BTF_ID_SOCK_COMMON. The helper accepting > > this arg can accept a btf-id-ptr (PTR_TO_BTF_ID + &btf_sock_ids[BTF_SOCK_TYPE_SOCK_COMMON]) > > or a legacy-ctx-convert-skc-ptr (PTR_TO_SOCK_COMMON). The bpf_skc_to_*() > > helpers are changed to take ARG_PTR_TO_BTF_ID_SOCK_COMMON such that > > they will accept pointer obtained from skb->sk. > > > > PTR_TO_*_OR_NULL is not accepted as an ARG_PTR_TO_BTF_ID_SOCK_COMMON > > at verification time. All PTR_TO_*_OR_NULL reg has to do a NULL check > > first before passing into the helper or else the bpf prog will be > > rejected by the verifier. > > > > [ ARG_PTR_TO_SOCK_COMMON_OR_NULL was attempted earlier. The _OR_NULL was > > needed because the PTR_TO_BTF_ID could be NULL but note that a could be NULL > > PTR_TO_BTF_ID is not a scalar NULL to the verifier. "_OR_NULL" implicitly > > gives an expectation that the helper can take a scalar NULL which does > > not make sense in most (except one) helpers. Passing scalar NULL > > should be rejected at the verification time. > > What is the benefit of requiring a !sk check from the user if all of > the helpers know how to deal with a NULL pointer? I don't see a reason why the verifier should not reject an incorrect program at load time if it can. > > > > > Thus, this patch uses ARG_PTR_TO_BTF_ID_SOCK_COMMON to specify that the > > helper can take both the btf-id ptr or the legacy PTR_TO_SOCK_COMMON but > > not scalar NULL. It requires the func_proto to explicitly specify the > > arg_btf_id such that there is a very clear expectation that the helper > > can handle a NULL PTR_TO_BTF_ID. ] > > I think ARG_PTR_TO_BTF_ID_SOCK_COMMON is actually a misnomer, since > nothing enforces that arg_btf_id is actually an ID for sock common. > This is where ARG_PTR_TO_SOCK_COMMON_OR_NULL is much easier to > understand, even though it's more permissive than it has to be. It > communicates very clearly what values the argument can take. _OR_NULL is incorrect which implies a scalar NULL as mentioned in this commit message. From verifier pov, _OR_NULL can take a scalar NULL. > > If you're set on ARG_PTR_TO_BTF_ID_SOCK_COMMON I'd suggest forcing the > btf_id in struct bpf_reg_types. This avoids the weird case where the > btf_id doesn't actually point at sock_common, and it also makes my I have considered the bpf_reg_types option. I prefer all arg info (arg_type and arg_btf_id) stay in the same one place (i.e. func_proto) as much as possible for now instead of introducing another place to specify/override it which then depends on a particular arg_type that some arg_type may be in func_proto while some may be in other places. The arg_btf_id can be checked in check_btf_id_ok() if it would be a big concern that it might slip through the review but I think the chance is pretty low.
On Tue, 22 Sep 2020 at 19:26, Martin KaFai Lau <kafai@fb.com> wrote: > > On Tue, Sep 22, 2020 at 10:46:41AM +0100, Lorenz Bauer wrote: > > On Tue, 22 Sep 2020 at 08:04, Martin KaFai Lau <kafai@fb.com> wrote: > > > > > > There is a constant need to add more fields into the bpf_tcp_sock > > > for the bpf programs running at tc, sock_ops...etc. > > > > > > A current workaround could be to use bpf_probe_read_kernel(). However, > > > other than making another helper call for reading each field and missing > > > CO-RE, it is also not as intuitive to use as directly reading > > > "tp->lsndtime" for example. While already having perfmon cap to do > > > bpf_probe_read_kernel(), it will be much easier if the bpf prog can > > > directly read from the tcp_sock. > > > > > > This patch tries to do that by using the existing casting-helpers > > > bpf_skc_to_*() whose func_proto returns a btf_id. For example, the > > > func_proto of bpf_skc_to_tcp_sock returns the btf_id of the > > > kernel "struct tcp_sock". > > > > > > These helpers are also added to is_ptr_cast_function(). > > > It ensures the returning reg (BPF_REF_0) will also carries the ref_obj_id. > > > That will keep the ref-tracking works properly. > > > > > > The bpf_skc_to_* helpers are made available to most of the bpf prog > > > types in filter.c. They are limited by perfmon cap. > > > > > > This patch adds a ARG_PTR_TO_BTF_ID_SOCK_COMMON. The helper accepting > > > this arg can accept a btf-id-ptr (PTR_TO_BTF_ID + &btf_sock_ids[BTF_SOCK_TYPE_SOCK_COMMON]) > > > or a legacy-ctx-convert-skc-ptr (PTR_TO_SOCK_COMMON). The bpf_skc_to_*() > > > helpers are changed to take ARG_PTR_TO_BTF_ID_SOCK_COMMON such that > > > they will accept pointer obtained from skb->sk. > > > > > > PTR_TO_*_OR_NULL is not accepted as an ARG_PTR_TO_BTF_ID_SOCK_COMMON > > > at verification time. All PTR_TO_*_OR_NULL reg has to do a NULL check > > > first before passing into the helper or else the bpf prog will be > > > rejected by the verifier. > > > > > > [ ARG_PTR_TO_SOCK_COMMON_OR_NULL was attempted earlier. The _OR_NULL was > > > needed because the PTR_TO_BTF_ID could be NULL but note that a could be NULL > > > PTR_TO_BTF_ID is not a scalar NULL to the verifier. "_OR_NULL" implicitly > > > gives an expectation that the helper can take a scalar NULL which does > > > not make sense in most (except one) helpers. Passing scalar NULL > > > should be rejected at the verification time. > > > > What is the benefit of requiring a !sk check from the user if all of > > the helpers know how to deal with a NULL pointer? > I don't see a reason why the verifier should not reject an incorrect > program at load time if it can. > > > > > > > > > Thus, this patch uses ARG_PTR_TO_BTF_ID_SOCK_COMMON to specify that the > > > helper can take both the btf-id ptr or the legacy PTR_TO_SOCK_COMMON but > > > not scalar NULL. It requires the func_proto to explicitly specify the > > > arg_btf_id such that there is a very clear expectation that the helper > > > can handle a NULL PTR_TO_BTF_ID. ] > > > > I think ARG_PTR_TO_BTF_ID_SOCK_COMMON is actually a misnomer, since > > nothing enforces that arg_btf_id is actually an ID for sock common. > > This is where ARG_PTR_TO_SOCK_COMMON_OR_NULL is much easier to > > understand, even though it's more permissive than it has to be. It > > communicates very clearly what values the argument can take. > _OR_NULL is incorrect which implies a scalar NULL as mentioned in > this commit message. From verifier pov, _OR_NULL can take > a scalar NULL. Yes, I know. I'm saying that the distinction between scalar NULL and runtime NULL only makes sense after you understand how BTF pointers are implemented. It only clicked for me after I read the support code in the JIT that Yonghong pointed out. Should everybody that writes a helper need to read the JIT? In my opinion we shouldn't. I guess I don't even care about the verifier rejecting scalar NULL or not, I'd just like the types to have a name that conveys their NULLness. > > > > > If you're set on ARG_PTR_TO_BTF_ID_SOCK_COMMON I'd suggest forcing the > > btf_id in struct bpf_reg_types. This avoids the weird case where the > > btf_id doesn't actually point at sock_common, and it also makes my > I have considered the bpf_reg_types option. I prefer all > arg info (arg_type and arg_btf_id) stay in the same one > place (i.e. func_proto) as much as possible for now > instead of introducing another place to specify/override it > which then depends on a particular arg_type that some arg_type may be > in func_proto while some may be in other places. In my opinion that ship sailed when we started aliasing arg_type to multiple reg_type, but OK. > > The arg_btf_id can be checked in check_btf_id_ok() if it would be a > big concern that it might slip through the review but I think the > chance is pretty low. Why increase the burden on human reviewers? Why add code to check an invariant that we could get rid of in the first place?
On Wed, Sep 23, 2020 at 10:27:27AM +0100, Lorenz Bauer wrote: > On Tue, 22 Sep 2020 at 19:26, Martin KaFai Lau <kafai@fb.com> wrote: > > > > On Tue, Sep 22, 2020 at 10:46:41AM +0100, Lorenz Bauer wrote: > > > On Tue, 22 Sep 2020 at 08:04, Martin KaFai Lau <kafai@fb.com> wrote: > > > > > > > > There is a constant need to add more fields into the bpf_tcp_sock > > > > for the bpf programs running at tc, sock_ops...etc. > > > > > > > > A current workaround could be to use bpf_probe_read_kernel(). However, > > > > other than making another helper call for reading each field and missing > > > > CO-RE, it is also not as intuitive to use as directly reading > > > > "tp->lsndtime" for example. While already having perfmon cap to do > > > > bpf_probe_read_kernel(), it will be much easier if the bpf prog can > > > > directly read from the tcp_sock. > > > > > > > > This patch tries to do that by using the existing casting-helpers > > > > bpf_skc_to_*() whose func_proto returns a btf_id. For example, the > > > > func_proto of bpf_skc_to_tcp_sock returns the btf_id of the > > > > kernel "struct tcp_sock". > > > > > > > > These helpers are also added to is_ptr_cast_function(). > > > > It ensures the returning reg (BPF_REF_0) will also carries the ref_obj_id. > > > > That will keep the ref-tracking works properly. > > > > > > > > The bpf_skc_to_* helpers are made available to most of the bpf prog > > > > types in filter.c. They are limited by perfmon cap. > > > > > > > > This patch adds a ARG_PTR_TO_BTF_ID_SOCK_COMMON. The helper accepting > > > > this arg can accept a btf-id-ptr (PTR_TO_BTF_ID + &btf_sock_ids[BTF_SOCK_TYPE_SOCK_COMMON]) > > > > or a legacy-ctx-convert-skc-ptr (PTR_TO_SOCK_COMMON). The bpf_skc_to_*() > > > > helpers are changed to take ARG_PTR_TO_BTF_ID_SOCK_COMMON such that > > > > they will accept pointer obtained from skb->sk. > > > > > > > > PTR_TO_*_OR_NULL is not accepted as an ARG_PTR_TO_BTF_ID_SOCK_COMMON > > > > at verification time. All PTR_TO_*_OR_NULL reg has to do a NULL check > > > > first before passing into the helper or else the bpf prog will be > > > > rejected by the verifier. > > > > > > > > [ ARG_PTR_TO_SOCK_COMMON_OR_NULL was attempted earlier. The _OR_NULL was > > > > needed because the PTR_TO_BTF_ID could be NULL but note that a could be NULL > > > > PTR_TO_BTF_ID is not a scalar NULL to the verifier. "_OR_NULL" implicitly > > > > gives an expectation that the helper can take a scalar NULL which does > > > > not make sense in most (except one) helpers. Passing scalar NULL > > > > should be rejected at the verification time. > > > > > > What is the benefit of requiring a !sk check from the user if all of > > > the helpers know how to deal with a NULL pointer? > > I don't see a reason why the verifier should not reject an incorrect > > program at load time if it can. > > > > > > > > > > > > > Thus, this patch uses ARG_PTR_TO_BTF_ID_SOCK_COMMON to specify that the > > > > helper can take both the btf-id ptr or the legacy PTR_TO_SOCK_COMMON but > > > > not scalar NULL. It requires the func_proto to explicitly specify the > > > > arg_btf_id such that there is a very clear expectation that the helper > > > > can handle a NULL PTR_TO_BTF_ID. ] > > > > > > I think ARG_PTR_TO_BTF_ID_SOCK_COMMON is actually a misnomer, since > > > nothing enforces that arg_btf_id is actually an ID for sock common. > > > This is where ARG_PTR_TO_SOCK_COMMON_OR_NULL is much easier to > > > understand, even though it's more permissive than it has to be. It > > > communicates very clearly what values the argument can take. > > _OR_NULL is incorrect which implies a scalar NULL as mentioned in > > this commit message. From verifier pov, _OR_NULL can take > > a scalar NULL. > > Yes, I know. I'm saying that the distinction between scalar NULL and > runtime NULL only makes sense after you understand how BTF pointers > are implemented. It only clicked for me after I read the support code > in the JIT that Yonghong pointed out. Should everybody that writes a > helper need to read the JIT? In my opinion we shouldn't. I guess I > don't even care about the verifier rejecting scalar NULL or not, I'd > just like the types to have a name that conveys their NULLness. It is not only about verifier and/or JIT, not sure why it is related to JIT also. For some helpers, explicitly passing NULL may make sense. e.g. bpf_sk_assign(ctx, NULL, 0) makes sense. For most helpers, the bpf prog is wrong for sure, for example in sockmap, what does bpf_map_update_elem(sock_map, key, NULL, 0) mean? I would expect a delete from the sock_map if the verifier accepted it. > > > > > > > > > If you're set on ARG_PTR_TO_BTF_ID_SOCK_COMMON I'd suggest forcing the > > > btf_id in struct bpf_reg_types. This avoids the weird case where the > > > btf_id doesn't actually point at sock_common, and it also makes my > > I have considered the bpf_reg_types option. I prefer all > > arg info (arg_type and arg_btf_id) stay in the same one > > place (i.e. func_proto) as much as possible for now > > instead of introducing another place to specify/override it > > which then depends on a particular arg_type that some arg_type may be > > in func_proto while some may be in other places. > > In my opinion that ship sailed when we started aliasing arg_type to > multiple reg_type, but OK. > > > > > The arg_btf_id can be checked in check_btf_id_ok() if it would be a > > big concern that it might slip through the review but I think the > > chance is pretty low. > > Why increase the burden on human reviewers? Why add code to check an > invariant that we could get rid of in the first place? Lets take the scalar NULL example that requires to read multiple pieces of codes in different places (verifier, JIT...etc.). As you also mentioned, yes, it may be easy for a few people. However, for most others, having some obvious things in the same place is easier to review. I think we have to agree we disagree on this one implementation details which I think it has been over-thought (and time also). If you insist that should go into bpf_reg_types (i.e. compatible->btf_id), I can do that in v4 and then add another check in another place to ensure "!compatible->btf_id" as in v2.
On Wed, 23 Sep 2020 at 18:06, Martin KaFai Lau <kafai@fb.com> wrote: > > On Wed, Sep 23, 2020 at 10:27:27AM +0100, Lorenz Bauer wrote: > > On Tue, 22 Sep 2020 at 19:26, Martin KaFai Lau <kafai@fb.com> wrote: > > > > > > On Tue, Sep 22, 2020 at 10:46:41AM +0100, Lorenz Bauer wrote: > > > > On Tue, 22 Sep 2020 at 08:04, Martin KaFai Lau <kafai@fb.com> wrote: > > > > > > > > > > There is a constant need to add more fields into the bpf_tcp_sock > > > > > for the bpf programs running at tc, sock_ops...etc. > > > > > > > > > > A current workaround could be to use bpf_probe_read_kernel(). However, > > > > > other than making another helper call for reading each field and missing > > > > > CO-RE, it is also not as intuitive to use as directly reading > > > > > "tp->lsndtime" for example. While already having perfmon cap to do > > > > > bpf_probe_read_kernel(), it will be much easier if the bpf prog can > > > > > directly read from the tcp_sock. > > > > > > > > > > This patch tries to do that by using the existing casting-helpers > > > > > bpf_skc_to_*() whose func_proto returns a btf_id. For example, the > > > > > func_proto of bpf_skc_to_tcp_sock returns the btf_id of the > > > > > kernel "struct tcp_sock". > > > > > > > > > > These helpers are also added to is_ptr_cast_function(). > > > > > It ensures the returning reg (BPF_REF_0) will also carries the ref_obj_id. > > > > > That will keep the ref-tracking works properly. > > > > > > > > > > The bpf_skc_to_* helpers are made available to most of the bpf prog > > > > > types in filter.c. They are limited by perfmon cap. > > > > > > > > > > This patch adds a ARG_PTR_TO_BTF_ID_SOCK_COMMON. The helper accepting > > > > > this arg can accept a btf-id-ptr (PTR_TO_BTF_ID + &btf_sock_ids[BTF_SOCK_TYPE_SOCK_COMMON]) > > > > > or a legacy-ctx-convert-skc-ptr (PTR_TO_SOCK_COMMON). The bpf_skc_to_*() > > > > > helpers are changed to take ARG_PTR_TO_BTF_ID_SOCK_COMMON such that > > > > > they will accept pointer obtained from skb->sk. > > > > > > > > > > PTR_TO_*_OR_NULL is not accepted as an ARG_PTR_TO_BTF_ID_SOCK_COMMON > > > > > at verification time. All PTR_TO_*_OR_NULL reg has to do a NULL check > > > > > first before passing into the helper or else the bpf prog will be > > > > > rejected by the verifier. > > > > > > > > > > [ ARG_PTR_TO_SOCK_COMMON_OR_NULL was attempted earlier. The _OR_NULL was > > > > > needed because the PTR_TO_BTF_ID could be NULL but note that a could be NULL > > > > > PTR_TO_BTF_ID is not a scalar NULL to the verifier. "_OR_NULL" implicitly > > > > > gives an expectation that the helper can take a scalar NULL which does > > > > > not make sense in most (except one) helpers. Passing scalar NULL > > > > > should be rejected at the verification time. > > > > > > > > What is the benefit of requiring a !sk check from the user if all of > > > > the helpers know how to deal with a NULL pointer? > > > I don't see a reason why the verifier should not reject an incorrect > > > program at load time if it can. > > > > > > > > > > > > > > > > > Thus, this patch uses ARG_PTR_TO_BTF_ID_SOCK_COMMON to specify that the > > > > > helper can take both the btf-id ptr or the legacy PTR_TO_SOCK_COMMON but > > > > > not scalar NULL. It requires the func_proto to explicitly specify the > > > > > arg_btf_id such that there is a very clear expectation that the helper > > > > > can handle a NULL PTR_TO_BTF_ID. ] > > > > > > > > I think ARG_PTR_TO_BTF_ID_SOCK_COMMON is actually a misnomer, since > > > > nothing enforces that arg_btf_id is actually an ID for sock common. > > > > This is where ARG_PTR_TO_SOCK_COMMON_OR_NULL is much easier to > > > > understand, even though it's more permissive than it has to be. It > > > > communicates very clearly what values the argument can take. > > > _OR_NULL is incorrect which implies a scalar NULL as mentioned in > > > this commit message. From verifier pov, _OR_NULL can take > > > a scalar NULL. > > > > Yes, I know. I'm saying that the distinction between scalar NULL and > > runtime NULL only makes sense after you understand how BTF pointers > > are implemented. It only clicked for me after I read the support code > > in the JIT that Yonghong pointed out. Should everybody that writes a > > helper need to read the JIT? In my opinion we shouldn't. I guess I > > don't even care about the verifier rejecting scalar NULL or not, I'd > > just like the types to have a name that conveys their NULLness. > It is not only about verifier and/or JIT, not sure why it is related to > JIT also. > > For some helpers, explicitly passing NULL may make sense. > e.g. bpf_sk_assign(ctx, NULL, 0) makes sense. > > For most helpers, the bpf prog is wrong for sure, for example > in sockmap, what does bpf_map_update_elem(sock_map, key, NULL, 0) > mean? I would expect a delete from the sock_map if the verifier > accepted it. > > > > > > > > > > > > > > If you're set on ARG_PTR_TO_BTF_ID_SOCK_COMMON I'd suggest forcing the > > > > btf_id in struct bpf_reg_types. This avoids the weird case where the > > > > btf_id doesn't actually point at sock_common, and it also makes my > > > I have considered the bpf_reg_types option. I prefer all > > > arg info (arg_type and arg_btf_id) stay in the same one > > > place (i.e. func_proto) as much as possible for now > > > instead of introducing another place to specify/override it > > > which then depends on a particular arg_type that some arg_type may be > > > in func_proto while some may be in other places. > > > > In my opinion that ship sailed when we started aliasing arg_type to > > multiple reg_type, but OK. > > > > > > > > The arg_btf_id can be checked in check_btf_id_ok() if it would be a > > > big concern that it might slip through the review but I think the > > > chance is pretty low. > > > > Why increase the burden on human reviewers? Why add code to check an > > invariant that we could get rid of in the first place? > Lets take the scalar NULL example that requires to read multiple > pieces of codes in different places (verifier, JIT...etc.). > As you also mentioned, yes, it may be easy for a few people. > However, for most others, having some obvious things in the same place is > easier to review. > > I think we have to agree we disagree on this one implementation details > which I think it has been over-thought (and time also). > > If you insist that should go into bpf_reg_types (i.e. compatible->btf_id), > I can do that in v4 and then add another check in another place to > ensure "!compatible->btf_id" as in v2. No, I don't insist. I was hoping I could convince you, but alas :)
On Thu, Sep 24, 2020 at 09:38:09AM +0100, Lorenz Bauer wrote: > On Wed, 23 Sep 2020 at 18:06, Martin KaFai Lau <kafai@fb.com> wrote: > > > > On Wed, Sep 23, 2020 at 10:27:27AM +0100, Lorenz Bauer wrote: > > > On Tue, 22 Sep 2020 at 19:26, Martin KaFai Lau <kafai@fb.com> wrote: > > > > > > > > On Tue, Sep 22, 2020 at 10:46:41AM +0100, Lorenz Bauer wrote: > > > > > On Tue, 22 Sep 2020 at 08:04, Martin KaFai Lau <kafai@fb.com> wrote: > > > > > > > > > > > > There is a constant need to add more fields into the bpf_tcp_sock > > > > > > for the bpf programs running at tc, sock_ops...etc. > > > > > > > > > > > > A current workaround could be to use bpf_probe_read_kernel(). However, > > > > > > other than making another helper call for reading each field and missing > > > > > > CO-RE, it is also not as intuitive to use as directly reading > > > > > > "tp->lsndtime" for example. While already having perfmon cap to do > > > > > > bpf_probe_read_kernel(), it will be much easier if the bpf prog can > > > > > > directly read from the tcp_sock. > > > > > > > > > > > > This patch tries to do that by using the existing casting-helpers > > > > > > bpf_skc_to_*() whose func_proto returns a btf_id. For example, the > > > > > > func_proto of bpf_skc_to_tcp_sock returns the btf_id of the > > > > > > kernel "struct tcp_sock". > > > > > > > > > > > > These helpers are also added to is_ptr_cast_function(). > > > > > > It ensures the returning reg (BPF_REF_0) will also carries the ref_obj_id. > > > > > > That will keep the ref-tracking works properly. > > > > > > > > > > > > The bpf_skc_to_* helpers are made available to most of the bpf prog > > > > > > types in filter.c. They are limited by perfmon cap. > > > > > > > > > > > > This patch adds a ARG_PTR_TO_BTF_ID_SOCK_COMMON. The helper accepting > > > > > > this arg can accept a btf-id-ptr (PTR_TO_BTF_ID + &btf_sock_ids[BTF_SOCK_TYPE_SOCK_COMMON]) > > > > > > or a legacy-ctx-convert-skc-ptr (PTR_TO_SOCK_COMMON). The bpf_skc_to_*() > > > > > > helpers are changed to take ARG_PTR_TO_BTF_ID_SOCK_COMMON such that > > > > > > they will accept pointer obtained from skb->sk. > > > > > > > > > > > > PTR_TO_*_OR_NULL is not accepted as an ARG_PTR_TO_BTF_ID_SOCK_COMMON > > > > > > at verification time. All PTR_TO_*_OR_NULL reg has to do a NULL check > > > > > > first before passing into the helper or else the bpf prog will be > > > > > > rejected by the verifier. > > > > > > > > > > > > [ ARG_PTR_TO_SOCK_COMMON_OR_NULL was attempted earlier. The _OR_NULL was > > > > > > needed because the PTR_TO_BTF_ID could be NULL but note that a could be NULL > > > > > > PTR_TO_BTF_ID is not a scalar NULL to the verifier. "_OR_NULL" implicitly > > > > > > gives an expectation that the helper can take a scalar NULL which does > > > > > > not make sense in most (except one) helpers. Passing scalar NULL > > > > > > should be rejected at the verification time. > > > > > > > > > > What is the benefit of requiring a !sk check from the user if all of > > > > > the helpers know how to deal with a NULL pointer? > > > > I don't see a reason why the verifier should not reject an incorrect > > > > program at load time if it can. > > > > > > > > > > > > > > > > > > > > > Thus, this patch uses ARG_PTR_TO_BTF_ID_SOCK_COMMON to specify that the > > > > > > helper can take both the btf-id ptr or the legacy PTR_TO_SOCK_COMMON but > > > > > > not scalar NULL. It requires the func_proto to explicitly specify the > > > > > > arg_btf_id such that there is a very clear expectation that the helper > > > > > > can handle a NULL PTR_TO_BTF_ID. ] > > > > > > > > > > I think ARG_PTR_TO_BTF_ID_SOCK_COMMON is actually a misnomer, since > > > > > nothing enforces that arg_btf_id is actually an ID for sock common. > > > > > This is where ARG_PTR_TO_SOCK_COMMON_OR_NULL is much easier to > > > > > understand, even though it's more permissive than it has to be. It > > > > > communicates very clearly what values the argument can take. > > > > _OR_NULL is incorrect which implies a scalar NULL as mentioned in > > > > this commit message. From verifier pov, _OR_NULL can take > > > > a scalar NULL. > > > > > > Yes, I know. I'm saying that the distinction between scalar NULL and > > > runtime NULL only makes sense after you understand how BTF pointers > > > are implemented. It only clicked for me after I read the support code > > > in the JIT that Yonghong pointed out. Should everybody that writes a > > > helper need to read the JIT? In my opinion we shouldn't. I guess I > > > don't even care about the verifier rejecting scalar NULL or not, I'd > > > just like the types to have a name that conveys their NULLness. > > It is not only about verifier and/or JIT, not sure why it is related to > > JIT also. > > > > For some helpers, explicitly passing NULL may make sense. > > e.g. bpf_sk_assign(ctx, NULL, 0) makes sense. > > > > For most helpers, the bpf prog is wrong for sure, for example > > in sockmap, what does bpf_map_update_elem(sock_map, key, NULL, 0) > > mean? I would expect a delete from the sock_map if the verifier > > accepted it. > > > > > > > > > > > > > > > > > > > If you're set on ARG_PTR_TO_BTF_ID_SOCK_COMMON I'd suggest forcing the > > > > > btf_id in struct bpf_reg_types. This avoids the weird case where the > > > > > btf_id doesn't actually point at sock_common, and it also makes my > > > > I have considered the bpf_reg_types option. I prefer all > > > > arg info (arg_type and arg_btf_id) stay in the same one > > > > place (i.e. func_proto) as much as possible for now > > > > instead of introducing another place to specify/override it > > > > which then depends on a particular arg_type that some arg_type may be > > > > in func_proto while some may be in other places. > > > > > > In my opinion that ship sailed when we started aliasing arg_type to > > > multiple reg_type, but OK. > > > > > > > > > > > The arg_btf_id can be checked in check_btf_id_ok() if it would be a > > > > big concern that it might slip through the review but I think the > > > > chance is pretty low. > > > > > > Why increase the burden on human reviewers? Why add code to check an > > > invariant that we could get rid of in the first place? > > Lets take the scalar NULL example that requires to read multiple > > pieces of codes in different places (verifier, JIT...etc.). > > As you also mentioned, yes, it may be easy for a few people. > > However, for most others, having some obvious things in the same place is > > easier to review. > > > > I think we have to agree we disagree on this one implementation details > > which I think it has been over-thought (and time also). > > > > If you insist that should go into bpf_reg_types (i.e. compatible->btf_id), > > I can do that in v4 and then add another check in another place to > > ensure "!compatible->btf_id" as in v2. > > No, I don't insist. I was hoping I could convince you, but alas :) Not very convinced but I don't feel very strongly on this ;) so I have posted v4 with btf_id in struct bpf_reg_types to continue the review of the set. Thanks.
diff --git a/include/linux/bpf.h b/include/linux/bpf.h index fc5c901c7542..d0937f1d2980 100644 --- a/include/linux/bpf.h +++ b/include/linux/bpf.h @@ -292,6 +292,7 @@ enum bpf_arg_type { ARG_PTR_TO_ALLOC_MEM, /* pointer to dynamically allocated memory */ ARG_PTR_TO_ALLOC_MEM_OR_NULL, /* pointer to dynamically allocated memory or NULL */ ARG_CONST_ALLOC_SIZE_OR_ZERO, /* number of allocated bytes requested */ + ARG_PTR_TO_BTF_ID_SOCK_COMMON, /* pointer to in-kernel sock_common or bpf-mirrored bpf_sock */ __BPF_ARG_TYPE_MAX, }; diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c index 3ce61c412ea0..2468533bc4a1 100644 --- a/kernel/bpf/verifier.c +++ b/kernel/bpf/verifier.c @@ -486,7 +486,12 @@ static bool is_acquire_function(enum bpf_func_id func_id, static bool is_ptr_cast_function(enum bpf_func_id func_id) { return func_id == BPF_FUNC_tcp_sock || - func_id == BPF_FUNC_sk_fullsock; + func_id == BPF_FUNC_sk_fullsock || + func_id == BPF_FUNC_skc_to_tcp_sock || + func_id == BPF_FUNC_skc_to_tcp6_sock || + func_id == BPF_FUNC_skc_to_udp6_sock || + func_id == BPF_FUNC_skc_to_tcp_timewait_sock || + func_id == BPF_FUNC_skc_to_tcp_request_sock; } /* string representation of 'enum bpf_reg_type' */ @@ -3973,6 +3978,16 @@ static const struct bpf_reg_types sock_types = { }, }; +static const struct bpf_reg_types btf_id_sock_common_types = { + .types = { + PTR_TO_SOCK_COMMON, + PTR_TO_SOCKET, + PTR_TO_TCP_SOCK, + PTR_TO_XDP_SOCK, + PTR_TO_BTF_ID, + }, +}; + static const struct bpf_reg_types mem_types = { .types = { PTR_TO_STACK, @@ -4014,6 +4029,7 @@ static const struct bpf_reg_types *compatible_reg_types[] = { [ARG_PTR_TO_CTX] = &context_types, [ARG_PTR_TO_CTX_OR_NULL] = &context_types, [ARG_PTR_TO_SOCK_COMMON] = &sock_types, + [ARG_PTR_TO_BTF_ID_SOCK_COMMON] = &btf_id_sock_common_types, [ARG_PTR_TO_SOCKET] = &fullsock_types, [ARG_PTR_TO_SOCKET_OR_NULL] = &fullsock_types, [ARG_PTR_TO_BTF_ID] = &btf_ptr_types, @@ -4579,9 +4595,13 @@ static bool check_btf_id_ok(const struct bpf_func_proto *fn) { int i; - for (i = 0; i < ARRAY_SIZE(fn->arg_type); i++) - if (fn->arg_type[i] == ARG_PTR_TO_BTF_ID && !fn->arg_btf_id[i]) - return false; + for (i = 0; i < ARRAY_SIZE(fn->arg_type); i++) { + if (fn->arg_type[i] == ARG_PTR_TO_BTF_ID || + fn->arg_type[i] == ARG_PTR_TO_BTF_ID_SOCK_COMMON) { + if (!fn->arg_btf_id[i]) + return false; + } + } return true; } diff --git a/net/core/filter.c b/net/core/filter.c index 6014e5f40c58..54b338de4bb8 100644 --- a/net/core/filter.c +++ b/net/core/filter.c @@ -77,6 +77,9 @@ #include <net/transp_v6.h> #include <linux/btf_ids.h> +static const struct bpf_func_proto * +bpf_sk_base_func_proto(enum bpf_func_id func_id); + int copy_bpf_fprog_from_user(struct sock_fprog *dst, sockptr_t src, int len) { if (in_compat_syscall()) { @@ -6619,7 +6622,7 @@ sock_addr_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog) return NULL; } default: - return bpf_base_func_proto(func_id); + return bpf_sk_base_func_proto(func_id); } } @@ -6638,7 +6641,7 @@ sk_filter_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog) case BPF_FUNC_perf_event_output: return &bpf_skb_event_output_proto; default: - return bpf_base_func_proto(func_id); + return bpf_sk_base_func_proto(func_id); } } @@ -6799,7 +6802,7 @@ tc_cls_act_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog) return &bpf_sk_assign_proto; #endif default: - return bpf_base_func_proto(func_id); + return bpf_sk_base_func_proto(func_id); } } @@ -6840,7 +6843,7 @@ xdp_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog) return &bpf_tcp_gen_syncookie_proto; #endif default: - return bpf_base_func_proto(func_id); + return bpf_sk_base_func_proto(func_id); } } @@ -6882,7 +6885,7 @@ sock_ops_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog) return &bpf_tcp_sock_proto; #endif /* CONFIG_INET */ default: - return bpf_base_func_proto(func_id); + return bpf_sk_base_func_proto(func_id); } } @@ -6928,7 +6931,7 @@ sk_msg_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog) return &bpf_get_cgroup_classid_curr_proto; #endif default: - return bpf_base_func_proto(func_id); + return bpf_sk_base_func_proto(func_id); } } @@ -6970,7 +6973,7 @@ sk_skb_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog) return &bpf_skc_lookup_tcp_proto; #endif default: - return bpf_base_func_proto(func_id); + return bpf_sk_base_func_proto(func_id); } } @@ -6981,7 +6984,7 @@ flow_dissector_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog) case BPF_FUNC_skb_load_bytes: return &bpf_flow_dissector_load_bytes_proto; default: - return bpf_base_func_proto(func_id); + return bpf_sk_base_func_proto(func_id); } } @@ -7008,7 +7011,7 @@ lwt_out_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog) case BPF_FUNC_skb_under_cgroup: return &bpf_skb_under_cgroup_proto; default: - return bpf_base_func_proto(func_id); + return bpf_sk_base_func_proto(func_id); } } @@ -9745,7 +9748,7 @@ sk_lookup_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog) case BPF_FUNC_sk_release: return &bpf_sk_release_proto; default: - return bpf_base_func_proto(func_id); + return bpf_sk_base_func_proto(func_id); } } @@ -9912,7 +9915,7 @@ const struct bpf_func_proto bpf_skc_to_tcp6_sock_proto = { .func = bpf_skc_to_tcp6_sock, .gpl_only = false, .ret_type = RET_PTR_TO_BTF_ID_OR_NULL, - .arg1_type = ARG_PTR_TO_BTF_ID, + .arg1_type = ARG_PTR_TO_BTF_ID_SOCK_COMMON, .arg1_btf_id = &btf_sock_ids[BTF_SOCK_TYPE_SOCK_COMMON], .ret_btf_id = &btf_sock_ids[BTF_SOCK_TYPE_TCP6], }; @@ -9929,7 +9932,7 @@ const struct bpf_func_proto bpf_skc_to_tcp_sock_proto = { .func = bpf_skc_to_tcp_sock, .gpl_only = false, .ret_type = RET_PTR_TO_BTF_ID_OR_NULL, - .arg1_type = ARG_PTR_TO_BTF_ID, + .arg1_type = ARG_PTR_TO_BTF_ID_SOCK_COMMON, .arg1_btf_id = &btf_sock_ids[BTF_SOCK_TYPE_SOCK_COMMON], .ret_btf_id = &btf_sock_ids[BTF_SOCK_TYPE_TCP], }; @@ -9953,7 +9956,7 @@ const struct bpf_func_proto bpf_skc_to_tcp_timewait_sock_proto = { .func = bpf_skc_to_tcp_timewait_sock, .gpl_only = false, .ret_type = RET_PTR_TO_BTF_ID_OR_NULL, - .arg1_type = ARG_PTR_TO_BTF_ID, + .arg1_type = ARG_PTR_TO_BTF_ID_SOCK_COMMON, .arg1_btf_id = &btf_sock_ids[BTF_SOCK_TYPE_SOCK_COMMON], .ret_btf_id = &btf_sock_ids[BTF_SOCK_TYPE_TCP_TW], }; @@ -9977,7 +9980,7 @@ const struct bpf_func_proto bpf_skc_to_tcp_request_sock_proto = { .func = bpf_skc_to_tcp_request_sock, .gpl_only = false, .ret_type = RET_PTR_TO_BTF_ID_OR_NULL, - .arg1_type = ARG_PTR_TO_BTF_ID, + .arg1_type = ARG_PTR_TO_BTF_ID_SOCK_COMMON, .arg1_btf_id = &btf_sock_ids[BTF_SOCK_TYPE_SOCK_COMMON], .ret_btf_id = &btf_sock_ids[BTF_SOCK_TYPE_TCP_REQ], }; @@ -9999,7 +10002,38 @@ const struct bpf_func_proto bpf_skc_to_udp6_sock_proto = { .func = bpf_skc_to_udp6_sock, .gpl_only = false, .ret_type = RET_PTR_TO_BTF_ID_OR_NULL, - .arg1_type = ARG_PTR_TO_BTF_ID, + .arg1_type = ARG_PTR_TO_BTF_ID_SOCK_COMMON, .arg1_btf_id = &btf_sock_ids[BTF_SOCK_TYPE_SOCK_COMMON], .ret_btf_id = &btf_sock_ids[BTF_SOCK_TYPE_UDP6], }; + +static const struct bpf_func_proto * +bpf_sk_base_func_proto(enum bpf_func_id func_id) +{ + const struct bpf_func_proto *func; + + switch (func_id) { + case BPF_FUNC_skc_to_tcp6_sock: + func = &bpf_skc_to_tcp6_sock_proto; + break; + case BPF_FUNC_skc_to_tcp_sock: + func = &bpf_skc_to_tcp_sock_proto; + break; + case BPF_FUNC_skc_to_tcp_timewait_sock: + func = &bpf_skc_to_tcp_timewait_sock_proto; + break; + case BPF_FUNC_skc_to_tcp_request_sock: + func = &bpf_skc_to_tcp_request_sock_proto; + break; + case BPF_FUNC_skc_to_udp6_sock: + func = &bpf_skc_to_udp6_sock_proto; + break; + default: + return bpf_base_func_proto(func_id); + } + + if (!perfmon_capable()) + return NULL; + + return func; +}
There is a constant need to add more fields into the bpf_tcp_sock for the bpf programs running at tc, sock_ops...etc. A current workaround could be to use bpf_probe_read_kernel(). However, other than making another helper call for reading each field and missing CO-RE, it is also not as intuitive to use as directly reading "tp->lsndtime" for example. While already having perfmon cap to do bpf_probe_read_kernel(), it will be much easier if the bpf prog can directly read from the tcp_sock. This patch tries to do that by using the existing casting-helpers bpf_skc_to_*() whose func_proto returns a btf_id. For example, the func_proto of bpf_skc_to_tcp_sock returns the btf_id of the kernel "struct tcp_sock". These helpers are also added to is_ptr_cast_function(). It ensures the returning reg (BPF_REF_0) will also carries the ref_obj_id. That will keep the ref-tracking works properly. The bpf_skc_to_* helpers are made available to most of the bpf prog types in filter.c. They are limited by perfmon cap. This patch adds a ARG_PTR_TO_BTF_ID_SOCK_COMMON. The helper accepting this arg can accept a btf-id-ptr (PTR_TO_BTF_ID + &btf_sock_ids[BTF_SOCK_TYPE_SOCK_COMMON]) or a legacy-ctx-convert-skc-ptr (PTR_TO_SOCK_COMMON). The bpf_skc_to_*() helpers are changed to take ARG_PTR_TO_BTF_ID_SOCK_COMMON such that they will accept pointer obtained from skb->sk. PTR_TO_*_OR_NULL is not accepted as an ARG_PTR_TO_BTF_ID_SOCK_COMMON at verification time. All PTR_TO_*_OR_NULL reg has to do a NULL check first before passing into the helper or else the bpf prog will be rejected by the verifier. [ ARG_PTR_TO_SOCK_COMMON_OR_NULL was attempted earlier. The _OR_NULL was needed because the PTR_TO_BTF_ID could be NULL but note that a could be NULL PTR_TO_BTF_ID is not a scalar NULL to the verifier. "_OR_NULL" implicitly gives an expectation that the helper can take a scalar NULL which does not make sense in most (except one) helpers. Passing scalar NULL should be rejected at the verification time. Thus, this patch uses ARG_PTR_TO_BTF_ID_SOCK_COMMON to specify that the helper can take both the btf-id ptr or the legacy PTR_TO_SOCK_COMMON but not scalar NULL. It requires the func_proto to explicitly specify the arg_btf_id such that there is a very clear expectation that the helper can handle a NULL PTR_TO_BTF_ID. ] [ ARG_PTR_TO_BTF_ID_SOCK_COMMON will be used to replace ARG_PTR_TO_SOCK* of other existing helpers later such that those existing helpers can take the PTR_TO_BTF_ID returned by the bpf_skc_to_*() helpers. The only special case is bpf_sk_lookup_assign() which can accept a scalar NULL ptr. It has to be handled specially in another follow up patch (e.g. by renaming ARG_PTR_TO_SOCKET_OR_NULL to ARG_PTR_TO_BTF_ID_SOCK_COMMON_OR_NULL). ] [ When converting the older helpers that take ARG_PTR_TO_SOCK* in the later patch, if the kernel does not support BTF, ARG_PTR_TO_BTF_ID_SOCK_COMMON will behave like ARG_PTR_TO_SOCK_COMMON because no reg->type could have PTR_TO_BTF_ID in this case. It is not a concern for the newer-btf-only helper like the bpf_skc_to_*() here though because these helpers must require BTF vmlinux to begin with. ] Signed-off-by: Martin KaFai Lau <kafai@fb.com> --- include/linux/bpf.h | 1 + kernel/bpf/verifier.c | 28 ++++++++++++++++--- net/core/filter.c | 64 +++++++++++++++++++++++++++++++++---------- 3 files changed, 74 insertions(+), 19 deletions(-)