Hi,

> 
> With the patch, kallsyms_show_value() might return true even when
> CONFIG_KALLYSMS are disabled. Did you check all users of this API
> that they could handle this?
> 
> For example, the comment in bpf_dump_raw_ok() suggests that
> it might require more kallsyms functionality.
>

Ok, we will check these other callers also, if it causes negative impact at other places.

> static inline bool bpf_dump_raw_ok(const struct cred *cred)
> {
> 	/* Reconstruction of call-sites is dependent on kallsyms,
> 	 * thus make dump the same restriction.
> 	 */
> 	return kallsyms_show_value(cred);
> }
> 
> You should definitely add into CC people from affected subsystems.
> I do not see:
> 
>     + Kees Cook who added/updated most of the callers
>     + BPF people that might require even more kallsyms functionality
>     + kprobe people that using it as well
>

Yes, I thought maintainer.pl has shown all people, but seems caller function owners
also need to be added, I will add  in next revision.



> > +/*
> > + * We show kallsyms information even to normal users if we've enabled
> > + * kernel profiling and are explicitly not paranoid (so kptr_restrict
> > + * is clear, and sysctl_perf_event_paranoid isn't set).
> > + *
> > + * Otherwise, require CAP_SYSLOG (assuming kptr_restrict isn't set to
> > + * block even that).
> > + */
> > +bool kallsyms_show_value(const struct cred *cred)
> > +{
> > +	switch (kptr_restrict) {
> > +	case 0:
> > +		if (kallsyms_for_perf())
> > +			return true;
> > +		fallthrough;
> > +	case 1:
> > +		if (security_capable(cred, &init_user_ns, CAP_SYSLOG,
> > +				     CAP_OPT_NOAUDIT) == 0)
> > +			return true;
> > +		fallthrough;
> > +	default:
> > +		return false;
> > +	}
> > +}
> 
> It is really weird that the function is declared in kallsyms.h
> and implemented in vsprintf.c.
>

Yes it does not look good.
Initially we thought to make it as static inline function in kallsyms.h only.
But as function is little big and it will increase code size, so we added
definition in vsprintf.c, because its alwyas compilable code and also it has
some wrapper APIs for kallsyms.
 
But as you suggested to make a new file, it will be good.

> What about splitting kallsyms.c into two source files where one
> would be always compiled? It would be usable also for the
> spring function introduced by
> https://lore.kernel.org/r/20220323164742.2984281-1-maninder1.s@samsung.com
> 
> It might be something like kallsyms_full.c and/or kallsyms_tiny.c.
>

This patch is not taken by Luis yet in module-tetsing branch.
So what will be the best approach to make new version of this patch ?

A) to make new file kallsyms_tiny.c and add only one definition in that file
and when above patch of spring functions is merged in next we can move definitions
to new file

or

B) we send patch to Luis's branch of module-testing with moving definition(of earlier patch)
to new file and current patch also.

Thanks,
Maninder Singh