Recently, some compile-time checking I added to the clamp_t family of
functions triggered a build error when a poorly written driver was
compiled on ARM, because the driver assumed that the naked `char` type
is signed, but ARM treats it as unsigned, and the C standard says it's
architecture-dependent.
I doubt this particular driver is the only instance in which
unsuspecting authors assume that `char` with no `signed` or `unsigned`
designation is signed, because that's how the other types work. We were
lucky enough this time that that driver used `clamp_t(char,
negative_value, positive_value)`, so the new checking code found it, and
I've sent a patch to fix it, but there are likely other places lurking
that won't be so easily unearthed.
So let's just eliminate this particular variety of heisensigned bugs
entirely. Set `-fsigned-char` globally, so that gcc makes the type
signed on all architectures.
Cc: Masahiro Yamada <[email protected]>
Cc: Kees Cook <[email protected]>
Cc: Andrew Morton <[email protected]>
Cc: Linus Torvalds <[email protected]>
Cc: Andy Shevchenko <[email protected]>
Cc: Greg Kroah-Hartman <[email protected]>
Link: https://lore.kernel.org/lkml/[email protected]/
Signed-off-by: Jason A. Donenfeld <[email protected]>
---
Makefile | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/Makefile b/Makefile
index f41ec8c8426b..f1abcaf7110e 100644
--- a/Makefile
+++ b/Makefile
@@ -562,7 +562,7 @@ KBUILD_AFLAGS := -D__ASSEMBLY__ -fno-PIE
KBUILD_CFLAGS := -Wall -Wundef -Werror=strict-prototypes -Wno-trigraphs \
-fno-strict-aliasing -fno-common -fshort-wchar -fno-PIE \
-Werror=implicit-function-declaration -Werror=implicit-int \
- -Werror=return-type -Wno-format-security \
+ -Werror=return-type -Wno-format-security -fsigned-char \
-std=gnu11
KBUILD_CPPFLAGS := -D__KERNEL__
KBUILD_RUSTFLAGS := $(rust_common_flags) \
--
2.38.1
On Wed, Oct 19, 2022 at 9:27 AM Jason A. Donenfeld <[email protected]> wrote:
>
> So let's just eliminate this particular variety of heisensigned bugs
> entirely. Set `-fsigned-char` globally, so that gcc makes the type
> signed on all architectures.
Btw, I do wonder if we might actually be better off doing this - but
doing it the other way around.
IOW, make 'char' always UNsigned. Unlike the signed char thing, it
shouldn't generate any worse code on any common architecture.
And I do think that having odd architecture differences is generally a
bad idea, and making the language rules stricter to avoid differences
is a good thing.
Now, you did '-fsigned-char', because that's the "common default" in
an x86-centric world.
You are also right that people might think that "char" works like
"int", and that if you don't specify the sign, it's signed.
But those people are obviously wrong anyway, so it's not a very strong argument.
And from a kernel perspective, I do think that "treat char as a byte"
and making it be unsigned is in many ways the saner model. There's a
reason we use 'unsigned char' in a fair number of places.
So using '-funsigned-char' might not be a bad idea.
Hilariously (and by "hilariously", I obviously mean "NOT
hilariously"), it doesn't actually fix the warning for
const unsigned char *c = "p";
which still complains about
warning: pointer targets in initialization of ‘const unsigned char
*’ from ‘char *’ differ in signedness
even when you've specified that 'char' should be unsigned with -funsigned-char.
Because gcc actually tries to be helpful, and has (reasonably, from a
"type sanity" standpoint) decided that
"The type char is always a distinct type from each of signed char
or unsigned char, even though its behavior is always just like one of
those two"
so using "-funsigned-char" gives us well-defined *behavior*, but
doesn't really help us with cleaning up our code.
I understand why gcc would want to make it clear that despite any
behavioral issues, "char" is *not* the same as "[un]signed char" in
general. But in this kind of use case, that warning is just pointless
and annoying.
Oh well. You *really* can't win this thing. The game is rigged like
some geeky carnival game.
Linus
Hi Linus,
On Wed, Oct 19, 2022 at 12:54:06PM -0700, Linus Torvalds wrote:
> On Wed, Oct 19, 2022 at 9:27 AM Jason A. Donenfeld <[email protected]> wrote:
> >
> > So let's just eliminate this particular variety of heisensigned bugs
> > entirely. Set `-fsigned-char` globally, so that gcc makes the type
> > signed on all architectures.
>
> Btw, I do wonder if we might actually be better off doing this - but
> doing it the other way around.
That could work. The argument here would be that most people indeed
treat char as a byte. I'll send a v2 doing this.
This will probably break some things, though, on drivers that are
already broken on e.g. ARM. For example, the wifi driver I fixed that
started this whole thing would now be broken on x86 too. But also, we're
barely past rc1, so maybe this is something to do now, and then we'll
spend the rest of the 6.1 cycle fixing issues as the pop up? Sounds fine
to me if you think that's doable.
Either way, I'll send a patch and you can do with it what you think is
best.
Jason
Recently, some compile-time checking I added to the clamp_t family of
functions triggered a build error when a poorly written driver was
compiled on ARM, because the driver assumed that the naked `char` type
is signed, but ARM treats it as unsigned, and the C standard says it's
architecture-dependent.
I doubt this particular driver is the only instance in which
unsuspecting authors make assumptions about `char` with no `signed` or
`unsigned` specifier. We were lucky enough this time that that driver
used `clamp_t(char, negative_value, positive_value)`, so the new
checking code found it, and I've sent a patch to fix it, but there are
likely other places lurking that won't be so easily unearthed.
So let's just eliminate this particular variety of heisensign bugs
entirely. Set `-funsigned-char` globally, so that gcc makes the type
unsigned on all architectures.
This will break things in some places and fix things in others, so this
will likely cause a bit of churn while reconciling the type misuse.
Cc: Masahiro Yamada <[email protected]>
Cc: Kees Cook <[email protected]>
Cc: Andrew Morton <[email protected]>
Cc: Linus Torvalds <[email protected]>
Cc: Andy Shevchenko <[email protected]>
Cc: Greg Kroah-Hartman <[email protected]>
Link: https://lore.kernel.org/lkml/[email protected]/
Signed-off-by: Jason A. Donenfeld <[email protected]>
---
Makefile | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/Makefile b/Makefile
index f41ec8c8426b..bbf376931899 100644
--- a/Makefile
+++ b/Makefile
@@ -562,7 +562,7 @@ KBUILD_AFLAGS := -D__ASSEMBLY__ -fno-PIE
KBUILD_CFLAGS := -Wall -Wundef -Werror=strict-prototypes -Wno-trigraphs \
-fno-strict-aliasing -fno-common -fshort-wchar -fno-PIE \
-Werror=implicit-function-declaration -Werror=implicit-int \
- -Werror=return-type -Wno-format-security \
+ -Werror=return-type -Wno-format-security -funsigned-char \
-std=gnu11
KBUILD_CPPFLAGS := -D__KERNEL__
KBUILD_RUSTFLAGS := $(rust_common_flags) \
--
2.38.1
From: Linus Torvalds
> Sent: 19 October 2022 20:54
>
> On Wed, Oct 19, 2022 at 9:27 AM Jason A. Donenfeld <[email protected]> wrote:
> >
> > So let's just eliminate this particular variety of heisensigned bugs
> > entirely. Set `-fsigned-char` globally, so that gcc makes the type
> > signed on all architectures.
>
> Btw, I do wonder if we might actually be better off doing this - but
> doing it the other way around.
>
> IOW, make 'char' always UNsigned. Unlike the signed char thing, it
> shouldn't generate any worse code on any common architecture.
>
> And I do think that having odd architecture differences is generally a
> bad idea, and making the language rules stricter to avoid differences
> is a good thing.
>
> Now, you did '-fsigned-char', because that's the "common default" in
> an x86-centric world.
I'm pretty sure char is signed because the pdp11 only had
sign-extending byte loads.
> You are also right that people might think that "char" works like
> "int", and that if you don't specify the sign, it's signed.
But even 'unsigned char' works like int.
The values are promoted to int (thanks to the brain-dead ANSI-C
committee) rather than unsigned int (which I think was in K&R C).
(There is an exception, int, short and char can all be the same size.
In which case unsigned char promotes to unsigned int.)
David
-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)
On Wed, Oct 19, 2022 at 09:07:01PM +0000, David Laight wrote:
> From: Linus Torvalds
> > Sent: 19 October 2022 19:11
> > Explicit casts are bad (unless, of course, you are explicitly trying
> > to violate the type system, when they are both required, and a great
> > way to say "look, I'm doing something dangerous").
> Casts really ought to be rare.
Sometimes you need casts for *data*, like where you write (u32)smth
because you really want the low 32 bits of that something. That only
happens in some kinds of code -- multi-precision integer, some crypto,
serialisation primitives.
You often want casts for varargs, too. The alternative is to make very
certain some other way that the actual arguments will have the correct
type, but that is often awkward to do, and not as clear to read.
Pointer casts are almost always a mistake. If you think you want one
you are almost always wrong.
Segher
On Wed, Oct 19, 2022 at 04:56:03PM -0700, Linus Torvalds wrote:
> On Wed, Oct 19, 2022 at 1:30 PM Jason A. Donenfeld <[email protected]> wrote:
> >
> > So let's just eliminate this particular variety of heisensign bugs
> > entirely. Set `-funsigned-char` globally, so that gcc makes the type
> > unsigned on all architectures.
> >
> > This will break things in some places and fix things in others, so this
> > will likely cause a bit of churn while reconciling the type misuse.
>
> Yeah, if we were still in the merge window, I'd probably apply this,
> but as things stand, I think it should go into linux-next and cook
> there for the next merge window.
>
> Anybody willing to put this in their -next trees?
Sure, happy to take it.
>
> Any breakage it causes is likely going to be fairly subtle, and in
> some random driver that isn't used on architectures that already have
> an unsigned 'char' type.
>
> I think the architectures with an unsigned 'char' are arm, powerpc and
> s390, in all their variations (ie both 32- and 64-bit).
>
> So all *core* code should be fine with this, but that still leaves a
> lot of drivers that have likely never been tested on anything but x86,
> and could just stop working.
>
> I don't think breakage is very *likely*, but I suspect it exists.
Given I've started with cleaning up one driver already, I'll keep my eye
on further breakage.
Jason
>
> Linus
On Wed, Oct 19, 2022 at 1:30 PM Jason A. Donenfeld <[email protected]> wrote:
>
> So let's just eliminate this particular variety of heisensign bugs
> entirely. Set `-funsigned-char` globally, so that gcc makes the type
> unsigned on all architectures.
>
> This will break things in some places and fix things in others, so this
> will likely cause a bit of churn while reconciling the type misuse.
Yeah, if we were still in the merge window, I'd probably apply this,
but as things stand, I think it should go into linux-next and cook
there for the next merge window.
Anybody willing to put this in their -next trees?
Any breakage it causes is likely going to be fairly subtle, and in
some random driver that isn't used on architectures that already have
an unsigned 'char' type.
I think the architectures with an unsigned 'char' are arm, powerpc and
s390, in all their variations (ie both 32- and 64-bit).
So all *core* code should be fine with this, but that still leaves a
lot of drivers that have likely never been tested on anything but x86,
and could just stop working.
I don't think breakage is very *likely*, but I suspect it exists.
Linus
On Wed, Oct 19, 2022 at 5:02 PM Jason A. Donenfeld <[email protected]> wrote:
>
> Given I've started with cleaning up one driver already, I'll keep my eye
> on further breakage.
I wonder if we could just check for code generation differences some way.
I tested a couple of files, and was able to find differences, eg
# kernel/sched/core.c:8861: pr_info("task:%-15.15s state:%c",
p->comm, task_state_to_char(p));
- movzbl state_char.149(%rax), %edx # state_char[_60], state_char[_60]
+ movsbl state_char.149(%rax), %edx # state_char[_60], state_char[_60]
call _printk #
because the 'char' for the '%c' is passed as an integer. And the
tracing code has the
.is_signed = is_signed_type(_type)
initializers, which obviously change when the type is 'char'.
But I also checked a number of other files that didn't have that
pattern at all, and there was zero code generation difference, even
when the "readable asm" output itself had some changes in some of the
internal label names.
That was what my old 'sparse' trial thing was actually *hoping* (but
failed) to do, ie notice when the signedness of a char actually
affects code generation. And it does in fact seem fairly rare.
Having some scripting automation that just notices "this changes code
generation in function X" might actually be interesting, and judging
by my quick tests might not be *too* verbose.
Linus
On Wed, Oct 19, 2022 at 05:38:55PM -0700, Linus Torvalds wrote:
> On Wed, Oct 19, 2022 at 5:02 PM Jason A. Donenfeld <[email protected]> wrote:
> >
> > Given I've started with cleaning up one driver already, I'll keep my eye
> > on further breakage.
>
> I wonder if we could just check for code generation differences some way.
> Having some scripting automation that just notices "this changes code
> generation in function X" might actually be interesting, and judging
> by my quick tests might not be *too* verbose.
Or even just some allyesconfig diffing.
> I tested a couple of files, and was able to find differences, eg
>
> # kernel/sched/core.c:8861: pr_info("task:%-15.15s state:%c",
> p->comm, task_state_to_char(p));
> - movzbl state_char.149(%rax), %edx # state_char[_60], state_char[_60]
> + movsbl state_char.149(%rax), %edx # state_char[_60], state_char[_60]
> call _printk #
>
> because the 'char' for the '%c' is passed as an integer. And the
Seems harmless though.
> tracing code has the
>
> .is_signed = is_signed_type(_type)
>
> initializers, which obviously change when the type is 'char'.
And likewise, looking at the types of initializers that's used with.
Actually, for the array one, unsigned is probably more sensible anyway.
The thing is, anyhow, that most code that works without -funsigned-char
*will* work with it, because the core of the kernel obviously works fine
on ARM already. The problematic areas will be x86-specific drivers that
have never been tested on other archs. i915 comes to mind -- as a
general rule, it already does all manner of insane things. But there's
obviously a lot of other hardware that's only ever run on Intel. So I'm
much more concerned about that than I am about code in, say, kernel/sched.
Jason
Hi Jason,
I love your patch! Perhaps something to improve:
[auto build test WARNING on masahiroy-kbuild/for-next]
[also build test WARNING on linus/master v6.1-rc1]
[cannot apply to next-20221020]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Jason-A-Donenfeld/kbuild-treat-char-as-always-signed/20221020-113408
base: https://git.kernel.org/pub/scm/linux/kernel/git/masahiroy/linux-kbuild.git for-next
patch link: https://lore.kernel.org/r/20221019162648.3557490-1-Jason%40zx2c4.com
patch subject: [PATCH] kbuild: treat char as always signed
config: s390-allmodconfig
compiler: s390-linux-gcc (GCC) 12.1.0
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/intel-lab-lkp/linux/commit/574c38568c2c4a092070a42616218871cf6b7ff9
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Jason-A-Donenfeld/kbuild-treat-char-as-always-signed/20221020-113408
git checkout 574c38568c2c4a092070a42616218871cf6b7ff9
# save the config file
mkdir build_dir && cp config build_dir/.config
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=s390 SHELL=/bin/bash drivers/s390/block/ drivers/s390/char/
If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <[email protected]>
All warnings (new ones prefixed by >>):
drivers/s390/block/dasd.c: In function '__dasd_process_cqr':
>> drivers/s390/block/dasd.c:1912:9: warning: case label value exceeds maximum value for type [-Wswitch-outside-range]
1912 | case DASD_CQR_ERROR:
| ^~~~
drivers/s390/block/dasd.c:1915:9: warning: case label value exceeds maximum value for type [-Wswitch-outside-range]
1915 | case DASD_CQR_CLEARED:
| ^~~~
drivers/s390/block/dasd.c:1909:9: warning: case label value exceeds maximum value for type [-Wswitch-outside-range]
1909 | case DASD_CQR_SUCCESS:
| ^~~~
drivers/s390/block/dasd.c: In function 'dasd_flush_device_queue':
drivers/s390/block/dasd.c:2113:17: warning: case label value exceeds maximum value for type [-Wswitch-outside-range]
2113 | case DASD_CQR_QUEUED:
| ^~~~
drivers/s390/block/dasd.c:2102:17: warning: case label value exceeds maximum value for type [-Wswitch-outside-range]
2102 | case DASD_CQR_IN_IO:
| ^~~~
drivers/s390/block/dasd.c: In function '__dasd_cancel_req':
drivers/s390/block/dasd.c:2615:9: warning: case label value exceeds maximum value for type [-Wswitch-outside-range]
2615 | case DASD_CQR_QUEUED:
| ^~~~
drivers/s390/block/dasd.c:2619:9: warning: case label value exceeds maximum value for type [-Wswitch-outside-range]
2619 | case DASD_CQR_IN_IO:
| ^~~~
--
drivers/s390/block/dasd_3990_erp.c: In function 'dasd_3990_handle_env_data':
>> drivers/s390/block/dasd_3990_erp.c:861:9: warning: case label value exceeds maximum value for type [-Wswitch-outside-range]
861 | case 0x80: /* Format 8 - Additional Device Equipment Checks */
| ^~~~
drivers/s390/block/dasd_3990_erp.c:914:9: warning: case label value exceeds maximum value for type [-Wswitch-outside-range]
914 | case 0x90: /* Format 9 - Device Read, Write, and Seek Checks */
| ^~~~
drivers/s390/block/dasd_3990_erp.c:943:9: warning: case label value exceeds maximum value for type [-Wswitch-outside-range]
943 | case 0xF0: /* Format F - Cache Storage Checks */
| ^~~~
--
drivers/s390/char/sclp_vt220.c: In function 'sclp_vt220_receiver_fn':
>> drivers/s390/char/sclp_vt220.c:536:9: warning: case label value exceeds maximum value for type [-Wswitch-outside-range]
536 | case SCLP_VT220_SESSION_STARTED:
| ^~~~
vim +1912 drivers/s390/block/dasd.c
^1da177e4c3f41 Linus Torvalds 2005-04-16 1902
5c618c0cf451f1 Sebastian Ott 2018-05-24 1903 static void __dasd_process_cqr(struct dasd_device *device,
5c618c0cf451f1 Sebastian Ott 2018-05-24 1904 struct dasd_ccw_req *cqr)
^1da177e4c3f41 Linus Torvalds 2005-04-16 1905 {
fc19f381b3828a Stefan Haberland 2009-03-26 1906 char errorstring[ERRORLENGTH];
f24acd4503270e Horst Hummel 2005-05-01 1907
8e09f21574ea30 Stefan Weinhuber 2008-01-26 1908 switch (cqr->status) {
8e09f21574ea30 Stefan Weinhuber 2008-01-26 1909 case DASD_CQR_SUCCESS:
8e09f21574ea30 Stefan Weinhuber 2008-01-26 1910 cqr->status = DASD_CQR_DONE;
d54853ef8cb172 Martin Schwidefsky 2007-02-05 1911 break;
8e09f21574ea30 Stefan Weinhuber 2008-01-26 @1912 case DASD_CQR_ERROR:
8e09f21574ea30 Stefan Weinhuber 2008-01-26 1913 cqr->status = DASD_CQR_NEED_ERP;
d54853ef8cb172 Martin Schwidefsky 2007-02-05 1914 break;
8e09f21574ea30 Stefan Weinhuber 2008-01-26 1915 case DASD_CQR_CLEARED:
8e09f21574ea30 Stefan Weinhuber 2008-01-26 1916 cqr->status = DASD_CQR_TERMINATED;
8e09f21574ea30 Stefan Weinhuber 2008-01-26 1917 break;
8e09f21574ea30 Stefan Weinhuber 2008-01-26 1918 default:
fc19f381b3828a Stefan Haberland 2009-03-26 1919 /* internal error 12 - wrong cqr status*/
fc19f381b3828a Stefan Haberland 2009-03-26 1920 snprintf(errorstring, ERRORLENGTH, "12 %p %x02", cqr, cqr->status);
fc19f381b3828a Stefan Haberland 2009-03-26 1921 dev_err(&device->cdev->dev,
fc19f381b3828a Stefan Haberland 2009-03-26 1922 "An error occurred in the DASD device driver, "
fc19f381b3828a Stefan Haberland 2009-03-26 1923 "reason=%s\n", errorstring);
8e09f21574ea30 Stefan Weinhuber 2008-01-26 1924 BUG();
d54853ef8cb172 Martin Schwidefsky 2007-02-05 1925 }
5c618c0cf451f1 Sebastian Ott 2018-05-24 1926 if (cqr->callback)
5c618c0cf451f1 Sebastian Ott 2018-05-24 1927 cqr->callback(cqr, cqr->callback_data);
5c618c0cf451f1 Sebastian Ott 2018-05-24 1928 }
5c618c0cf451f1 Sebastian Ott 2018-05-24 1929
--
0-DAY CI Kernel Test Service
https://01.org/lkp
On Thu, Oct 20, 2022 at 2:40 AM kernel test robot <[email protected]> wrote:
> >> drivers/s390/block/dasd.c:1912:9: warning: case label value exceeds maximum value for type [-Wswitch-outside-range]
> 1912 | case DASD_CQR_ERROR:
Just to save other readers the momentary "huh?" that I experienced,
this warning/error is from the -fsigned-char patch. We ultimately went
with (or are trying to go with) the -funsigned-char approach instead.
So safely ignore this kernel test bot error, as it applies to v1
rather than the v2 here:
https://lore.kernel.org/lkml/[email protected]/
Jason
On Wed, Oct 19, 2022 at 05:38:55PM -0700, Linus Torvalds wrote:
> Having some scripting automation that just notices "this changes code
> generation in function X" might actually be interesting, and judging
> by my quick tests might not be *too* verbose.
On the reproducible build comparison system[1] we use for checking a lot
of the KSPP work for .text deltas, an allmodconfig finds a fair bit for
this change. Out of 33900 .o files, 1005 have changes.
Spot checking matches a lot of what you found already...
u64 flags = how->flags;
...
fs/open.c:1123:
int acc_mode = ACC_MODE(flags);
- 1c86: movsbl 0x0(%rdx),%edx
+ 1c86: movzbl 0x0(%rdx),%edx
#define ACC_MODE(x) ("\004\002\006\006"[(x)&O_ACCMODE])
Ignoring those, it goes down to 625, and spot checking those is more
difficult, but looks to be mostly register selection changes dominating
the delta. The resulting vmlinux sizes are identical, though.
-Kees
[1] A fancier version of:
https://outflux.net/blog/archives/2022/06/24/finding-binary-differences/
--
Kees Cook
On Wed, Oct 19, 2022 at 04:56:03PM -0700, Linus Torvalds wrote:
> I think the architectures with an unsigned 'char' are arm, powerpc and
> s390, in all their variations (ie both 32- and 64-bit).
xtensa and most MIPS configurations as well, fwiw.
Segher
On Thu, Oct 20, 2022 at 11:41:29AM -0700, Kees Cook wrote:
> On Wed, Oct 19, 2022 at 05:38:55PM -0700, Linus Torvalds wrote:
> > Having some scripting automation that just notices "this changes code
> > generation in function X" might actually be interesting, and judging
> > by my quick tests might not be *too* verbose.
>
> On the reproducible build comparison system[1] we use for checking a lot
> of the KSPP work for .text deltas, an allmodconfig finds a fair bit for
> this change. Out of 33900 .o files, 1005 have changes.
>
> Spot checking matches a lot of what you found already...
>
> u64 flags = how->flags;
> ...
> fs/open.c:1123:
> int acc_mode = ACC_MODE(flags);
> - 1c86: movsbl 0x0(%rdx),%edx
> + 1c86: movzbl 0x0(%rdx),%edx
>
> #define ACC_MODE(x) ("\004\002\006\006"[(x)&O_ACCMODE])
>
> Ignoring those, it goes down to 625, and spot checking those is more
> difficult, but looks to be mostly register selection changes dominating
> the delta. The resulting vmlinux sizes are identical, though.
>
> -Kees
>
> [1] A fancier version of:
> https://outflux.net/blog/archives/2022/06/24/finding-binary-differences/
Say, don't we have some way of outputting LLVM IL? I saw some
-fno-discard-value-names floating through a few days ago. Apparently you
can do `make LLVM=1 fs/select.ll`? This might have less noise in it.
I'll play on the airplane tomorrow.
Jason
On Wed, Oct 19, 2022 at 02:30:34PM -0600, Jason A. Donenfeld wrote:
> Recently, some compile-time checking I added to the clamp_t family of
> functions triggered a build error when a poorly written driver was
> compiled on ARM, because the driver assumed that the naked `char` type
> is signed, but ARM treats it as unsigned, and the C standard says it's
> architecture-dependent.
>
> I doubt this particular driver is the only instance in which
> unsuspecting authors make assumptions about `char` with no `signed` or
> `unsigned` specifier. We were lucky enough this time that that driver
> used `clamp_t(char, negative_value, positive_value)`, so the new
> checking code found it, and I've sent a patch to fix it, but there are
> likely other places lurking that won't be so easily unearthed.
>
> So let's just eliminate this particular variety of heisensign bugs
> entirely. Set `-funsigned-char` globally, so that gcc makes the type
> unsigned on all architectures.
>
> This will break things in some places and fix things in others, so this
> will likely cause a bit of churn while reconciling the type misuse.
>
This is a very daring change and obviously is going to introduce bugs.
It might be better to create a static checker rule that says "char"
without explicit signedness can only be used for strings.
arch/parisc/kernel/drivers.c:337 print_hwpath() warn: impossible condition '(path->bc[i] == -1) => (0-255 == (-1))'
arch/parisc/kernel/drivers.c:410 setup_bus_id() warn: impossible condition '(path.bc[i] == -1) => (0-255 == (-1))'
arch/parisc/kernel/drivers.c:486 create_parisc_device() warn: impossible condition '(modpath->bc[i] == -1) => (0-255 == (-1))'
arch/parisc/kernel/drivers.c:759 hwpath_to_device() warn: impossible condition '(modpath->bc[i] == -1) => (0-255 == (-1))'
drivers/media/dvb-frontends/stv0288.c:471 stv0288_set_frontend() warn: assigning (-9) to unsigned variable 'tm'
drivers/media/dvb-frontends/stv0288.c:471 stv0288_set_frontend() warn: we never enter this loop
drivers/misc/sgi-gru/grumain.c:711 gru_check_chiplet_assignment() warn: 'gts->ts_user_chiplet_id' is unsigned
drivers/net/wireless/cisco/airo.c:5316 proc_wepkey_on_close() warn: assigning (-16) to unsigned variable 'key[i / 3]'
drivers/net/wireless/ralink/rt2x00/rt2800lib.c:9415 rt2800_iq_search() warn: assigning (-32) to unsigned variable 'idx0'
drivers/net/wireless/ralink/rt2x00/rt2800lib.c:9470 rt2800_iq_search() warn: assigning (-32) to unsigned variable 'perr'
drivers/video/fbdev/sis/init301.c:3549 SiS_GetCRT2Data301() warn: 'SiS_Pr->SiS_EModeIDTable[ModeIdIndex]->ROMMODEIDX661' is unsigned
sound/pci/au88x0/au88x0_core.c:2029 vortex_adb_checkinout() warn: signedness bug returning '(-22)'
sound/pci/au88x0/au88x0_core.c:2046 vortex_adb_checkinout() warn: signedness bug returning '(-12)'
sound/pci/au88x0/au88x0_core.c:2125 vortex_adb_allocroute() warn: 'vortex_adb_checkinout(vortex, (0), en, 0)' is unsigned
sound/pci/au88x0/au88x0_core.c:2170 vortex_adb_allocroute() warn: 'vortex_adb_checkinout(vortex, stream->resources, en, 4)' is unsigned
sound/pci/rme9652/hdsp.c:3953 hdsp_channel_buffer_location() warn: 'hdsp->channel_map[channel]' is unsigned
sound/pci/rme9652/rme9652.c:1833 rme9652_channel_buffer_location() warn: 'rme9652->channel_map[channel]' is unsigned
I did not know that ARM had unsigned chars. I only knew about PPC and
on that arch they use char aggressively so that no one forgets that char
is unsigned. Changing char to signed would have made people very
annoyed. :P
regards,
dan carpenter
On Mon, Oct 24, 2022 at 12:24:24PM +0300, Dan Carpenter wrote:
> On Wed, Oct 19, 2022 at 02:30:34PM -0600, Jason A. Donenfeld wrote:
> > Recently, some compile-time checking I added to the clamp_t family of
> > functions triggered a build error when a poorly written driver was
> > compiled on ARM, because the driver assumed that the naked `char` type
> > is signed, but ARM treats it as unsigned, and the C standard says it's
> > architecture-dependent.
> >
> > I doubt this particular driver is the only instance in which
> > unsuspecting authors make assumptions about `char` with no `signed` or
> > `unsigned` specifier. We were lucky enough this time that that driver
> > used `clamp_t(char, negative_value, positive_value)`, so the new
> > checking code found it, and I've sent a patch to fix it, but there are
> > likely other places lurking that won't be so easily unearthed.
> >
> > So let's just eliminate this particular variety of heisensign bugs
> > entirely. Set `-funsigned-char` globally, so that gcc makes the type
> > unsigned on all architectures.
> >
> > This will break things in some places and fix things in others, so this
> > will likely cause a bit of churn while reconciling the type misuse.
> >
>
> This is a very daring change and obviously is going to introduce bugs.
> It might be better to create a static checker rule that says "char"
> without explicit signedness can only be used for strings.
>
> arch/parisc/kernel/drivers.c:337 print_hwpath() warn: impossible condition '(path->bc[i] == -1) => (0-255 == (-1))'
> arch/parisc/kernel/drivers.c:410 setup_bus_id() warn: impossible condition '(path.bc[i] == -1) => (0-255 == (-1))'
> arch/parisc/kernel/drivers.c:486 create_parisc_device() warn: impossible condition '(modpath->bc[i] == -1) => (0-255 == (-1))'
> arch/parisc/kernel/drivers.c:759 hwpath_to_device() warn: impossible condition '(modpath->bc[i] == -1) => (0-255 == (-1))'
> drivers/media/dvb-frontends/stv0288.c:471 stv0288_set_frontend() warn: assigning (-9) to unsigned variable 'tm'
> drivers/media/dvb-frontends/stv0288.c:471 stv0288_set_frontend() warn: we never enter this loop
> drivers/misc/sgi-gru/grumain.c:711 gru_check_chiplet_assignment() warn: 'gts->ts_user_chiplet_id' is unsigned
> drivers/net/wireless/cisco/airo.c:5316 proc_wepkey_on_close() warn: assigning (-16) to unsigned variable 'key[i / 3]'
> drivers/net/wireless/ralink/rt2x00/rt2800lib.c:9415 rt2800_iq_search() warn: assigning (-32) to unsigned variable 'idx0'
> drivers/net/wireless/ralink/rt2x00/rt2800lib.c:9470 rt2800_iq_search() warn: assigning (-32) to unsigned variable 'perr'
> drivers/video/fbdev/sis/init301.c:3549 SiS_GetCRT2Data301() warn: 'SiS_Pr->SiS_EModeIDTable[ModeIdIndex]->ROMMODEIDX661' is unsigned
> sound/pci/au88x0/au88x0_core.c:2029 vortex_adb_checkinout() warn: signedness bug returning '(-22)'
> sound/pci/au88x0/au88x0_core.c:2046 vortex_adb_checkinout() warn: signedness bug returning '(-12)'
> sound/pci/au88x0/au88x0_core.c:2125 vortex_adb_allocroute() warn: 'vortex_adb_checkinout(vortex, (0), en, 0)' is unsigned
> sound/pci/au88x0/au88x0_core.c:2170 vortex_adb_allocroute() warn: 'vortex_adb_checkinout(vortex, stream->resources, en, 4)' is unsigned
> sound/pci/rme9652/hdsp.c:3953 hdsp_channel_buffer_location() warn: 'hdsp->channel_map[channel]' is unsigned
> sound/pci/rme9652/rme9652.c:1833 rme9652_channel_buffer_location() warn: 'rme9652->channel_map[channel]' is unsigned
Here are some more:
drivers/net/wireless/ralink/rt2x00/rt2800lib.c:9472 rt2800_iq_search() warn: impossible condition '(gerr < -7) => (0-255 < (-7))'
drivers/net/wireless/ralink/rt2x00/rt2800lib.c:9476 rt2800_iq_search() warn: impossible condition '(perr < -31) => (0-255 < (-31))'
drivers/staging/rtl8192e/rtllib_softmac_wx.c:459 rtllib_wx_set_essid() warn: impossible condition '(extra[i] < 0) => (0-255 < 0)'
sound/pci/rme9652/hdsp.c:4153 snd_hdsp_channel_info() warn: impossible condition '(hdsp->channel_map[channel] < 0) => (0-255 < 0)'
This might be interesting for backports if everyone starts to rely on
the fact that char is unsigned as the PPC people currently do.
regards,
dan carpenter
On Mon, Oct 24, 2022 at 12:24:24PM +0300, Dan Carpenter wrote:
> On Wed, Oct 19, 2022 at 02:30:34PM -0600, Jason A. Donenfeld wrote:
> > Recently, some compile-time checking I added to the clamp_t family of
> > functions triggered a build error when a poorly written driver was
> > compiled on ARM, because the driver assumed that the naked `char` type
> > is signed, but ARM treats it as unsigned, and the C standard says it's
> > architecture-dependent.
> >
> > I doubt this particular driver is the only instance in which
> > unsuspecting authors make assumptions about `char` with no `signed` or
> > `unsigned` specifier. We were lucky enough this time that that driver
> > used `clamp_t(char, negative_value, positive_value)`, so the new
> > checking code found it, and I've sent a patch to fix it, but there are
> > likely other places lurking that won't be so easily unearthed.
> >
> > So let's just eliminate this particular variety of heisensign bugs
> > entirely. Set `-funsigned-char` globally, so that gcc makes the type
> > unsigned on all architectures.
> >
> > This will break things in some places and fix things in others, so this
> > will likely cause a bit of churn while reconciling the type misuse.
> >
>
> This is a very daring change and obviously is going to introduce bugs.
> It might be better to create a static checker rule that says "char"
> without explicit signedness can only be used for strings.
Indeed this would be great.
>
> arch/parisc/kernel/drivers.c:337 print_hwpath() warn: impossible condition '(path->bc[i] == -1) => (0-255 == (-1))'
> arch/parisc/kernel/drivers.c:410 setup_bus_id() warn: impossible condition '(path.bc[i] == -1) => (0-255 == (-1))'
> arch/parisc/kernel/drivers.c:486 create_parisc_device() warn: impossible condition '(modpath->bc[i] == -1) => (0-255 == (-1))'
> arch/parisc/kernel/drivers.c:759 hwpath_to_device() warn: impossible condition '(modpath->bc[i] == -1) => (0-255 == (-1))'
> drivers/media/dvb-frontends/stv0288.c:471 stv0288_set_frontend() warn: assigning (-9) to unsigned variable 'tm'
> drivers/media/dvb-frontends/stv0288.c:471 stv0288_set_frontend() warn: we never enter this loop
> drivers/misc/sgi-gru/grumain.c:711 gru_check_chiplet_assignment() warn: 'gts->ts_user_chiplet_id' is unsigned
> drivers/net/wireless/cisco/airo.c:5316 proc_wepkey_on_close() warn: assigning (-16) to unsigned variable 'key[i / 3]'
> drivers/net/wireless/ralink/rt2x00/rt2800lib.c:9415 rt2800_iq_search() warn: assigning (-32) to unsigned variable 'idx0'
> drivers/net/wireless/ralink/rt2x00/rt2800lib.c:9470 rt2800_iq_search() warn: assigning (-32) to unsigned variable 'perr'
> drivers/video/fbdev/sis/init301.c:3549 SiS_GetCRT2Data301() warn: 'SiS_Pr->SiS_EModeIDTable[ModeIdIndex]->ROMMODEIDX661' is unsigned
> sound/pci/au88x0/au88x0_core.c:2029 vortex_adb_checkinout() warn: signedness bug returning '(-22)'
> sound/pci/au88x0/au88x0_core.c:2046 vortex_adb_checkinout() warn: signedness bug returning '(-12)'
> sound/pci/au88x0/au88x0_core.c:2125 vortex_adb_allocroute() warn: 'vortex_adb_checkinout(vortex, (0), en, 0)' is unsigned
> sound/pci/au88x0/au88x0_core.c:2170 vortex_adb_allocroute() warn: 'vortex_adb_checkinout(vortex, stream->resources, en, 4)' is unsigned
> sound/pci/rme9652/hdsp.c:3953 hdsp_channel_buffer_location() warn: 'hdsp->channel_map[channel]' is unsigned
> sound/pci/rme9652/rme9652.c:1833 rme9652_channel_buffer_location() warn: 'rme9652->channel_map[channel]' is unsigned
Thanks. I'll fix these up.
Jason
On Mon, Oct 24, 2022 at 9:34 AM Jason A. Donenfeld <[email protected]> wrote:
>
> Give these a minute to hit Lore, but patches just submitted to various
> maintainers as fixes (for 6.1), since these are already broken on some
> architecture.
Hold up a minute.
Some of those may need more thought. For example, that first one:
> https://lore.kernel.org/all/[email protected]
looks just *strange*. As far as I can tell, no other wireless drivers
do any sign checks at all.
Now, I didn't really look around a lot, but looking at a few other
SIOCSIWESSID users, most don't even seem to treat it as a string at
all, but as just a byte dump (so memcpy() instead of strncpy())
As far as I know, there are no actual rules for SSID character sets,
and while using utf-8 or something else might cause interoperability
problems, this driver seems to be just confused. If you want to check
for "printable characters", that check is still wrong.
So I don't think this is a "assume char is signed" issue. I think this
is a "driver is confused" issue.
IOW, I don't think these are 6.1 material as some kind of obvious
fixes, at least not without driver author acks.
Linus
Hi Linus,
On Mon, Oct 24, 2022 at 7:11 PM Linus Torvalds
<[email protected]> wrote:
> IOW, I don't think these are 6.1 material as some kind of obvious
> fixes, at least not without driver author acks.
Right, these are posted to the authors and maintainers to look at.
Maybe they punt them until 6.2 which would be fine too.
> On Mon, Oct 24, 2022 at 9:34 AM Jason A. Donenfeld <[email protected]> wrote:
> Some of those may need more thought. For example, that first one:
>
> > https://lore.kernel.org/all/[email protected]
>
> looks just *strange*. As far as I can tell, no other wireless drivers
> do any sign checks at all.
>
> Now, I didn't really look around a lot, but looking at a few other
> SIOCSIWESSID users, most don't even seem to treat it as a string at
> all, but as just a byte dump (so memcpy() instead of strncpy())
>
> As far as I know, there are no actual rules for SSID character sets,
> and while using utf-8 or something else might cause interoperability
> problems, this driver seems to be just confused. If you want to check
> for "printable characters", that check is still wrong.
>
> So I don't think this is a "assume char is signed" issue. I think this
> is a "driver is confused" issue.
Yea I had a few versions of this. In one of them, I changed `char
*extra` throughout the wireless stack into `s8 *extra` and in another
`u8 *extra`, after realizing they're mostly just bags of bits. But
that seemed pretty invasive when, indeed, this staging driver is just
a little screwy.
So perhaps the right fix is to just kill that whole snippet? Kalle - opinions?
Jason
On Mon, Oct 24, 2022 at 12:30:11PM +0300, Dan Carpenter wrote:
> On Mon, Oct 24, 2022 at 12:24:24PM +0300, Dan Carpenter wrote:
> > On Wed, Oct 19, 2022 at 02:30:34PM -0600, Jason A. Donenfeld wrote:
> > > Recently, some compile-time checking I added to the clamp_t family of
> > > functions triggered a build error when a poorly written driver was
> > > compiled on ARM, because the driver assumed that the naked `char` type
> > > is signed, but ARM treats it as unsigned, and the C standard says it's
> > > architecture-dependent.
> > >
> > > I doubt this particular driver is the only instance in which
> > > unsuspecting authors make assumptions about `char` with no `signed` or
> > > `unsigned` specifier. We were lucky enough this time that that driver
> > > used `clamp_t(char, negative_value, positive_value)`, so the new
> > > checking code found it, and I've sent a patch to fix it, but there are
> > > likely other places lurking that won't be so easily unearthed.
> > >
> > > So let's just eliminate this particular variety of heisensign bugs
> > > entirely. Set `-funsigned-char` globally, so that gcc makes the type
> > > unsigned on all architectures.
> > >
> > > This will break things in some places and fix things in others, so this
> > > will likely cause a bit of churn while reconciling the type misuse.
> > >
> >
> > This is a very daring change and obviously is going to introduce bugs.
> > It might be better to create a static checker rule that says "char"
> > without explicit signedness can only be used for strings.
> >
> > arch/parisc/kernel/drivers.c:337 print_hwpath() warn: impossible condition '(path->bc[i] == -1) => (0-255 == (-1))'
> > arch/parisc/kernel/drivers.c:410 setup_bus_id() warn: impossible condition '(path.bc[i] == -1) => (0-255 == (-1))'
> > arch/parisc/kernel/drivers.c:486 create_parisc_device() warn: impossible condition '(modpath->bc[i] == -1) => (0-255 == (-1))'
> > arch/parisc/kernel/drivers.c:759 hwpath_to_device() warn: impossible condition '(modpath->bc[i] == -1) => (0-255 == (-1))'
> > drivers/media/dvb-frontends/stv0288.c:471 stv0288_set_frontend() warn: assigning (-9) to unsigned variable 'tm'
> > drivers/media/dvb-frontends/stv0288.c:471 stv0288_set_frontend() warn: we never enter this loop
> > drivers/misc/sgi-gru/grumain.c:711 gru_check_chiplet_assignment() warn: 'gts->ts_user_chiplet_id' is unsigned
> > drivers/net/wireless/cisco/airo.c:5316 proc_wepkey_on_close() warn: assigning (-16) to unsigned variable 'key[i / 3]'
> > drivers/net/wireless/ralink/rt2x00/rt2800lib.c:9415 rt2800_iq_search() warn: assigning (-32) to unsigned variable 'idx0'
> > drivers/net/wireless/ralink/rt2x00/rt2800lib.c:9470 rt2800_iq_search() warn: assigning (-32) to unsigned variable 'perr'
> > drivers/video/fbdev/sis/init301.c:3549 SiS_GetCRT2Data301() warn: 'SiS_Pr->SiS_EModeIDTable[ModeIdIndex]->ROMMODEIDX661' is unsigned
> > sound/pci/au88x0/au88x0_core.c:2029 vortex_adb_checkinout() warn: signedness bug returning '(-22)'
> > sound/pci/au88x0/au88x0_core.c:2046 vortex_adb_checkinout() warn: signedness bug returning '(-12)'
> > sound/pci/au88x0/au88x0_core.c:2125 vortex_adb_allocroute() warn: 'vortex_adb_checkinout(vortex, (0), en, 0)' is unsigned
> > sound/pci/au88x0/au88x0_core.c:2170 vortex_adb_allocroute() warn: 'vortex_adb_checkinout(vortex, stream->resources, en, 4)' is unsigned
> > sound/pci/rme9652/hdsp.c:3953 hdsp_channel_buffer_location() warn: 'hdsp->channel_map[channel]' is unsigned
> > sound/pci/rme9652/rme9652.c:1833 rme9652_channel_buffer_location() warn: 'rme9652->channel_map[channel]' is unsigned
>
> Here are some more:
>
> drivers/net/wireless/ralink/rt2x00/rt2800lib.c:9472 rt2800_iq_search() warn: impossible condition '(gerr < -7) => (0-255 < (-7))'
> drivers/net/wireless/ralink/rt2x00/rt2800lib.c:9476 rt2800_iq_search() warn: impossible condition '(perr < -31) => (0-255 < (-31))'
> drivers/staging/rtl8192e/rtllib_softmac_wx.c:459 rtllib_wx_set_essid() warn: impossible condition '(extra[i] < 0) => (0-255 < 0)'
> sound/pci/rme9652/hdsp.c:4153 snd_hdsp_channel_info() warn: impossible condition '(hdsp->channel_map[channel] < 0) => (0-255 < 0)'
>
> This might be interesting for backports if everyone starts to rely on
> the fact that char is unsigned as the PPC people currently do.
Give these a minute to hit Lore, but patches just submitted to various
maintainers as fixes (for 6.1), since these are already broken on some
architecture.
https://lore.kernel.org/all/[email protected]
https://lore.kernel.org/all/[email protected]
https://lore.kernel.org/all/[email protected]
https://lore.kernel.org/all/[email protected]
https://lore.kernel.org/all/[email protected]
https://lore.kernel.org/all/[email protected]
https://lore.kernel.org/all/[email protected]
Jason
From: Linus Torvalds
> Sent: 24 October 2022 18:11
...
>
> As far as I know, there are no actual rules for SSID character sets,
> and while using utf-8 or something else might cause interoperability
> problems, this driver seems to be just confused. If you want to check
> for "printable characters", that check is still wrong.
Are SSID even required to be printable at all?
While most systems only let you configure 'strings' I don't
remember that actually being a requirement.
(I've sure I read up on this years ago.)
The frame format will be using an explicit length.
So even embedded zeros may be valid!
David
-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)
On 19/10/2022 21.54, Linus Torvalds wrote:
> On Wed, Oct 19, 2022 at 9:27 AM Jason A. Donenfeld <[email protected]> wrote:
>>
>> So let's just eliminate this particular variety of heisensigned bugs
>> entirely. Set `-fsigned-char` globally, so that gcc makes the type
>> signed on all architectures.
>
> Btw, I do wonder if we might actually be better off doing this - but
> doing it the other way around.
Only very tangentially related (because it has to do with chars...): Can
we switch our ctype to be ASCII only, just as it was back in the good'ol
mid 90s [i.e. before
https://git.kernel.org/pub/scm/linux/kernel/git/mpe/linux-fullhistory.git/commit/lib/ctype.c?id=036b97b05489161be06e63be77c5fad9247d23ff].
It bugs me that it's almost-but-not-quite-latin1, that toupper() isn't
idempotent, and that one can hit an isalpha() with toupper() and get
something that isn't isalpha().
Rasmus
On Tue, Oct 25, 2022 at 5:10 PM Rasmus Villemoes
<[email protected]> wrote:
>
> Only very tangentially related (because it has to do with chars...): Can
> we switch our ctype to be ASCII only, just as it was back in the good'ol
> mid 90s
Those US-ASCII days weren't really very "good" old days, but I forget
why we did this (it's attributed to me, but that's from the
pre-BK/pre-git days before we actually tracked things all that well,
so..)
Anyway, I think anybody using ctype.h on 8-bit chars gets what they
deserve, and I think Latin1 (or something close to it) is better than
US-ASCII, in that it's at least the same as Unicode in the low 8
chars.
So no, I'm disinclined to go back in time to what I think is an even
worse situation. Latin1 isn't great, but it sure beats US-ASCII. And
if you really want just US-ASII, then don't use the high bit, and make
your disgusting 7-bit code be *explicitly* 7-bit.
Now, if there are errors in that table wrt Latin1 / "first 256
codepoints of Unicode" too, then we can fix those.
Not that anybody has apparently cared since 2.0.1 was released back in
July of 1996 (btw, it's sad how none of the old linux git archive
creations seem to have tried to import the dates, so you have to look
those up separately)
And if nobody has cared since 1996, I don't really think it matters.
But fundamentally, I think anybody calling US-ASCII "good" is either
very very very confused, or is comparing it to EBCDIC.
Linus
On 26/10/2022 20.10, Linus Torvalds wrote:
> On Tue, Oct 25, 2022 at 5:10 PM Rasmus Villemoes
> <[email protected]> wrote:
>>
>> Only very tangentially related (because it has to do with chars...): Can
>> we switch our ctype to be ASCII only, just as it was back in the good'ol
>> mid 90s
>
> Those US-ASCII days weren't really very "good" old days, but I forget
> why we did this (it's attributed to me, but that's from the
> pre-BK/pre-git days before we actually tracked things all that well,
> so..)
>
> Anyway, I think anybody using ctype.h on 8-bit chars gets what they
> deserve, and I think Latin1 (or something close to it) is better than
> US-ASCII, in that it's at least the same as Unicode in the low 8
> chars.
My concern is that it's currently somewhat ill specified what our ctype
actually represents, and that would be a lot easier to specify if we
just said ASCII, everything above 0x7f is neither punct or ctrl or alpha
or anything else.
For example, people may do stuff like isprint(c) ? c : '.' in a printk()
call, but most likely the consumer (somebody doing dmesg) would, at
least these days, use utf-8, so that just results in a broken utf-8
sequence. Now I see that a lot of callers actually do "isascii(c) &&
isprint(c)", so they already know about this, but there are also many
instances where isprint() is used by itself.
There's also stuff like fs/afs/cell.c and other places that use
isprint/isalnum/... to make decisions on what is allowed on the wire
and/or in a disk format, where it's then hard to reason about just
exactly what is accepted. And places that use toupper() on their strings
to normalize them; that's broken when toupper() isn't idempotent.
> So no, I'm disinclined to go back in time to what I think is an even
> worse situation. Latin1 isn't great, but it sure beats US-ASCII. And
> if you really want just US-ASII, then don't use the high bit, and make
> your disgusting 7-bit code be *explicitly* 7-bit.
>
> Now, if there are errors in that table wrt Latin1 / "first 256
> codepoints of Unicode" too, then we can fix those.
AFAICT, the differences are:
- 0xaa (FEMININE ORDINAL INDICATOR), 0xb5 (MICRO SIGN), 0xba (FEMININE
ORDINAL INDICATOR) should be lower (hence alpha and alnum), not punct.
- depending a little on just exactly what one wants latin1 to mean, but
if it does mean "first 256 codepoints of Unicode", 0x80-0x9f should be cntrl
- for some reason at least glibc seems to classify 0xa0 as punctuation
and not space (hence also as isgraph)
- 0xdf and 0xff are correctly classified as lower, but since they don't
have upper-case versions (at least not any that are representable in
latin1), correct toupper() behaviour is to return them unchanged, but we
just subtract 0x20, so 0xff becomes 0xdf which isn't isupper() and 0xdf
becomes something that isn't even isalpha().
Fixing the first would create more instances of the last, and I think
the only sane way to fix that would be a 256 byte lookup table to use by
toupper().
> Not that anybody has apparently cared since 2.0.1 was released back in
> July of 1996
(btw, it's sad how none of the old linux git archive
> creations seem to have tried to import the dates, so you have to look
> those up separately)
Huh? That commit has 1996 as the author date, while its commit date is
indeed 2007. The very first line says:
author linus1 <[email protected]> 1996-07-02 11:00:00 -0600
> And if nobody has cared since 1996, I don't really think it matters.
Indeed, I don't think it's a huge problem in practice. But it still
bothers me that such a simple (and usually overlooked) corner of the
kernel's C library is ill-defined and arguably a little buggy.
Rasmus
On Thu, Oct 27, 2022 at 12:59 AM Rasmus Villemoes
<[email protected]> wrote:
>
> AFAICT, the differences are:
>
> - 0xaa (FEMININE ORDINAL INDICATOR), 0xb5 (MICRO SIGN), 0xba (FEMININE
> ORDINAL INDICATOR) should be lower (hence alpha and alnum), not punct.
>
> - depending a little on just exactly what one wants latin1 to mean, but
> if it does mean "first 256 codepoints of Unicode", 0x80-0x9f should be cntrl
>
> - for some reason at least glibc seems to classify 0xa0 as punctuation
> and not space (hence also as isgraph)
>
> - 0xdf and 0xff are correctly classified as lower, but since they don't
> have upper-case versions (at least not any that are representable in
> latin1), correct toupper() behaviour is to return them unchanged, but we
> just subtract 0x20, so 0xff becomes 0xdf which isn't isupper() and 0xdf
> becomes something that isn't even isalpha().
Heh.
Honestly, I don't think we should care at all.
For the byte range 128-255, anybody who uses ctype on them gets what
they get. In the kernel, the most likely use of it is for 'isprint()',
and if those care, they can (and some do) use 'isascii()' in addition.
I don't know if you realize, but the kernel already says "screw libc",
and makes all the isxyz() things just cast the argument to 'unsigned
char', and doesn't care about EOF.
And for the rest, let's just call it the "kernel locale", and just
admit that the kernel locale is entirely historical.
Boom - problem solved, and it's entirely standards conformant (apart
possibly from the EOF case, I think that is marked as a "lower case
character" right now ;)
Looking through
https://pubs.opengroup.org/onlinepubs/9699919799/
I'm not actually seeing anything that says that we don't do *exactly*
what the standard requires.
You thinking that the kernel locale is US-ASCII is just wrong.
Linus
Hi Günter,
On Wed, Dec 21, 2022 at 3:54 PM Guenter Roeck <[email protected]> wrote:
> On Wed, Oct 19, 2022 at 02:30:34PM -0600, Jason A. Donenfeld wrote:
> > Recently, some compile-time checking I added to the clamp_t family of
> > functions triggered a build error when a poorly written driver was
> > compiled on ARM, because the driver assumed that the naked `char` type
> > is signed, but ARM treats it as unsigned, and the C standard says it's
> > architecture-dependent.
> >
> > I doubt this particular driver is the only instance in which
> > unsuspecting authors make assumptions about `char` with no `signed` or
> > `unsigned` specifier. We were lucky enough this time that that driver
> > used `clamp_t(char, negative_value, positive_value)`, so the new
> > checking code found it, and I've sent a patch to fix it, but there are
> > likely other places lurking that won't be so easily unearthed.
> >
> > So let's just eliminate this particular variety of heisensign bugs
> > entirely. Set `-funsigned-char` globally, so that gcc makes the type
> > unsigned on all architectures.
> >
> > This will break things in some places and fix things in others, so this
> > will likely cause a bit of churn while reconciling the type misuse.
> >
>
> There is an interesting fallout: When running the m68k:q800 qemu emulation,
> there are lots of warning backtraces.
>
> WARNING: CPU: 0 PID: 23 at crypto/testmgr.c:5724 alg_test.part.0+0x7c/0x326
> testmgr: alg_test_descs entries in wrong order: 'adiantum(xchacha12,aes)' before 'adiantum(xchacha20,aes)'
> ------------[ cut here ]------------
> WARNING: CPU: 0 PID: 23 at crypto/testmgr.c:5724 alg_test.part.0+0x7c/0x326
> testmgr: alg_test_descs entries in wrong order: 'adiantum(xchacha20,aes)' before 'aegis128'
>
> and so on for pretty much every entry in the alg_test_descs[] array.
>
> Bisect points to this patch, and reverting it fixes the problem.
>
> It looks like the problem is that arch/m68k/include/asm/string.h
> uses "char res" to store the result of strcmp(), and char is now
> unsigned - meaning strcmp() will now never return a value < 0.
> Effectively that means that strcmp() is broken on m68k if
> CONFIG_COLDFIRE=n.
>
> The fix is probably quite simple.
>
> diff --git a/arch/m68k/include/asm/string.h b/arch/m68k/include/asm/string.h
> index f759d944c449..b8f4ae19e8f6 100644
> --- a/arch/m68k/include/asm/string.h
> +++ b/arch/m68k/include/asm/string.h
> @@ -42,7 +42,7 @@ static inline char *strncpy(char *dest, const char *src, size_t n)
> #define __HAVE_ARCH_STRCMP
> static inline int strcmp(const char *cs, const char *ct)
> {
> - char res;
> + signed char res;
>
> asm ("\n"
> "1: move.b (%0)+,%2\n" /* get *cs */
>
> Does that make sense ? If so I can send a patch.
Thanks, been there, done that
https://lore.kernel.org/all/bce014e60d7b1a3d1c60009fc3572e2f72591f21.1671110959.git.geert@linux-m68k.org
Note that we detected other issues with the m68k strcmp(), so
probably that patch wouldn't go in as-is.
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
On Wed, Dec 21, 2022 at 04:05:45PM +0100, Geert Uytterhoeven wrote:
> Hi G?nter,
>
> On Wed, Dec 21, 2022 at 3:54 PM Guenter Roeck <[email protected]> wrote:
> > On Wed, Oct 19, 2022 at 02:30:34PM -0600, Jason A. Donenfeld wrote:
> > > Recently, some compile-time checking I added to the clamp_t family of
> > > functions triggered a build error when a poorly written driver was
> > > compiled on ARM, because the driver assumed that the naked `char` type
> > > is signed, but ARM treats it as unsigned, and the C standard says it's
> > > architecture-dependent.
> > >
> > > I doubt this particular driver is the only instance in which
> > > unsuspecting authors make assumptions about `char` with no `signed` or
> > > `unsigned` specifier. We were lucky enough this time that that driver
> > > used `clamp_t(char, negative_value, positive_value)`, so the new
> > > checking code found it, and I've sent a patch to fix it, but there are
> > > likely other places lurking that won't be so easily unearthed.
> > >
> > > So let's just eliminate this particular variety of heisensign bugs
> > > entirely. Set `-funsigned-char` globally, so that gcc makes the type
> > > unsigned on all architectures.
> > >
> > > This will break things in some places and fix things in others, so this
> > > will likely cause a bit of churn while reconciling the type misuse.
> > >
> >
> > There is an interesting fallout: When running the m68k:q800 qemu emulation,
> > there are lots of warning backtraces.
> >
> > WARNING: CPU: 0 PID: 23 at crypto/testmgr.c:5724 alg_test.part.0+0x7c/0x326
> > testmgr: alg_test_descs entries in wrong order: 'adiantum(xchacha12,aes)' before 'adiantum(xchacha20,aes)'
> > ------------[ cut here ]------------
> > WARNING: CPU: 0 PID: 23 at crypto/testmgr.c:5724 alg_test.part.0+0x7c/0x326
> > testmgr: alg_test_descs entries in wrong order: 'adiantum(xchacha20,aes)' before 'aegis128'
> >
> > and so on for pretty much every entry in the alg_test_descs[] array.
> >
> > Bisect points to this patch, and reverting it fixes the problem.
> >
> > It looks like the problem is that arch/m68k/include/asm/string.h
> > uses "char res" to store the result of strcmp(), and char is now
> > unsigned - meaning strcmp() will now never return a value < 0.
> > Effectively that means that strcmp() is broken on m68k if
> > CONFIG_COLDFIRE=n.
> >
> > The fix is probably quite simple.
> >
> > diff --git a/arch/m68k/include/asm/string.h b/arch/m68k/include/asm/string.h
> > index f759d944c449..b8f4ae19e8f6 100644
> > --- a/arch/m68k/include/asm/string.h
> > +++ b/arch/m68k/include/asm/string.h
> > @@ -42,7 +42,7 @@ static inline char *strncpy(char *dest, const char *src, size_t n)
> > #define __HAVE_ARCH_STRCMP
> > static inline int strcmp(const char *cs, const char *ct)
> > {
> > - char res;
> > + signed char res;
> >
> > asm ("\n"
> > "1: move.b (%0)+,%2\n" /* get *cs */
> >
> > Does that make sense ? If so I can send a patch.
>
> Thanks, been there, done that
> https://lore.kernel.org/all/bce014e60d7b1a3d1c60009fc3572e2f72591f21.1671110959.git.geert@linux-m68k.org
>
> Note that we detected other issues with the m68k strcmp(), so
> probably that patch wouldn't go in as-is.
>
So anything non-Coldfire is and will remain broken on m68k for the time
being ? Wouldn't it be better to fix the acute problem now and address
the long-standing problem(s) separately ?
Thanks,
Guenter
On Wed, Oct 19, 2022 at 02:30:34PM -0600, Jason A. Donenfeld wrote:
> Recently, some compile-time checking I added to the clamp_t family of
> functions triggered a build error when a poorly written driver was
> compiled on ARM, because the driver assumed that the naked `char` type
> is signed, but ARM treats it as unsigned, and the C standard says it's
> architecture-dependent.
>
> I doubt this particular driver is the only instance in which
> unsuspecting authors make assumptions about `char` with no `signed` or
> `unsigned` specifier. We were lucky enough this time that that driver
> used `clamp_t(char, negative_value, positive_value)`, so the new
> checking code found it, and I've sent a patch to fix it, but there are
> likely other places lurking that won't be so easily unearthed.
>
> So let's just eliminate this particular variety of heisensign bugs
> entirely. Set `-funsigned-char` globally, so that gcc makes the type
> unsigned on all architectures.
>
> This will break things in some places and fix things in others, so this
> will likely cause a bit of churn while reconciling the type misuse.
>
There is an interesting fallout: When running the m68k:q800 qemu emulation,
there are lots of warning backtraces.
WARNING: CPU: 0 PID: 23 at crypto/testmgr.c:5724 alg_test.part.0+0x7c/0x326
testmgr: alg_test_descs entries in wrong order: 'adiantum(xchacha12,aes)' before 'adiantum(xchacha20,aes)'
------------[ cut here ]------------
WARNING: CPU: 0 PID: 23 at crypto/testmgr.c:5724 alg_test.part.0+0x7c/0x326
testmgr: alg_test_descs entries in wrong order: 'adiantum(xchacha20,aes)' before 'aegis128'
and so on for pretty much every entry in the alg_test_descs[] array.
Bisect points to this patch, and reverting it fixes the problem.
It looks like the problem is that arch/m68k/include/asm/string.h
uses "char res" to store the result of strcmp(), and char is now
unsigned - meaning strcmp() will now never return a value < 0.
Effectively that means that strcmp() is broken on m68k if
CONFIG_COLDFIRE=n.
The fix is probably quite simple.
diff --git a/arch/m68k/include/asm/string.h b/arch/m68k/include/asm/string.h
index f759d944c449..b8f4ae19e8f6 100644
--- a/arch/m68k/include/asm/string.h
+++ b/arch/m68k/include/asm/string.h
@@ -42,7 +42,7 @@ static inline char *strncpy(char *dest, const char *src, size_t n)
#define __HAVE_ARCH_STRCMP
static inline int strcmp(const char *cs, const char *ct)
{
- char res;
+ signed char res;
asm ("\n"
"1: move.b (%0)+,%2\n" /* get *cs */
Does that make sense ? If so I can send a patch.
Guenter
On 21/12/2022 16.05, Geert Uytterhoeven wrote:
> Hi Günter,
>
> On Wed, Dec 21, 2022 at 3:54 PM Guenter Roeck <[email protected]> wrote:
>> On Wed, Oct 19, 2022 at 02:30:34PM -0600, Jason A. Donenfeld wrote:
>>> Recently, some compile-time checking I added to the clamp_t family of
>>> functions triggered a build error when a poorly written driver was
>>> compiled on ARM, because the driver assumed that the naked `char` type
>>> is signed, but ARM treats it as unsigned, and the C standard says it's
>>> architecture-dependent.
>>>
>>> I doubt this particular driver is the only instance in which
>>> unsuspecting authors make assumptions about `char` with no `signed` or
>>> `unsigned` specifier. We were lucky enough this time that that driver
>>> used `clamp_t(char, negative_value, positive_value)`, so the new
>>> checking code found it, and I've sent a patch to fix it, but there are
>>> likely other places lurking that won't be so easily unearthed.
>>>
>>> So let's just eliminate this particular variety of heisensign bugs
>>> entirely. Set `-funsigned-char` globally, so that gcc makes the type
>>> unsigned on all architectures.
>>>
>>> This will break things in some places and fix things in others, so this
>>> will likely cause a bit of churn while reconciling the type misuse.
>>>
>>
>> There is an interesting fallout: When running the m68k:q800 qemu emulation,
>> there are lots of warning backtraces.
>>
>> WARNING: CPU: 0 PID: 23 at crypto/testmgr.c:5724 alg_test.part.0+0x7c/0x326
>> testmgr: alg_test_descs entries in wrong order: 'adiantum(xchacha12,aes)' before 'adiantum(xchacha20,aes)'
>> ------------[ cut here ]------------
>> WARNING: CPU: 0 PID: 23 at crypto/testmgr.c:5724 alg_test.part.0+0x7c/0x326
>> testmgr: alg_test_descs entries in wrong order: 'adiantum(xchacha20,aes)' before 'aegis128'
>>
>> and so on for pretty much every entry in the alg_test_descs[] array.
>>
>> Bisect points to this patch, and reverting it fixes the problem.
>>
>> It looks like the problem is that arch/m68k/include/asm/string.h
>> uses "char res" to store the result of strcmp(), and char is now
>> unsigned - meaning strcmp() will now never return a value < 0.
>> Effectively that means that strcmp() is broken on m68k if
>> CONFIG_COLDFIRE=n.
>>
>> The fix is probably quite simple.
>>
>> diff --git a/arch/m68k/include/asm/string.h b/arch/m68k/include/asm/string.h
>> index f759d944c449..b8f4ae19e8f6 100644
>> --- a/arch/m68k/include/asm/string.h
>> +++ b/arch/m68k/include/asm/string.h
>> @@ -42,7 +42,7 @@ static inline char *strncpy(char *dest, const char *src, size_t n)
>> #define __HAVE_ARCH_STRCMP
>> static inline int strcmp(const char *cs, const char *ct)
>> {
>> - char res;
>> + signed char res;
>>
>> asm ("\n"
>> "1: move.b (%0)+,%2\n" /* get *cs */
>>
>> Does that make sense ? If so I can send a patch.
>
> Thanks, been there, done that
> https://lore.kernel.org/all/bce014e60d7b1a3d1c60009fc3572e2f72591f21.1671110959.git.geert@linux-m68k.org
Well, looks like that would still leave strcmp() buggy, you can't
represent all possible differences between two char values (signed or
not) in an 8-bit quantity. So any implementation based on returning the
first non-zero value of *a - *b must store that intermediate value in
something wider. Otherwise you'll get -128 from strcmp("\x40", "\xc0"),
but _also_ -128 when you do strcmp("\xc0", "\x40"), which is obviously
bogus.
I recently fixed that long-standing bug in U-Boot's strcmp() and a
similar one in nolibc in the linux tree. I wonder how many more
instances exist.
Rasmus
On Wed, Dec 21, 2022 at 04:29:11PM +0100, Rasmus Villemoes wrote:
> On 21/12/2022 16.05, Geert Uytterhoeven wrote:
> > Hi Günter,
> >
> > On Wed, Dec 21, 2022 at 3:54 PM Guenter Roeck <[email protected]> wrote:
> >> On Wed, Oct 19, 2022 at 02:30:34PM -0600, Jason A. Donenfeld wrote:
> >>> Recently, some compile-time checking I added to the clamp_t family of
> >>> functions triggered a build error when a poorly written driver was
> >>> compiled on ARM, because the driver assumed that the naked `char` type
> >>> is signed, but ARM treats it as unsigned, and the C standard says it's
> >>> architecture-dependent.
> >>>
> >>> I doubt this particular driver is the only instance in which
> >>> unsuspecting authors make assumptions about `char` with no `signed` or
> >>> `unsigned` specifier. We were lucky enough this time that that driver
> >>> used `clamp_t(char, negative_value, positive_value)`, so the new
> >>> checking code found it, and I've sent a patch to fix it, but there are
> >>> likely other places lurking that won't be so easily unearthed.
> >>>
> >>> So let's just eliminate this particular variety of heisensign bugs
> >>> entirely. Set `-funsigned-char` globally, so that gcc makes the type
> >>> unsigned on all architectures.
> >>>
> >>> This will break things in some places and fix things in others, so this
> >>> will likely cause a bit of churn while reconciling the type misuse.
> >>>
> >>
> >> There is an interesting fallout: When running the m68k:q800 qemu emulation,
> >> there are lots of warning backtraces.
> >>
> >> WARNING: CPU: 0 PID: 23 at crypto/testmgr.c:5724 alg_test.part.0+0x7c/0x326
> >> testmgr: alg_test_descs entries in wrong order: 'adiantum(xchacha12,aes)' before 'adiantum(xchacha20,aes)'
> >> ------------[ cut here ]------------
> >> WARNING: CPU: 0 PID: 23 at crypto/testmgr.c:5724 alg_test.part.0+0x7c/0x326
> >> testmgr: alg_test_descs entries in wrong order: 'adiantum(xchacha20,aes)' before 'aegis128'
> >>
> >> and so on for pretty much every entry in the alg_test_descs[] array.
> >>
> >> Bisect points to this patch, and reverting it fixes the problem.
> >>
> >> It looks like the problem is that arch/m68k/include/asm/string.h
> >> uses "char res" to store the result of strcmp(), and char is now
> >> unsigned - meaning strcmp() will now never return a value < 0.
> >> Effectively that means that strcmp() is broken on m68k if
> >> CONFIG_COLDFIRE=n.
> >>
> >> The fix is probably quite simple.
> >>
> >> diff --git a/arch/m68k/include/asm/string.h b/arch/m68k/include/asm/string.h
> >> index f759d944c449..b8f4ae19e8f6 100644
> >> --- a/arch/m68k/include/asm/string.h
> >> +++ b/arch/m68k/include/asm/string.h
> >> @@ -42,7 +42,7 @@ static inline char *strncpy(char *dest, const char *src, size_t n)
> >> #define __HAVE_ARCH_STRCMP
> >> static inline int strcmp(const char *cs, const char *ct)
> >> {
> >> - char res;
> >> + signed char res;
> >>
> >> asm ("\n"
> >> "1: move.b (%0)+,%2\n" /* get *cs */
> >>
> >> Does that make sense ? If so I can send a patch.
> >
> > Thanks, been there, done that
> > https://lore.kernel.org/all/bce014e60d7b1a3d1c60009fc3572e2f72591f21.1671110959.git.geert@linux-m68k.org
>
> Well, looks like that would still leave strcmp() buggy, you can't
> represent all possible differences between two char values (signed or
> not) in an 8-bit quantity. So any implementation based on returning the
> first non-zero value of *a - *b must store that intermediate value in
> something wider. Otherwise you'll get -128 from strcmp("\x40", "\xc0"),
> but _also_ -128 when you do strcmp("\xc0", "\x40"), which is obviously
> bogus.
>
The above assumes an unsigned char as input to strcmp(). I consider that
a hypothetical problem because "comparing" strings with upper bits
set doesn't really make sense in practice (How does one compare Günter
against Gunter ? And how about Gǖnter ?). On the other side, the problem
observed here is real and immediate.
Guenter
Hi Rasmus,
On Wed, Dec 21, 2022 at 4:29 PM Rasmus Villemoes
<[email protected]> wrote:
> On 21/12/2022 16.05, Geert Uytterhoeven wrote:
> > On Wed, Dec 21, 2022 at 3:54 PM Guenter Roeck <[email protected]> wrote:
> >> On Wed, Oct 19, 2022 at 02:30:34PM -0600, Jason A. Donenfeld wrote:
> >>> Recently, some compile-time checking I added to the clamp_t family of
> >>> functions triggered a build error when a poorly written driver was
> >>> compiled on ARM, because the driver assumed that the naked `char` type
> >>> is signed, but ARM treats it as unsigned, and the C standard says it's
> >>> architecture-dependent.
> >>>
> >>> I doubt this particular driver is the only instance in which
> >>> unsuspecting authors make assumptions about `char` with no `signed` or
> >>> `unsigned` specifier. We were lucky enough this time that that driver
> >>> used `clamp_t(char, negative_value, positive_value)`, so the new
> >>> checking code found it, and I've sent a patch to fix it, but there are
> >>> likely other places lurking that won't be so easily unearthed.
> >>>
> >>> So let's just eliminate this particular variety of heisensign bugs
> >>> entirely. Set `-funsigned-char` globally, so that gcc makes the type
> >>> unsigned on all architectures.
> >>>
> >>> This will break things in some places and fix things in others, so this
> >>> will likely cause a bit of churn while reconciling the type misuse.
> >>>
> >>
> >> There is an interesting fallout: When running the m68k:q800 qemu emulation,
> >> there are lots of warning backtraces.
> >>
> >> WARNING: CPU: 0 PID: 23 at crypto/testmgr.c:5724 alg_test.part.0+0x7c/0x326
> >> testmgr: alg_test_descs entries in wrong order: 'adiantum(xchacha12,aes)' before 'adiantum(xchacha20,aes)'
> >> ------------[ cut here ]------------
> >> WARNING: CPU: 0 PID: 23 at crypto/testmgr.c:5724 alg_test.part.0+0x7c/0x326
> >> testmgr: alg_test_descs entries in wrong order: 'adiantum(xchacha20,aes)' before 'aegis128'
> >>
> >> and so on for pretty much every entry in the alg_test_descs[] array.
> >>
> >> Bisect points to this patch, and reverting it fixes the problem.
> >>
> >> It looks like the problem is that arch/m68k/include/asm/string.h
> >> uses "char res" to store the result of strcmp(), and char is now
> >> unsigned - meaning strcmp() will now never return a value < 0.
> >> Effectively that means that strcmp() is broken on m68k if
> >> CONFIG_COLDFIRE=n.
> >>
> >> The fix is probably quite simple.
> >>
> >> diff --git a/arch/m68k/include/asm/string.h b/arch/m68k/include/asm/string.h
> >> index f759d944c449..b8f4ae19e8f6 100644
> >> --- a/arch/m68k/include/asm/string.h
> >> +++ b/arch/m68k/include/asm/string.h
> >> @@ -42,7 +42,7 @@ static inline char *strncpy(char *dest, const char *src, size_t n)
> >> #define __HAVE_ARCH_STRCMP
> >> static inline int strcmp(const char *cs, const char *ct)
> >> {
> >> - char res;
> >> + signed char res;
> >>
> >> asm ("\n"
> >> "1: move.b (%0)+,%2\n" /* get *cs */
> >>
> >> Does that make sense ? If so I can send a patch.
> >
> > Thanks, been there, done that
> > https://lore.kernel.org/all/bce014e60d7b1a3d1c60009fc3572e2f72591f21.1671110959.git.geert@linux-m68k.org
>
> Well, looks like that would still leave strcmp() buggy, you can't
> represent all possible differences between two char values (signed or
> not) in an 8-bit quantity. So any implementation based on returning the
> first non-zero value of *a - *b must store that intermediate value in
> something wider. Otherwise you'll get -128 from strcmp("\x40", "\xc0"),
> but _also_ -128 when you do strcmp("\xc0", "\x40"), which is obviously
> bogus.
So we have https://lore.kernel.org/all/[email protected] ;-)
And the other issue is m68k strcmp() calls being dropped by the
optimizer, cfr. the discussion in
https://lore.kernel.org/all/[email protected]
> I recently fixed that long-standing bug in U-Boot's strcmp() and a
> similar one in nolibc in the linux tree. I wonder how many more
> instances exist.
Thanks, commit fb63362c63c7aeac ("lib: fix buggy strcmp and strncmp") in
v2023.01-rc1, which is not yet in a released version.
(and in plain C, not in asm ;-)
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
On Wed, Dec 21, 2022 at 7:56 AM Guenter Roeck <[email protected]> wrote:
>
> The above assumes an unsigned char as input to strcmp(). I consider that
> a hypothetical problem because "comparing" strings with upper bits
> set doesn't really make sense in practice (How does one compare Günter
> against Gunter ? And how about Gǖnter ?). On the other side, the problem
> observed here is real and immediate.
POSIX does actually specify "Günter" vs "Gunter".
The way strcmp is supposed to work is to return the sign of the
difference between the byte values ("unsigned char").
But that sign has to be computed in 'int', not in 'signed char'.
So yes, the m68k implementation is broken regardless, but with a
signed char it just happened to work for the US-ASCII case that the
crypto case tested.
I think the real fix is to just remove that broken implementation
entirely, and rely on the generic one.
I'll commit that, and see what happens.
Linus
On Wed, Dec 21, 2022 at 09:06:41AM -0800, Linus Torvalds wrote:
> On Wed, Dec 21, 2022 at 7:56 AM Guenter Roeck <[email protected]> wrote:
> >
> > The above assumes an unsigned char as input to strcmp(). I consider that
> > a hypothetical problem because "comparing" strings with upper bits
> > set doesn't really make sense in practice (How does one compare Günter
> > against Gunter ? And how about Gǖnter ?). On the other side, the problem
> > observed here is real and immediate.
>
> POSIX does actually specify "Günter" vs "Gunter".
>
> The way strcmp is supposed to work is to return the sign of the
> difference between the byte values ("unsigned char").
>
> But that sign has to be computed in 'int', not in 'signed char'.
>
> So yes, the m68k implementation is broken regardless, but with a
> signed char it just happened to work for the US-ASCII case that the
> crypto case tested.
>
I understand. I just prefer a known limited breakage to completely
broken code.
> I think the real fix is to just remove that broken implementation
> entirely, and rely on the generic one.
Perfectly fine with me.
Thanks,
Guenter
On Dez 21 2022, Guenter Roeck wrote:
> The above assumes an unsigned char as input to strcmp().
That's how strcmp is defined.
See <https://lore.kernel.org/all/[email protected]>
--
Andreas Schwab, [email protected]
GPG Key fingerprint = 7578 EB47 D4E5 4D69 2510 2552 DF73 E780 A9DA AEC1
"And now for something completely different."
On Wed, Dec 21, 2022 at 9:19 AM Guenter Roeck <[email protected]> wrote:
>
> On Wed, Dec 21, 2022 at 09:06:41AM -0800, Linus Torvalds wrote:
> >
> > I think the real fix is to just remove that broken implementation
> > entirely, and rely on the generic one.
>
> Perfectly fine with me.
That got pushed out as commit 7c0846125358 ("m68k: remove broken
strcmp implementation") but it's obviously entirely untested. I don't
do m68k cross-compiles, much less boot tests.
Just FYI for everybody - I may have screwed something up for some very
non-obvious reason.
But it looked very obvious indeed, and I hate having buggy code that
is architecture-specific when we have generic code that isn't buggy.
Linus
On Wed, Dec 21, 2022 at 10:46 AM Linus Torvalds
<[email protected]> wrote:
>
> But it looked very obvious indeed, and I hate having buggy code that
> is architecture-specific when we have generic code that isn't buggy.
Side note: we have an x86-64 implementation that looks fine (but not
really noticeably better than the generic one) that is based on the
'return subtraction' model. But it seems to get it right.
And we have a 32-bit x86 assembly thing that is based on 'rep scasb',
that then uses the carry bit to also get things right.
That 32-bit asm goes back to Linux 0.01 (with some changes since to
use "sbbl+or" instead of a conditional neg). I was playing around a
lot with the 'rep' instructions back when, since it was all part of
"learn the instruction set" for me.
Both of them should probably be removed as pointless too, but they
don't seem actively buggy.
Linus
On Wed, Dec 21, 2022 at 10:46:08AM -0800, Linus Torvalds wrote:
> On Wed, Dec 21, 2022 at 9:19 AM Guenter Roeck <[email protected]> wrote:
> >
> > On Wed, Dec 21, 2022 at 09:06:41AM -0800, Linus Torvalds wrote:
> > >
> > > I think the real fix is to just remove that broken implementation
> > > entirely, and rely on the generic one.
> >
> > Perfectly fine with me.
>
> That got pushed out as commit 7c0846125358 ("m68k: remove broken
> strcmp implementation") but it's obviously entirely untested. I don't
> do m68k cross-compiles, much less boot tests.
>
> Just FYI for everybody - I may have screwed something up for some very
> non-obvious reason.
>
No worries:
Build reference: msi-fixes-6.2-1-2644-g0a924817d2ed
Building mcf5208evb:m5208:m5208evb_defconfig:initrd ... running ..... passed
Building q800:m68040:mac_defconfig:initrd ... running ..... passed
Building q800:m68040:mac_defconfig:rootfs ... running ..... passed
Guenter
From: Linus Torvalds
> Sent: 21 December 2022 17:07
>
> On Wed, Dec 21, 2022 at 7:56 AM Guenter Roeck <[email protected]> wrote:
> >
> > The above assumes an unsigned char as input to strcmp(). I consider that
> > a hypothetical problem because "comparing" strings with upper bits
> > set doesn't really make sense in practice (How does one compare Günter
> > against Gunter ? And how about Gǖnter ?). On the other side, the problem
> > observed here is real and immediate.
>
> POSIX does actually specify "Günter" vs "Gunter".
>
> The way strcmp is supposed to work is to return the sign of the
> difference between the byte values ("unsigned char").
>
> But that sign has to be computed in 'int', not in 'signed char'.
>
> So yes, the m68k implementation is broken regardless, but with a
> signed char it just happened to work for the US-ASCII case that the
> crypto case tested.
>
> I think the real fix is to just remove that broken implementation
> entirely, and rely on the generic one.
I wonder how much slower it is - m68k is likely to be microcoded
and I don't think instruction timings are actually available.
The fastest version probably uses subx (with carry) to generate
0/-1 and leaves +delta for the other result - but getting the
compares and branches in the right order is hard.
I believe some of the other m68k asm functions are also missing
the "memory" 'clobber' and so could get mis-optimised.
While I can write (or rather have written) m68k asm I don't have
a compiler.
I also suspect that any x86 code that uses 'rep scas' is going
to be slow on anything modern.
David
-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)
Hi Linus,
On Wed, Dec 21, 2022 at 7:46 PM Linus Torvalds
<[email protected]> wrote:
> On Wed, Dec 21, 2022 at 9:19 AM Guenter Roeck <[email protected]> wrote:
> > On Wed, Dec 21, 2022 at 09:06:41AM -0800, Linus Torvalds wrote:
> > > I think the real fix is to just remove that broken implementation
> > > entirely, and rely on the generic one.
> >
> > Perfectly fine with me.
>
> That got pushed out as commit 7c0846125358 ("m68k: remove broken
> strcmp implementation") but it's obviously entirely untested. I don't
> do m68k cross-compiles, much less boot tests.
>
> Just FYI for everybody - I may have screwed something up for some very
> non-obvious reason.
>
> But it looked very obvious indeed, and I hate having buggy code that
> is architecture-specific when we have generic code that isn't buggy.
Thank you for being proactive!
It works fine (and slightly reduced kernel size, too ;-)
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
Hi Holger,
On Sat, Dec 24, 2022 at 10:34 AM Holger Lubitz
<[email protected]> wrote:
> On Thu, 2022-12-22 at 10:41 +0000, David Laight wrote:
> > I wonder how much slower it is - m68k is likely to be microcoded
> > and I don't think instruction timings are actually available.
>
> Not sure if these are in any way official, but
> http://oldwww.nvg.ntnu.no/amiga/MC680x0_Sections/mc68030timing.HTML
>
> (There's also
> http://oldwww.nvg.ntnu.no/amiga/MC680x0_Sections/mc68000timing.HTML
> but that is probably only interesting to demo coders by now)
Yes, instruction timings are available. Unlike for e.g. x86, there
is only a very limited number of parts to consider.
> > I believe some of the other m68k asm functions are also missing
> > the "memory" 'clobber' and so could get mis-optimised.
>
> In which case would that happen? This function doesn't clobber memory
> and its result does get used. If gcc mistakenly thinks the parameters
> haven't changed and uses a previously cached result, wouldn't that
> apply to a C function too?
For a pure C inline function, the compiler knows exactly what it does.
For an external C function, the compiler assumes all odds are off.
For inline asm, the compiler doesn't know what happens with (the data
pointed to by) the pointers, unless that's described in the constraints.
We do have some inline asm that has "*ptr" in the constraints, but
that applies to a single value, not to an array. And in case of
strings, the size of the array is not known without looking for the
zero-terminator.
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
From: Holger Lubitz
> Sent: 24 December 2022 09:34
>
> On Thu, 2022-12-22 at 10:41 +0000, David Laight wrote:
> > I wonder how much slower it is - m68k is likely to be microcoded
> > and I don't think instruction timings are actually available.
>
> Not sure if these are in any way official, but
> http://oldwww.nvg.ntnu.no/amiga/MC680x0_Sections/mc68030timing.HTML
I thought about that some more and remember seeing memory timings
on a logic analyser - and getting timings that (more or less)
implied sequential execution limited by the obvious memory (cache)
accesses.
The microcoding is more apparent in the large mid-instruction
interrupt stack frames - eg for page faults.
>
> (There's also
> http://oldwww.nvg.ntnu.no/amiga/MC680x0_Sections/mc68000timing.HTML
> but that is probably only interesting to demo coders by now)
>
> > The fastest version probably uses subx (with carry) to generate
> > 0/-1 and leaves +delta for the other result - but getting the
> > compares and branches in the right order is hard.
>
> Guess it must have been over 20 years since I wrote any 68k asm, but
> now I actually ended up installing Debian on qemu to experiment.
>
> There are two interesting differences between 68k and x86 that can be
> useful here: Unlike x86, MOV on 68k sets the flags. And also, subx
> differs from sbb in that it resets the zero flag on a non-zero result,
> but does not set it on a zero result. So if it is set, it must have
> been set before.
>
> Here are the two functions I came up with (tested only stand-alone, not
> in a kernel build. Also no benchmarks because this 68040 is only
> emulated)
> #1 (optimized for minimum instruction count in loop,
> 68k + Coldfire ISA_B)
>
> int strcmp1(const char *cs, const char *ct)
> {
> int res;
>
> asm ("\n"
> "1: move.b (%0)+,%2\n" /* get *cs */
> " jeq 2f\n" /* end of first string? */
> " cmp.b (%1)+,%2\n" /* compare *ct */
> " jeq 1b\n" /* if equal, continue */
> " jra 3f\n" /* else skip to tail */
> "2: cmp.b (%1)+,%2\n" /* compare one last byte */
> "3: subx.l %2, %2\n" /* -1 if borrow, 0 if not */
> " jls 4f\n" /* if set, z is from sub.b */
The subx will set Z unless C was set.
So that doesn't seem right.
> " moveq.l #1, %2\n" /* 1 if !borrow */
> "4:"
> : "+a" (cs), "+a" (ct), "=d" (res));
> return res;
> }
I think this should work:
(But the jc might need to be jnc.)
" moveq.l #0,%2\n" /* zero high bits of result */
"1: move.b (%1)+,%2\n" /* get *ct */
" jeq 2f\n" /* end of second string? */
" cmp.b (%0)+,%2\n" /* compare *cs */
" jeq 1b\n" /* if equal, continue */
" jc 4f /* return +ve */
" moveq.l #-1, %2\n" /* return -ve */
" jra 4f\n"
"2: move.b (%0),%2\n" /* check for matching strings */
"4:"
> #2 (optimized for minimum code size,
> Coldfire ISA_A compatible)
>
> int strcmp2(const char *cs, const char *ct)
> {
> int res = 0, tmp = 0;
>
> asm ("\n"
> "1: move.b (%0)+,%2\n" /* get *cs */
> " move.b (%1)+,%3\n" /* get *ct */
> " subx.l %3,%2\n" /* compare a byte */
> " jeq 2f\n" /* both inputs were zero */
That doesn't seem right.
Z will be set if either *ct is zero or the bytes match.
> " tst.l %2\n" /* check result */
This only sets Z when it was already set by the subx.
> " jeq 1b\n" /* if zero, continue */
> "2:"
> : "+a" (cs), "+a" (ct), "+d" (res), "+d" (tmp));
> return res;
> }
>
> However, this one needs res and tmp to be set to zero, because we read
> only bytes (no automatic zero-extend on 68k), but then do a long
> operation on them. Coldfire ISA_A dropped cmpb, it only reappeared in
> ISA_B.
>
> So the real instruction count is likely to be two more, unless gcc
> happens to have one or two zeros it can reuse.
>
> > I believe some of the other m68k asm functions are also missing
> > the "memory" 'clobber' and so could get mis-optimised.
>
> In which case would that happen? This function doesn't clobber memory
> and its result does get used. If gcc mistakenly thinks the parameters
> haven't changed and uses a previously cached result, wouldn't that
> apply to a C function too?
You need a memory 'clobber' on anything that READS memory as well
as writes it.
> > While I can write (or rather have written) m68k asm I don't have
> > a compiler.
>
> Well, I now have an emulated Quadra 800 running Debian 68k.(Getting the
> emulated networking to work reliably was a bit problematic, though. But
> now it runs Kernel 6.0) qemu could emulate Coldfire too, but I am not
> sure where I would find a distribution for that.
>
> I did not attach a patch because it seems already to be decided that
> the function is gone. But should anyone still want to include one (or
> both) of these functions, just give credit to me and I'm fine.
Thinking further the fastest strcmp() probably uses big-endian word compares
with a check for a zero byte.
Especially on 64 bit systems that support misaligned loads.
But I'd need to think hard about the actual details.
David
-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)
....
> > int strcmp1(const char *cs, const char *ct)
> > {
> > int res;
> >
> > asm ("\n"
> > "1: move.b (%0)+,%2\n" /* get *cs */
> > " jeq 2f\n" /* end of first string? */
> > " cmp.b (%1)+,%2\n" /* compare *ct */
> > " jeq 1b\n" /* if equal, continue */
> > " jra 3f\n" /* else skip to tail */
> > "2: cmp.b (%1)+,%2\n" /* compare one last byte */
> > "3: subx.l %2, %2\n" /* -1 if borrow, 0 if not */
> > " jls 4f\n" /* if set, z is from sub.b */
>
> The subx will set Z unless C was set.
> So that doesn't seem right.
Clearly my brain was asleep earlier.
subx will clear Z not set it.
David
-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)
Hi David,
On Fri, Dec 30, 2022 at 12:39 PM David Laight <[email protected]> wrote:
> Thinking further the fastest strcmp() probably uses big-endian word compares
> with a check for a zero byte.
> Especially on 64 bit systems that support misaligned loads.
> But I'd need to think hard about the actual details.
arch/arc/lib/strcmp-archs.S
arch/csky/abiv2/strcmp.S
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds