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

[Mateusz Guzik]

Indeed! Fixed in r367144, thanks.

On 10/29/20, Ravi Pokala <rpokala at freebsd.org> wrote:
> 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
>
>
>


-- 
Mateusz Guzik <mjguzik gmail.com>