Skip to contentSkip to conversation

Fix memset in NFS zap caches

PATCH In review Andi Kleen 2 participants 5 messages Sep 3, 2012 1 message not in the archive NFS client

lore
Share & tools
lore

Andi Kleen

author
#0 lore raw

PATCH 1/1 Fix memset in NFS zap caches

Fix memset in nfs_zap_caches_locked

This memset overruns the buffer by 4 bytes on 64bit systems.

gcc 4.8 correct complains:

/backup/lsrc/git/linux-lto-2.6/fs/nfs/inode.c: In function 'nfs_zap_caches_locked':
/backup/lsrc/git/linux-lto-2.6/fs/nfs/inode.c:157:41: warning: argument to 'sizeof' in 'memset' call is the same pointer type '__be32 *' as the destination; expected '__be32' or an explicit length [-Wsizeof-pointer-memaccess]

  memset(NFS_COOKIEVERF(inode), 0, sizeof(NFS_COOKIEVERF(inode)));
                                         ^

Add a * to sizeof the correct type.

Signed-off-by Andi Kleen <[email protected]>
fs/nfs/inode.c +1 -1
index c6e895f..69e7f0f 100644
@@ -154,7 +154,7 @@ static void nfs_zap_caches_locked(struct inode *inode)
 	nfsi->attrtimeo = NFS_MINATTRTIMEO(inode);
 	nfsi->attrtimeo_timestamp = jiffies;
 
-	memset(NFS_COOKIEVERF(inode), 0, sizeof(NFS_COOKIEVERF(inode)));
+	memset(NFS_COOKIEVERF(inode), 0, sizeof(*NFS_COOKIEVERF(inode)));
 	if (S_ISREG(mode) || S_ISDIR(mode) || S_ISLNK(mode))
 		nfsi->cache_validity |= NFS_INO_INVALID_ATTR|NFS_INO_INVALID_DATA|NFS_INO_INVALID_ACCESS|NFS_INO_INVALID_ACL|NFS_INO_REVAL_PAGECACHE;
 	else
Signature · 1 line
[email protected] -- Speaking for myself only.
#1 lore raw
Quoted 23 lines · Andi Kleen

Fix memset in nfs_zap_caches_locked

This memset overruns the buffer by 4 bytes on 64bit systems.

gcc 4.8 correct complains:

/backup/lsrc/git/linux-lto-2.6/fs/nfs/inode.c: In function 'nfs_zap_caches_locked':
/backup/lsrc/git/linux-lto-2.6/fs/nfs/inode.c:157:41: warning: argument to 'sizeof' in 'memset' call is the same pointer type '__be32 *' as the destination; expected '__be32' or an explicit length [-Wsizeof-pointer-memaccess]

  memset(NFS_COOKIEVERF(inode), 0, sizeof(NFS_COOKIEVERF(inode)));
                                         ^

Add a * to sizeof the correct type.

Signed-off-by Andi Kleen <[email protected]>
fs/nfs/inode.c +0 -0
index c6e895f..69e7f0f 100644

fs/nfs/inode.c static void nfs_zap_caches_locked(struct inode *inode) · line 160

Quoted 8 lines
fs/nfs/inode.c +1 -1
@@ -154,7 +154,7 @@ static void nfs_zap_caches_locked(struct inode *inode)
 	nfsi->attrtimeo = NFS_MINATTRTIMEO(inode);
 	nfsi->attrtimeo_timestamp = jiffies;
 
-	memset(NFS_COOKIEVERF(inode), 0, sizeof(NFS_COOKIEVERF(inode)));
+	memset(NFS_COOKIEVERF(inode), 0, sizeof(*NFS_COOKIEVERF(inode)));
 	if (S_ISREG(mode) || S_ISDIR(mode) || S_ISLNK(mode))
 		nfsi->cache_validity |= NFS_INO_INVALID_ATTR|NFS_INO_INVALID_DATA|NFS_INO_INVALID_ACCESS|NFS_INO_INVALID_ACL|NFS_INO_REVAL_PAGECACHE;
 	else

Hi Andi,

No, this is a gcc bug.

NFS_COOKIEVERF(inode) resolves to an array, so the current code is correct. The above change will cause the 2nd half of the array to remain uninitialised...

Cheers
Trond

Signature · 8 lines
Trond Myklebust
Linux NFS client maintainer

NetApp
[email protected]
www.netapp.com

���{.n�+�������+%�����ݶ��w��{.n�+����{��G�����{ay�ʇڙ�,j��f���h��������z_�(�階�ݢj"���m������G������?����&���~��iO��z��v�^�m����������?�I�
author
#2 lore raw
Quoted 5 lines

No, this is a gcc bug.

NFS_COOKIEVERF(inode) resolves to an array, so the current code is correct. The above change will cause the 2nd half of the array to remain uninitialised...

Are you sure?

include/linux/nfs_fs.h:268:static inline __be32 *NFS_COOKIEVERF(const struct inode *inode)

That doesn't look like an array type to me.

-Andi

#3 lore raw
Quoted 11 lines · Andi Kleen
Quoted 5 lines

No, this is a gcc bug.

NFS_COOKIEVERF(inode) resolves to an array, so the current code is correct. The above change will cause the 2nd half of the array to remain uninitialised...

Are you sure?

include/linux/nfs_fs.h:268:static inline __be32 *NFS_COOKIEVERF(const struct inode *inode)

That doesn't look like an array type to me.

Argh... It used to be a #define, but got converted in the commit 99fadcd7646 static inline blitz...

OK, let's just get rid of the NFS_COOKIEVERF thing altogether. At this point it is clearly just obfuscating the code.

Signature · 8 lines
Trond Myklebust
Linux NFS client maintainer

NetApp
[email protected]
www.netapp.com

���{.n�+�������+%�����ݶ��w��{.n�+����{��G�����{ay�ʇڙ�,j��f���h��������z_�(�階�ݢj"���m������G������?����&���~��iO��z��v�^�m����������?�I�

A message that is not in the archive (sent off-list or not yet received): [email protected]

#5 lore raw
Quoted 11 lines · Trond Myklebust
Quoted 10 lines · Andi Kleen
Quoted 5 lines

No, this is a gcc bug.

NFS_COOKIEVERF(inode) resolves to an array, so the current code is correct. The above change will cause the 2nd half of the array to remain uninitialised...

Are you sure?

include/linux/nfs_fs.h:268:static inline __be32 *NFS_COOKIEVERF(const struct inode *inode)

Quoted 8 lines
Quoted 1 line

That doesn't look like an array type to me.

Argh... It used to be a #define, but got converted in the commit 99fadcd7646 static inline blitz...

OK, let's just get rid of the NFS_COOKIEVERF thing altogether. At this point it is clearly just obfuscating the code.

This should do the right thing:

8<------------------------------------------------------------------

Quoted 1 line

From c8879cbdf7c4697e450c4a001f24b88e04b70857 Mon Sep 17 00:00:00 2001

From: Trond Myklebust <[email protected]>
Date: Mon, 3 Sep 2012 14:56:02 -0400
Subject: [PATCH] NFS: Fix the initialisation of the readdir 'cookieverf'
array

When the NFS_COOKIEVERF helper macro was converted into a static inline function, we broke the initialisation of the readdir cookies, since it depended on a 'sizeof(NFS_COOKIEVERF(inode))'.

At this point, NFS_COOKIEVERF seems to be more of an obfuscation than a helper, so the best thing would be to just get rid of it.

Reported-by Andi Kleen <[email protected]>
Signed-off-by Trond Myklebust <[email protected]>
---
 fs/nfs/inode.c         | 2 +-
 fs/nfs/nfs3proc.c      | 2 +-
 fs/nfs/nfs4proc.c      | 4 ++--
 include/linux/nfs_fs.h | 5 -----
 4 files changed, 4 insertions(+), 9 deletions(-)
fs/nfs/inode.c +1 -1
index c6e895f..9b47610 100644
@@ -154,7 +154,7 @@ static void nfs_zap_caches_locked(struct inode *inode)
 	nfsi->attrtimeo = NFS_MINATTRTIMEO(inode);
 	nfsi->attrtimeo_timestamp = jiffies;
 
-	memset(NFS_COOKIEVERF(inode), 0, sizeof(NFS_COOKIEVERF(inode)));
+	memset(NFS_I(inode)->cookieverf, 0, sizeof(NFS_I(inode)->cookieverf));
 	if (S_ISREG(mode) || S_ISDIR(mode) || S_ISLNK(mode))
 		nfsi->cache_validity |= NFS_INO_INVALID_ATTR|NFS_INO_INVALID_DATA|NFS_INO_INVALID_ACCESS|NFS_INO_INVALID_ACL|NFS_INO_REVAL_PAGECACHE;
 	else
fs/nfs/nfs3proc.c +1 -1
index d6b3b5f..6932209 100644
@@ -643,7 +643,7 @@ nfs3_proc_readdir(struct dentry *dentry, struct rpc_cred *cred,
 		  u64 cookie, struct page **pages, unsigned int count, int plus)
 {
 	struct inode		*dir = dentry->d_inode;
-	__be32			*verf = NFS_COOKIEVERF(dir);
+	__be32			*verf = NFS_I(dir)->cookieverf;
 	struct nfs3_readdirargs	arg = {
 		.fh		= NFS_FH(dir),
 		.cookie		= cookie,
fs/nfs/nfs4proc.c +2 -2
index 74f5c26..1e50326 100644
@@ -3215,11 +3215,11 @@ static int _nfs4_proc_readdir(struct dentry *dentry, struct rpc_cred *cred,
 			dentry->d_parent->d_name.name,
 			dentry->d_name.name,
 			(unsigned long long)cookie);
-	nfs4_setup_readdir(cookie, NFS_COOKIEVERF(dir), dentry, &args);
+	nfs4_setup_readdir(cookie, NFS_I(dir)->cookieverf, dentry, &args);
 	res.pgbase = args.pgbase;
 	status = nfs4_call_sync(NFS_SERVER(dir)->client, NFS_SERVER(dir), &msg, &args.seq_args, &res.seq_res, 0);
 	if (status >= 0) {
-		memcpy(NFS_COOKIEVERF(dir), res.verifier.data, NFS4_VERIFIER_SIZE);
+		memcpy(NFS_I(dir)->cookieverf, res.verifier.data, NFS4_VERIFIER_SIZE);
 		status += args.pgbase;
 	}
include/linux/nfs_fs.h +0 -5
index 1f8fc7f..4b03f56 100644
@@ -265,11 +265,6 @@ static inline const struct nfs_rpc_ops *NFS_PROTO(const struct inode *inode)
 	return NFS_SERVER(inode)->nfs_client->rpc_ops;
 }
 
-static inline __be32 *NFS_COOKIEVERF(const struct inode *inode)
-{
-	return NFS_I(inode)->cookieverf;
-}
-
 static inline unsigned NFS_MINATTRTIMEO(const struct inode *inode)
 {
 	struct nfs_server *nfss = NFS_SERVER(inode);
Signature · 12 lines
1.7.11.4


-- 
Trond Myklebust
Linux NFS client maintainer

NetApp
[email protected]
www.netapp.com

���{.n�+�������+%�����ݶ��w��{.n�+����{��G�����{ay�ʇڙ�,j��f���h��������z_�(�階�ݢj"���m������G������?����&���~��iO��z��v�^�m����������?�I�