Re: [PATCH bpf-next] bpf: btf: restore resolve_mode when popping the resolve stack

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

 



On 5/17/23 2:01 AM, Lorenz Bauer wrote:
On Wed, May 17, 2023 at 7:26 AM Martin KaFai Lau <martin.lau@xxxxxxxxx> wrote:

On 5/15/23 5:15 AM, Lorenz Bauer wrote:
In commit 9b459804ff99 ("btf: fix resolving BTF_KIND_VAR after ARRAY, STRUCT, UNION, PTR")
I fixed a bug that occurred during resolving of a DATASEC by strategically resetting
resolve_mode. This fixes the immediate bug but leaves us open to future bugs where
nested types have to be resolved.

hmm... future bugs like when adding new BTF_KIND in the future?

It could just be refactoring of the codebase? What is the downside of
restoring the mode when popping the item? It also makes push and pop
symmetrical.

I can see your point to refactor it to make it work for all different BTF_KIND.

Other than BTF_KIND_DATASEC, env->resolve_mode stays the same for all other kinds once it is decided. It is why resolve_mode is in the "env" instead of "v". My concern is this will hide some bugs (existing or future) that accidentally changed the resolve_mode in the middle. If there is another legit case that could be found other than BTF_KIND_DATASEC, that will be a better time to do this refactoring with a proper test case considering most bpf progs need btf to load nowadays and probably need to veristat test also. If it came to that, might as well consider moving resolve_mode from "env" to "v".

btf_datasec_resolve() is acting as a very top level resolver like btf_resolve(), so it reset env->resolve_mode before resolving its var member like how btf_resolve() does. imo, together with env->resolve_mode stays the same for others, it is more straight forward to reason. I understand that it is personal preference and could argue either way.




[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