From owner-p4-projects Sat Nov 16 1:47:46 2002 Delivered-To: p4-projects@freebsd.org Received: by hub.freebsd.org (Postfix, from userid 32767) id 9132F37B404; Sat, 16 Nov 2002 01:47:43 -0800 (PST) Delivered-To: perforce@freebsd.org Received: from green.bikeshed.org (freefall.freebsd.org [216.136.204.21]) by hub.freebsd.org (Postfix) with ESMTP id E4C8937B401; Sat, 16 Nov 2002 01:47:42 -0800 (PST) Received: from green.bikeshed.org (wzyzd7ekkrsqv5e2@green.bikeshed.org [10.0.0.1] (may be forged)) by green.bikeshed.org (8.12.6/8.12.6) with ESMTP id gAG9lg56000918; Sat, 16 Nov 2002 04:47:42 -0500 (EST) (envelope-from green@green.bikeshed.org) Received: from localhost (green@localhost) by green.bikeshed.org (8.12.6/8.12.6/Submit) with ESMTP id gAG9lgfB000914; Sat, 16 Nov 2002 04:47:42 -0500 (EST) Message-Id: <200211160947.gAG9lgfB000914@green.bikeshed.org> X-Mailer: exmh version 2.5 07/13/2001 with nmh-1.0.4 To: Robert Watson Cc: Brian Feldman , Perforce Change Reviews Subject: Re: PERFORCE change 21079 for review In-Reply-To: Your message of "Fri, 15 Nov 2002 19:17:42 EST." From: "Brian F. Feldman" Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Date: Sat, 16 Nov 2002 04:47:42 -0500 Sender: owner-p4-projects@FreeBSD.ORG Precedence: bulk List-ID: List-Archive: (Web Archive) List-Help: (List Instructions) List-Subscribe: List-Unsubscribe: X-Loop: FreeBSD.ORG Robert Watson wrote: > > 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. No, you're right; it's of course always be non-NULL... The only potential sources of linker data are from that same function called recursively. The check shouldn't be done. -- Brian Fundakowski Feldman \'[ FreeBSD ]''''''''''\ <> green@FreeBSD.org <> bfeldman@tislabs.com \ The Power to Serve! \ Opinions expressed are my own. \,,,,,,,,,,,,,,,,,,,,,,\ To Unsubscribe: send mail to majordomo@FreeBSD.org with "unsubscribe p4-projects" in the body of the message