Re: [PATCH v5 7/7] string: Implement strstarts() with strprefix()

[Alejandro Colomar <alx+linux-hardening@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 8747 bytes --]

> Date: 2026-11-13 21:57:24+0100
> From: Alejandro Colomar <alx+linux-hardening@kernel.org>
>
[...]
> > > -  strstarts():
> > > 	It could be improved by returning a boolean-like pointer,
> > > 	instead of a bool, but:
> > > 
> > > 	-  We may need to fix some callers, since some of them might
> > > 	   need an explicit conversion with !!.  It'd be risky to change
> > > 	   the implementation without first verifying that.
> > 
> > How many cases do we have that needs special attention? Can you run spatch to
> > grep those?
> 
> I *think* the only ones I expect this would be necessary are functions
> that return a boolean-like int.
> 
> 	// returns 1:true 0:false -1:error=
> 	int
> 	foo(...)
> 	{
> 		...
> 		return !!strprefix(...);
> 	}

I've searched for cases of this, and also for cases of assignment to
a variable that might be of type int (I expect this to be less
frequent).  I haven't used spatch(1) so far, but just grep(1):

	$ grep -rn -e '= .*strstarts(' -e 'return .*strstarts('
	drivers/mtd/spi-nor/issi.c:141:	return name && strstarts(name, "pm25lv");
	drivers/platform/x86/dell/dell-lis3lv02d.c:109:	return strstarts(adap->name, "SMBus I801 adapter");
	arch/arm/kernel/module.c:25:	return strstarts(name, ".init") ||
	arch/arm/kernel/module.c:32:	return strstarts(name, ".exit") ||
	arch/powerpc/perf/imc-pmu.c:1026:	return strstarts(event->pmu->name, "thread_imc");
	tools/perf/util/map.c:21:	return strstarts(filename, "/data/app-lib/") ||
	tools/perf/util/annotate-arch/annotate-x86.c:144:	if (arch->family >= 0x15 && (strstarts(ins1, "test") ||
	tools/perf/util/annotate-arch/annotate-x86.c:150:	if (arch->family >= 0x19 && (strstarts(ins1, "add") ||
	tools/perf/util/annotate-arch/annotate-x86.c:200:		arch->ins_is_fused = strstarts(cpuid, "AuthenticAMD") ?
	tools/perf/util/symbol.c:854:	if (e_machine == EM_S390 && strstarts(name, "psw_idle")) {
	tools/perf/util/env.c:733:		return strstarts(perf_arch, "ppc64") ? EM_PPC64 : EM_PPC;
	tools/perf/util/env.c:1049:		is_amd = env->cpuid && strstarts(env->cpuid, "AuthenticAMD") ? 1 : -1;
	tools/perf/util/env.c:1072:		is_intel = env->cpuid && strstarts(env->cpuid, "GenuineIntel") ? 1 : -1;
	tools/perf/util/symbol.h:55:	return strstarts(str, KLP_SYM_PREFIX);
	tools/perf/builtin-trace.c:2272:			if (*after == '=' && (strstarts(after + 1, "0x") ||
	tools/perf/perf.c:143:	return (strstarts(var, header) && !strcmp(var + strlen(header), c->cmd));
	tools/perf/tests/symbols.c:155:	if (dso != text_dso && strstarts(dso__short_name(dso), dso__short_name(text_dso)))
	tools/testing/selftests/bpf/prog_tests/sockmap_redir.c:321:	if (type == SK_SKB_INGRESS && strstarts(out, "v_"))
	tools/objtool/klp-sympos.c:372:	return strstarts(sym->sec->name, ".init");
	tools/objtool/klp-checksum.c:66:	return strstarts(sym->sec->name, ".rodata.cst") &&
	tools/objtool/klp-diff.c:204:		export->mod_ns = strstarts(namespace, "module:");
	tools/objtool/klp-diff.c:339:	return strstarts(sym->name, ".L");
	tools/objtool/klp-diff.c:380:	return strstarts(sec->name, ".discard.");
	tools/objtool/klp-diff.c:422:	return strstarts(sym->name, "__initcall__") ||
	kernel/module/main.c:1866:	return strstarts(name, ".init");
	kernel/module/main.c:1871:	return strstarts(name, ".exit");

From all these, only one case was using int.  Here's a patch for making
it more robust:

	diff --git i/tools/perf/util/map.c w/tools/perf/util/map.c
	index 41cdddc987ee..944f10d32796 100644
	--- i/tools/perf/util/map.c
	+++ w/tools/perf/util/map.c
	@@ -16,7 +16,7 @@
	 #include "thread.h"
	 #include "vdso.h"
	 
	-static inline int is_android_lib(const char *filename)
	+static inline bool is_android_lib(const char *filename)
	 {
		return strstarts(filename, "/data/app-lib/") ||
		       strstarts(filename, "/system/lib/");

It's technically not necessary, since the '||' operator converts to
bool, but just in case someone removes one of those calls, using bool
will prevent a bug.

All the other cases were either using 'bool', or didn't need a change
for other reasons.

So, I can include this diff as a preparation patch in this set, and then
make strstarts() be
	#define strstarts(s, prefix)  strprefix(s, prefix)

And then have a global search-and-replace for it.


Cheers,
Alex

> 
> > 
> > > 	   To avoid that risk, I prefer adding a new API, and
> > > 	   deprecating strstarts() for new code.  Over time, I could
> > > 	   slowly migrate users from strstarts() to strprefix().
> > 
> > I believe this is unneeded step which may last for a few releases without even
> > guarantee that it will be finished. In such a case I would like to see a branch
> > with full conversion done before you start sending per-subsystem pieces out.
> 
> Okay; I can do that.
> 
> > > 	-  The name isn't ideal, since "starts" doesn't specify whether
> > > 	   it looks for a string that starts with a given prefix, or if
> > > 	   it looks for a string that starts with any of the given
> > > 	   characters.
> > > 
> > > 	   That is, by the name, I wouldn't be able to know whether
> > > 	   strstarts implements
> > > 
> > > 		bool strstarts(const char *s, const char *xxx)
> > > 		{
> > > 			return strspn(s, xxx);
> > > 		}
> > > 	   or
> > > 		bool strstarts(const char *s, const char *xxx)
> > > 		{
> > > 			return strprefix(s, xxx);
> > > 		}
> > 
> > I didn't get this. We should not really need an implementation details of
> > the API beneath. Documentation what should be read before use. Do we have
> > documentation for strstarts()? Is it clear? Maybe that what should be addressed
> > first?
> > 
> > > -  strends():
> > > 	Same as with strstarts() but from the end.
> > > 
> > > -  str_has_prefix():
> > > 	It was an attempt at strprefix(), but with a problematic
> > > 	approach.  It's frozen because of trace code.
> > > 
> > > -  strprefix()/strsuffix():
> > > 	I'd add them to slowly replace strstarts()/strends().
> > 
> > See above about "slowly"...
> > 
> > > > > I prefer replacing strstarts()/strends() with strprefix()/strsuffix().
> > > > > 
> > > > > But if you prefer changing strstarts() to return a pointer, I'm also
> > > > > okay-ish with it.  It's not a perfect name, but I can live with it.
> > > > > 
> > > > > > However, after looking at the uses in the kernel, I prefer
> > > > > > to see the strprefix() be not implemented at all, just optimization against
> > > > > > strstarts()/strends() if required can be performed.
> > > > > 
> > > > > I don't understand this last sentence.
> > > > 
> > > > It's basically repeats the idea of having only one pair out of two.
> > > 
> > > Ok.
> > > 
> > > > With this series will be
> > > > 
> > > > str_has_previx()
> > > > strstarts()
> > > > strends()
> > > > strprefix()
> > > > // potentially strsuffix()
> > > 
> > > Yes.  I'd like to replace strstarts()/strends() with
> > > strprefix()/strsuffix(), but prefer to do that slowly, to avoid
> > > introducing bugs due to missing explicit conversions to bool.
> > > 
> > > Should I add notes that the strstarts()/strends() APIs are deprecated?
> > 
> > Isn't Steven against that? I already forgot his points.
> 
> As far as I understood, he was against deprecating str_has_prefix(), but
> we didn't yet talk about strstarts().
> 
> > Keeping them
> > just as aliases to strprefix()/strsuffix() perhaps the good start
> > (see below).
> 
> Ok.
> 
> > > > To me seems too many for the similar task. I understand that trace user is
> > > > special one, but does it use strstarts()/strends()?
> > > 
> > > I didn't find any uses:
> > > 
> > > 	$ git ls-files | grep kernel/trace/ | xargs grep strstarts
> > > 	$ git ls-files | grep kernel/trace/ | xargs grep strends
> > > 
> > > > Despite on naming I think we just good to have only str_has*() and one of
> > > > strprefix()/strsuffix(), strstarts()/strends().
> > > 
> > > In the long term, I agree.  But I think the transition should be
> > > careful.  What do you think?
> > 
> > Yes, it should take a few phases, first one perhaps to align strstarts() to not
> > rely on the returned value to be boolean.
> 
> Ok.
> 
> > In parallel with that, your current series can also be considered. Then
> > similar, if even required, to be done against strends().
> 
> Agree.
> 
> > And one more phase
> > is to replace strstarts()/strends() with strprefix()/strsuffix() all over
> > the kernel.
> 
> Ok.
> 
> Thanks!
> 
> 
> Have a lovely night!
> Alex
> 
> > 
> > -- 
> > With Best Regards,
> > Andy Shevchenko
> > 
> > 
> > 
> 
> -- 
> <https://www.alejandro-colomar.es>



-- 
<https://www.alejandro-colomar.es>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]