Re: [PATCH] slab: Redefine ZERO_SIZE_PTR to include ERR_PTR range

Re: [PATCH] slab: Redefine ZERO_SIZE_PTR to include ERR_PTR range

[Christopher Lameter]

On Thu, 10 Oct 2019, Shyam Saini wrote:

> This will help error related to ERR_PTR stand out better.

Maybe make ZERO_SIZE_PTR an ERRNO value instead? Then allow ERR_PTRs to be
used instead of ZERO_SIZE_PTRs

ERRNO_ZERO_OBJECT

or something like that?

>   */
> -#define ZERO_SIZE_PTR ((void *)16)

#define ZERO_SIZE_PTR ((void *)-ERRNO_ZERO_OBJECT)

> +
> +#define ZERO_OR_NULL_PTR(x) ((unsigned long)(x) - 1 >= \
> +		(unsigned long)ZERO_SIZE_PTR - 1)

And call this something different?

Re: [PATCH] slab: Redefine ZERO_SIZE_PTR to include ERR_PTR range

[Matthew Wilcox]

On Thu, Oct 10, 2019 at 02:22:40PM +0000, Christopher Lameter wrote:
> On Thu, 10 Oct 2019, Shyam Saini wrote:
> 
> > This will help error related to ERR_PTR stand out better.
> 
> Maybe make ZERO_SIZE_PTR an ERRNO value instead? Then allow ERR_PTRs to be
> used instead of ZERO_SIZE_PTRs
> 
> ERRNO_ZERO_OBJECT
> 
> or something like that?

I was wondering about something like that too, but allocating zero bytes
isn't actually an error, and if we have code that does something like:

	void *p = my_funky_alloc(size, ...);

	if (IS_ERR(p))
		return PTR_ERR(p);

then we might get this errno returned to userspace.

The change is definitely worth thinking about.

Re: [PATCH] slab: Redefine ZERO_SIZE_PTR to include ERR_PTR range

[Christopher Lameter]

On Thu, 10 Oct 2019, Matthew Wilcox wrote:

> I was wondering about something like that too, but allocating zero bytes
> isn't actually an error, and if we have code that does something like:

True. But it is in a greyzone. You cannot store anything in zero bytes
after all.

Re: [PATCH] slab: Redefine ZERO_SIZE_PTR to include ERR_PTR range

[Shyam Saini]

Hi Matthew, Christopher,

> > > This will help error related to ERR_PTR stand out better.
> >
> > Maybe make ZERO_SIZE_PTR an ERRNO value instead? Then allow ERR_PTRs to be
> > used instead of ZERO_SIZE_PTRs
> >
> > ERRNO_ZERO_OBJECT
> >
> > or something like that?
>
> I was wondering about something like that too, but allocating zero bytes
> isn't actually an error, and if we have code that does something like:
>
>         void *p = my_funky_alloc(size, ...);
>
>         if (IS_ERR(p))
>                 return PTR_ERR(p);
>
> then we might get this errno returned to userspace.
>
> The change is definitely worth thinking about.

Any further comments on this ?

Please let me know.

Thanks!!

Re: [PATCH] slab: Redefine ZERO_SIZE_PTR to include ERR_PTR range

[Jann Horn]

On Thu, Oct 10, 2019 at 12:32 PM Shyam Saini <mayhs11saini@gmail.com> wrote:
> Currently kfree does not accept ERR_PTR range so redefine ZERO_SIZE_PTR
> to include this and also change ZERO_OR_NULL_PTR macro to check this new
> range. With this change kfree will skip and behave as no-ops when ERR_PTR
> is passed.
>
> This will help error related to ERR_PTR stand out better.

What do you mean by "stand out better"? To me it sounds like before,
the kernel would probably blow up in some way if you passed an error
pointer into kfree(), and with this change, it will silently ignore it
instead, right? If you actually wanted this kind of error to stand
out, wouldn't it make more sense to add something like "if
(WARN_ON(IS_ERR(x))) return;" to the implementations of kfree()?
I would prefer that, since "kfree(<error pointer>)" probably indicates
that someone messed up their error handling jumps.

> After this, we don't need to reset any ERR_PTR variable to NULL before
> being passed to any kfree or related wrappers calls, as everything would
> be handled by ZERO_SIZE_PTR itself.

With the caveat that you still can't do it in code that might be
stable-backported, otherwise it will blow up occasionally in the rare
case where the error path is hit?

[...]
> +#define ZERO_SIZE_PTR                          \
> +({                                             \
> +       BUILD_BUG_ON(!(MAX_ERRNO & ~PAGE_MASK));\
> +       (void *)(-MAX_ERRNO-1L);                \
> +})
> +
> +#define ZERO_OR_NULL_PTR(x) ((unsigned long)(x) - 1 >= \
> +               (unsigned long)ZERO_SIZE_PTR - 1)

If you do go through with this change, you'll probably want to adjust
the message in check_bogus_address() - "null address" really isn't an
appropriate error message for an address near the end of the address
space.