Re: port-arm/50563: pool allocator corruption due to __MUTEX_PRIVATE

[Frank Zerangue <frank.zerangue%gmail.com@localhost>]

> Yes, if the types of ipl_cookie_t or __cpu_simple_lock_t are not defined
> to be 8 bits. Is that the case? We should add a:

Yes, that is the case. 

What is the need to define two different structs with the same name based upon which kernel file that happened to include it, as in this case struct kmutex? Two source files that share a type should have the same view of the type, yes?

We can guard this with the CTASSERT as you suggested below, but if they have to be 8 bits why defined them as a types ipl_cookie_t and _cpu_simple_lock_t, why not rather just defined them as 8 bit ints or char?

> On Dec 15, 2015, at 8:55 AM, Christos Zoulas <christos%zoulas.com@localhost> wrote:
> 
> The following reply was made to PR port-arm/50563; it has been noted by GNATS.
> 
> From: christos%zoulas.com@localhost (Christos Zoulas)
> To: gnats-bugs%NetBSD.org@localhost, port-arm-maintainer%netbsd.org@localhost, 
> 	gnats-admin%netbsd.org@localhost, netbsd-bugs%netbsd.org@localhost
> Cc: 
> Subject: Re: port-arm/50563: pool allocator corruption due to __MUTEX_PRIVATE
> Date: Tue, 15 Dec 2015 09:53:20 -0500
> 
> On Dec 15,  2:30pm, frank.zerangue%gmail.com@localhost (frank.zerangue%gmail.com@localhost) wrote:
> -- Subject: port-arm/50563: pool allocator corruption due to __MUTEX_PRIVATE
> 
> | Problem occurs on a private port of the arm architecture but should be problematic on others as well where the size of struct kmutex is different when __MUTEX_PRIVATE is defined or not. 
> | 
> | In the latter stage of initarm(), pmap_bootstrap() is called which in turn calls pool_cache_bootstrap() with parameter palloc=NULL. Then pool_cache_bootstrap assigns palloc = &pool_allocator_nointr then calls pool_init(). Pool_init() will initialize the pool_allocator_nointr.pa_list taill queue head by calling TAILQ_INIT(). This is then followed by a call to mutex_init() to initialize pool_allocator_nointr.pa_lock. 
> | 
> | The problem is that subr_pool.c includes sys/pool.h with __MUTEX_PRIVATE not defined which yields a sizeof(pa.lock) == 4 but kern_mutex.c defines __MUTEX_PRIVATE and so sees the sizeof(pa_lock) == 12. So when mutex_init() is called and does a memset(&pa_lock,0,sizeof(pa_lock)) it clears the pa_list tail queue head that follows it in struct pool_allocator.
> 
> Yes, if the types of ipl_cookie_t or __cpu_simple_lock_t are not defined
> to be 8 bits. Is that the case? We should add a:
> 
> CTASSERT(sizeof(uintptr_t) == sizeof(struct kmutex));
> 
> in the header file to enforce that.
> 
> christos
> 

Frank Zerangue