From: Sasha Levin To: linux-kernel@vger.kernel.org, stable@vger.kernel.org Cc: Dmitry Mastykin , Trond Myklebust , Sasha Levin , trondmy@kernel.org, anna@kernel.org, linux-nfs@vger.kernel.org Subject: [PATCH AUTOSEL 6.9 01/21] NFSv4: Fix memory leak in nfs4_set_security_label Date: Sun, 23 Jun 2024 09:43:34 -0400 Message-ID: <20240623134405.809025-1-sashal@kernel.org> X-Mailing-List: linux-kernel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 X-stable: review X-Patchwork-Hint: Ignore X-stable-base: Linux 6.9.6 Content-Transfer-Encoding: 8bit Xref: photonic.trudheim.com org.kernel.vger.linux-kernel:1255936 org.kernel.vger.linux-nfs:86239 Newsgroups: org.kernel.vger.linux-kernel,org.kernel.vger.linux-nfs,org.kernel.vger.stable Path: photonic.trudheim.com!nntp.lore.kernel.org!not-for-mail From: Dmitry Mastykin [ Upstream commit aad11473f8f4be3df86461081ce35ec5b145ba68 ] We leak nfs_fattr and nfs4_label every time we set a security xattr. Signed-off-by: Dmitry Mastykin Signed-off-by: Trond Myklebust Signed-off-by: Sasha Levin --- fs/nfs/nfs4proc.c | 1 + 1 file changed, 1 insertion(+) diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c index 3a816c4a6d5e2..a691fa10b3e95 100644 --- a/fs/nfs/nfs4proc.c +++ b/fs/nfs/nfs4proc.c @@ -6289,6 +6289,7 @@ nfs4_set_security_label(struct inode *inode, const void *buf, size_t buflen) if (status == 0) nfs_setsecurity(inode, fattr); + nfs_free_fattr(fattr); return status; } #endif /* CONFIG_NFS_V4_SECURITY_LABEL */ -- 2.43.0 . From: Sasha Levin To: linux-kernel@vger.kernel.org, stable@vger.kernel.org Cc: Dmitry Mastykin , Trond Myklebust , Sasha Levin , trondmy@kernel.org, anna@kernel.org, linux-nfs@vger.kernel.org Subject: [PATCH AUTOSEL 6.6 01/16] NFSv4: Fix memory leak in nfs4_set_security_label Date: Sun, 23 Jun 2024 09:44:30 -0400 Message-ID: <20240623134448.809470-1-sashal@kernel.org> X-Mailing-List: linux-nfs@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 X-stable: review X-Patchwork-Hint: Ignore X-stable-base: Linux 6.6.35 Content-Transfer-Encoding: 8bit Xref: photonic.trudheim.com org.kernel.vger.linux-nfs:86243 org.kernel.vger.linux-kernel:1255955 Newsgroups: org.kernel.vger.linux-nfs,org.kernel.vger.linux-kernel,org.kernel.vger.stable Path: photonic.trudheim.com!nntp.lore.kernel.org!not-for-mail From: Dmitry Mastykin [ Upstream commit aad11473f8f4be3df86461081ce35ec5b145ba68 ] We leak nfs_fattr and nfs4_label every time we set a security xattr. Signed-off-by: Dmitry Mastykin Signed-off-by: Trond Myklebust Signed-off-by: Sasha Levin --- fs/nfs/nfs4proc.c | 1 + 1 file changed, 1 insertion(+) diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c index f0953200acd08..05490d4784f1a 100644 --- a/fs/nfs/nfs4proc.c +++ b/fs/nfs/nfs4proc.c @@ -6268,6 +6268,7 @@ nfs4_set_security_label(struct inode *inode, const void *buf, size_t buflen) if (status == 0) nfs_setsecurity(inode, fattr); + nfs_free_fattr(fattr); return status; } #endif /* CONFIG_NFS_V4_SECURITY_LABEL */ -- 2.43.0 . From: Sasha Levin To: linux-kernel@vger.kernel.org, stable@vger.kernel.org Cc: Dmitry Mastykin , Trond Myklebust , Sasha Levin , trondmy@kernel.org, anna@kernel.org, linux-nfs@vger.kernel.org Subject: [PATCH AUTOSEL 6.1 01/12] NFSv4: Fix memory leak in nfs4_set_security_label Date: Sun, 23 Jun 2024 09:45:04 -0400 Message-ID: <20240623134518.809802-1-sashal@kernel.org> X-Mailing-List: linux-kernel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 X-stable: review X-Patchwork-Hint: Ignore X-stable-base: Linux 6.1.95 Content-Transfer-Encoding: 8bit Xref: photonic.trudheim.com org.kernel.vger.linux-kernel:1255974 org.kernel.vger.linux-nfs:86247 Newsgroups: org.kernel.vger.linux-kernel,org.kernel.vger.linux-nfs,org.kernel.vger.stable Path: photonic.trudheim.com!nntp.lore.kernel.org!not-for-mail From: Dmitry Mastykin [ Upstream commit aad11473f8f4be3df86461081ce35ec5b145ba68 ] We leak nfs_fattr and nfs4_label every time we set a security xattr. Signed-off-by: Dmitry Mastykin Signed-off-by: Trond Myklebust Signed-off-by: Sasha Levin --- fs/nfs/nfs4proc.c | 1 + 1 file changed, 1 insertion(+) diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c index ec641a8f6604b..cc620fc7aaf7b 100644 --- a/fs/nfs/nfs4proc.c +++ b/fs/nfs/nfs4proc.c @@ -6274,6 +6274,7 @@ nfs4_set_security_label(struct inode *inode, const void *buf, size_t buflen) if (status == 0) nfs_setsecurity(inode, fattr); + nfs_free_fattr(fattr); return status; } #endif /* CONFIG_NFS_V4_SECURITY_LABEL */ -- 2.43.0 . Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: 7bit X-Mailing-List: linux-nfs@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 From: "NeilBrown" To: "Mike Snitzer" Cc: linux-nfs@vger.kernel.org, "Jeff Layton" , "Chuck Lever" , "Trond Myklebust" , snitzer@hammerspace.com Subject: Re: [PATCH v6 06/18] nfs/nfsd: add "localio" support In-reply-to: References: <>, Date: Mon, 24 Jun 2024 08:27:39 +1000 Message-id: <171918165963.14261.959545364150864599@noble.neil.brown.name> Xref: photonic.trudheim.com org.kernel.vger.linux-nfs:86250 Newsgroups: org.kernel.vger.linux-nfs Path: photonic.trudheim.com!nntp.lore.kernel.org!not-for-mail On Sat, 22 Jun 2024, Mike Snitzer wrote: > On Fri, Jun 21, 2024 at 04:08:20PM +1000, NeilBrown wrote: > > > > Both branches of this if() do exactly the same thing. iov_iter_advance > > is a no-op if the size arg is zero. > > iov_iter_advance doesn't look to be a no-op if the size arg is zero. void iov_iter_advance(struct iov_iter *i, size_t size) { if (unlikely(i->count < size)) size = i->count; if (likely(iter_is_ubuf(i)) || unlikely(iov_iter_is_xarray(i))) { i->iov_offset += size; i->count -= size; } else if (likely(iter_is_iovec(i) || iov_iter_is_kvec(i))) { /* iovec and kvec have identical layouts */ iov_iter_iovec_advance(i, size); } else if (iov_iter_is_bvec(i)) { iov_iter_bvec_advance(i, size); } else if (iov_iter_is_discard(i)) { i->count -= size; } } This adds "size" to offset, and subtracts "size" from count. For iovec and bvec it is a slightly complicated dance to achieve this, but that is the net result. So if "size" is zero there is no change to the iov_iter. Just some wasted cycles. Do those cycles justify the extra conditional branch? I don't know. I would generally prefer simpler code which is only optimised with evidence. Admittedly I don't always follow that preference myself and I won't hold you to it. But I thought the review would be incomplete without mentioning it. > > > Is it really worth increasing the code size to sometimes avoid a > > function call? > > > > At least we should for the iov_iter_bvec() inconditionally, then maybe > > call _advance(). > > For v7, I've fixed it so we do what you suggest. Thanks. > > > +static void > > > +nfs_get_vfs_attr(struct file *filp, struct nfs_fattr *fattr) > > > +{ > > > + struct kstat stat; > > > + > > > + if (fattr != NULL && vfs_getattr(&filp->f_path, &stat, > > > + STATX_INO | > > > + STATX_ATIME | > > > + STATX_MTIME | > > > + STATX_CTIME | > > > + STATX_SIZE | > > > + STATX_BLOCKS, > > > + AT_STATX_SYNC_AS_STAT) == 0) { > > > + fattr->valid = NFS_ATTR_FATTR_FILEID | > > > + NFS_ATTR_FATTR_CHANGE | > > > + NFS_ATTR_FATTR_SIZE | > > > + NFS_ATTR_FATTR_ATIME | > > > + NFS_ATTR_FATTR_MTIME | > > > + NFS_ATTR_FATTR_CTIME | > > > + NFS_ATTR_FATTR_SPACE_USED; > > > + fattr->fileid = stat.ino; > > > + fattr->size = stat.size; > > > + fattr->atime = stat.atime; > > > + fattr->mtime = stat.mtime; > > > + fattr->ctime = stat.ctime; > > > + fattr->change_attr = nfs_timespec_to_change_attr(&fattr->ctime); > > > > This looks wrong for NFSv4. I think we should use > > nfsd4_change_attribute(). > > Maybe it isn't important, but if it isn't I'd like to see an explanation > > why. > > > > > + fattr->du.nfs3.used = stat.blocks << 9; > > > + } > > > +} > > Not following, this is client code so it doesn't have access to > nfsd4_change_attribute(). This is nfs-localio code which blurs the boundary between server and client... The change_attr is used by NFS to detect if a file might have changed. This code is used to get the attributes after a write request. NFS uses a GETATTR request to the server at other times. The change_attr should be consistent between the two else comparisons will be meaningless. So I think that nfs_get_vfs_attr() should use the same change_attr as the one that would be used if the NFS GETATTR request were made. For NFSv3, that is nfs_timespec_to_change_attr() as you have. For NFSv4 it is something different. I think that having inconsistent change_attrs could cause NFS to flush its page cache unnecessarily. As it can read directly from the server-side where is likely is cached, that might not be a problem. If that reasoning does apply it should be explained. However there is talk of exporting the "i_version" number to userspace through xattr. For NFS that is essentially the change_attr. If we did that we would really want to keep the number consistent. We could easily move nfsd4_change_attribute() into nfs_common or even make it an inline in some common include file. It doesn't use any nfsd internals. Thanks, NeilBrown > > Pending clarification, and further review on my part, leaving this > item to one side (so won't be addressed in v7). > > Thanks, > Mike > .