Date: Thu, 29 Oct 2020 19:44:16 +0100 From: Mateusz Guzik <mjguzik@gmail.com> To: Ravi Pokala <rpokala@freebsd.org> Cc: src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org Subject: Re: svn commit: r367130 - in head/sys: kern sys Message-ID: <CAGudoHH_Z13J0B5pkMkkEsN-8xmTYXFRn1PLzsnDvU4nWvLCBw@mail.gmail.com> In-Reply-To: <C6C04FFC-C7BE-4BBF-864B-8AABEB22C645@panasas.com> References: <202010291256.09TCu2BF025339@repo.freebsd.org> <C6C04FFC-C7BE-4BBF-864B-8AABEB22C645@panasas.com>
next in thread | previous in thread | raw e-mail | index | archive | help
Indeed! Fixed in r367144, thanks. On 10/29/20, Ravi Pokala <rpokala@freebsd.org> wrote: > Hi Mateusz, > > You define NAMEI_DBG_HADSTARTDIR, you check for it being set, but it does= n't > look like you actually set it anywhere...? > > Thanks, > > Ravi (rpokala@) > > =EF=BB=BF-----Original Message----- > From: <owner-src-committers@freebsd.org> on behalf of Mateusz Guzik > <mjg@FreeBSD.org> > Date: 2020-10-29, Thursday at 05:56 > To: <src-committers@freebsd.org>, <svn-src-all@freebsd.org>, > <svn-src-head@freebsd.org> > Subject: svn commit: r367130 - in head/sys: kern sys > > Author: mjg > Date: Thu Oct 29 12:56:02 2020 > New Revision: 367130 > URL: https://svnweb.freebsd.org/changeset/base/367130 > > Log: > vfs: add NDREINIT to facilitate repeated namei calls > > struct nameidata mixes caller arguments, internal state and output, > which > can be quite error prone. > > Recent addition of valdiating ni_resflags uncovered a caller which > could > repeatedly call namei, effectively operating on partially populated > state. > > Add bare minimium validation this does not happen. The real fix wou= ld > decouple aforementioned state. > > Reported by: pho > Tested by: pho (different variant) > > Modified: > head/sys/kern/vfs_lookup.c > head/sys/kern/vfs_vnops.c > head/sys/sys/namei.h > > Modified: head/sys/kern/vfs_lookup.c > > =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D > --- head/sys/kern/vfs_lookup.c Thu Oct 29 11:19:47 2020 (r367129) > +++ head/sys/kern/vfs_lookup.c Thu Oct 29 12:56:02 2020 (r367130) > @@ -502,6 +502,11 @@ namei(struct nameidata *ndp) > cnp =3D &ndp->ni_cnd; > td =3D cnp->cn_thread; > #ifdef INVARIANTS > + KASSERT((ndp->ni_debugflags & NAMEI_DBG_CALLED) =3D=3D 0, > + ("%s: repeated call to namei without NDREINIT", __func__)); > + KASSERT(ndp->ni_debugflags =3D=3D NAMEI_DBG_INITED, > + ("%s: bad debugflags %d", __func__, ndp->ni_debugflags)); > + ndp->ni_debugflags |=3D NAMEI_DBG_CALLED; > /* > * For NDVALIDATE. > * > > Modified: head/sys/kern/vfs_vnops.c > > =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D > --- head/sys/kern/vfs_vnops.c Thu Oct 29 11:19:47 2020 (r367129) > +++ head/sys/kern/vfs_vnops.c Thu Oct 29 12:56:02 2020 (r367130) > @@ -259,6 +259,7 @@ restart: > if ((error =3D vn_start_write(NULL, &mp, > V_XSLEEP | PCATCH)) !=3D 0) > return (error); > + NDREINIT(ndp); > goto restart; > } > if ((vn_open_flags & VN_OPEN_NAMECACHE) !=3D 0) > > Modified: head/sys/sys/namei.h > > =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D > --- head/sys/sys/namei.h Thu Oct 29 11:19:47 2020 (r367129) > +++ head/sys/sys/namei.h Thu Oct 29 12:56:02 2020 (r367130) > @@ -95,11 +95,15 @@ struct nameidata { > */ > u_int ni_resflags; > /* > + * Debug for validating API use by the callers. > + */ > + u_short ni_debugflags; > + /* > * Shared between namei and lookup/commit routines. > */ > + u_short ni_loopcnt; /* count of symlinks encountered */ > size_t ni_pathlen; /* remaining chars in path */ > char *ni_next; /* next location in pathname */ > - u_int ni_loopcnt; /* count of symlinks encountered */ > /* > * Lookup parameters: this structure describes the subset of > * information from the nameidata structure that is passed > @@ -122,7 +126,15 @@ int cache_fplookup(struct nameidata *ndp, enum > cache_f > * > * If modifying the list make sure to check whether NDVALIDATE needs > updating. > */ > + > /* > + * Debug. > + */ > +#define NAMEI_DBG_INITED 0x0001 > +#define NAMEI_DBG_CALLED 0x0002 > +#define NAMEI_DBG_HADSTARTDIR 0x0004 > + > +/* > * namei operational modifier flags, stored in ni_cnd.flags > */ > #define NC_NOMAKEENTRY 0x0001 /* name must not be added to cache */ > @@ -215,8 +227,18 @@ int cache_fplookup(struct nameidata *ndp, enum > cache_f > */ > #ifdef INVARIANTS > #define NDINIT_PREFILL(arg) memset(arg, 0xff, sizeof(*arg)) > +#define NDINIT_DBG(arg) { (arg)->ni_debugflags =3D NAMEI_DBG_INITED= ; } > +#define NDREINIT_DBG(arg) { \ > + if (((arg)->ni_debugflags & NAMEI_DBG_INITED) =3D=3D 0) \ > + panic("namei data not inited"); \ > + if (((arg)->ni_debugflags & NAMEI_DBG_HADSTARTDIR) !=3D 0) \ > + panic("NDREINIT on namei data with NAMEI_DBG_HADSTARTDIR"); \ > + (arg)->ni_debugflags =3D NAMEI_DBG_INITED; \ > +} > #else > #define NDINIT_PREFILL(arg) do { } while (0) > +#define NDINIT_DBG(arg) do { } while (0) > +#define NDREINIT_DBG(arg) do { } while (0) > #endif > > #define NDINIT_ALL(ndp, op, flags, segflg, namep, dirfd, startdir, > rightsp, td) \ > @@ -225,6 +247,7 @@ do { \ > cap_rights_t *_rightsp =3D (rightsp); \ > MPASS(_rightsp !=3D NULL); \ > NDINIT_PREFILL(_ndp); \ > + NDINIT_DBG(_ndp); \ > _ndp->ni_cnd.cn_nameiop =3D op; \ > _ndp->ni_cnd.cn_flags =3D flags; \ > _ndp->ni_segflg =3D segflg; \ > @@ -235,6 +258,13 @@ do { \ > filecaps_init(&_ndp->ni_filecaps); \ > _ndp->ni_cnd.cn_thread =3D td; \ > _ndp->ni_rightsneeded =3D _rightsp; \ > +} while (0) > + > +#define NDREINIT(ndp) do { \ > + struct nameidata *_ndp =3D (ndp); \ > + NDREINIT_DBG(_ndp); \ > + _ndp->ni_resflags =3D 0; \ > + _ndp->ni_startdir =3D NULL; \ > } while (0) > > #define NDF_NO_DVP_RELE 0x00000001 > > > --=20 Mateusz Guzik <mjguzik gmail.com>
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?CAGudoHH_Z13J0B5pkMkkEsN-8xmTYXFRn1PLzsnDvU4nWvLCBw>