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

[Jann Horn <jannh@google.com>]

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.