Re: [PATCH v5 1/3] VT: Use macros to define ioctls

[Al Viro]

On Wed, May 29, 2024 at 09:29:23AM +0200, Jiri Slaby wrote:
> On 18. 04. 24, 8:18, Greg Kroah-Hartman wrote:
> > On Wed, Apr 17, 2024 at 07:37:35PM +0200, Alexey Gladkov wrote:
> > > 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@xxxxxxxxxx>
> > > ---
> > >   include/uapi/linux/kd.h | 96 +++++++++++++++++++++--------------------
> > >   1 file changed, 49 insertions(+), 47 deletions(-)
> > 
> > This is a nice cleanup, thanks for doing it, I'll just take this one
> > change now if you don't object.
> 
> Unfortunately, _IOC_NONE is 1 on some archs as noted by Arnd, and this
> commit changed the kd ioctl values in there which broke stuff as noted by
> Al.
> 
> We either:
> * use _IOC(0, X, Y) in here, instead of _IO(X, Y), or
> * define KDIOC(X) as _IOC(0, KD_IOCTL_BASE, X), or
> * revert the commit which landed to -rc1 already.

Revert, IMO.  If nothing else, _IO... macros are there to
	* document the copyin/copyout direction
	* make sure that argument size mismatches between the kernel
and userland get caught

Turning 16bit hex constants into that won't serve any purpose other
than preventing such patches in the future; adding a comment would
deal with that just fine.

BTW, the original odd choice for _IOC_NONE appears to have been motivated
by avoiding conflicts with legacy ioctl numbers: in CSRG repo there's
this:
commit 02cbc6a653b48bb4ec6052cbe6b3979530011c46
Author: Sam Leffler <sam@xxxxxxxxxxxxxxxxxxx>
Date:   Mon Aug 2 02:22:17 1982 -0800

    new ioctl's

..

+/*
+ * Ioctl's have the command encoded in the lower word,
+ * and the size of any in or out parameters in the upper
+ * word.  The high 2 bits of the upper word are used
+ * to encode the in/out status of the parameter; for now
+ * we restrict parameters to at most 128 bytes.
+ */
+#define        IOCPARM_MASK    0x7f            /* parameters must be < 128 bytes */
+#define        IOC_VOID        0x20000000      /* no parameters */
+#define        IOC_OUT         0x40000000      /* copy out parameters */
+#define        IOC_IN          0x80000000      /* copy in parameters */
+#define        IOC_INOUT       (IOC_IN|IOC_OUT)
+/* the 0x20000000 is so we can distinguish new ioctl's from old */
+#define        _IO(x,y)        (IOC_VOID|('x'<<8)|y)
+#define        _IOR(x,y,t)     (IOC_OUT|((sizeof(t)&IOCPARM_MASK)<<16)|('x'<<8)|y)
+#define        _IOW(x,y,t)     (IOC_IN|((sizeof(t)&IOCPARM_MASK)<<16)|('x'<<8)|y)
+/* this should be _IORW, but stdio got there first */
+#define        _IOWR(x,y,t)    (IOC_INOUT|((sizeof(t)&IOCPARM_MASK)<<16)|('x'<<8)|y)

FWIW, the upper limit on parameter size went up from 128 bytes to 8Kb in

commit 856ed9ecd2795280feed42baf3889ac7c7f9a26d
Author: Mike Karels <karels@xxxxxxxxxxxxxxxxxxx>
Date:   Thu Feb 19 22:16:28 1987 -0800

    larger ioctl's (for disklabels)

Wonder what got us (in sparc port?) to lift it from 8Kb to 16Kb...
In our historical tree that has happened in
commit 1c29224f169362b671a4bef4cd864464ef810d42
Author: Pete Zaitcev <zaitcev@xxxxxxxxxx>
Date:   Sun Mar 2 08:45:12 2003 -0800

    [SPARC32/64]: Expand ioctl size field in backwards-compatible way.

No explanations there...