On Fri, Nov 10, 2023 at 9:37 AM Stanislav Fomichev <sdf@xxxxxxxxxx> wrote: > > On 11/10, Andrii Nakryiko wrote: > > Move a good chunk of code from verifier.c to log.c: verifier state > > verbose printing logic. This is an important and very much > > logging/debugging oriented code. It fits the overlall log.c's focus on > > verifier logging, and moving it allows to keep growing it without > > unnecessarily adding to verifier.c code that otherwise contains a core > > verification logic. > > > > There are not many shared dependencies between this code and the rest of > > verifier.c code, except a few single-line helpers for various register > > type checks and a bit of state "scratching" helpers. We move all such > > trivial helpers into include/bpf/bpf_verifier.h as static inlines. > > > > No functional changes in this patch. > > > > Signed-off-by: Andrii Nakryiko <andrii@xxxxxxxxxx> > > --- > > include/linux/bpf_verifier.h | 72 +++++++ > > kernel/bpf/log.c | 342 +++++++++++++++++++++++++++++ > > kernel/bpf/verifier.c | 403 ----------------------------------- > > 3 files changed, 414 insertions(+), 403 deletions(-) > > > > diff --git a/include/linux/bpf_verifier.h b/include/linux/bpf_verifier.h > > index d7898f636929..22f56f1eb27d 100644 > > --- a/include/linux/bpf_verifier.h > > +++ b/include/linux/bpf_verifier.h > > @@ -782,4 +782,76 @@ static inline bool bpf_type_has_unsafe_modifiers(u32 type) > > return type_flag(type) & ~BPF_REG_TRUSTED_MODIFIERS; > > } > > Does it make sense to have a new bpf_log.h and move these in there? > We can then include it from verifier.c only. Looks like bpf_verifier.h > is included in a bunch of places and those symbols don't have a prefix > and might (potentially) clash with something else in the future. > > Or is not a super clear cut? bpf_verifier.h should be used in very BPF-specific portions of code, so I think this shouldn't be a big problem. Doesn't feel justified to add a new small header for 4-5 functions, but I don't feel very strongly about this.
