From: Alejandro Colomar <alx+linux-hardening@kernel.org>
To: Steven Rostedt <rostedt@goodmis.org>
Cc: Alejandro Colomar <alx@kernel.org>,
linux-hardening@vger.kernel.org,
Bartosz Golaszewski <bartosz.golaszewski@oss.qualcomm.com>,
Andreas Schwab <schwab@linux-m68k.org>,
Andy Shevchenko <andriy.shevchenko@linux.intel.com>,
Andy Shevchenko <andy@kernel.org>,
Bartosz Golaszewski <brgl@kernel.org>,
Daniel Scally <djrscally@gmail.com>,
Danilo Krummrich <dakr@kernel.org>,
Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
Hans Verkuil <hverkuil-cisco@xs4all.nl>,
Heikki Krogerus <heikki.krogerus@linux.intel.com>,
Joe Perches <joe@perches.com>, Johan Hovold <johan@kernel.org>,
Kees Cook <kees@kernel.org>,
Laurent Pinchart <laurent.pinchart@ideasonboard.com>,
Len Brown <lenb@kernel.org>,
Linus Torvalds <torvalds@linux-foundation.org>,
Linus Walleij <linusw@kernel.org>,
Namhyung Kim <namhyung@kernel.org>,
"Rafael J. Wysocki" <rafael@kernel.org>,
Rob Herring <robh@kernel.org>,
Sakari Ailus <sakari.ailus@linux.intel.com>,
Saravana Kannan <saravanak@kernel.org>,
linux-acpi@vger.kernel.org, driver-core@lists.linux.dev,
linux-kernel@vger.kernel.org, devicetree@vger.kernel.org,
linux-gpio@vger.kernel.org
Subject: Re: [PATCH v1 1/2] string: Add strprefix(), and deprecate str_has_prefix()
Date: Thu, 5 Nov 2026 18:46:18 +0100 [thread overview]
Message-ID: <auy0O467UqOzqYub@devuan> (raw)
In-Reply-To: <20261105115019.2fe7c91b@gandalf.local.home>
[-- Attachment #1: Type: text/plain, Size: 14800 bytes --]
Hi Steve,
> Date: 2026-11-05 11:50:19-0500
> From: Steven Rostedt <rostedt@goodmis.org>
>
[...]
> > > > Reported-by: Bartosz Golaszewski <bartosz.golaszewski@oss.qualcomm.com>
> > >
> > > Is there a link to the report? Please add that.
> >
> > Message-Id: <20261104-fwnode-for-each-prop-v1-1-6e28fe1c72c4@oss.qualcomm.com>
> >
> > or in link format,
> >
> > Link: <https://lore.kernel.org/linux-hardening/20261104-fwnode-for-each-prop-v1-1-6e28fe1c72c4@oss.qualcomm.com/>
> >
> > Which one do you prefer?
>
> The correct method is:
>
> Link: https://lore.kernel.org/linux-hardening/20261104-fwnode-for-each-prop-v1-1-6e28fe1c72c4@oss.qualcomm.com/
Ok; done.
[...]
> > > > + * DEPRECATED: Use strprefix()
> > >
> > > No, it is still useful. Converting trace_event_filter() to strprefix()
> > > would be very painful, and likely error prone.
> >
> > What is trace_event_filter()?
>
> Oops, typo, I meant trace_event_filter.c
Still not found.
alx@devuan:~/src/linux/linux/master$ find | grep trace_event_filter.c
alx@devuan:~/src/linux/linux/master$ find | grep trace.event.filter
alx@devuan:~/src/linux/linux/master$
Oh, now I see:
$ grep -rl str_has_prefix | grep trace.event
kernel/trace/trace_events_user.c
kernel/trace/trace_events.c
kernel/trace/trace_events_synth.c
kernel/trace/trace_events_filter.c
kernel/trace/trace_events_hist.c
<trace_events_filter.c>, then. I've been having a look at that file,
out of curiosity, and it'd be simpler (more robust) with pointers:
diff --git i/kernel/trace/trace_events_filter.c w/kernel/trace/trace_events_filter.c
index c10951bcfc5f..44594a41d2d6 100644
--- i/kernel/trace/trace_events_filter.c
+++ w/kernel/trace/trace_events_filter.c
@@ -1674 +1674 @@ static int parse_pred(const char *str, void *data,
- int s;
+ char *s;
@@ -1675,0 +1676,9 @@ static int parse_pred(const char *str, void *data,
+ char *p;
+
+#define CTYPE_LOWER_C "abcdefghijklmnopqrstuvwxyz"
+#define CTYPE_UPPER_C "ABCDEFGHIJKLMNOPQRSTUVWXYZ"
+#define CTYPE_DIGIT_C "0123456789"
+#define CTYPE_BLANK_C " \t"
+#define CTYPE_SPACE_C CTYPE_BLANK_C "\n\v\f\r"
+#define CTYPE_ALPHA_C CTYPE_LOWER_C CTYPE_UPPER_C
+#define CTYPE_ALNUM_C CTYPE_ALPHA_C CTYPE_DIGIT_C
@@ -1678,3 +1687,2 @@ static int parse_pred(const char *str, void *data,
- while (isspace(str[i]))
- i++;
- s = i;
+ s = str;
+ s = stpspn(s, CTYPE_SPACE_C);
@@ -1682,6 +1690,3 @@ static int parse_pred(const char *str, void *data,
- while (isalnum(str[i]) || str[i] == '_')
- i++;
-
- len = i - s;
-
- if (!len)
+ p = s;
+ p = stpspn(p, CTYPE_ALNUM_C "_");
+ if (p == s)
@@ -1690 +1695 @@ static int parse_pred(const char *str, void *data,
- field_name = kmemdup_nul(str + s, len, GFP_KERNEL);
+ field_name = kmemdup_nul(s, p - s, GFP_KERNEL);
@@ -1704 +1709 @@ static int parse_pred(const char *str, void *data,
- if ((len = str_has_prefix(str + i, ".ustring"))) {
+ if (char *p_ = strprefix(p, ".ustring")) {
@@ -1706 +1711 @@ static int parse_pred(const char *str, void *data,
- i += len;
+ p = p_;
@@ -1710 +1715 @@ static int parse_pred(const char *str, void *data,
- if ((len = str_has_prefix(str + i, ".function"))) {
+ if (char *p_ = strprefix(p, ".function")) {
@@ -1712 +1717 @@ static int parse_pred(const char *str, void *data,
- i += len;
+ p = p_;
@@ -1715,2 +1720 @@ static int parse_pred(const char *str, void *data,
- while (isspace(str[i]))
- i++;
+ p = stpspn(p, CTYPE_SPACE_C);
@@ -1721 +1725 @@ static int parse_pred(const char *str, void *data,
- if (strncmp(str + i, ops[op], strlen(ops[op])) == 0)
+ if (strprefix(p, ops[op]))
@@ -1726 +1730 @@ static int parse_pred(const char *str, void *data,
- parse_error(pe, FILT_ERR_INVALID_OP, pos + i);
+ parse_error(pe, FILT_ERR_INVALID_OP, pos + (p - str));
@@ -1730,6 +1734,3 @@ static int parse_pred(const char *str, void *data,
- i += strlen(ops[op]);
-
- while (isspace(str[i]))
- i++;
-
- s = i;
+ p += strlen(ops[op]);
+ p = stpspn(p, CTYPE_SPACE_C);
+ s = p;
@@ -1748 +1749 @@ static int parse_pred(const char *str, void *data,
- parse_error(pe, FILT_ERR_ILLEGAL_FIELD_OP, pos + i);
+ parse_error(pe, FILT_ERR_ILLEGAL_FIELD_OP, pos + (p - str));
@@ -1758 +1759 @@ static int parse_pred(const char *str, void *data,
- parse_error(pe, FILT_ERR_INVALID_OP, pos + i);
+ parse_error(pe, FILT_ERR_INVALID_OP, pos + (p - str));
@@ -1762 +1763 @@ static int parse_pred(const char *str, void *data,
- if (isdigit(str[i])) {
+ if (isdigit(*p)) {
@@ -1764,2 +1765 @@ static int parse_pred(const char *str, void *data,
- while (isalnum(str[i]))
- i++;
+ p = stpspn(p, CTYPE_ALNUM_C);
@@ -1767 +1766,0 @@ static int parse_pred(const char *str, void *data,
- len = i - s;
@@ -1769,2 +1768,3 @@ static int parse_pred(const char *str, void *data,
- if (len >= sizeof(num_buf)) {
- parse_error(pe, FILT_ERR_OPERAND_TOO_LONG, pos + i);
+ len = p - s;
+ if (len >= ARRAY_SIZE(num_buf)) {
+ parse_error(pe, FILT_ERR_OPERAND_TOO_LONG, pos + (p - str));
@@ -1774,2 +1774 @@ static int parse_pred(const char *str, void *data,
- memcpy(num_buf, str + s, len);
- num_buf[len] = 0;
+ strcpy(mempcpy(num_buf, s, len), "");
@@ -1779 +1778 @@ static int parse_pred(const char *str, void *data,
- parse_error(pe, FILT_ERR_INVALID_VALUE, pos + i);
+ parse_error(pe, FILT_ERR_INVALID_VALUE, pos + (p - str));
@@ -1783,3 +1782,2 @@ static int parse_pred(const char *str, void *data,
- s = i;
- for (; str[i] && !isspace(str[i]); i++)
- ;
+ s = p; // XXX: Dead code? (line 1734+2)
+ p = stpcspn(p, CTYPE_SPACE_C);
@@ -1787,2 +1785,2 @@ static int parse_pred(const char *str, void *data,
- len = i - s;
- name = kmemdup_nul(str + s, len, GFP_KERNEL);
+ len = p - s;
+ name = kmemdup_nul(s, len, GFP_KERNEL);
@@ -1794 +1792 @@ static int parse_pred(const char *str, void *data,
- parse_error(pe, FILT_ERR_NO_FUNCTION, pos + i);
+ parse_error(pe, FILT_ERR_NO_FUNCTION, pos + (p - str));
@@ -1801 +1799 @@ static int parse_pred(const char *str, void *data,
- parse_error(pe, FILT_ERR_NO_FUNCTION, pos + i);
+ parse_error(pe, FILT_ERR_NO_FUNCTION, pos + (p - str));
@@ -1818 +1816 @@ static int parse_pred(const char *str, void *data,
- parse_error(pe, FILT_ERR_IP_FIELD_ONLY, pos + i);
+ parse_error(pe, FILT_ERR_IP_FIELD_ONLY, pos + (p - str));
@@ -1827,2 +1825,4 @@ static int parse_pred(const char *str, void *data,
- if (str[i] == '\'' || str[i] == '"')
- q = str[i];
+ if (strspn(p, "'"))
+ q = "'";
+ else if (strspn(p, "\""))
+ q = "\"";
@@ -1830 +1830 @@ static int parse_pred(const char *str, void *data,
- q = 0;
+ q = NULL;
@@ -1832,2 +1832,2 @@ static int parse_pred(const char *str, void *data,
- for (i++; str[i]; i++) {
- if (q && str[i] == q)
+ for (p++; !streq(p, ""); p++) {
+ if (q && strspn(p, q))
@@ -1835,2 +1835 @@ static int parse_pred(const char *str, void *data,
- if (!q && (str[i] == ')' || str[i] == '&' ||
- str[i] == '|'))
+ if (!q && strspn(p, ")&|"))
@@ -1842 +1841 @@ static int parse_pred(const char *str, void *data,
- len = i - s;
+ len = p - s;
@@ -1844 +1843 @@ static int parse_pred(const char *str, void *data,
- parse_error(pe, FILT_ERR_OPERAND_TOO_LONG, pos + i);
+ parse_error(pe, FILT_ERR_OPERAND_TOO_LONG, pos + (p - str));
@@ -1852,2 +1851 @@ static int parse_pred(const char *str, void *data,
- memcpy(pred->regex->pattern, str + s, len);
- pred->regex->pattern[len] = 0;
+ strcpy(memcpy(pred->regex->pattern, s, len), "");
@@ -1855 +1853 @@ static int parse_pred(const char *str, void *data,
- } else if (!strncmp(str + i, "CPUS", 4)) {
+ } else if (strprefix(p, "CPUS")) {
@@ -1866 +1864 @@ static int parse_pred(const char *str, void *data,
- parse_error(pe, FILT_ERR_ILLEGAL_FIELD_OP, pos + i);
+ parse_error(pe, FILT_ERR_ILLEGAL_FIELD_OP, pos + (p - str));
@@ -1876 +1874 @@ static int parse_pred(const char *str, void *data,
- parse_error(pe, FILT_ERR_ILLEGAL_FIELD_OP, pos + i);
+ parse_error(pe, FILT_ERR_ILLEGAL_FIELD_OP, pos + (p - str));
@@ -1880,4 +1878,3 @@ static int parse_pred(const char *str, void *data,
- /* Skip CPUS */
- i += 4;
- if (str[i++] != '{') {
- parse_error(pe, FILT_ERR_MISSING_BRACE_OPEN, pos + i);
+ p = strprefix(p, "CPUS");
+ if (!strspn(p, "{")) {
+ parse_error(pe, FILT_ERR_MISSING_BRACE_OPEN, pos + (p - str));
@@ -1886 +1883 @@ static int parse_pred(const char *str, void *data,
- maskstart = i;
+ maskstart = p;
@@ -1889,5 +1886,3 @@ static int parse_pred(const char *str, void *data,
- for (; str[i] && str[i] != '}'; i++)
- ;
-
- if (str[i] != '}') {
- parse_error(pe, FILT_ERR_MISSING_BRACE_CLOSE, pos + i);
+ p = stpcspn(p, "}");
+ if (streq(p, "")) {
+ parse_error(pe, FILT_ERR_MISSING_BRACE_CLOSE, pos + (p - str));
@@ -1897,2 +1892,2 @@ static int parse_pred(const char *str, void *data,
- if (maskstart == i) {
- parse_error(pe, FILT_ERR_INVALID_CPULIST, pos + i);
+ if (maskstart == p) {
+ parse_error(pe, FILT_ERR_INVALID_CPULIST, pos + (p - str));
@@ -1903 +1898 @@ static int parse_pred(const char *str, void *data,
- tmp = kmalloc((i - maskstart) + 1, GFP_KERNEL);
+ tmp = kmalloc((p - maskstart) + 1, GFP_KERNEL);
@@ -1907 +1902 @@ static int parse_pred(const char *str, void *data,
- strscpy(tmp, str + maskstart, (i - maskstart) + 1);
+ strcpy(mempcpy(tmp, maskstart, p - maskstart), "");
@@ -1917 +1912 @@ static int parse_pred(const char *str, void *data,
- parse_error(pe, FILT_ERR_INVALID_CPULIST, pos + i);
+ parse_error(pe, FILT_ERR_INVALID_CPULIST, pos + (p - str));
@@ -1923 +1918 @@ static int parse_pred(const char *str, void *data,
- i++;
+ p = strprefix(p, "}");
BTW, I've included there a comment in a line of code that I think might
be dead code (I didn't verify thoroughly, but at first glance, it seems
to be). grep 'XXX' above.
> > alx@devuan:~/src/linux/linux/master$ grep -rn '\<trace_event_filter\>'
> > alx@devuan:~/src/linux/linux/master$ grepc trace_event_filter .
> > alx@devuan:~/src/linux/linux/master$ grep -rn trace_event_filter
> > kernel/trace/trace_events.c:2833:static const struct file_operations ftrace_event_filter_fops = {
> > kernel/trace/trace_events.c:3222: *fops = &ftrace_event_filter_fops;
> >
> > If you point me to a specific line of code, I can attempt a rewrite.
>
> I rather not touch that code.
Yeah, I don't intend to touch that code seriously; it was just to show
that it can be done, and it doesn't make it worse. If the code was
written from scratch, I'd certainly write it with strprefix(). The only
reason to keep str_has_prefix() in it is to not touch old code that
works, but not because it's more appropriate.
So, I'd like to mark str_has_prefix() as deprecated.
> > > > + *
> > > > * A common way to test a prefix of a string is to do:
> > > > * strncmp(str, prefix, sizeof(prefix) - 1)
> > > > *
> > > > * But this can lead to bugs due to typos, or if prefix is a pointer
> > > > - * and not a constant. Instead use str_has_prefix().
> > > > + * and not a constant. Instead use strprefix().
> > >
> > > The above can be replaced with:
> > >
> > > * This is useful for testing for a prefix and returning the length.
> > > * If you only need to test the prefix and perhaps get the pointer to the
> > > * position of the string after the prefix, then use strprefix().
> >
> > Ok, conditional to being convinced that str_has_prefix() is
> > actually useful in some case.
>
> Note, the only "bug" is that it can't handle an empty prefix case, which
> was never what it was designed to do. The place that the above fixes, was a
> helper function that used this inappropriately.
>
> Hence, the use of str_has_prefix() was never meant for an empty string. I
> should have added (and can still add)
>
> /* Empty prefix is undefined */
> WARN_ON_ONCE(!prefix[0]);
Why make it not work for a valid string unnecessarily? We can do better
by returning a pointer, which doesn't need to have this limitation at
all. It's the "good taste" design of
<https://www.youtube.com/watch?v=78Y17hAo96I>.
> The fwnode_name_eq() could have done:
>
> /* Empty prefix always matches */
> return name[0] || str_has_prefix(node_name, name) == len;
Yes, you can. But why would you do it, if you can avoid it?
> Instead.
>
> -- Steve
Have a lovely night!
Alex
--
<https://www.alejandro-colomar.es>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
next prev parent reply other threads:[~2026-11-05 17:46 UTC|newest]
Thread overview: 29+ messages / expand[flat|nested] mbox.gz Atom feed top
2026-11-05 11:12 [PATCH v1 0/2] Add strprefix() and fix a bug with it Alejandro Colomar
2026-11-05 11:13 ` [PATCH v1 1/2] string: Add strprefix(), and deprecate str_has_prefix() Alejandro Colomar
2026-11-05 11:27 ` Andy Shevchenko
2026-11-05 12:28 ` Alejandro Colomar
2026-11-05 16:14 ` Steven Rostedt
2026-11-05 16:33 ` Alejandro Colomar
2026-11-05 16:50 ` Steven Rostedt
2026-11-05 17:46 ` Alejandro Colomar [this message]
2026-11-05 11:13 ` [PATCH v1 2/2] device property: Fix fwnode_name_eq() for empty node names Alejandro Colomar
2026-11-05 11:24 ` [PATCH v1 0/2] Add strprefix() and fix a bug with it Andy Shevchenko
2026-11-05 11:38 ` Alejandro Colomar
2026-11-05 11:59 ` Andy Shevchenko
2026-11-05 12:30 ` Bartosz Golaszewski
2026-11-05 12:49 ` Alejandro Colomar
2026-11-05 12:46 ` [PATCH v2 " Alejandro Colomar
2026-11-05 12:46 ` [PATCH v2 1/2] string: Add strprefix(), and deprecate str_has_prefix() Alejandro Colomar
2026-11-05 12:51 ` Geert Uytterhoeven
2026-11-05 13:12 ` Alejandro Colomar
2026-11-05 12:46 ` [PATCH v2 2/2] device property: Fix fwnode_name_eq() for empty node names Alejandro Colomar
2026-11-05 14:01 ` [PATCH v2 0/2] Add strprefix() and fix a bug with it Bartosz Golaszewski
2026-11-05 14:16 ` Alejandro Colomar
2026-11-05 14:42 ` Andy Shevchenko
2026-11-05 15:06 ` Alejandro Colomar
2026-11-05 15:00 ` Bartosz Golaszewski
2026-11-05 15:14 ` Alejandro Colomar
2026-11-05 15:21 ` Krzysztof Kozlowski
2026-11-05 16:27 ` Steven Rostedt
2026-11-05 16:30 ` Krzysztof Kozlowski
2026-11-05 16:42 ` Alejandro Colomar
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=auy0O467UqOzqYub@devuan \
--to=alx+linux-hardening@kernel.org \
--cc=alx@kernel.org \
--cc=andriy.shevchenko@linux.intel.com \
--cc=andy@kernel.org \
--cc=bartosz.golaszewski@oss.qualcomm.com \
--cc=brgl@kernel.org \
--cc=dakr@kernel.org \
--cc=devicetree@vger.kernel.org \
--cc=djrscally@gmail.com \
--cc=driver-core@lists.linux.dev \
--cc=gregkh@linuxfoundation.org \
--cc=heikki.krogerus@linux.intel.com \
--cc=hverkuil-cisco@xs4all.nl \
--cc=joe@perches.com \
--cc=johan@kernel.org \
--cc=kees@kernel.org \
--cc=laurent.pinchart@ideasonboard.com \
--cc=lenb@kernel.org \
--cc=linusw@kernel.org \
--cc=linux-acpi@vger.kernel.org \
--cc=linux-gpio@vger.kernel.org \
--cc=linux-hardening@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=namhyung@kernel.org \
--cc=rafael@kernel.org \
--cc=robh@kernel.org \
--cc=rostedt@goodmis.org \
--cc=sakari.ailus@linux.intel.com \
--cc=saravanak@kernel.org \
--cc=schwab@linux-m68k.org \
--cc=torvalds@linux-foundation.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox