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>