PERFORCE change 21079 for review

[Brian F. Feldman]

Robert Watson <rwatson at 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 at FreeBSD.org  <> bfeldman at tislabs.com      \  The Power to Serve! \
 Opinions expressed are my own.                       \,,,,,,,,,,,,,,,,,,,,,,\


To Unsubscribe: send mail to majordomo at trustedbsd.org
with "unsubscribe trustedbsd-cvs" in the body of the message