Re: CVS commit: src/sys/kern

[Rin Okuyama <rokuyama.rk%gmail.com@localhost>]

Hi,

Running Linux binaries on amd64 hits this KASSERT:

sys/kern/vfs_lookup.c
https://nxr.netbsd.org/xref/src/sys/kern/vfs_lookup.c#592
   585          /*
   586           * Get a reference to the start dir so we can safely unlock cwdi.
   587           *
   588           * Must hold references to rootdir and erootdir while we're running.
   589           * A multithreaded process may chroot during namei.
   590           */
   591          vref(startdir);
> > > 592          KASSERT(! state->root_referenced);
   ...

This is screen shot of backtrace:
http://www.netbsd.org/~rin/backtrace_20190317.png

In a similar manner, panic occurs when running 32-bit binaries in
/emul/netbsd32.

If vfs_lookup.c is reverted to 1.208, the problem disappears.

Could you please look into that?

Thanks,
rin

On 2019/03/12 23:03, Juergen Hannken-Illjes wrote:
> Module Name:	src
> Committed By:	hannken
> Date:		Tue Mar 12 14:03:35 UTC 2019
>
> Modified Files:
> 	src/sys/kern: vfs_lookup.c
>
> Log Message:
> Take a reference on ndp->ni_rootdir and ndp->ni_erootdir.
>
> A multithreaded process may chroot during namei() and we end up with
> vn_under() trying to reference the now unreferenced ni_rootdir.
>
> Ok: David Holland <dholland%netbsd.org@localhost>
>
> Reported-by: syzbot+889319cdf91a3d0373a9%syzkaller.appspotmail.com@localhost
>
>
> To generate a diff of this commit:
> cvs rdiff -u -r1.208 -r1.209 src/sys/kern/vfs_lookup.c
>
> Please note that diffs are not public domain; they are subject to the
> copyright notices on the relevant files.
>
>
> Modified files:
>
> Index: src/sys/kern/vfs_lookup.c
> diff -u src/sys/kern/vfs_lookup.c:1.208 src/sys/kern/vfs_lookup.c:1.209
> --- src/sys/kern/vfs_lookup.c:1.208	Sun Jul  9 22:48:44 2017
> +++ src/sys/kern/vfs_lookup.c	Tue Mar 12 14:03:35 2019
> @@ -1,4 +1,4 @@
> -/*	$NetBSD: vfs_lookup.c,v 1.208 2017/07/09 22:48:44 dholland Exp $	*/
> +/*	$NetBSD: vfs_lookup.c,v 1.209 2019/03/12 14:03:35 hannken Exp $	*/
>   
>   /*
>    * Copyright (c) 1982, 1986, 1989, 1993
> @@ -37,7 +37,7 @@
>    */
>   
>   #include <sys/cdefs.h>
> -__KERNEL_RCSID(0, "$NetBSD: vfs_lookup.c,v 1.208 2017/07/09 22:48:44 dholland Exp $");
> +__KERNEL_RCSID(0, "$NetBSD: vfs_lookup.c,v 1.209 2019/03/12 14:03:35 hannken Exp $");
>   
>   #ifdef _KERNEL_OPT
>   #include "opt_magiclinks.h"
> @@ -469,6 +469,8 @@ struct namei_state {
>   	int slashes;
>   
>   	unsigned attempt_retry:1;	/* true if error allows emul retry */
> +	unsigned root_referenced:1;	/* true if ndp->ni_rootdir and
> +					     ndp->ni_erootdir were referenced */
>   };
>   
>   
> @@ -486,6 +488,8 @@ namei_init(struct namei_state *state, st
>   	state->rdonly = 0;
>   	state->slashes = 0;
>   
> +	state->root_referenced = 0;
> +
>   	KASSERTMSG((state->cnp->cn_cred != NULL), "namei: bad cred/proc");
>   	KASSERTMSG(((state->cnp->cn_nameiop & (~OPMASK)) == 0),
>   	    "namei: nameiop contaminated with flags: %08"PRIx32,
> @@ -510,8 +514,11 @@ namei_cleanup(struct namei_state *state)
>   {
>   	KASSERT(state->cnp == &state->ndp->ni_cnd);
>   
> -	/* nothing for now */
> -	(void)state;
> +	if (state->root_referenced) {
> +		vrele(state->ndp->ni_rootdir);
> +		if (state->ndp->ni_erootdir != NULL)
> +			vrele(state->ndp->ni_erootdir);
> +	}
>   }
>   
>   //////////////////////////////
> @@ -578,11 +585,15 @@ namei_getstartdir(struct namei_state *st
>   	/*
>   	 * Get a reference to the start dir so we can safely unlock cwdi.
>   	 *
> -	 * XXX: should we hold references to rootdir and erootdir while
> -	 * we're running? What happens if a multithreaded process chroots
> -	 * during namei?
> +	 * Must hold references to rootdir and erootdir while we're running.
> +	 * A multithreaded process may chroot during namei.
>   	 */
>   	vref(startdir);
> +	KASSERT(! state->root_referenced);
> +	vref(state->ndp->ni_rootdir);
> +	if (state->ndp->ni_erootdir != NULL)
> +		vref(state->ndp->ni_erootdir);
> +	state->root_referenced = 1;
>   
>   	rw_exit(&cwdi->cwdi_lock);
>   	return startdir;
> @@ -603,6 +614,9 @@ namei_getstartdir_for_nfsd(struct namei_
>   	state->ndp->ni_erootdir = NULL;
>   
>   	vref(state->ndp->ni_atdir);
> +	KASSERT(! state->root_referenced);
> +	vref(state->ndp->ni_rootdir);
> +	state->root_referenced = 1;
>   	return state->ndp->ni_atdir;
>   }
>