Re: r340343 triggers kernel assertion if file is opened with O_BENEATH flag set through symlink

On Wed, Nov 28, 2018 at 10:50:59AM +0100, Peter Holm wrote:
> On Wed, Nov 28, 2018 at 01:46:17AM +0200, Konstantin Belousov wrote:
> > On Wed, Nov 28, 2018 at 12:54:21AM +0300, Vladimir Kondratyev wrote:
> > > Following test case triggers assertion after r340343:
> > > 
> > > 
> > > #include <fcntl.h>
> > > 
> > > int
> > > main(int argc, char **argv)
> > > {
> > >         openat(open("/etc", O_RDONLY), "termcap", O_RDONLY | O_BENEATH);
> > > }
> > > 
> > > It results in:
> > > 
> > > panic: Assertion (ndp->ni_lcf & NI_LCF_LATCH) != 0 failed at
> > > /usr/src/sys/kern/vfs_lookup.c:182
> > > 
> > 
> > The following should fix it. Problem was that the topping directory was
> > only latched when the initial path was absolute. Since your example
> > switched from the relative argument to the absolute symlink, the BENEATH
> > tracker rightfully complained that there were no recorded top.
> > 
> > I also added some asserts I used during the debugging.
> > 
> > diff --git a/sys/kern/vfs_lookup.c b/sys/kern/vfs_lookup.c
> > index 78893c4f2bd..7a80775d91d 100644
> > --- a/sys/kern/vfs_lookup.c
> 
> With this patch I got a:
> 
> $ ./beneath.sh
> open("a/b") succeeded
> stat("a/b
> panic: Assertion (ndp->ni_lcf & NI_LCF_LATCH) != 0 failed at ../../../kern/vfs_lookup.c:269
> cpuid = 4
> time = 1543397647
> KDB: stack backtrace:
> db_trace_self_wrapper() at db_trace_self_wrapper+0x2b/frame 0xfffffe00c71881a0
> vpanic() at vpanic+0x1a3/frame 0xfffffe00c7188200
> panic() at panic+0x43/frame 0xfffffe00c7188260
> namei_handle_root() at namei_handle_root+0xf7/frame 0xfffffe00c71882b0
> namei() at namei+0x617/frame 0xfffffe00c71884f0
> vn_open_cred() at vn_open_cred+0x526/frame 0xfffffe00c7188640
> vn_open() at vn_open+0x4c/frame 0xfffffe00c7188680
> kern_openat() at kern_openat+0x2e9/frame 0xfffffe00c71888e0
> sys_openat() at sys_openat+0x69/frame 0xfffffe00c7188910
> syscallenter() at syscallenter+0x4e3/frame 0xfffffe00c71889f0
> amd64_syscall() at amd64_syscall+0x4d/frame 0xfffffe00c7188ab0
> fast_syscall_common() at fast_syscall_common+0x101/frame 0xfffffe00c7188ab0
> --- syscall (499, FreeBSD ELF64, sys_openat), rip = 0x8003a215a, rsp = 0x7fffffffe4f8, rbp = 0x7fffffffe5e0 ---
> 
> https://people.freebsd.org/~pho/stress/log/kostik1127.txt

Thank you for reporting this.  The issue is due to wrong assert, which is
valid for later call to namei_handle_root(), but not for the very first
call.  Below is the updated patch.

diff --git a/sys/kern/vfs_lookup.c b/sys/kern/vfs_lookup.c
index 78893c4f2bd..cb69a75ea65 100644
--- a/sys/kern/vfs_lookup.c
+++ b/sys/kern/vfs_lookup.c
_at__at_ -202,8 +202,10 _at__at_ nameicap_cleanup(struct nameidata *ndp, bool clean_latch)
 		vdrop(nt->dp);
 		uma_zfree(nt_zone, nt);
 	}
-	if (clean_latch && (ndp->ni_lcf & NI_LCF_LATCH) != 0)
+	if (clean_latch && (ndp->ni_lcf & NI_LCF_LATCH) != 0) {
+		ndp->ni_lcf &= ~NI_LCF_LATCH;
 		vrele(ndp->ni_beneath_latch);
+	}
 }
 
 /*
_at__at_ -446,7 +448,7 _at__at_ namei(struct nameidata *ndp)
 		if (error == 0 && dp->v_type != VDIR)
 			error = ENOTDIR;
 	}
-	if (error == 0 && (ndp->ni_lcf & NI_LCF_BENEATH_ABS) != 0) {
+	if (error == 0 && (cnp->cn_flags & BENEATH) != 0) {
 		if (ndp->ni_dirfd == AT_FDCWD) {
 			ndp->ni_beneath_latch = fdp->fd_cdir;
 			vrefact(ndp->ni_beneath_latch);
_at__at_ -471,6 +473,8 _at__at_ namei(struct nameidata *ndp)
 			vrele(dp);
 		goto out;
 	}
+	MPASS((ndp->ni_lcf & (NI_LCF_BENEATH_ABS | NI_LCF_LATCH)) !=
+	    NI_LCF_BENEATH_ABS);
 	if (((ndp->ni_lcf & NI_LCF_STRICTRELATIVE) != 0 &&
 	    lookup_cap_dotdot != 0) ||
 	    ((ndp->ni_lcf & NI_LCF_STRICTRELATIVE) == 0 &&