[PATCH] err_ptr.h: introduce ERR_PTR_SAFE()

[PATCH] err_ptr.h: introduce ERR_PTR_SAFE()

[Amir Goldstein]

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);\
+})
+
+#endif /* _LINUX_ERR_PTR_H */
-- 
2.54.0

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@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 */

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

[Jori Koolstra]

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
> 

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

[David Laight]

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 */

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

[Amir Goldstein]

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.

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

[David Laight]

On Fri, 15 May 2026 21:26:04 +0200
Amir Goldstein <amir73il@gmail.com> wrote:

> 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:
> >  
...
> >
> > 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.

Actually this might be better still (or just more succinct):
	void *__e = (void *)error;
	BUILD_BUG_ON(!statically_true(IS_ERR_OR_NULL(__e));
	if (WARN_ON(!IS_ERR_OR_NULL(__e))
		__e = (void *)-EINVAL;
	__e;

The WARN_ON() will be optimised away (valid) constants.

> > 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?

The 'size' check in READ_ONCE() measurably slows down the kernel compilation.
But there are a lot of those.
I think I know the main reason - the extern for the 'error message function'.

-- David

> 
> Thanks,
> Amir.

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

[Amir Goldstein]

On Sat, May 16, 2026 at 10:42 AM David Laight
<david.laight.linux@gmail.com> wrote:
>
> On Fri, 15 May 2026 21:26:04 +0200
> Amir Goldstein <amir73il@gmail.com> wrote:
>
> > 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:
> > >
> ...
> > >
> > > 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.
>
> Actually this might be better still (or just more succinct):
>         void *__e = (void *)error;
>         BUILD_BUG_ON(!statically_true(IS_ERR_OR_NULL(__e));

This condition is wrong but also my compiler does not evaluate
__builtin_constant_p(IS_ERR_OR_NULL(__e)) as true.

This works
BUILD_BUG_ON(statically_true(!IS_ERR_VALUE(__e)));

I think it is enough to statically assert on ERR_PTR(EINVAL)
and no need to bother with ERR_PTR(0)

>         if (WARN_ON(!IS_ERR_OR_NULL(__e))
>                 __e = (void *)-EINVAL;

Oh, anything but EINVAL please - the most overloaded error value
My choice of meaningful error value would be EFAULT
because without the safe helper we would be returning an address
which is in most likelihood bad, so better be explicit about it.

>         __e;
>
> The WARN_ON() will be optimised away (valid) constants.
>

Yeh this looks nice I'll use this:

#define ERR_PTR_SAFE(error) ({                                  \
        void *__e = (void *)(long)(error);                      \
        BUILD_BUG_ON(statically_true(!IS_ERR_VALUE(__e)));      \
        if (WARN_ON(!IS_ERR_OR_NULL(__e)))                      \
                __e = (void *)(long)-EFAULT;                    \
        __e;                                                    \
})


Thanks!
Amir.

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

[David Laight]

On Sat, 16 May 2026 13:39:11 +0200
Amir Goldstein <amir73il@gmail.com> wrote:

> On Sat, May 16, 2026 at 10:42 AM David Laight
> <david.laight.linux@gmail.com> wrote:
> >
> > On Fri, 15 May 2026 21:26:04 +0200
> > Amir Goldstein <amir73il@gmail.com> wrote:
> >  
> > > 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:
> > > >  
> > ...  
> > > >
> > > > 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.  
> >
> > Actually this might be better still (or just more succinct):
> >         void *__e = (void *)error;
> >         BUILD_BUG_ON(!statically_true(IS_ERR_OR_NULL(__e));  
> 
> This condition is wrong but also my compiler does not evaluate
> __builtin_constant_p(IS_ERR_OR_NULL(__e)) as true.
> 
> This works
> BUILD_BUG_ON(statically_true(!IS_ERR_VALUE(__e)));

Yes, it is easy to get those wrong - especially when typing quickly.

> 
> I think it is enough to statically assert on ERR_PTR(EINVAL)
> and no need to bother with ERR_PTR(0)

Then the tests don't match - which looks funny.
IS_ERR_VALUE(val) should be: val += 4095; jump_carry ...
and IS_ERR_OR_NULL(val): val--; val += 4096; jump_carry ...
but I can't remember whether gcc manages to do that.

> 
> >         if (WARN_ON(!IS_ERR_OR_NULL(__e))
> >                 __e = (void *)-EINVAL;  
> 
> Oh, anything but EINVAL please - the most overloaded error value
> My choice of meaningful error value would be EFAULT
> because without the safe helper we would be returning an address
> which is in most likelihood bad, so better be explicit about it.

I'm not sure about EFAULT; it is only really used for user copy failures.
IIRC at least one Unix (I've forgotten which) generates SIGSEGV when a
system call return of EFAULT.

There is also the 'problem' of PANIC_ON_WARN which is set by a lot
of distributions.
That (sort of) means than you might as well use BUG_ON() and get the
associated slightly smaller code size.
On x86-64 (and maybe a few others) both BUG_ON() and WARN_ON() just
execute UD2 (an undefined instruction) and the trap handler finds the
associated info and does the printk().
That makes the code smaller than pr_warn().
Someone needs to add a 'I_REALLY_MEAN_WARN_ON()' that never panics.
(And maybe with an option to not dump all the stack.)

-- David

> 
> >         __e;
> >
> > The WARN_ON() will be optimised away (valid) constants.
> >  
> 
> Yeh this looks nice I'll use this:
> 
> #define ERR_PTR_SAFE(error) ({                                  \
>         void *__e = (void *)(long)(error);                      \
>         BUILD_BUG_ON(statically_true(!IS_ERR_VALUE(__e)));      \
>         if (WARN_ON(!IS_ERR_OR_NULL(__e)))                      \
>                 __e = (void *)(long)-EFAULT;                    \
>         __e;                                                    \
> })
> 
> 
> Thanks!
> Amir.