[PATCH v2 11/19] powerpc/rtas: add work area allocator

[Michael Ellerman]

Nathan Lynch <nathanl at linux.ibm.com> writes:
> Michael Ellerman <mpe at ellerman.id.au> writes:
>> Nathan Lynch via B4 Submission Endpoint
>> <devnull+nathanl.linux.ibm.com at kernel.org> writes:
...
>>> +struct rtas_work_area * __ref rtas_work_area_alloc(size_t size)
>>> +{
>>> +	struct rtas_work_area *area;
>>> +	unsigned long addr;
>>> +
>>> +	might_sleep();
>>> +
>>> +	WARN_ON(size > RTAS_WORK_AREA_MAX_ALLOC_SZ);
>>> +	size = min_t(size_t, size, RTAS_WORK_AREA_MAX_ALLOC_SZ);
>>
>> This seems unsafe.
>>
>> If you return a buffer smaller than the caller asks for they're likely
>> to read/write past the end of it and corrupt memory.
>
> OK, let's figure out another way to handle this.
>
>> AFAIK genalloc doesn't have guard pages or anything fancy to save us
>> from that - but maybe I'm wrong, I've never used it.
>
> Yeah we would have to build our own thing on top of it. And I don't
> think it could be something that traps on access, it would have to be a
> check in rtas_work_area_free(), after the fact.

I *think* we could use the MMU. We'd just have to allocate whole pages,
and then vmap() them (create a mapping in vmalloc space), and then give
the vmalloc space address back to the caller. They'd then operate on
that address, meaning any overflow would trap. You already have
rtas_work_area_phys() for passing the phys address to RTAS.

But that would be a lot more complicated than your suggestion below.

>> There's only three callers in the end, seems like we should just return
>> NULL if the size is too large and have callers check the return value.
>
> There are more conversions to do, and a property I hope to maintain is
> that requests can't fail. Existing users of rtas_data_buf don't have
> error paths for failure to acquire the buffer.
>
> I believe the allocation size passed to rtas_work_area_alloc() can be
> known at build time in all cases. Maybe we could prevent inappropriate
> requests from being built with a compile-time assertion (untested):
>
> /* rtas-work-area.h */
>
> static inline struct rtas_work_area *rtas_work_area_alloc(size_t sz)
> {
> 	static_assert(sz < RTAS_WORK_AREA_MAX_ALLOC_SZ);
>         return __rtas_work_area_alloc(sz);
> }
>
> I think this would be OK? If I can't make it work I'll fall back to
> returning NULL as you suggest, but it will make for more churn (and
> risk) in the conversions.

Yeah if the sizes are always known at compile time that is a much better
solution.

cheers