PROBLEM: kbd busted in linux 6.10-rc1 (regression)

PROBLEM: kbd busted in linux 6.10-rc1 (regression)

[Nick Bowler]

Hi,

It seems kbd utilities are all broken on linux 6.10-rc1.  For example:

   # loadkeys en-latin9
   Couldn't get a file descriptor referring to the console.
   [and it did not change the console keymap]

This is a regression from linux 6.9.  If I run strace on a working
kernel, at the point of the first difference we see:

   openat(AT_FDCWD, "/dev/tty0", O_RDWR)   = 3
   ioctl(3, TCGETS, {c_iflag=BRKINT|ICRNL|IXON|IMAXBEL|IUTF8, c_oflag=NL0|CR0|TAB0|BS0|VT0|FF0|OPOST|ONLCR, c_cflag=B38400|CS8|CREAD|HUPCL, c_lflag=, ...}) = 0
   ioctl(3, KDGKBTYPE, [KB_101])           = 0
   ...

On a busted kernel, we get:

   openat(AT_FDCWD, "/dev/tty0", O_RDWR)   = 3
   ioctl(3, TCGETS, {c_iflag=IUTF8, c_oflag=NL0|CR0|TAB0|BS0|VT0|FF0|OPOST|ONLCR, c_cflag=B38400|CS8|CREAD|HUPCL, c_lflag=, ...}) = 0
   ioctl(3, KDGKBTYPE, 0xffceaebb)         = -1 ENOTTY (Inappropriate ioctl for device)
   close(3)                                = 0
   ...

A very similar problem occurs with kbd_mode, chvt, etc.

Bisection implicates the following:

   8c467f3300591a206fa8dcc6988d768910799872 is the first bad commit
   commit 8c467f3300591a206fa8dcc6988d768910799872
   Author: Alexey Gladkov <legion@kernel.org>
   Date:   Wed Apr 17 19:37:35 2024 +0200
   
       VT: Use macros to define ioctls
   
       All other headers use _IOC() macros to describe ioctls for a long time
       now. This header is stuck in the last century.
   
       Simply use the _IO() macro. No other changes.
   
       Signed-off-by: Alexey Gladkov <legion@kernel.org>
       Link: https://lore.kernel.org/r/e4229fe2933a003341e338b558ab1ea8b63a51f6.1713375378.git.legion@kernel.org
       Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

Reverting this commit corrects the issue.

This testing was done on my sparc system in case it matters.

Let me know if you need any more info!

Thanks,
  Nick

Re: PROBLEM: kbd busted in linux 6.10-rc1 (regression)

[Al Viro]

On Wed, May 29, 2024 at 12:45:56AM -0400, Nick Bowler quoted:

>        All other headers use _IOC() macros to describe ioctls for a long time
>        now. This header is stuck in the last century.
>    
>        Simply use the _IO() macro. No other changes.

... are needed, since _IO() is arch-dependent; this is quite enough to fuck
alpha and sparc over.  _IO(x,y) is (1<<29) + 256*x + y there; both ports
got started with compat userland support, so _IO...() family there is
modelled after OSF/1 and Solaris resp.

kbd ioctls predate all of that.

Please, revert 8c467f330059 - commit in question breaks userland on alpha
and on sparc for no reason whatsoever.  Might be worth adding a comment
to those definitions at some point, but that can go on top of revert.

Folks, 0xXYZW is *not* an uncool way to spell _IO(0xXY,0xZW) - if there's
any chance that those definitions are seen on all architectures, they
should be left alone.

Re: PROBLEM: kbd busted in linux 6.10-rc1 (regression)

[Nick Bowler]

On 2024-05-29 01:25, Al Viro wrote:
> On Wed, May 29, 2024 at 12:45:56AM -0400, Nick Bowler quoted:
> 
>>        All other headers use _IOC() macros to describe ioctls for a long time
>>        now. This header is stuck in the last century.
>>    
>>        Simply use the _IO() macro. No other changes.
> 
> ... are needed, since _IO() is arch-dependent; this is quite enough to fuck
> alpha and sparc over.  _IO(x,y) is (1<<29) + 256*x + y there; both ports
> got started with compat userland support, so _IO...() family there is
> modelled after OSF/1 and Solaris resp.
>
> kbd ioctls predate all of that.
> 
> Please, revert 8c467f330059 - commit in question breaks userland on alpha
> and on sparc for no reason whatsoever.  Might be worth adding a comment
> to those definitions at some point, but that can go on top of revert.

FWIW I see exactly the same problem with 6.10-rc1 on powerpc too.

> Folks, 0xXYZW is *not* an uncool way to spell _IO(0xXY,0xZW) - if there's
> any chance that those definitions are seen on all architectures, they
> should be left alone.

Thanks,
  Nick

Re: PROBLEM: kbd busted in linux 6.10-rc1 (regression)

[Al Viro]

On Wed, May 29, 2024 at 01:36:28AM -0400, Nick Bowler wrote:
> On 2024-05-29 01:25, Al Viro wrote:
> > On Wed, May 29, 2024 at 12:45:56AM -0400, Nick Bowler quoted:
> > 
> >>        All other headers use _IOC() macros to describe ioctls for a long time
> >>        now. This header is stuck in the last century.
> >>    
> >>        Simply use the _IO() macro. No other changes.
> > 
> > ... are needed, since _IO() is arch-dependent; this is quite enough to fuck
> > alpha and sparc over.  _IO(x,y) is (1<<29) + 256*x + y there; both ports
> > got started with compat userland support, so _IO...() family there is
> > modelled after OSF/1 and Solaris resp.
> >
> > kbd ioctls predate all of that.
> > 
> > Please, revert 8c467f330059 - commit in question breaks userland on alpha
> > and on sparc for no reason whatsoever.  Might be worth adding a comment
> > to those definitions at some point, but that can go on top of revert.
> 
> FWIW I see exactly the same problem with 6.10-rc1 on powerpc too.
> 
> > Folks, 0xXYZW is *not* an uncool way to spell _IO(0xXY,0xZW) - if there's
> > any chance that those definitions are seen on all architectures, they
> > should be left alone.

arch/alpha/include/uapi/asm/ioctl.h:36:#define _IOC_NONE        1U
arch/mips/include/uapi/asm/ioctl.h:22:#define _IOC_NONE 1U
arch/powerpc/include/uapi/asm/ioctl.h:8:#define _IOC_NONE       1U
arch/sparc/include/uapi/asm/ioctl.h:35:#define _IOC_NONE        1U
include/uapi/asm-generic/ioctl.h:57:#ifndef _IOC_NONE
include/uapi/asm-generic/ioctl.h:58:# define _IOC_NONE  0U

FWIW, ioctl number is bits 0..7 and type - 8..15 on everything.
The fun is in upper 16 bits:

alpha, powerpc, mips - bits 29..31 are for direction (001 - none,
010 - read, 100 - write, 110 - read/write) and bits 28..16 are
for argument size.

sparc - bits 29..31 are for direction (001 - none, 010 - read,
100 - write, 110 - read/write) and bits 29..16 are used for
for argument size if bit 30 or bit 31 are set (i.e. when it's
not "none").  Uses the fact that "none" does not combine with
"read" or "write", so we can treat 011.... as "write with argument
size in range 8K..16K".

everything else - bits 30..31 are for direction (again, bit 30
is for read, bit 31 - write), bits 29..16 for size.

You get the arch-independent values from _IOR, _IOW and _IOWR
(with argument size limited by 8Kb on alpha, powerpc and mips,
and by 16Kb everywhere).  Upper halfword in range 0xc000--0xffff 
is read/write, 0x8000--0xbfff - write, 0x4000--0x7fff - read.

_IO, however, is arch-dependent - you get 0x2000 in upper halfword
on alpha, powerpc, mips and sparc and 0 on everything else.

Rationale: compatibility with definitions on other Unices on
the same platform; not sure about powerpc, but alpha, mips
and sparc ports used to have binary compatibility with OSF/1,
IRIX and Solaris resp.  Incomplete, but having compatible
ioctl numbers layout avoided a lot of needless PITA...

Re: PROBLEM: kbd busted in linux 6.10-rc1 (regression)

[Al Viro]

On Wed, May 29, 2024 at 07:36:48AM +0100, Al Viro wrote:
> not "none").  Uses the fact that "none" does not combine with
> "read" or "write", so we can treat 011.... as "write with argument
                                                 ^^^^^ read, that is.
> size in range 8K..16K".

Re: PROBLEM: kbd busted in linux 6.10-rc1 (regression)

[Greg Kroah-Hartman]

On Wed, May 29, 2024 at 06:25:43AM +0100, Al Viro wrote:
> On Wed, May 29, 2024 at 12:45:56AM -0400, Nick Bowler quoted:
> 
> >        All other headers use _IOC() macros to describe ioctls for a long time
> >        now. This header is stuck in the last century.
> >    
> >        Simply use the _IO() macro. No other changes.
> 
> ... are needed, since _IO() is arch-dependent; this is quite enough to fuck
> alpha and sparc over.  _IO(x,y) is (1<<29) + 256*x + y there; both ports
> got started with compat userland support, so _IO...() family there is
> modelled after OSF/1 and Solaris resp.
> 
> kbd ioctls predate all of that.
> 
> Please, revert 8c467f330059 - commit in question breaks userland on alpha
> and on sparc for no reason whatsoever.  Might be worth adding a comment
> to those definitions at some point, but that can go on top of revert.
> 
> Folks, 0xXYZW is *not* an uncool way to spell _IO(0xXY,0xZW) - if there's
> any chance that those definitions are seen on all architectures, they
> should be left alone.
> 

Ick, I missed that and to be fair, I should have remembered that and not
taken this commit.  I'll get this reverted later tonight when I get back
to my development systems.

thanks,

greg k-h