svn commit: r367130 - in head/sys: kern sys

[Ravi Pokala]

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-committers at freebsd.org> on behalf of Mateusz Guzik <mjg at FreeBSD.org>
Date: 2020-10-29, Thursday at 05:56
To: <src-committers at freebsd.org>, <svn-src-all at freebsd.org>, <svn-src-head at 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