Date: Thu, 9 Jul 2015 00:07:07 +0200 From: Mateusz Guzik <mjguzik@gmail.com> To: Konstantin Belousov <kostikbel@gmail.com> Cc: rwatson@FreeBSD.org, freebsd-fs@freebsd.org, Mateusz Guzik <mjg@freebsd.org> Subject: [PATCH 0/4] namei + audit changes to prepare for getting rid of filedesc lock Message-ID: <1436393231-5831-1-git-send-email-mjguzik@gmail.com> In-Reply-To: <20150707085857.GZ2080@kib.kiev.ua> References: <20150707085857.GZ2080@kib.kiev.ua>
next in thread | previous in thread | raw e-mail | index | archive | help
From: Mateusz Guzik <mjg@freebsd.org>
On Tue, Jul 07, 2015 at 11:58:57AM +0300, Konstantin Belousov wrote:
> On Mon, Jul 06, 2015 at 05:07:14AM +0200, Mateusz Guzik wrote:
> > From: Mateusz Guzik <mjg@freebsd.org>
> >
> > namei used to vref fd_cdir, which was immediatley vrele'd on entry to
> > the loop.
> Does it make sense to do this, if the other patch, for interlock-less
> vref/vrele on holdcount > 0, is in progress ?
>
Well it is optional, but I would argue it makes the code more readable.
It also simplifies future code which may remove the need to vref root
vnode for lookups.
> >
> > Simplify error handling and remove type checking for ni_startdir vnode.
> > It is only set by nfs which does the check on its own. Assert the
> > correct type instead.
> > ---
> > sys/kern/vfs_lookup.c | 92 ++++++++++++++++++++++++++++-----------------------
> > 1 file changed, 51 insertions(+), 41 deletions(-)
> >
> > diff --git a/sys/kern/vfs_lookup.c b/sys/kern/vfs_lookup.c
> > index 5dc07dc..c5218ec 100644
> > --- a/sys/kern/vfs_lookup.c
> > +++ b/sys/kern/vfs_lookup.c
> > @@ -109,6 +109,27 @@ namei_cleanup_cnp(struct componentname *cnp)
> > #endif
> > }
> >
> > +static int
> > +namei_handle_root(struct nameidata *ndp, struct vnode **dpp)
> > +{
> > + struct componentname *cnp = &ndp->ni_cnd;
> > +
> > + if (ndp->ni_strictrelative != 0) {
> > +#ifdef KTRACE
> > + if (KTRPOINT(curthread, KTR_CAPFAIL))
> > + ktrcapfail(CAPFAIL_LOOKUP, NULL, NULL);
> > +#endif
> > + return (ENOTCAPABLE);
> > + }
> > + while (*(cnp->cn_nameptr) == '/') {
> > + cnp->cn_nameptr++;
> > + ndp->ni_pathlen--;
> > + }
> > + *dpp = ndp->ni_rootdir;
> > + VREF(*dpp);
> > + return (0);
> > +}
> > +
> > /*
> > * Convert a pathname into a pointer to a locked vnode.
> > *
> > @@ -148,6 +169,8 @@ namei(struct nameidata *ndp)
> > ("namei: nameiop contaminated with flags"));
> > KASSERT((cnp->cn_flags & OPMASK) == 0,
> > ("namei: flags contaminated with nameiops"));
> > + if (ndp->ni_startdir != NULL)
> > + MPASS(ndp->ni_startdir->v_type == VDIR);
> ni_startdir is not locked, am I correct ? If yes, the assert is not safe.
>
Added a || v_type == BAD check.
> > if (!lookup_shared)
> > cnp->cn_flags &= ~LOCKSHARED;
> > fdp = p->p_fd;
>
> Could this patch be further split ? E.g. could the introduction of the
> namei_handle_root() and its use twice be done in the first patch, while
> the loop logic reorganization come into the follow-up ?
>
> As it is now, the patch is almost impossible to review without rewriting
> the logic independently.
Patch split.
I completely forgot about a pre-existing bug with a use-after-free of
fd_rdir vnode when writing the previous patchset. see the first patch in
this one.
Mateusz Guzik (4):
vfs: plug a use-after-free of fd_rdir in namei
vfs: avoid spurious vref/vrele for absolute lookups
vfs: simplify error handling in namei
audit: utilize vnode pointer found by namei instead of looking it up
again
sys/kern/vfs_lookup.c | 127 +++++++++++++++++++++---------------
sys/security/audit/audit.h | 14 ++++
sys/security/audit/audit_arg.c | 36 ++++++++++
sys/security/audit/audit_bsm_klib.c | 82 +++++++++++++++--------
sys/security/audit/audit_private.h | 2 +
5 files changed, 181 insertions(+), 80 deletions(-)
--
2.4.5
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?1436393231-5831-1-git-send-email-mjguzik>