With the latest sparse, I do not see the error claimed by commit
d55875f5d52c ("include/asm-generic/ioctl.h: fix _IOC_TYPECHECK sparse
error").
Anyway, using BUILD_BUG_ON_ZERO() is clearer, and we do not need
to worry about sparse because BUILD_BUG_ON_ZERO() definition in
<linux/build_bug.h> is a constant zero when __CHECKER__ is defined.
Also, remove #ifndef __KERNEL__ from <uapi/asm-generic/ioctl.h>.
Signed-off-by: Masahiro Yamada <[email protected]>
---
include/asm-generic/ioctl.h | 12 ++++--------
include/uapi/asm-generic/ioctl.h | 13 ++++++-------
2 files changed, 10 insertions(+), 15 deletions(-)
diff --git a/include/asm-generic/ioctl.h b/include/asm-generic/ioctl.h
index 9fda9ed000cd..d5129d70ee1c 100644
--- a/include/asm-generic/ioctl.h
+++ b/include/asm-generic/ioctl.h
@@ -2,17 +2,13 @@
#ifndef _ASM_GENERIC_IOCTL_H
#define _ASM_GENERIC_IOCTL_H
+#include <linux/build_bug.h>
#include <uapi/asm-generic/ioctl.h>
-#ifdef __CHECKER__
-#define _IOC_TYPECHECK(t) (sizeof(t))
-#else
/* provoke compile error for invalid uses of size argument */
-extern unsigned int __invalid_size_argument_for_IOC;
+#undef _IOC_TYPECHECK
#define _IOC_TYPECHECK(t) \
- ((sizeof(t) == sizeof(t[1]) && \
- sizeof(t) < (1 << _IOC_SIZEBITS)) ? \
- sizeof(t) : __invalid_size_argument_for_IOC)
-#endif
+ BUILD_BUG_ON_ZERO(sizeof(t) != sizeof(t[1]) || \
+ sizeof(t) >= (1 << _IOC_SIZEBITS))
#endif /* _ASM_GENERIC_IOCTL_H */
diff --git a/include/uapi/asm-generic/ioctl.h b/include/uapi/asm-generic/ioctl.h
index a84f4db8a250..d50bd39ec3e3 100644
--- a/include/uapi/asm-generic/ioctl.h
+++ b/include/uapi/asm-generic/ioctl.h
@@ -72,9 +72,8 @@
((nr) << _IOC_NRSHIFT) | \
((size) << _IOC_SIZESHIFT))
-#ifndef __KERNEL__
-#define _IOC_TYPECHECK(t) (sizeof(t))
-#endif
+#define _IOC_TYPECHECK(t) 0
+#define _IOC_SIZE_WITH_TYPECHECK(t) (sizeof(t) + _IOC_TYPECHECK(t))
/*
* Used to create numbers.
@@ -82,10 +81,10 @@
* NOTE: _IOW means userland is writing and kernel is reading. _IOR
* means userland is reading and kernel is writing.
*/
-#define _IO(type,nr) _IOC(_IOC_NONE,(type),(nr),0)
-#define _IOR(type,nr,size) _IOC(_IOC_READ,(type),(nr),(_IOC_TYPECHECK(size)))
-#define _IOW(type,nr,size) _IOC(_IOC_WRITE,(type),(nr),(_IOC_TYPECHECK(size)))
-#define _IOWR(type,nr,size) _IOC(_IOC_READ|_IOC_WRITE,(type),(nr),(_IOC_TYPECHECK(size)))
+#define _IO(type,nr) _IOC(_IOC_NONE, type, nr, 0)
+#define _IOR(type,nr,size) _IOC(_IOC_READ, type, nr, _IOC_SIZE_WITH_TYPECHECK(size))
+#define _IOW(type,nr,size) _IOC(_IOC_WRITE, type, nr, _IOC_SIZE_WITH_TYPECHECK(size))
+#define _IOWR(type,nr,size) _IOC(_IOC_READ|_IOC_WRITE, type, nr, _IOC_SIZE_WITH_TYPECHECK(size))
#define _IOR_BAD(type,nr,size) _IOC(_IOC_READ,(type),(nr),sizeof(size))
#define _IOW_BAD(type,nr,size) _IOC(_IOC_WRITE,(type),(nr),sizeof(size))
#define _IOWR_BAD(type,nr,size) _IOC(_IOC_READ|_IOC_WRITE,(type),(nr),sizeof(size))
--
2.27.0
On Tue, Feb 23, 2021 at 11:06 AM Masahiro Yamada <[email protected]> wrote:
>
> -#ifdef __CHECKER__
> -#define _IOC_TYPECHECK(t) (sizeof(t))
> -#else
> /* provoke compile error for invalid uses of size argument */
> -extern unsigned int __invalid_size_argument_for_IOC;
> +#undef _IOC_TYPECHECK
> #define _IOC_TYPECHECK(t) \
> - ((sizeof(t) == sizeof(t[1]) && \
> - sizeof(t) < (1 << _IOC_SIZEBITS)) ? \
> - sizeof(t) : __invalid_size_argument_for_IOC)
> -#endif
> + BUILD_BUG_ON_ZERO(sizeof(t) != sizeof(t[1]) || \
> + sizeof(t) >= (1 << _IOC_SIZEBITS))
Using BUILD_BUG_ON_ZERO sounds like a good idea
> #endif /* _ASM_GENERIC_IOCTL_H */
> diff --git a/include/uapi/asm-generic/ioctl.h b/include/uapi/asm-generic/ioctl.h
> index a84f4db8a250..d50bd39ec3e3 100644
> --- a/include/uapi/asm-generic/ioctl.h
> +++ b/include/uapi/asm-generic/ioctl.h
> @@ -72,9 +72,8 @@
> ((nr) << _IOC_NRSHIFT) | \
> ((size) << _IOC_SIZESHIFT))
>
> -#ifndef __KERNEL__
> -#define _IOC_TYPECHECK(t) (sizeof(t))
> -#endif
> +#define _IOC_TYPECHECK(t) 0
> +#define _IOC_SIZE_WITH_TYPECHECK(t) (sizeof(t) + _IOC_TYPECHECK(t))
But I think replacing the #ifndef with an #undef in the other file makes it
harder to understand when reading through it and trying to understand
what it would do when this gets included from kernel and user space.
Arnd
On Wed, Feb 24, 2021 at 5:04 AM Arnd Bergmann <[email protected]> wrote:
>
> On Tue, Feb 23, 2021 at 11:06 AM Masahiro Yamada <[email protected]> wrote:
>
> >
> > -#ifdef __CHECKER__
> > -#define _IOC_TYPECHECK(t) (sizeof(t))
> > -#else
> > /* provoke compile error for invalid uses of size argument */
> > -extern unsigned int __invalid_size_argument_for_IOC;
> > +#undef _IOC_TYPECHECK
> > #define _IOC_TYPECHECK(t) \
> > - ((sizeof(t) == sizeof(t[1]) && \
> > - sizeof(t) < (1 << _IOC_SIZEBITS)) ? \
> > - sizeof(t) : __invalid_size_argument_for_IOC)
> > -#endif
> > + BUILD_BUG_ON_ZERO(sizeof(t) != sizeof(t[1]) || \
> > + sizeof(t) >= (1 << _IOC_SIZEBITS))
>
> Using BUILD_BUG_ON_ZERO sounds like a good idea
>
> > #endif /* _ASM_GENERIC_IOCTL_H */
> > diff --git a/include/uapi/asm-generic/ioctl.h b/include/uapi/asm-generic/ioctl.h
> > index a84f4db8a250..d50bd39ec3e3 100644
> > --- a/include/uapi/asm-generic/ioctl.h
> > +++ b/include/uapi/asm-generic/ioctl.h
> > @@ -72,9 +72,8 @@
> > ((nr) << _IOC_NRSHIFT) | \
> > ((size) << _IOC_SIZESHIFT))
> >
> > -#ifndef __KERNEL__
> > -#define _IOC_TYPECHECK(t) (sizeof(t))
> > -#endif
> > +#define _IOC_TYPECHECK(t) 0
> > +#define _IOC_SIZE_WITH_TYPECHECK(t) (sizeof(t) + _IOC_TYPECHECK(t))
>
> But I think replacing the #ifndef with an #undef in the other file makes it
> harder to understand when reading through it and trying to understand
> what it would do when this gets included from kernel and user space.
>
> Arnd
My intention is to improve the UAPI/KAPI decoupling
to decrease the task of scripts/headers_install.sh
Ideally, we could export UAPI headers with
almost no modification.
It is true that scripts/unifdef can remove #ifndef __KERNEL__
blocks, but having the kernel-space code in UAPI headers
does not make sense. Otherwise, our initial motivation
"separate them by directory structure" would be lost.
So, I believe redefining _IOC_TYPECHECK is the right direction.
I can add comments if this is not clear.
--
Best Regards
Masahiro Yamada
On Wed, Feb 24, 2021 at 2:57 AM Masahiro Yamada <[email protected]> wrote:
>
> On Wed, Feb 24, 2021 at 5:04 AM Arnd Bergmann <[email protected]> wrote:
> >
> > On Tue, Feb 23, 2021 at 11:06 AM Masahiro Yamada <[email protected]> wrote:
>
> My intention is to improve the UAPI/KAPI decoupling
> to decrease the task of scripts/headers_install.sh
>
> Ideally, we could export UAPI headers with
> almost no modification.
>
> It is true that scripts/unifdef can remove #ifndef __KERNEL__
> blocks, but having the kernel-space code in UAPI headers
> does not make sense. Otherwise, our initial motivation
> "separate them by directory structure" would be lost.
>
> So, I believe redefining _IOC_TYPECHECK is the right direction.
> I can add comments if this is not clear.
Maybe using '#ifndef _IOC_TYPECHECK' would help here?
Another alternative might be to find a way to rewrite the typecheck
macro to make it safe to be used in user space as well, and not
have two different versions.
Arnd