From nobody Tue Aug 27 21:20:43 2024 X-Original-To: dev-commits-src-all@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 4WtgSW1spRz5MQcr; Tue, 27 Aug 2024 21:20:43 +0000 (UTC) (envelope-from git@FreeBSD.org) Received: from mxrelay.nyi.freebsd.org (mxrelay.nyi.freebsd.org [IPv6:2610:1c1:1:606c::19:3]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (4096 bits) server-digest SHA256 client-signature RSA-PSS (4096 bits) client-digest SHA256) (Client CN "mxrelay.nyi.freebsd.org", Issuer "R11" (verified OK)) by mx1.freebsd.org (Postfix) with ESMTPS id 4WtgSW1Mnqz4xb2; Tue, 27 Aug 2024 21:20:43 +0000 (UTC) (envelope-from git@FreeBSD.org) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=freebsd.org; s=dkim; t=1724793643; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding; bh=/g11u6OMdWW/u2Q8fs3VO15PwFSys9Rupj+T6n+lOxQ=; b=ZN+rom4WGQ9pFIUgGQvZG//ltddD7S/d3mIppxzB2NrVLW6GY3J7eZFE/N3FOaG+4tmmlb Zdm5Vr23lv4WorlUx/VvgC2R3VW9RsNd7tTX/9VEYnySGHzJaTS+1yXKVwZFPogYk739KH hfC/0KObX7piEvAWS+xWfEp57H9JgorReNjEtKw/Oq3bEo6/KXRYSDwLPg1jziyvI2h+Xq u4IV/Tqxd9sr3oACgLT9bM1acx2j0Mx/gvdaPhq2h7cboFUyk15omhQN6DYkmEiUXBojTg 16qhBaEEQT+HfYdyOUFBD1SSXyVE3IiD0Foid28lB/Y9iiUfbrnMcBYDw9eLMA== ARC-Seal: i=1; s=dkim; d=freebsd.org; t=1724793643; a=rsa-sha256; cv=none; b=D7N5jAjHIpuT7N247aAatLrcQizhqPs+gkerGR9vwkft/dzewpQMSjzfiNJBPA7W0E725c bXvwoZQY3v45lxZMXLF+BC10jFWZMuoha/0jhndgEMrtF6sz7ER4RcxqfUqI030LuMu9OE wpsFZE6LmzOlg7IaKuCcoD2QdG1w++++oYkJS8+aScIvjFo8t3600MnK8Je7FL6jfRm2QH tZx7sjP5+fratab/BeObGSx8/HcN8IUmgykQxYJxftI+N4GA2DBSUFbFxnYAtY45xY+/PU lqDHRklvzY67LBdEnxdC4Wt4ef0rKBlJt6133833gzIzQShyx4o8GMw+34Ymug== ARC-Authentication-Results: i=1; mx1.freebsd.org; none ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=freebsd.org; s=dkim; t=1724793643; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding; bh=/g11u6OMdWW/u2Q8fs3VO15PwFSys9Rupj+T6n+lOxQ=; b=ZKxAZYI5qL3Jn7uQ39UiEFDmXFIbvF/prRJv1TSfV3cAJ5DztxXZTAa5Z/LgoJjcOQMGi/ RPpabtFOaeA4Jly1nx1l27pmKKrTKB8j6JCxrCVDLPnm+do2Ic6vTxPURY7cmnRTrCVVE9 rTgSIjoLOU1Iva7jMKMJKOBdYtkRjQ/W4pFz0KgZYBHCkrXvnjTCwP+EshOvPMgPMMSTZk y4J+ON/OjOKB/01dtlCUmswOfwVLQ4q0xLh3d1XItjL8QX9G5bR8EPN9N1n3lgIHud+0af XKnt8hs+pQ6jvV9X8TcnX/cXyNtrCzIsIxG3cK6CPAxAw2J4YtSQOVtlGeMj+w== Received: from gitrepo.freebsd.org (gitrepo.freebsd.org [IPv6:2610:1c1:1:6068::e6a:5]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (4096 bits) server-digest SHA256) (Client did not present a certificate) by mxrelay.nyi.freebsd.org (Postfix) with ESMTPS id 4WtgSW0zG6z15m4; Tue, 27 Aug 2024 21:20:43 +0000 (UTC) (envelope-from git@FreeBSD.org) Received: from gitrepo.freebsd.org ([127.0.1.44]) by gitrepo.freebsd.org (8.18.1/8.18.1) with ESMTP id 47RLKh0t027363; Tue, 27 Aug 2024 21:20:43 GMT (envelope-from git@gitrepo.freebsd.org) Received: (from git@localhost) by gitrepo.freebsd.org (8.18.1/8.18.1/Submit) id 47RLKhBV027330; Tue, 27 Aug 2024 21:20:43 GMT (envelope-from git) Date: Tue, 27 Aug 2024 21:20:43 GMT Message-Id: <202408272120.47RLKhBV027330@gitrepo.freebsd.org> To: src-committers@FreeBSD.org, dev-commits-src-all@FreeBSD.org, dev-commits-src-main@FreeBSD.org From: Rick Macklem Subject: git: 5037c6398b23 - main - nfsd: Fix handling of NFSv4 setable attributes List-Id: Commit messages for all branches of the src repository List-Archive: https://lists.freebsd.org/archives/dev-commits-src-all List-Help: List-Post: List-Subscribe: List-Unsubscribe: X-BeenThere: dev-commits-src-all@freebsd.org Sender: owner-dev-commits-src-all@FreeBSD.org MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8bit X-Git-Committer: rmacklem X-Git-Repository: src X-Git-Refname: refs/heads/main X-Git-Reftype: branch X-Git-Commit: 5037c6398b2327366494a0434a894dc17ba8d023 Auto-Submitted: auto-generated The branch main has been updated by rmacklem: URL: https://cgit.FreeBSD.org/src/commit/?id=5037c6398b2327366494a0434a894dc17ba8d023 commit 5037c6398b2327366494a0434a894dc17ba8d023 Author: Rick Macklem AuthorDate: 2024-08-27 21:19:33 +0000 Commit: Rick Macklem CommitDate: 2024-08-27 21:19:33 +0000 nfsd: Fix handling of NFSv4 setable attributes Commit d8a5961 made a change to nfsv4_sattr() that broke parsing of the setable attributes for a NFSv4 SETATTR. (It broke out of the code by setting "error" and returning right away, instead of noting the error in nd_repstat and allowing parsing of the attributes to continue.) By returning prematurely, it was possible for SETATTR to return the error, but with a bogus set of attribute bits set, since "retbits" had not yet been set to all zeros. (I am not sure if any client could be affected by this bug. The patch was done for a failure case detected by a pynfs test suite and not an actual client.) While here, the patch also fixes a few cases where the value of attributes gets set for attributes after an error has been set in nd_repstat. This would not really break the protocol, since a SETATTR is allowed to set some attributes and still return an failure, but should not really be done. MFC after: 2 weeks --- sys/fs/nfsserver/nfs_nfsdport.c | 74 ++++++++++++++++++++++++----------------- sys/fs/nfsserver/nfs_nfsdserv.c | 2 +- 2 files changed, 44 insertions(+), 32 deletions(-) diff --git a/sys/fs/nfsserver/nfs_nfsdport.c b/sys/fs/nfsserver/nfs_nfsdport.c index 12592d4c5c59..8a2a39052506 100644 --- a/sys/fs/nfsserver/nfs_nfsdport.c +++ b/sys/fs/nfsserver/nfs_nfsdport.c @@ -3026,6 +3026,8 @@ nfsv4_sattr(struct nfsrv_descript *nd, vnode_t vp, struct nfsvattr *nvap, /* * Loop around getting the setable attributes. If an unsupported * one is found, set nd_repstat == NFSERR_ATTRNOTSUPP and return. + * Once nd_repstat != 0, do not set the attribute value, but keep + * parsing the attribute(s). */ if (retnotsup) { nd->nd_repstat = NFSERR_ATTRNOTSUPP; @@ -3043,12 +3045,13 @@ nfsv4_sattr(struct nfsrv_descript *nd, vnode_t vp, struct nfsvattr *nvap, switch (bitpos) { case NFSATTRBIT_SIZE: NFSM_DISSECT(tl, u_int32_t *, NFSX_HYPER); - if (vp != NULL && vp->v_type != VREG) { - error = (vp->v_type == VDIR) ? NFSERR_ISDIR : - NFSERR_INVAL; - goto nfsmout; + if (!nd->nd_repstat) { + if (vp != NULL && vp->v_type != VREG) + nd->nd_repstat = (vp->v_type == VDIR) ? + NFSERR_ISDIR : NFSERR_INVAL; + else + nvap->na_size = fxdr_hyper(tl); } - nvap->na_size = fxdr_hyper(tl); attrsum += NFSX_HYPER; break; case NFSATTRBIT_ACL: @@ -3085,7 +3088,8 @@ nfsv4_sattr(struct nfsrv_descript *nd, vnode_t vp, struct nfsvattr *nvap, case NFSATTRBIT_MODE: moderet = NFSERR_INVAL; /* Can't do MODESETMASKED. */ NFSM_DISSECT(tl, u_int32_t *, NFSX_UNSIGNED); - nvap->na_mode = nfstov_mode(*tl); + if (!nd->nd_repstat) + nvap->na_mode = nfstov_mode(*tl); attrsum += NFSX_UNSIGNED; break; case NFSATTRBIT_OWNER: @@ -3153,10 +3157,11 @@ nfsv4_sattr(struct nfsrv_descript *nd, vnode_t vp, struct nfsvattr *nvap, attrsum += NFSX_UNSIGNED; if (fxdr_unsigned(int, *tl)==NFSV4SATTRTIME_TOCLIENT) { NFSM_DISSECT(tl, u_int32_t *, NFSX_V4TIME); - fxdr_nfsv4time(tl, &nvap->na_atime); + if (!nd->nd_repstat) + fxdr_nfsv4time(tl, &nvap->na_atime); toclient = 1; attrsum += NFSX_V4TIME; - } else { + } else if (!nd->nd_repstat) { vfs_timestamp(&nvap->na_atime); nvap->na_vaflags |= VA_UTIMES_NULL; } @@ -3169,7 +3174,8 @@ nfsv4_sattr(struct nfsrv_descript *nd, vnode_t vp, struct nfsvattr *nvap, break; case NFSATTRBIT_TIMECREATE: NFSM_DISSECT(tl, u_int32_t *, NFSX_V4TIME); - fxdr_nfsv4time(tl, &nvap->na_btime); + if (!nd->nd_repstat) + fxdr_nfsv4time(tl, &nvap->na_btime); attrsum += NFSX_V4TIME; break; case NFSATTRBIT_TIMEMODIFYSET: @@ -3177,10 +3183,11 @@ nfsv4_sattr(struct nfsrv_descript *nd, vnode_t vp, struct nfsvattr *nvap, attrsum += NFSX_UNSIGNED; if (fxdr_unsigned(int, *tl)==NFSV4SATTRTIME_TOCLIENT) { NFSM_DISSECT(tl, u_int32_t *, NFSX_V4TIME); - fxdr_nfsv4time(tl, &nvap->na_mtime); + if (!nd->nd_repstat) + fxdr_nfsv4time(tl, &nvap->na_mtime); nvap->na_vaflags &= ~VA_UTIMES_NULL; attrsum += NFSX_V4TIME; - } else { + } else if (!nd->nd_repstat) { vfs_timestamp(&nvap->na_mtime); if (!toclient) nvap->na_vaflags |= VA_UTIMES_NULL; @@ -3198,18 +3205,21 @@ nfsv4_sattr(struct nfsrv_descript *nd, vnode_t vp, struct nfsvattr *nvap, * specified and this attribute cannot be done in the * same Setattr operation. */ - if ((nd->nd_flag & ND_NFSV41) == 0) - nd->nd_repstat = NFSERR_ATTRNOTSUPP; - else if ((mode & ~07777) != 0 || (mask & ~07777) != 0 || - vp == NULL) - nd->nd_repstat = NFSERR_INVAL; - else if (moderet == 0) - moderet = VOP_GETATTR(vp, &va, nd->nd_cred); - if (moderet == 0) - nvap->na_mode = (mode & mask) | - (va.va_mode & ~mask); - else - nd->nd_repstat = moderet; + if (!nd->nd_repstat) { + if ((nd->nd_flag & ND_NFSV41) == 0) + nd->nd_repstat = NFSERR_ATTRNOTSUPP; + else if ((mode & ~07777) != 0 || + (mask & ~07777) != 0 || vp == NULL) + nd->nd_repstat = NFSERR_INVAL; + else if (moderet == 0) + moderet = VOP_GETATTR(vp, &va, + nd->nd_cred); + if (moderet == 0) + nvap->na_mode = (mode & mask) | + (va.va_mode & ~mask); + else + nd->nd_repstat = moderet; + } attrsum += 2 * NFSX_UNSIGNED; break; case NFSATTRBIT_MODEUMASK: @@ -3220,13 +3230,15 @@ nfsv4_sattr(struct nfsrv_descript *nd, vnode_t vp, struct nfsvattr *nvap, * If moderet != 0, mode has already been done. * If vp != NULL, this is not a file object creation. */ - if ((nd->nd_flag & ND_NFSV42) == 0) - nd->nd_repstat = NFSERR_ATTRNOTSUPP; - else if ((mask & ~0777) != 0 || vp != NULL || - moderet != 0) - nd->nd_repstat = NFSERR_INVAL; - else - nvap->na_mode = (mode & ~mask); + if (!nd->nd_repstat) { + if ((nd->nd_flag & ND_NFSV42) == 0) + nd->nd_repstat = NFSERR_ATTRNOTSUPP; + else if ((mask & ~0777) != 0 || vp != NULL || + moderet != 0) + nd->nd_repstat = NFSERR_INVAL; + else + nvap->na_mode = (mode & ~mask); + } attrsum += 2 * NFSX_UNSIGNED; break; default: @@ -3241,7 +3253,7 @@ nfsv4_sattr(struct nfsrv_descript *nd, vnode_t vp, struct nfsvattr *nvap, /* * some clients pad the attrlist, so we need to skip over the - * padding. + * padding. This also skips over unparsed non-supported attributes. */ if (attrsum > attrsize) { error = NFSERR_BADXDR; diff --git a/sys/fs/nfsserver/nfs_nfsdserv.c b/sys/fs/nfsserver/nfs_nfsdserv.c index 47e3a20390f4..a38ef3d47946 100644 --- a/sys/fs/nfsserver/nfs_nfsdserv.c +++ b/sys/fs/nfsserver/nfs_nfsdserv.c @@ -375,6 +375,7 @@ nfsrvd_setattr(struct nfsrv_descript *nd, __unused int isdgram, NFSACL_T *aclp = NULL; struct thread *p = curthread; + NFSZERO_ATTRBIT(&retbits); if (nd->nd_repstat) { nfsrv_wcc(nd, preat_ret, &nva2, postat_ret, &nva); goto out; @@ -402,7 +403,6 @@ nfsrvd_setattr(struct nfsrv_descript *nd, __unused int isdgram, goto nfsmout; /* For NFSv4, only va_uid is used from nva2. */ - NFSZERO_ATTRBIT(&retbits); NFSSETBIT_ATTRBIT(&retbits, NFSATTRBIT_OWNER); preat_ret = nfsvno_getattr(vp, &nva2, nd, p, 1, &retbits); if (!nd->nd_repstat)