Hi Steve, > Date: 2026-11-05 11:50:19-0500 > From: Steven Rostedt > [...] > > > > Reported-by: Bartosz Golaszewski > > > > > > 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: > > > > 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 , 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 '\' > > 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 . > 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 --