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@xxxxxxxxx> wrote:
>
> On Thu, 14 May 2026 22:01:29 +0200
> Amir Goldstein <amir73il@xxxxxxxxx> 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>
> > ---
> >
> > 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.