* Re: [PATCH] err_ptr.h: introduce ERR_PTR_SAFE()
2026-05-14 20:01 [PATCH] err_ptr.h: introduce ERR_PTR_SAFE() Amir Goldstein
@ 2026-05-15 12:25 ` Nirmoy Das
2026-05-15 13:15 ` Jori Koolstra
2026-05-15 18:30 ` David Laight
2 siblings, 0 replies; 5+ messages in thread
From: Nirmoy Das @ 2026-05-15 12:25 UTC (permalink / raw)
To: Amir Goldstein, Miklos Szeredi
Cc: Christian Brauner, Jan Kara, Al Viro, Linus Torvalds,
linux-unionfs, linux-fsdevel, linux-kernel
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@mail.gmail.com/
> Signed-off-by: Amir Goldstein <amir73il@gmail.com>
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@nvidia.com>
Acked-by: Nirmoy Das <nirmoyd@nvidia.com>
[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 */
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH] err_ptr.h: introduce ERR_PTR_SAFE()
2026-05-14 20:01 [PATCH] err_ptr.h: introduce ERR_PTR_SAFE() Amir Goldstein
2026-05-15 12:25 ` Nirmoy Das
@ 2026-05-15 13:15 ` Jori Koolstra
2026-05-15 18:30 ` David Laight
2 siblings, 0 replies; 5+ messages in thread
From: Jori Koolstra @ 2026-05-15 13:15 UTC (permalink / raw)
To: Amir Goldstein
Cc: Miklos Szeredi, Christian Brauner, Jan Kara, Al Viro,
Linus Torvalds, Nirmoy Das, linux-unionfs, linux-fsdevel,
linux-kernel
On Thu, May 14, 2026 at 10:01:29PM +0200, 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@mail.gmail.com/
> Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> ---
>
This seems like a good idea. Perhaps, we should also not pass error
pointers as parameters. It just invites similar issues. For example,
in
if (!error) {
dentry = vfs_mkdir(mnt_idmap(path.mnt), path.dentry->d_inode,
dentry, mode, &delegated_inode);
if (IS_ERR(dentry))
error = PTR_ERR(dentry);
}
end_creating_path(&path, dentry);
Suppose you want to make changes to end_creating_path, nothing indicates
that dentry may hold an error here. In this case it is not too hard to
find out, but it seems like a bad idea in general. But I might miss
cases where this is really useful... :)
> 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 */
> --
> 2.54.0
>
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH] err_ptr.h: introduce ERR_PTR_SAFE()
2026-05-14 20:01 [PATCH] err_ptr.h: introduce ERR_PTR_SAFE() Amir Goldstein
2026-05-15 12:25 ` Nirmoy Das
2026-05-15 13:15 ` Jori Koolstra
@ 2026-05-15 18:30 ` David Laight
2026-05-15 19:26 ` Amir Goldstein
2 siblings, 1 reply; 5+ messages in thread
From: David Laight @ 2026-05-15 18:30 UTC (permalink / raw)
To: Amir Goldstein
Cc: Miklos Szeredi, Christian Brauner, Jan Kara, Al Viro,
Linus Torvalds, Nirmoy Das, linux-unionfs, linux-fsdevel,
linux-kernel
On Thu, 14 May 2026 22:01:29 +0200
Amir Goldstein <amir73il@gmail.com> 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@mail.gmail.com/
> Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> ---
>
> 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);\
The object code bloat would be noticeable if this were used everywhere.
But you could make it a bit simpler:
if (__builtin_constant_p(__e))
BUILD_BUG_ON(__e && !IS_ERR_VALUE(__e));
else if WARN_ON(__e && !IS_ERR_VALUE(__e))
__e = -MAX_ERRNO; // Or maybe -EINVAL to stop and other boundary errors
(void *)__e;
The check for constants may be fairly pointless.
One of the static checkers may already detect the obvious fubar ERR_PTR(EINVAL).
-- David
> +})
> +
> +#endif /* _LINUX_ERR_PTR_H */
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH] err_ptr.h: introduce ERR_PTR_SAFE()
2026-05-15 18:30 ` David Laight
@ 2026-05-15 19:26 ` Amir Goldstein
0 siblings, 0 replies; 5+ messages in thread
From: Amir Goldstein @ 2026-05-15 19:26 UTC (permalink / raw)
To: David Laight
Cc: Miklos Szeredi, Christian Brauner, Jan Kara, Al Viro,
Linus Torvalds, Nirmoy Das, linux-unionfs, linux-fsdevel,
linux-kernel
On Fri, May 15, 2026 at 8:30 PM David Laight
<david.laight.linux@gmail.com> wrote:
>
> On Thu, 14 May 2026 22:01:29 +0200
> Amir Goldstein <amir73il@gmail.com> 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@mail.gmail.com/
> > Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> > ---
> >
> > 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);\
>
> The object code bloat would be noticeable if this were used everywhere.
> But you could make it a bit simpler:
> if (__builtin_constant_p(__e))
> BUILD_BUG_ON(__e && !IS_ERR_VALUE(__e));
> else if WARN_ON(__e && !IS_ERR_VALUE(__e))
> __e = -MAX_ERRNO; // Or maybe -EINVAL to stop and other boundary errors
> (void *)__e;
Yeh that's nicer thanks.
>
> The check for constants may be fairly pointless.
> One of the static checkers may already detect the obvious fubar ERR_PTR(EINVAL).
True, but I figured it didn't add much overhead if we are placing
the runtime assertions anyway?
Thanks,
Amir.
^ permalink raw reply [flat|nested] 5+ messages in thread