NetBSD Problem Report #50563
From www@NetBSD.org Tue Dec 15 14:28:53 2015
Return-Path: <www@NetBSD.org>
Received: from mail.netbsd.org (mail.NetBSD.org [199.233.217.200])
(using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits))
(Client CN "mail.netbsd.org", Issuer "Postmaster NetBSD.org" (verified OK))
by mollari.NetBSD.org (Postfix) with ESMTPS id 284D17ACA4
for <gnats-bugs@gnats.NetBSD.org>; Tue, 15 Dec 2015 14:28:53 +0000 (UTC)
Message-Id: <20151215142849.825147ACCB@mollari.NetBSD.org>
Date: Tue, 15 Dec 2015 14:28:49 +0000 (UTC)
From: frank.zerangue@gmail.com
Reply-To: frank.zerangue@gmail.com
To: gnats-bugs@NetBSD.org
Subject: pool allocator corruption due to __MUTEX_PRIVATE
X-Send-Pr-Version: www-1.0
>Number: 50563
>Category: port-arm
>Synopsis: pool allocator corruption due to __MUTEX_PRIVATE
>Confidential: no
>Severity: serious
>Priority: medium
>Responsible: port-arm-maintainer
>State: open
>Class: sw-bug
>Submitter-Id: net
>Arrival-Date: Tue Dec 15 14:30:00 +0000 2015
>Last-Modified: Wed Dec 16 13:25:02 +0000 2015
>Originator: Frank Zerangue
>Release: NetBSD-7.0-RELEASE
>Organization:
>Environment:
Darwin Franks-Mac-Pro-3.local 14.5.0 Darwin Kernel Version 14.5.0: Wed Jul 29 02:26:53 PDT 2015; root:xnu-2782.40.9~1/RELEASE_X86_64 x86_64
>Description:
usr/src/sys/kern/subr_pool.c
usr/src/sys/kern/kern_mutex.c
usr/src/sys/arch/arm/arm32/pmap.c
pool_allocator_nointr.pa_list is corrupted by mutex_init():
CPU_ARM11
ARM_MMU_V6N defined
ARM_MMU_EXTENDED defined
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.
>How-To-Repeat:
>Fix:
>Audit-Trail:
From: christos@zoulas.com (Christos Zoulas)
To: gnats-bugs@NetBSD.org, port-arm-maintainer@netbsd.org,
gnats-admin@netbsd.org, netbsd-bugs@netbsd.org
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 (frank.zerangue@gmail.com) 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
From: Frank Zerangue <frank.zerangue@gmail.com>
To: gnats-bugs@NetBSD.org
Cc: port-arm-maintainer@netbsd.org,
gnats-admin@netbsd.org,
netbsd-bugs@netbsd.org
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> =
wrote:
>=20
> The following reply was made to PR port-arm/50563; it has been noted =
by GNATS.
>=20
> From: christos@zoulas.com (Christos Zoulas)
> To: gnats-bugs@NetBSD.org, port-arm-maintainer@netbsd.org,=20
> gnats-admin@netbsd.org, netbsd-bugs@netbsd.org
> 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 =
(frank.zerangue@gmail.com) 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=
From: christos@zoulas.com (Christos Zoulas)
To: gnats-bugs@NetBSD.org, port-arm-maintainer@netbsd.org,
gnats-admin@netbsd.org, netbsd-bugs@netbsd.org,
frank.zerangue@gmail.com
Cc:
Subject: Re: port-arm/50563: pool allocator corruption due to __MUTEX_PRIVATE
Date: Tue, 15 Dec 2015 12:34:19 -0500
On Dec 15, 5:05pm, frank.zerangue@gmail.com (Frank Zerangue) wrote:
-- Subject: Re: port-arm/50563: pool allocator corruption due to __MUTEX_PRIV
| 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?
Because if we did, the result would be even worse if the size of those
types changed. You would end up storing truncated versions of them in
the struct since C does not by default warn you about size mismatches
in assignments. I think that the better solution is to add the CTASSERT
so that one is notified that something needs to change...
christos
From: matthew green <mrg@eterna.com.au>
To: christos@zoulas.com (Christos Zoulas)
Cc: gnats-bugs@NetBSD.org, port-arm-maintainer@netbsd.org,
gnats-admin@netbsd.org, netbsd-bugs@netbsd.org
Subject: re: port-arm/50563: pool allocator corruption due to __MUTEX_PRIVATE
Date: Wed, 16 Dec 2015 20:34:57 +1100
> 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.
that doesn't seem sane to me. or even valid. alpha has a uint32 and
a uintprt_t.
struct kmutex is MD-defined. i don't know of a good way to check
this without a helper. we have to compile machine/mutex.h with and
without __MUTEX_PRIVATE defined, and compare their sizes. it seems
way to ugly to try to do this in a single C file (you'd have to
#undef all the sys/arch/foo/include/mutex.h's #ifdef _FOO_MUTEX_H,
but it would work i think...)
.mrg.
From: christos@zoulas.com (Christos Zoulas)
To: matthew green <mrg@eterna.com.au>
Cc: gnats-bugs@NetBSD.org, port-arm-maintainer@netbsd.org,
gnats-admin@netbsd.org, netbsd-bugs@netbsd.org
Subject: re: port-arm/50563: pool allocator corruption due to __MUTEX_PRIVATE
Date: Wed, 16 Dec 2015 08:24:38 -0500
On Dec 16, 8:34pm, mrg@eterna.com.au (matthew green) wrote:
-- Subject: re: port-arm/50563: pool allocator corruption due to __MUTEX_PRIV
| > 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.
|
| that doesn't seem sane to me. or even valid. alpha has a uint32 and
| a uintprt_t.
|
| struct kmutex is MD-defined. i don't know of a good way to check
| this without a helper. we have to compile machine/mutex.h with and
| without __MUTEX_PRIVATE defined, and compare their sizes. it seems
| way to ugly to try to do this in a single C file (you'd have to
| #undef all the sys/arch/foo/include/mutex.h's #ifdef _FOO_MUTEX_H,
| but it would work i think...)
That CTASSERT goes in the MD file.
christos
(Contact us)
$NetBSD: query-full-pr,v 1.39 2013/11/01 18:47:49 spz Exp $
$NetBSD: gnats_config.sh,v 1.8 2006/05/07 09:23:38 tsutsui Exp $
Copyright © 1994-2014
The NetBSD Foundation, Inc. ALL RIGHTS RESERVED.