From owner-p4-projects  Fri Nov 15 16:17:52 2002
Delivered-To: p4-projects@freebsd.org
Received: by hub.freebsd.org (Postfix, from userid 32767)
	id ECEB437B404; Fri, 15 Nov 2002 16:17:49 -0800 (PST)
Delivered-To: perforce@freebsd.org
Received: from mx1.FreeBSD.org (mx1.freebsd.org [216.136.204.125])
	by hub.freebsd.org (Postfix) with ESMTP
	id 94E8D37B401; Fri, 15 Nov 2002 16:17:49 -0800 (PST)
Received: from fledge.watson.org (fledge.watson.org [204.156.12.50])
	by mx1.FreeBSD.org (Postfix) with ESMTP
	id 908C343E77; Fri, 15 Nov 2002 16:17:48 -0800 (PST)
	(envelope-from robert@fledge.watson.org)
Received: from fledge.watson.org (fledge.pr.watson.org [192.0.2.3])
	by fledge.watson.org (8.12.6/8.12.5) with SMTP id gAG0HgBF061332;
	Fri, 15 Nov 2002 19:17:43 -0500 (EST)
	(envelope-from robert@fledge.watson.org)
Date: Fri, 15 Nov 2002 19:17:42 -0500 (EST)
From: Robert Watson <rwatson@FreeBSD.org>
X-Sender: robert@fledge.watson.org
To: Brian Feldman <green@FreeBSD.org>
Cc: Perforce Change Reviews <perforce@FreeBSD.org>
Subject: Re: PERFORCE change 21079 for review
In-Reply-To: <200211151843.gAFIhTNU019175@repoman.freebsd.org>
Message-ID: <Pine.NEB.3.96L.1021115191246.62811D-100000@fledge.watson.org>
MIME-Version: 1.0
Content-Type: TEXT/PLAIN; charset=US-ASCII
Sender: owner-p4-projects@FreeBSD.ORG
Precedence: bulk
List-ID: <p4-projects.FreeBSD.ORG>
List-Archive: <http://docs.freebsd.org/mail/> (Web Archive)
List-Help: <mailto:majordomo@FreeBSD.ORG?subject=help> (List Instructions)
List-Subscribe: <mailto:majordomo@FreeBSD.ORG?subject=subscribe%20p4-projects>
List-Unsubscribe: <mailto:majordomo@FreeBSD.ORG?subject=unsubscribe%20p4-projects>
X-Loop: FreeBSD.ORG


Generally looks good, but a couple of questions below.


On Fri, 15 Nov 2002, Brian Feldman wrote:

> 	Add three new checks for kernel modules:
> 		mac_check_kldload(cred, vnode)
> 		mac_check_kldunload(cred)
> 		mac_check_kldobserve(cred)

The naming seems a bit inconsistent here -- in some places it's in the
system namespace. I'd be tempted to rename them as:

	mac_check_kld_load()
	mac_check_kld_stat()
	mac_check_kld_unload()

>  int
> +mac_check_system_kldload(struct ucred *cred, struct vnode *vp)
> +{
> +	int error;
> +
> +	if (vp != NULL) {
> +		ASSERT_VOP_LOCKED(vp, "mac_check_system_acct");
> +	}

Two questions:

(1) Can vp ever be NULL here?  If so, when and why?
(2) Looks like a copy-and-paste-o: should be kldload not acct.

> +	if (!mac_enforce_system)
> +		return (0);

Adam's recent comments about "system" vs "kld" sound good.

> @@ -556,6 +558,13 @@
>      if (error)
>  	return error;
>      NDFREE(&nd, NDF_ONLY_PNBUF);
> +#ifdef MAC
> +    error = mac_check_system_kldload(curthread->td_ucred, nd.ni_vp);
> +    if (error) {
> +	firstpage = NULL;
> +	goto out;

It looks like you can only get here if the vn_open() succeeds, suggesting
vp will always be non-NULL.  If the goal is to leave the door open for
other potential sources of linker data later, I suggest we just handle
that case with a different entry point in the event that happens.

Robert N M Watson             FreeBSD Core Team, TrustedBSD Projects
robert@fledge.watson.org      Network Associates Laboratories


To Unsubscribe: send mail to majordomo@FreeBSD.org
with "unsubscribe p4-projects" in the body of the message