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

[David Laight <david.laight.linux@gmail.com>]

On Mon, 18 May 2026 14:52:05 +0200
Amir Goldstein <amir73il@gmail.com> wrote:

> On Mon, May 18, 2026 at 2:39 PM Christian Brauner <brauner@kernel.org> wrote:
> >
> > 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>
> > > ---  
> >
> > I think this is backwards. You can of course do whatever you want in
> > overlayfs but I think as a first-class concept in err.h it's not that
> > great. Then we have to separate macros/inlines and almost no one will
> > use ERR_PTR_SAFE().
> >
> > I think the correct thing would be to add an assert into ERR_PTR() and a
> > debug-only one very likely at that and combine this with the static
> > analyzer thing mentioned in the thread below.
> >
> > diff --git a/include/linux/err.h b/include/linux/err.h
> > index 8c37be0620ab..6bf768adf157 100644
> > --- a/include/linux/err.h
> > +++ b/include/linux/err.h
> > @@ -38,6 +38,9 @@
> >   */
> >  static inline void * __must_check ERR_PTR(long error)
> >  {
> > +#ifdef CONFIG_DEBUG_INFO
> > +       WARN_ON_ONCE(!IS_ERR_VALUE(error));

If you do this, you need to change the value.
Even if just 'error | PAGE_MASK' (and check for zero).

You could just do:
	return error ? (void *)(error | ~4095ul) : NULL;
or (avoiding the conditional):
	return ((error - 1) | ~4095ul) + 1;

That will ensure the value is treated as an error value.

> > +#endif
> >         return (void *) error;
> >  }
> >
> > I'm not convinced yet about ERR_PTR_SAFE().  
> 
> Maybe, but
> 1. err.h does not include bug.h, hence err_ptr.h
> 2. I think CONFIG_DEBUG_INFO is pretty common in distro kernels
> and it means its ok to bloat the kernel object sizes
> but it does not mean distro is willing to pay performance penalty
> but David may be able to share more insight about the performance
> cost of your suggested variant.

The check itself (on x86) should just be 'add $4095, error; jnc bad'.
WARN_ON() and BUG_ON() are implemented using 'ud2' (undefined opcode)
and picking up the info from the exception table.
I can't remember about WARN_ON_ONCE() - there has to be some writable
global data somewhere.
But other architectures end up adding more in line code.
In particular anything that adds a function call can badly affect
register allocation and code generation.

Not the least of the problems is the sheer number of places where
extra code can get added - especially for something that static analysis
ought to be able to detect.

-- David

> 
> Thanks,
> Amir.