From patchwork Mon Dec 23 18:07:26 2024 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Chuck Lever X-Patchwork-Id: 13919194 Received: from smtp.kernel.org (aws-us-west-2-korg-mail-1.web.codeaurora.org [10.30.226.201]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 35D4618A6B7 for ; Mon, 23 Dec 2024 18:07:35 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=10.30.226.201 ARC-Seal: i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1734977255; cv=none; b=Oc8VvJAlH2PAN+n6SOrWMl3npov9VRKaVju7mc7kRP1oYWKVi4IyLPhOJxKdc3jHZ0+QadovFsf5HQsYrkQPotqvQIFebNQNdLlIeAsZ0Siu95tmaTXxLxcZi9nIaW7tTkiZIjRAA8oK0EQxUbunRC035A073yEnE+l0G6htLUM= ARC-Message-Signature: i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1734977255; c=relaxed/simple; bh=fF7L8SORLShgP9+zfYC/zn3TR/vGeNVj+dETpfeUJCk=; h=From:To:Cc:Subject:Date:Message-ID:In-Reply-To:References: MIME-Version; b=LseklQDjmHRcCU/mTQFkfdRIWBoADNV/ff491XyyyhcJFUssTwm3MIU+k5RVHy07dGZdIgu0zwoDSYXl/F/DkNBWlIF5swoWyQfcjdFpz785/RKHXe9ff1d/HKZSjor/sYv5eHQVrsk9IJpftuCnz1xXp/1X0E38o7tacGTmppo= ARC-Authentication-Results: i=1; smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=kernel.org header.i=@kernel.org header.b=qlRIDxih; arc=none smtp.client-ip=10.30.226.201 Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=kernel.org header.i=@kernel.org header.b="qlRIDxih" Received: by smtp.kernel.org (Postfix) with ESMTPSA id 4229BC4CED7; Mon, 23 Dec 2024 18:07:34 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1734977255; bh=fF7L8SORLShgP9+zfYC/zn3TR/vGeNVj+dETpfeUJCk=; h=From:To:Cc:Subject:Date:In-Reply-To:References:From; b=qlRIDxihj8geSaqZ8/rw+30tdZbGBNAzz9UugrDzN3Z/bXBiLn5emFhLJ9JFJCcO8 1x6+lxCCsFV0Wa0livnSFqqAv8y6w0OW3DXH9wmUhqX2O9qsLvs+a+hh7hzwVRFHMd I+DWrkRlOTzAr21LwhZMEJFk9r0pwL/ZrcjgJMo+5kKLtILgZU9Mp81PpJKX38vG0O C74FZykhViHeZXI0X8LbSdFZ3APs3SMSUQilLy/ImTR/xd1XwPaoBRrvw3mU7N4NcY 6pCyZ2g7me1YT0rGtLFVDeVhhU+XS/8OiBAaei+UVYUQXSLkn04EwUAeZDfGYH7sL1 mtsOc6AHFRuTA== From: cel@kernel.org To: Neil Brown , Jeff Layton , Olga Kornievskaia , Dai Ngo , Tom Talpey Cc: , Rick Macklem , Chuck Lever , J David Subject: [PATCH v2 1/2] NFSD: Encode COMPOUND operation status on page boundaries Date: Mon, 23 Dec 2024 13:07:26 -0500 Message-ID: <20241223180724.1804-5-cel@kernel.org> X-Mailer: git-send-email 2.47.0 In-Reply-To: <20241223180724.1804-4-cel@kernel.org> References: <20241223180724.1804-4-cel@kernel.org> Precedence: bulk X-Mailing-List: linux-nfs@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 X-Developer-Signature: v=1; a=openpgp-sha256; l=4427; i=chuck.lever@oracle.com; h=from:subject; bh=LYJfaZtNX+HXrRxHzcDNZg0ZR7/EUWImJtn5TanD9S4=; b=owEBbQKS/ZANAwAIATNqszNvZn+XAcsmYgBnaabiuKlnQy1PKIrTurwPvVGOWRNSm1KlX7kAd r50+KsGSwiJAjMEAAEIAB0WIQQosuWwEobfJDzyPv4zarMzb2Z/lwUCZ2mm4gAKCRAzarMzb2Z/ l3t5EAC/G6HdGrTCUSPPLpdwyqGpFJ7C2xRwXUghDHS2KwVLbKXQ0VrBFd6TX8M8dOEPfiqGMvD J/22U/czcocnqV7Cz2aBl5I80TXp7c13tYbgB5yMJNWkovwATNvRFQ0DgMxhGQEOwujxoEA4YtD sygwsZuKSaQwmPeSoT5SHf1GdoQ8fcC0yvP/M+O2B27VGMcIlJNmpt9jeGAIWkYLY/rD1nqABxg wKGrurDfZHJFDG/w26E7CUGODAHbgIYkyAlOi1PyZoErNwUKaAMHHeV5oXxRvhTAIEzSWg3IizX SGRpdGEmmxro+zHYPC1VUZ344BlyTD4EJk/NTQS0zw63PiqJqVAwFqZ5fLggc+bf8LoB3wyY6/M yEzdplrYPhtfhREMWvgPp7A/qJjPpgt7QQ7cmb33695sd6COZdiNOr5dEP4ACkPR5GFQgBEQIfc GbPruhU9lnBQyDAiVF95cYQGE1CpuRJ5+ismatzaTcbZ16Qu6lkb4zTs2WIXx+Q3BHxhc+DwqK1 PP0S4BM3umiHrEPmLHYUvSppUSRRWzfT5W+YCGB9ZhtvqkUt5qqWr55Lt6VDHwFiilPEUxaZYVO zlAAk4d2LjTlHpZXaq6nF7aJtgjJ2UUVVE8sdZ/3HD4iDJgrM6CnSpZWyP/tB7S3HH1qznaLIVo 0y3Ybm8Dnwv360w== X-Developer-Key: i=chuck.lever@oracle.com; a=openpgp; fpr=28B2E5B01286DF243CF23EFE336AB3336F667F97 From: Chuck Lever J. David reports an odd corruption of a READDIR reply sent to a FreeBSD client. xdr_reserve_space() has to do a special trick when the @nbytes value requests more space than there is in the current page of the XDR buffer. In that case, xdr_reserve_space() returns a pointer to the start of the next page, and then the next call to xdr_reserve_space() invokes __xdr_commit_encode() to copy enough of the data item back into the previous page to make that data item contiguous across the page boundary. But we need to be careful in the case where buffer space is reserved early for a data item that will be inserted into the buffer later. One such caller, nfsd4_encode_operation(), reserves 8 bytes in the encoding buffer for each COMPOUND operation. However, a READDIR result can sometimes encode file names so that there are only 4 bytes left at the end of the current XDR buffer page (though plenty of pages are left to handle the remaining encoding tasks). If a COMPOUND operation follows the READDIR result (say, a GETATTR), then nfsd4_encode_operation() will reserve 8 bytes for the op number (9) and the op status (usually NFS4_OK). In this weird case, xdr_reserve_space() returns a pointer to byte zero of the next buffer page, as it assumes the data item will be copied back into place (in the previous page) on the next call to xdr_reserve_space(). nfsd4_encode_operation() writes the op num into the buffer, then saves the next 4-byte location for the op's status code. The next xdr_reserve_space() call is part of GETATTR encoding, so the op num gets copied back into the previous page, but the saved location for the op status continues to point to the wrong spot in the current XDR buffer page because __xdr_commit_encode() moved that data item. After GETATTR encoding is complete, nfsd4_encode_operation() writes the op status over the first XDR data item in the GETATTR result. The NFS4_OK status code (0) makes it look like there are zero items in the GETATTR's attribute bitmask. The patch description of commit 2825a7f90753 ("nfsd4: allow encoding across page boundaries") [2014] remarks that NFSD "can't handle a new operation starting close to the end of a page." This bug appears to be one reason for that remark. Reported-by: J David Closes: https://lore.kernel.org/linux-nfs/3998d739-c042-46b4-8166-dbd6c5f0e804@oracle.com/T/#t X-Cc: stable@vger.kernel.org Signed-off-by: Chuck Lever Reviewed-by: NeilBrown --- fs/nfsd/nfs4xdr.c | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/fs/nfsd/nfs4xdr.c b/fs/nfsd/nfs4xdr.c index 53fac037611c..15cd716e9d91 100644 --- a/fs/nfsd/nfs4xdr.c +++ b/fs/nfsd/nfs4xdr.c @@ -5760,15 +5760,14 @@ nfsd4_encode_operation(struct nfsd4_compoundres *resp, struct nfsd4_op *op) struct nfs4_stateowner *so = resp->cstate.replay_owner; struct svc_rqst *rqstp = resp->rqstp; const struct nfsd4_operation *opdesc = op->opdesc; - int post_err_offset; + unsigned int op_status_offset; nfsd4_enc encoder; - __be32 *p; - p = xdr_reserve_space(xdr, 8); - if (!p) + if (xdr_stream_encode_u32(xdr, op->opnum) != XDR_UNIT) + goto release; + op_status_offset = xdr_stream_pos(xdr); + if (!xdr_reserve_space(xdr, 4)) goto release; - *p++ = cpu_to_be32(op->opnum); - post_err_offset = xdr->buf->len; if (op->opnum == OP_ILLEGAL) goto status; @@ -5809,20 +5808,21 @@ nfsd4_encode_operation(struct nfsd4_compoundres *resp, struct nfsd4_op *op) * bug if we had to do this on a non-idempotent op: */ warn_on_nonidempotent_op(op); - xdr_truncate_encode(xdr, post_err_offset); + xdr_truncate_encode(xdr, op_status_offset + XDR_UNIT); } if (so) { - int len = xdr->buf->len - post_err_offset; + int len = xdr->buf->len - (op_status_offset + XDR_UNIT); so->so_replay.rp_status = op->status; so->so_replay.rp_buflen = len; - read_bytes_from_xdr_buf(xdr->buf, post_err_offset, + read_bytes_from_xdr_buf(xdr->buf, op_status_offset + XDR_UNIT, so->so_replay.rp_buf, len); } status: op->status = nfsd4_map_status(op->status, resp->cstate.minorversion); - *p = op->status; + write_bytes_to_xdr_buf(xdr->buf, op_status_offset, + &op->status, XDR_UNIT); release: if (opdesc && opdesc->op_release) opdesc->op_release(&op->u); From patchwork Mon Dec 23 18:07:27 2024 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Chuck Lever X-Patchwork-Id: 13919196 Received: from smtp.kernel.org (aws-us-west-2-korg-mail-1.web.codeaurora.org [10.30.226.201]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 79BB71B413F for ; Mon, 23 Dec 2024 18:07:36 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=10.30.226.201 ARC-Seal: i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1734977256; cv=none; b=q1OzDHU0G7VcTOcajM7VWOzcAD6k1ACjS84iqTv/m6xUa7MPQ1YvWnUSeIpCzSFxVABPHGH/g6GUElzEigFE/66i/WFTmgZQM4Xg08naVF85QxIXqS+ic1uIfhckPzOI9XTJrCYItvrXABOHFlK0I5AeBfJKvt3BcUrIyK9N0Tg= ARC-Message-Signature: i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1734977256; c=relaxed/simple; bh=vAHOURc2QFDsQ+Vf8bG4s7SmgSxxIAqY+fbvIc+HAmo=; h=From:To:Cc:Subject:Date:Message-ID:In-Reply-To:References: MIME-Version; b=Arv5fiE4T43jW0T8DOQdomU7VDWPJDqfUjtnWSC9QOWQDGHga/FhCm6qAGtWMaT5yGXsq2IsxaAqq/+ZHuc4LfOiLuhVkdweGfDp02cU9rJVcNaKg79RbJQUUz3ptge4c8HHPtdYLMWSDTPd9xwMhEoSK2tMCFd8y0MexiCDDq4= ARC-Authentication-Results: i=1; smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=kernel.org header.i=@kernel.org header.b=IS9S45Me; arc=none smtp.client-ip=10.30.226.201 Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=kernel.org header.i=@kernel.org header.b="IS9S45Me" Received: by smtp.kernel.org (Postfix) with ESMTPSA id 4DDD9C4CED4; Mon, 23 Dec 2024 18:07:35 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1734977256; bh=vAHOURc2QFDsQ+Vf8bG4s7SmgSxxIAqY+fbvIc+HAmo=; h=From:To:Cc:Subject:Date:In-Reply-To:References:From; b=IS9S45MejnANLMTn/4jEJn5L8pnDLIm48yP2Tu/PP/9ruRYIWlLVKsMjZJ9YSU2So FIrRGD03msAX+ZSlJrkXHlkfHqAr2LoSnjpCNK7LGQqiC17CEQlZewI45UN/tH0BQW 4RoI03yEQ4p6aXLG9OiGpXhaQhiPjg1ZySnwxyy/NxJp35U4kHAJg8ELdv/IOJLwMv m9JOpHCQSTwgZvolv1qdi1WlJXV7PndCpdVkl0CPV76wYiK8bpDDQXbyoKGVkKsU3g qI0bihhGTWpeL6RBoE4QPmemAz9EmYJx9qOGDcWrvvwmVqKPEMPAToO2l2spjieAkl 3LfrbWnggrE2Q== From: cel@kernel.org To: Neil Brown , Jeff Layton , Olga Kornievskaia , Dai Ngo , Tom Talpey Cc: , Rick Macklem , Chuck Lever Subject: [PATCH v2 2/2] SUNRPC: Document validity guarantees of the pointer returned by reserve_space Date: Mon, 23 Dec 2024 13:07:27 -0500 Message-ID: <20241223180724.1804-6-cel@kernel.org> X-Mailer: git-send-email 2.47.0 In-Reply-To: <20241223180724.1804-4-cel@kernel.org> References: <20241223180724.1804-4-cel@kernel.org> Precedence: bulk X-Mailing-List: linux-nfs@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 X-Developer-Signature: v=1; a=openpgp-sha256; l=1605; i=chuck.lever@oracle.com; h=from:subject; bh=uLjWKL2TqJ36N4+l0iS/D1tISUCfw5s2iZUTBBNq+1k=; b=owEBbQKS/ZANAwAIATNqszNvZn+XAcsmYgBnaabiYpTlpGfVrZeCU0efdCBKJxwWnKgtiAbre 2PvUG+5zNaJAjMEAAEIAB0WIQQosuWwEobfJDzyPv4zarMzb2Z/lwUCZ2mm4gAKCRAzarMzb2Z/ lxXFD/4gIhUIYJ4tekDCLqHCZ27ots+bRfNyYH493H/aeqk2mFugOLhUJDtjzJzDelc94rYvomk C223ZwtzPahjNeHliMv4yn7/0diCrfL08ODvppDBd3XASg3NA3eHPYH8DzUdsADAa5R6JfKjbZe nB71/GFFBSyTWPzFaxyBFFSAQQBNzfrenS/akiHQU8wki/lLghjM5zZDrrJXWwK0NnwKpAOCHLR qCK4w2FsCkeV/EIQoCwZ4p139kFa9WUz79oWjuH0hXaQJ/zWaVFsONRP2cxJb0g5XFyUPec1/eO qr3Zrul7aW3bW7oOAgdljYOH86t7a21jdpypLDQwc3DqZCBM1qSk8E1JAoH2VwbI9l/fLKw9pba caqIQ/tdXYdZNGJ34O0pGq1MOsH7IyDa/Qmk49k7R7pTxbvRY6POBPuD+0aUOLjvu9vIKlE/ibC a/0BDwDMxjyvPiYNje+2OFqjZ2GgLy+aoDaVnNiGy/pcMDFyYbj7pwXnb7ehhbV0tE9/azcS3pg G6v7qpDSaLvO+2yHZ8mY2y/m0VGyM3H+w10edkS2fsWBhXd3DIi+0pTV/aQ9G4zuK/EtoEMvvxo EvNyBkldcbJuSdK5D/AaWlY9up2BDCzl9TcSxdneEhrBR0ImavKWclEPotZtsjSjHuDvycjZLni dM5h86zR9tz43Fg== X-Developer-Key: i=chuck.lever@oracle.com; a=openpgp; fpr=28B2E5B01286DF243CF23EFE336AB3336F667F97 From: Chuck Lever A subtlety of this API is that if the @nbytes region traverses a page boundary, the next __xdr_commit_encode will shift the data item in the XDR encode buffer. This makes the returned pointer point to something else, leading to unexpected behavior. There are a few cases where the caller saves the returned pointer and then later uses it to insert a computed value into an earlier part of the stream. This can be safe only if either: - the data item is guaranteed to be in the XDR buffer's head, and thus is not ever going to be near a page boundary, or - the data item is no larger than 4 octets, since XDR alignment rules require all data items to start on 4-octet boundaries But that safety is only an artifact of the current implementation. It would be less brittle if these "safe" uses were eventually replaced. Signed-off-by: Chuck Lever --- net/sunrpc/xdr.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/net/sunrpc/xdr.c b/net/sunrpc/xdr.c index 62e07c330a66..f198bb043e2f 100644 --- a/net/sunrpc/xdr.c +++ b/net/sunrpc/xdr.c @@ -1097,6 +1097,9 @@ static noinline __be32 *xdr_get_next_encode_buffer(struct xdr_stream *xdr, * Checks that we have enough buffer space to encode 'nbytes' more * bytes of data. If so, update the total xdr_buf length, and * adjust the length of the current kvec. + * + * The returned pointer is valid only until the next call to + * xdr_reserve_space() or xdr_commit_encode() on this stream. */ __be32 * xdr_reserve_space(struct xdr_stream *xdr, size_t nbytes) {