Date: Thu, 29 Oct 2020 11:35:11 -0700 From: Ravi Pokala <rpokala@freebsd.org> To: Mateusz Guzik <mjg@FreeBSD.org>, <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: <C6C04FFC-C7BE-4BBF-864B-8AABEB22C645@panasas.com> In-Reply-To: <202010291256.09TCu2BF025339@repo.freebsd.org> References: <202010291256.09TCu2BF025339@repo.freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
Hi Mateusz, You define NAMEI_DBG_HADSTARTDIR, you check for it being set, but it doesn'= 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@Fr= eeBSD.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, w= hich can be quite error prone. Recent addition of valdiating ni_resflags uncovered a caller which co= uld repeatedly call namei, effectively operating on partially populated s= tate. Add bare minimium validation this does not happen. The real fix would 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 cac= he_f * * If modifying the list make sure to check whether NDVALIDATE needs u= pdating. */ + /* + * 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 cac= he_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, rig= htsp, 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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?C6C04FFC-C7BE-4BBF-864B-8AABEB22C645>