From nobody Sat Oct 16 23:48:25 2021 X-Original-To: dev-commits-src-main@mlmmj.nyi.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2610:1c1:1:606c::19:1]) by mlmmj.nyi.freebsd.org (Postfix) with ESMTP id F042817F5E62; Sat, 16 Oct 2021 23:48:28 +0000 (UTC) (envelope-from markjdb@gmail.com) Received: from mail-qv1-xf2c.google.com (mail-qv1-xf2c.google.com [IPv6:2607:f8b0:4864:20::f2c]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits) key-exchange X25519 server-signature RSA-PSS (4096 bits) server-digest SHA256 client-signature RSA-PSS (2048 bits) client-digest SHA256) (Client CN "smtp.gmail.com", Issuer "GTS CA 1O1" (verified OK)) by mx1.freebsd.org (Postfix) with ESMTPS id 4HX0Fm5rxZz4jGt; Sat, 16 Oct 2021 23:48:28 +0000 (UTC) (envelope-from markjdb@gmail.com) Received: by mail-qv1-xf2c.google.com with SMTP id e13so2727628qvf.5; Sat, 16 Oct 2021 16:48:28 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20210112; h=sender:date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to; bh=4EoFoRZXqbZoWhfBDSInIQUpmeVWHhulR9oD/S7Md6k=; b=E/995OMc5e7/MX6G3KXT/bew9q0vQ88crkThSvYFcm+FvEJM/cDw4hyFsxld9MYhna fNMTQsXMuBXs+CjP+WYqbqK+KVhJUzgL9jfEFY+YDh3nnDi7F1egBbIaRmaQIQeoEE4g /3LsfafUCcE1mFun4GobYMWrSMQVWFak49zDqxbaXq0t9FBxeXqCP8UA3cRPuVlTQDkM MNf34d+LjBe72ckiUym0rrx3zySV9oBNAe+zAcnzEq7KGUwPGsK7rY3csX0Msp6jWUw5 diqRztvkQne/1toAhrhkVfD22x9j6KUQxhAlu0PTm6/ejAy9vImpu6+HCVE+uqsF383P 6RvQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:sender:date:from:to:cc:subject:message-id :references:mime-version:content-disposition:in-reply-to; bh=4EoFoRZXqbZoWhfBDSInIQUpmeVWHhulR9oD/S7Md6k=; b=1L9t3iLTu/xPaJMhoXl4rh/dOpVjibunHw+BXRAU4j30kPsqfQpEzK/aavRHdo1nBk R/IcbIt4+9iYeAbYHoxwaFBikbP7BAB3N71Dw67zsIKU2//2hRxV9QkRtPZhLcXvJhz6 zlIMXvC8cBpb8N0H8CC0k3yPhOL7r/LgGIBkZzV5exQk1t8cZ1FcK6GwzC+MTgmYl8I1 xq5Cze8ieZBjRP/+eKvrrWN8eqSNCPXuu/oNVCOnta+bZ5TXTksVVoep5NrqZOcMh/1V 2DBkW2OILl6UE6Qik98T87QYR/gA+EiGR2yZV7igdL7ZX/HDxlBdfoJj6PneWUoqGNxY ONag== X-Gm-Message-State: AOAM5325PEW40Ot4zvF9p2IuHkYgQjXos/NMK1otU6i7vKatFEfrQR7V Rbq6yBoJtNmqZ5FdmbCkIegDGoCznWs= X-Google-Smtp-Source: ABdhPJzV7TUWRaXMQHgjg3/qVfbiCHVxu6heqoMa9NgGaR1h2XBGs8p2sxLG0tnvFp8eVJd2a6GVkA== X-Received: by 2002:a0c:ed4e:: with SMTP id v14mr19278508qvq.40.1634428108151; Sat, 16 Oct 2021 16:48:28 -0700 (PDT) Received: from nuc ([142.126.186.191]) by smtp.gmail.com with ESMTPSA id g11sm5012656qko.31.2021.10.16.16.48.27 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Sat, 16 Oct 2021 16:48:27 -0700 (PDT) Sender: Mark Johnston Date: Sat, 16 Oct 2021 19:48:25 -0400 From: Mark Johnston To: Mateusz Guzik Cc: src-committers@freebsd.org, dev-commits-src-all@freebsd.org, dev-commits-src-main@freebsd.org Subject: Re: git: 7dd419cabc6b - main - cache: add empty path support Message-ID: References: <202110162009.19GK9GPd096217@gitrepo.freebsd.org> List-Id: Commit messages for the main branch of the src repository List-Archive: https://lists.freebsd.org/archives/dev-commits-src-main List-Help: List-Post: List-Subscribe: List-Unsubscribe: Sender: owner-dev-commits-src-main@freebsd.org X-BeenThere: dev-commits-src-main@freebsd.org MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <202110162009.19GK9GPd096217@gitrepo.freebsd.org> X-Rspamd-Queue-Id: 4HX0Fm5rxZz4jGt X-Spamd-Bar: ---- Authentication-Results: mx1.freebsd.org; none X-Spamd-Result: default: False [-4.00 / 15.00]; REPLY(-4.00)[] X-ThisMailContainsUnwantedMimeParts: N On Sat, Oct 16, 2021 at 08:09:16PM +0000, Mateusz Guzik wrote: > The branch main has been updated by mjg: > > URL: https://cgit.FreeBSD.org/src/commit/?id=7dd419cabc6bb9e019c56d15f8e6a88ee2f46859 > > commit 7dd419cabc6bb9e019c56d15f8e6a88ee2f46859 > Author: Mateusz Guzik > AuthorDate: 2021-09-26 13:00:24 +0000 > Commit: Mateusz Guzik > CommitDate: 2021-10-16 20:08:37 +0000 > > cache: add empty path support > > This avoids spurious drop offs as EMPTY is passed regardless of the > actual path name. > > Pushign the work inside the lookup instead of just ignorign the flag > allows avoid checking for empty pathname for all other lookups. Hi, syzbot hit a bug in this commit: https://syzkaller.appspot.com/bug?id=283995ae4346041c1757f62f3322a3545d0a62a4 There's no reproducer yet but I expect one would appear within a day or so (hopefully much less). > --- > sys/kern/kern_descrip.c | 2 +- > sys/kern/vfs_cache.c | 67 ++++++++++++++++++++++++++++++++++++++++++++++--- > sys/kern/vfs_lookup.c | 18 ++++++------- > 3 files changed, 74 insertions(+), 13 deletions(-) > > diff --git a/sys/kern/kern_descrip.c b/sys/kern/kern_descrip.c > index 55c2a36955a5..a7e3785bc672 100644 > --- a/sys/kern/kern_descrip.c > +++ b/sys/kern/kern_descrip.c > @@ -2903,7 +2903,7 @@ fgetvp_lookup_smr(int fd, struct nameidata *ndp, struct vnode **vpp, bool *fsear > return (EAGAIN); > *fsearch = ((fp->f_flag & FSEARCH) != 0); > vp = fp->f_vnode; > - if (__predict_false(vp == NULL || vp->v_type != VDIR)) { > + if (__predict_false(vp == NULL)) { > return (EAGAIN); > } > if (!filecaps_copy(&fde->fde_caps, &ndp->ni_filecaps, false)) { > diff --git a/sys/kern/vfs_cache.c b/sys/kern/vfs_cache.c > index ae26dc70bd05..c1a3b0dab1e8 100644 > --- a/sys/kern/vfs_cache.c > +++ b/sys/kern/vfs_cache.c > @@ -4176,9 +4176,9 @@ cache_fpl_terminated(struct cache_fpl *fpl) > > #define CACHE_FPL_SUPPORTED_CN_FLAGS \ > (NC_NOMAKEENTRY | NC_KEEPPOSENTRY | LOCKLEAF | LOCKPARENT | WANTPARENT | \ > - FAILIFEXISTS | FOLLOW | LOCKSHARED | SAVENAME | SAVESTART | WILLBEDIR | \ > - ISOPEN | NOMACCHECK | AUDITVNODE1 | AUDITVNODE2 | NOCAPCHECK | OPENREAD | \ > - OPENWRITE) > + FAILIFEXISTS | FOLLOW | EMPTYPATH | LOCKSHARED | SAVENAME | SAVESTART | \ > + WILLBEDIR | ISOPEN | NOMACCHECK | AUDITVNODE1 | AUDITVNODE2 | NOCAPCHECK | \ > + OPENREAD | OPENWRITE) > > #define CACHE_FPL_INTERNAL_CN_FLAGS \ > (ISDOTDOT | MAKEENTRY | ISLASTCN) > @@ -4197,6 +4197,7 @@ static bool > cache_fpl_istrailingslash(struct cache_fpl *fpl) > { > > + MPASS(fpl->nulchar > fpl->cnp->cn_pnbuf); > return (*(fpl->nulchar - 1) == '/'); > } > > @@ -4767,6 +4768,54 @@ cache_fplookup_degenerate(struct cache_fpl *fpl) > return (cache_fpl_handled(fpl)); > } > > +static int __noinline > +cache_fplookup_emptypath(struct cache_fpl *fpl) > +{ > + struct nameidata *ndp; > + struct componentname *cnp; > + enum vgetstate tvs; > + struct vnode *tvp; > + seqc_t tvp_seqc; > + int error, lkflags; > + > + fpl->tvp = fpl->dvp; > + fpl->tvp_seqc = fpl->dvp_seqc; > + > + ndp = fpl->ndp; > + cnp = fpl->cnp; > + tvp = fpl->tvp; > + tvp_seqc = fpl->tvp_seqc; > + > + MPASS(*cnp->cn_pnbuf == '\0'); > + MPASS((cnp->cn_flags & (LOCKPARENT | WANTPARENT)) == 0); > + > + if (__predict_false((cnp->cn_flags & EMPTYPATH) == 0)) { > + cache_fpl_smr_exit(fpl); > + return (cache_fpl_handled_error(fpl, ENOENT)); > + } > + > + tvs = vget_prep_smr(tvp); > + cache_fpl_smr_exit(fpl); > + if (__predict_false(tvs == VGET_NONE)) { > + return (cache_fpl_aborted(fpl)); > + } > + > + if ((cnp->cn_flags & LOCKLEAF) != 0) { > + lkflags = LK_SHARED; > + if ((cnp->cn_flags & LOCKSHARED) == 0) > + lkflags = LK_EXCLUSIVE; > + error = vget_finish(tvp, lkflags, tvs); > + if (__predict_false(error != 0)) { > + return (cache_fpl_aborted(fpl)); > + } > + } else { > + vget_finish_ref(tvp, tvs); > + } > + > + ndp->ni_resflags |= NIRES_EMPTYPATH; > + return (cache_fpl_handled(fpl)); > +} > + > static int __noinline > cache_fplookup_noentry(struct cache_fpl *fpl) > { > @@ -4799,6 +4848,10 @@ cache_fplookup_noentry(struct cache_fpl *fpl) > return (cache_fplookup_skip_slashes(fpl)); > } > > + if (cnp->cn_pnbuf[0] == '\0') { > + return (cache_fplookup_emptypath(fpl)); > + } > + > if (cnp->cn_nameptr[0] == '\0') { > if (fpl->tvp == NULL) { > return (cache_fplookup_degenerate(fpl)); > @@ -5486,6 +5539,7 @@ cache_fplookup_parse(struct cache_fpl *fpl) > * > * TODO: fix this to be word-sized. > */ > + MPASS(&cnp->cn_nameptr[fpl->debug.ni_pathlen - 1] >= cnp->cn_pnbuf); > KASSERT(&cnp->cn_nameptr[fpl->debug.ni_pathlen - 1] == fpl->nulchar, > ("%s: mismatch between pathlen (%zu) and nulchar (%p != %p), string [%s]\n", > __func__, fpl->debug.ni_pathlen, &cnp->cn_nameptr[fpl->debug.ni_pathlen - 1], > @@ -5739,6 +5793,13 @@ cache_fplookup_failed_vexec(struct cache_fpl *fpl, int error) > dvp = fpl->dvp; > dvp_seqc = fpl->dvp_seqc; > > + /* > + * Hack: delayed empty path checking. > + */ > + if (cnp->cn_pnbuf[0] == '\0') { > + return (cache_fplookup_emptypath(fpl)); > + } > + > /* > * TODO: Due to ignoring trailing slashes lookup will perform a > * permission check on the last dir when it should not be doing it. It > diff --git a/sys/kern/vfs_lookup.c b/sys/kern/vfs_lookup.c > index 9e10c3092f5a..8cccd93152ef 100644 > --- a/sys/kern/vfs_lookup.c > +++ b/sys/kern/vfs_lookup.c > @@ -463,13 +463,6 @@ namei_getpath(struct nameidata *ndp) > if (__predict_false(error != 0)) > return (error); > > - /* > - * Don't allow empty pathnames unless EMPTYPATH is specified. > - * Caller checks for ENOENT as an indication for the empty path. > - */ > - if (__predict_false(*cnp->cn_pnbuf == '\0')) > - return (ENOENT); > - > cnp->cn_nameptr = cnp->cn_pnbuf; > return (0); > } > @@ -598,8 +591,6 @@ namei(struct nameidata *ndp) > > error = namei_getpath(ndp); > if (__predict_false(error != 0)) { > - if (error == ENOENT && (cnp->cn_flags & EMPTYPATH) != 0) > - return (namei_emptypath(ndp)); > namei_cleanup_cnp(cnp); > SDT_PROBE4(vfs, namei, lookup, return, error, NULL, > false, ndp); > @@ -642,6 +633,15 @@ namei(struct nameidata *ndp) > case CACHE_FPL_STATUS_ABORTED: > TAILQ_INIT(&ndp->ni_cap_tracker); > MPASS(ndp->ni_lcf == 0); > + if (*cnp->cn_pnbuf == '\0') { > + if ((cnp->cn_flags & EMPTYPATH) != 0) { > + return (namei_emptypath(ndp)); > + } > + namei_cleanup_cnp(cnp); > + SDT_PROBE4(vfs, namei, lookup, return, ENOENT, NULL, > + false, ndp); > + return (ENOENT); > + } > error = namei_setup(ndp, &dp, &pwd); > if (error != 0) { > namei_cleanup_cnp(cnp);