Re: [PATCH] [RFC] instrumented.h: mark instrument_write non-const

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issues to consider:
- [Low] kasan: instrument_read_write() hides uninitialized variables
  under KASAN
- [Low] instrumented.h: inaccurate Fixes tag and RFC question in commit
  message
--

commit e49ee4fd80c81d953b77b9f3cd5a327db833a726
Author: Arnd Bergmann <arnd@arndb.de>

> instrumented.h: mark instrument_write non-const
>
> I got another instance of a -Wmaybe-uninitialized in ntfs, as this is
> the only place that enables the warning:
>
> In file included from include/asm-generic/rwonce.h:26,
>                  from fs/ntfs3/file.c:10:
> In function 'instrument_copy_from_user_before',
>     inlined from '_inline_copy_from_user' at include/linux/uaccess.h:184:2,
>     inlined from 'copy_from_user' at include/linux/uaccess.h:222:9,
>     inlined from 'ntfs_ioctl_fitrim' at fs/ntfs3/file.c:78:6,
>     inlined from 'ntfs_ioctl' at fs/ntfs3/file.c:246:10:
> include/linux/kasan-checks.h:38:27: error: 'range' may be used uninitialized [-Werror=maybe-uninitialized]
>    38 | #define kasan_check_write __kasan_check_write
> include/linux/instrumented.h:146:9: note: in expansion of macro 'kasan_check_write'
>   146 |         kasan_check_write(to, n);
>       |         ^~~~~~~~~~~~~~~~~
> include/linux/kasan-checks.h: In function 'ntfs_ioctl':
> include/linux/kasan-checks.h:20:6: note: by argument 1 of type 'const volatile void *' to '__kasan_check_write' declared here
>    20 | bool __kasan_check_write(const volatile void *p, unsigned int size);
>       |      ^~~~~~~~~~~~~~~~~~~
> fs/ntfs3/file.c:66:29: note: 'range' declared here
>    66 |         struct fstrim_range range;
>       |                             ^~~~~
>
> The problem here is that copy_from_user() is called on an uninitialized
> object, but the instrumentation passes the pointer into a function that
> takes a const object. gcc deduces that any access to that object is
> undefined behavior and warns about it.
>
> The only workaround I could come up with is to remove the 'const' keyword
> from these annotations, which of course is not great either, as the
> functions do not actually access the data at all.
>
> This also found incorrect 'const' annotations in
> arch/s390/include/asm/fpu-insn.h, which writes to those pointers.
>
> Any ideas?
>
> Fixes: 33b75c1d884e ("instrumented.h: allow instrumenting both sides of copy_from_user()")
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>

[Severity: Low]
33b75c1d884e ("instrumented.h: allow instrumenting both sides of
copy_from_user()") only renamed instrument_copy_from_user() to
instrument_copy_from_user_before().  The const void *to parameter calling
kasan_check_write(to, n) was introduced in 36e4d4dd4fc4 ("include/linux: Add
instrumented.h infrastructure"), and __kasan_check_write(const volatile void
*p, ...) was introduced in 7d8ad890dad0 ("mm/kasan: introduce
__kasan_check_{read,write}").  Also, the "Any ideas?" question from the RFC is
still in the commit message.

> diff --git a/include/linux/instrumented.h b/include/linux/instrumented.h
> index a1b4cf81adc2a..d28aa6b0aeb8c 100644
> --- a/include/linux/instrumented.h
> +++ b/include/linux/instrumented.h
> @@ -36,7 +36,7 @@ static __always_inline void instrument_read(const volatile void *v, size_t size)
>   * Instrument a regular write access. The instrumentation should be inserted
>   * before the actual write happens.
>   */
> -static __always_inline void instrument_write(const volatile void *v, size_t size)
> +static __always_inline void instrument_write(volatile void *v, size_t size)
>  {
>  	kasan_check_write(v, size);
>  	kcsan_check_write(v, size);
> @@ -50,7 +50,7 @@ static __always_inline void instrument_write(const volatile void *v, size_t size
>   * Instrument a regular write access. The instrumentation should be inserted
>   * before the actual write happens.
>   */
> -static __always_inline void instrument_read_write(const volatile void *v, size_t size)
> +static __always_inline void instrument_read_write(volatile void *v, size_t size)
>  {
>  	kasan_check_write(v, size);
>  	kcsan_check_read_write(v, size);
> @@ -92,7 +92,7 @@ static __always_inline void instrument_atomic_read(const volatile void *v, size_
>   * Instrument an atomic write access. The instrumentation should be inserted
>   * before the actual write happens.
>   */
> -static __always_inline void instrument_atomic_write(const volatile void *v, size_t size)
> +static __always_inline void instrument_atomic_write(volatile void *v, size_t size)
>  {
>  	kasan_check_write(v, size);
>  	kcsan_check_atomic_write(v, size);
> @@ -107,7 +107,7 @@ static __always_inline void instrument_atomic_write(const volatile void *v, size
>   * Instrument an atomic read-write access. The instrumentation should be
>   * inserted before the actual write happens.
>   */
> -static __always_inline void instrument_atomic_read_write(const volatile void *v, size_t size)
> +static __always_inline void instrument_atomic_read_write(volatile void *v, size_t size)
>  {
>  	kasan_check_write(v, size);
>  	kcsan_check_atomic_read_write(v, size);

[ ... ]

> diff --git a/include/linux/kasan-checks.h b/include/linux/kasan-checks.h
> index 3d6d22a25bdc3..90b878b876f5c 100644
> --- a/include/linux/kasan-checks.h
> +++ b/include/linux/kasan-checks.h
> @@ -17,13 +17,13 @@
>   */
>  #if defined(CONFIG_KASAN_GENERIC) || defined(CONFIG_KASAN_SW_TAGS)
>  bool __kasan_check_read(const volatile void *p, unsigned int size);
> -bool __kasan_check_write(const volatile void *p, unsigned int size);
> +bool __kasan_check_write(volatile void *p, unsigned int size);

[Severity: Low]
Does removing const from __kasan_check_write() suppress legitimate
-Wuninitialized and -Wmaybe-uninitialized warnings for callers of
instrument_read_write() and instrument_atomic_read_write() when KASAN is
enabled?

When a local variable's address is passed to an external function taking a
non-const pointer, GCC assumes the call may initialize the object.  Because
instrument_read_write() and instrument_atomic_read_write() call
kasan_check_write(v, size) before kcsan_check_read_write(v, size) and before
the caller's read-modify-write operation (such as oldp in try_cmpxchg()),
uninitialized read-modify-write operands will be treated as initialized by
__kasan_check_write().

07a1a6562ce2 ("kcsan: Silence -Wmaybe-uninitialized when calling
__kcsan_check_access()") avoided the same fs/ntfs3/file.c warning for KCSAN by
keeping const volatile void * on the external function and wrapping the
pointer in absolute_pointer() only in the write-only macros while leaving
read-write checks unwrapped.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261111092231.2027735-1-arnd@kernel.org?part=1