On 02/04/2019 01.55, Sultan Alsawaf wrote: > On Mon, Apr 01, 2019 at 10:43:13PM +0200, Rasmus Villemoes wrote:
>> No. First, these are concerns for all arches. Second, if you can find >> some particular place where string parsing/matching is in any way >> performance relevant and not just done once during driver init or >> whatnot, maybe the maintainers of that file would take a patch >> hand-optimizing some strcmps to memcmps, or, depending on what the code >> does, perhaps replacing the whole *cmp logic with a custom hash table. > > FWIW, I didn't have a specific place in the kernel in mind that heavily relied > on str* operations and needed optimization. I just thought it could be a > "free" > optimization, but apparently not. Well, it wouldn't be completely free; at the very least, .text size would usually increase, and for the majority of sites that are execute-at-most-once-per-full-moon that's actually a small overall pessimization. But the point is moot, I think. >> But a patch implicitly and silently touching thousands of lines of code, >> without an analysis of why none of the above is a problem for any of >> those lines, for any .config, arch, compiler version? No. > > Well, I thought it could be proven to be correct regardless of the context, > which would imply that making such a global change touching thousands lof > lines > of code would be safe. But sadly it isn't. > > This does bring into question a greater problem though: usage of memcmp > throughout the kernel where the size provided is not the size of the smaller > container being compared. This usage of memcmp (which is quite easy to find > all > across the kernel) could run into the unaligned memory access across a page > boundary that you gave as an example, no? Yes, as I said, I think you may have identified some actual bugs. In some cases, the runtime string is known to live in an array of sufficient size, so we would at most be reading uninitialized bytes (but not making decisions based on them) - that could cause a sanitizer warning, but otherwise harmless (stuff like this is a rather common source of false positives with valgrind). In other cases, there's no such guarantee. It really needs to be decided on a case-by-case basis. And if in doubt, I'd change the code to use strcmp(), strncmp(), strstarts() as appropriate - that's _much_ more readable, and avoids having to worry, and would usually avoid duplicating the literal string (which is error-prone when the code gets copy-pasted to handle a new option). Rasmus