| Message ID | 20210912160149.2227137-5-linux@roeck-us.net (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
| Series | Introduce and use absolute_pointer macro | expand |
On Sun, Sep 12, 2021 at 9:02 AM Guenter Roeck <linux@roeck-us.net> wrote: > > - if (strcmp(COMMAND_LINE, "INSTALL") == 0) { > + if (strcmp(absolute_pointer(COMMAND_LINE), "INSTALL") == 0) { Again, this feels very much like treating the symptoms, not actually fixing the real issue. It's COMMAND_LINE itself that should have been fixed up, not that one use of it. Because the only reason you didn't get a warning from later uses is that 'strcmp()' is doing compiler-specific magic. You're just delaying the inevitable warnings about the other uses of that thing. Linus
On 9/12/21 12:13 PM, Linus Torvalds wrote: > On Sun, Sep 12, 2021 at 9:02 AM Guenter Roeck <linux@roeck-us.net> wrote: >> >> - if (strcmp(COMMAND_LINE, "INSTALL") == 0) { >> + if (strcmp(absolute_pointer(COMMAND_LINE), "INSTALL") == 0) { > > Again, this feels very much like treating the symptoms, not actually > fixing the real issue. > > It's COMMAND_LINE itself that should have been fixed up, not that one use of it. > > Because the only reason you didn't get a warning from later uses is > that 'strcmp()' is doing compiler-specific magic. You're just delaying > the inevitable warnings about the other uses of that thing. > Makes sense. I'll change the patch accordingly. Thanks, Guenter
On 9/12/21 12:13 PM, Linus Torvalds wrote: > On Sun, Sep 12, 2021 at 9:02 AM Guenter Roeck <linux@roeck-us.net> wrote: >> >> - if (strcmp(COMMAND_LINE, "INSTALL") == 0) { >> + if (strcmp(absolute_pointer(COMMAND_LINE), "INSTALL") == 0) { > > Again, this feels very much like treating the symptoms, not actually > fixing the real issue. > > It's COMMAND_LINE itself that should have been fixed up, not that one use of it. > > Because the only reason you didn't get a warning from later uses is > that 'strcmp()' is doing compiler-specific magic. You're just delaying > the inevitable warnings about the other uses of that thing. > COMMAND_LINE is, for whatever reason, defined in arch/alpha/include/uapi/asm/setup.h, ie in the uapi. Can I even touch that ? Guenter
On Sun, Sep 12, 2021 at 1:37 PM Guenter Roeck <linux@roeck-us.net> wrote: > > COMMAND_LINE is, for whatever reason, defined in > arch/alpha/include/uapi/asm/setup.h, ie in the uapi. > > Can I even touch that ? I think that's entirely a historical accident. Note how most of those #define's don't even work for user space because they use PAGE_OFFSET, which is defined in <asm/page.h>. And others depend on the kernel config system. There's a couple that do seem to be potentially for user space (MILO bootloader), and who knows just what hacks that code might have with internal knowledge of this header file. But anything I can find on the net seems to predate our move to 'uapi' headers, so I wouldn't really worry about it. So I'd suggest just moving that whole file back to <asm/setup.h>, changing it as necessary, and then seeing if anybody notices. Because I suspect the answer to that is just crickets chirping.. Linus
diff --git a/arch/alpha/kernel/setup.c b/arch/alpha/kernel/setup.c index b4fbbba30aa2..aab477a76c30 100644 --- a/arch/alpha/kernel/setup.c +++ b/arch/alpha/kernel/setup.c @@ -490,7 +490,7 @@ setup_arch(char **cmdline_p) /* Hack for Jensen... since we're restricted to 8 or 16 chars for boot flags depending on the boot mode, we need some shorthand. This should do for installation. */ - if (strcmp(COMMAND_LINE, "INSTALL") == 0) { + if (strcmp(absolute_pointer(COMMAND_LINE), "INSTALL") == 0) { strlcpy(command_line, "root=/dev/fd0 load_ramdisk=1", sizeof command_line); } else { strlcpy(command_line, COMMAND_LINE, sizeof command_line);
alpha:allmodconfig fails to build with the following error when using gcc 11.x. arch/alpha/kernel/setup.c: In function 'setup_arch': arch/alpha/kernel/setup.c:493:13: error: 'strcmp' reading 1 or more bytes from a region of size 0 Avoid the problem by using absolute_pointer() when providing a memory address to strcmp(). Signed-off-by: Guenter Roeck <linux@roeck-us.net> --- arch/alpha/kernel/setup.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)