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@)
-----Original Message-----
From: <owner-src-committ...@freebsd.org> on behalf of Mateusz Guzik
<m...@freebsd.org>
Date: 2020-10-29, Thursday at 05:56
To: <src-committ...@freebsd.org>, <svn-src-all@freebsd.org>,
<svn-src-h...@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 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
==============================================================================
--- 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 = &ndp->ni_cnd;
td = cnp->cn_thread;
#ifdef INVARIANTS
+ KASSERT((ndp->ni_debugflags & NAMEI_DBG_CALLED) == 0,
+ ("%s: repeated call to namei without NDREINIT", __func__));
+ KASSERT(ndp->ni_debugflags == NAMEI_DBG_INITED,
+ ("%s: bad debugflags %d", __func__, ndp->ni_debugflags));
+ ndp->ni_debugflags |= NAMEI_DBG_CALLED;
/*
* For NDVALIDATE.
*
Modified: head/sys/kern/vfs_vnops.c
==============================================================================
--- 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 = vn_start_write(NULL, &mp,
V_XSLEEP | PCATCH)) != 0)
return (error);
+ NDREINIT(ndp);
goto restart;
}
if ((vn_open_flags & VN_OPEN_NAMECACHE) != 0)
Modified: head/sys/sys/namei.h
==============================================================================
--- 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 =
NAMEI_DBG_INITED; }
+#define NDREINIT_DBG(arg) {
\
+ if (((arg)->ni_debugflags & NAMEI_DBG_INITED) == 0)
\
+ panic("namei data not inited");
\
+ if (((arg)->ni_debugflags & NAMEI_DBG_HADSTARTDIR) != 0)
\
+ panic("NDREINIT on namei data with NAMEI_DBG_HADSTARTDIR");
\
+ (arg)->ni_debugflags = 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 = (rightsp);
\
MPASS(_rightsp != NULL);
\
NDINIT_PREFILL(_ndp);
\
+ NDINIT_DBG(_ndp);
\
_ndp->ni_cnd.cn_nameiop = op;
\
_ndp->ni_cnd.cn_flags = flags;
\
_ndp->ni_segflg = segflg;
\
@@ -235,6 +258,13 @@ do {
\
filecaps_init(&_ndp->ni_filecaps);
\
_ndp->ni_cnd.cn_thread = td;
\
_ndp->ni_rightsneeded = _rightsp;
\
+} while (0)
+
+#define NDREINIT(ndp) do {
\
+ struct nameidata *_ndp = (ndp);
\
+ NDREINIT_DBG(_ndp);
\
+ _ndp->ni_resflags = 0;
\
+ _ndp->ni_startdir = NULL;
\
} while (0)
#define NDF_NO_DVP_RELE 0x00000001
_______________________________________________
svn-src-all@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-all
To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"
