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

NetBSD Home
NetBSD PR Database Search

(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.