Date: Sat, 16 Nov 2002 04:47:42 -0500 From: "Brian F. Feldman" <green@FreeBSD.org> To: Robert Watson <rwatson@FreeBSD.org> Cc: Brian Feldman <green@FreeBSD.org>, Perforce Change Reviews <perforce@FreeBSD.org> Subject: Re: PERFORCE change 21079 for review Message-ID: <200211160947.gAG9lgfB000914@green.bikeshed.org> In-Reply-To: Your message of "Fri, 15 Nov 2002 19:17:42 EST." <Pine.NEB.3.96L.1021115191246.62811D-100000@fledge.watson.org>
next in thread | previous in thread | raw e-mail | index | archive | help
Robert Watson <rwatson@FreeBSD.org> 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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?200211160947.gAG9lgfB000914>