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>