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

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

The following reply was made to PR port-arm/50563; it has been noted by GNATS.

From: Frank Zerangue <frank.zerangue%gmail.com@localhost>
To: gnats-bugs%NetBSD.org@localhost
Cc: port-arm-maintainer%netbsd.org@localhost,
 gnats-admin%netbsd.org@localhost,
 netbsd-bugs%netbsd.org@localhost
Subject: Re: port-arm/50563: pool allocator corruption due to __MUTEX_PRIVATE
Date: Tue, 15 Dec 2015 11:01:33 -0600

 > 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.=20
 
 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:
 >=20
 > The following reply was made to PR port-arm/50563; it has been noted =
 by GNATS.
 >=20
 > From: christos%zoulas.com@localhost (Christos Zoulas)
 > To: gnats-bugs%NetBSD.org@localhost, port-arm-maintainer%netbsd.org@localhost,=20
 > 	gnats-admin%netbsd.org@localhost, netbsd-bugs%netbsd.org@localhost
 > Cc:=20
 > Subject: Re: port-arm/50563: pool allocator corruption due to =
 __MUTEX_PRIVATE
 > Date: Tue, 15 Dec 2015 09:53:20 -0500
 >=20
 > 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
 >=20
 > | 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.=20
 > |=20
 > | In the latter stage of initarm(), pmap_bootstrap() is called which =
 in turn calls pool_cache_bootstrap() with parameter palloc=3DNULL. Then =
 pool_cache_bootstrap assigns palloc =3D &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.=20
 > |=20
 > | The problem is that subr_pool.c includes sys/pool.h with =
 __MUTEX_PRIVATE not defined which yields a sizeof(pa.lock) =3D=3D 4 but =
 kern_mutex.c defines __MUTEX_PRIVATE and so sees the sizeof(pa_lock) =3D=3D=
  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.
 >=20
 > 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:
 >=20
 > CTASSERT(sizeof(uintptr_t) =3D=3D sizeof(struct kmutex));
 >=20
 > in the header file to enforce that.
 >=20
 > christos
 >=20
 
 Frank Zerangue=