Re: [PATCH net-next v5 1/3] net: flow_dissector: extend bpf flow dissector support with vnet hdr

On Fri, Jun 11, 2021 at 02:07:10PM -0700, Tanner Love wrote:

quoted

A nit. It is a good chance to move the new BTF_ID_LIST_SINGLE
and most of the check_flow_keys_access() to filter.c.
Take a look at check_sock_access().

It's not clear to me why it's preferable to move most of the
check_flow_keys_access() to filter.c. In particular, the part of
your comment that I don't understand is the "most of" part. Why
would we want to separate the flow-keys-access-checking logic
into two separate functions? Thanks

Right, actually, the whole function can be moved.
I found it easier to follow from flow_dissector_is_valid_access()
to flow_keys's access check without jumping around between two
different files.

Additionally, it seems that we don't actually need the
BTF_ID_LIST_SINGLE. Can we not, instead, set
regs[value_regno].btf_id to
btf_find_by_name_kind(btf_vmlinux, "virtio_net_hdr", BTF_KIND_STRUCT)
? (And we'll check that that value is not <= 0.)

BTF_ID_LIST_SINGLE is resolved during compilation.
btf_find_by_name_kind() will be repeatedly finding the btf_id during runtime.
It is not like a killer but still unnecessary.

On Fri, Jun 11, 2021 at 06:13:42PM -0700, Martin KaFai Lau wrote:

On Fri, Jun 11, 2021 at 02:07:10PM -0700, Tanner Love wrote:

quoted

quoted

A nit. It is a good chance to move the new BTF_ID_LIST_SINGLE
and most of the check_flow_keys_access() to filter.c.
Take a look at check_sock_access().

It's not clear to me why it's preferable to move most of the
check_flow_keys_access() to filter.c. In particular, the part of
your comment that I don't understand is the "most of" part. Why
would we want to separate the flow-keys-access-checking logic
into two separate functions? Thanks

Right, actually, the whole function can be moved.
I found it easier to follow from flow_dissector_is_valid_access()
to flow_keys's access check without jumping around between two
different files.

The verifier verbose() logs can be kept in verifier.c though.  I
think all the -EACCES cases can be consolidated to one verbose()
call in check_mem_access() under PTR_TO_FLOW_KEYS.