Re: [PATCH] err_ptr.h: introduce ERR_PTR_SAFE()

[Nirmoy Das]

On 14.05.26 23:01, Amir Goldstein wrote:
> Code using ERR_PTR() is almost certainly intending to produce a value
> which qualified as IS_ERR_OR_NULL(), but this is not the case when
> code calls ERR_PTR(err) with positive or large negative err.
>
> Introduce a fortified variant of ERR_PTR() whose return value is
> guaranteed to qualify as IS_ERR_OR_NULL().
>
> We add this in a new header file err_ptr.h which includes bug.h
> for the build/run time assertions.
>
> Subsystems may opt-in for fortified ERR_PTR() for specific call sites
> or by #define ERR_PTR(err) ERR_PTR_SAFE(err).
>
> Link: https://lore.kernel.org/r/CAOQ4uxg=gONUh5QEW5KJcyXLDF15HbLnc9Ea7RKPcgtyfPasTA@xxxxxxxxxxxxxx/
> Signed-off-by: Amir Goldstein <amir73il@xxxxxxxxx>

I tested this on top of Amir's ovl-fixes branch[0], with overlayfs opted 
in to
ERR_PTR_SAFE() and with ovl_iterate_merged() fix reverted.

The syz reproducer triggered the new WARN_ON() from ERR_PTR_SAFE():
WARNING: fs/overlayfs/readdir.c:511 at ovl_iterate+0x4c0/0x5bc
 Call trace:
   ovl_iterate+0x4c0/0x5bc
   wrap_directory_iterator+0x60/0x90
   shared_ovl_iterate+0x18/0x24
   iterate_dir+0x10c/0x3a4
   __arm64_sys_getdents64+0xe0/0x1e4


Tested-by: Nirmoy Das <nirmoyd@xxxxxxxxxx>
Acked-by: Nirmoy Das <nirmoyd@xxxxxxxxxx>

[0] https://github.com/amir73il/linux/commits/ovl-fixes/

> ---
>
> Guys,
>
> Please follow the Link to see the sneaky bug that Nirmoy tracked down.
> syzbot has complained about this a while ago, but neither me nor my AI
> helpers were able to track it down from code analysis.
>
> Honestly, with AI review, this class of bugs (return a stale err value)
> should not be happening anymore, but it annoyed me that ERR_PTR() can
> return a value which is not an IS_ERR(). It messes with code flow
> analysis.
>
> What do you think about this macro?
>
> I intend to #define ERR_PTR(err) ERR_PTR_SAFE(err) in overlayfs.h
> to fortify all of the ERR_PTR() in overlayfs code.
>
> What do you think about this opt-in method?
> Any reason to make this more widespread by default?
>
> Thanks,
> Amir.
>
>
>   include/linux/err_ptr.h | 29 +++++++++++++++++++++++++++++
>   1 file changed, 29 insertions(+)
>   create mode 100644 include/linux/err_ptr.h
>
> diff --git a/include/linux/err_ptr.h b/include/linux/err_ptr.h
> new file mode 100644
> index 0000000000000..829ec5f771528
> --- /dev/null
> +++ b/include/linux/err_ptr.h
> @@ -0,0 +1,29 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +#ifndef _LINUX_ERR_PTR_H
> +#define _LINUX_ERR_PTR_H
> +
> +#include <linux/err.h>
> +#include <linux/bug.h>
> +
> +/**
> + * ERR_PTR_SAFE - Create an error pointer, with validation.
> + * @error: An error code to encode as an error pointer.
> + *
> + * Like ERR_PTR(), but validates @error:
> + * - For constant @error: fails the build if the value is not a valid errno
> + *   (zero is allowed, producing NULL).
> + * - For variable @error: warns and clamps to -MAX_ERRNO if out of range.
> + *
> + * Subsystems may opt in for all ERR_PTR() call sites by adding after includes:
> + *   #undef ERR_PTR
> + *   #define ERR_PTR(err) ERR_PTR_SAFE(err)
> + */
> +#define ERR_PTR_SAFE(error) ({						\
> +	long __e = (error);						\
> +	if (__builtin_constant_p(__e))					\
> +		BUILD_BUG_ON(__e && !IS_ERR_VALUE(__e));		\
> +	__builtin_constant_p(__e) ? (void *)__e :			\
> +		(void *)(WARN_ON(__e && !IS_ERR_VALUE(__e)) ? -MAX_ERRNO : __e);\
> +})
> +
> +#endif /* _LINUX_ERR_PTR_H */