* [PATCH] Fix memset in NFS zap caches
@ 2012-09-03 18:35 Andi Kleen
2012-09-03 18:39 ` Myklebust, Trond
0 siblings, 1 reply; 5+ messages in thread
From: Andi Kleen @ 2012-09-03 18:35 UTC (permalink / raw)
To: Trond.Myklebust, linux-kernel
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 <ak@linux.intel.com>
diff --git a/fs/nfs/inode.c b/fs/nfs/inode.c
index c6e895f..69e7f0f 100644
--- a/fs/nfs/inode.c
+++ b/fs/nfs/inode.c
@@ -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
--
ak@linux.intel.com -- Speaking for myself only.
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] Fix memset in NFS zap caches
2012-09-03 18:35 [PATCH] Fix memset in NFS zap caches Andi Kleen
@ 2012-09-03 18:39 ` Myklebust, Trond
2012-09-03 18:47 ` Andi Kleen
0 siblings, 1 reply; 5+ messages in thread
From: Myklebust, Trond @ 2012-09-03 18:39 UTC (permalink / raw)
To: Andi Kleen; +Cc: linux-kernel
[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 1828 bytes --]
On Mon, 2012-09-03 at 20:35 +0200, Andi Kleen wrote:
> 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 <ak@linux.intel.com>
>
> diff --git a/fs/nfs/inode.c b/fs/nfs/inode.c
> index c6e895f..69e7f0f 100644
> --- a/fs/nfs/inode.c
> +++ b/fs/nfs/inode.c
> @@ -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
--
Trond Myklebust
Linux NFS client maintainer
NetApp
Trond.Myklebust@netapp.com
www.netapp.com
ÿôèº{.nÇ+·®+%Ëÿ±éݶ\x17¥wÿº{.nÇ+·¥{±þG«éÿ{ayº\x1dÊÚë,j\a¢f£¢·hïêÿêçz_è®\x03(éÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?¨èÚ&£ø§~á¶iOæ¬z·vØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?I¥
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] Fix memset in NFS zap caches
2012-09-03 18:39 ` Myklebust, Trond
@ 2012-09-03 18:47 ` Andi Kleen
2012-09-03 18:52 ` Myklebust, Trond
[not found] ` <1346698368.3648.6.camel@lade.trondhjem.org>
0 siblings, 2 replies; 5+ messages in thread
From: Andi Kleen @ 2012-09-03 18:47 UTC (permalink / raw)
To: Myklebust, Trond; +Cc: Andi Kleen, linux-kernel
> 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
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] Fix memset in NFS zap caches
2012-09-03 18:47 ` Andi Kleen
@ 2012-09-03 18:52 ` Myklebust, Trond
[not found] ` <1346698368.3648.6.camel@lade.trondhjem.org>
1 sibling, 0 replies; 5+ messages in thread
From: Myklebust, Trond @ 2012-09-03 18:52 UTC (permalink / raw)
To: Andi Kleen; +Cc: linux-kernel
[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 899 bytes --]
On Mon, 2012-09-03 at 20:47 +0200, Andi Kleen wrote:
> > 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.
--
Trond Myklebust
Linux NFS client maintainer
NetApp
Trond.Myklebust@netapp.com
www.netapp.com
ÿôèº{.nÇ+·®+%Ëÿ±éݶ\x17¥wÿº{.nÇ+·¥{±þG«éÿ{ayº\x1dÊÚë,j\a¢f£¢·hïêÿêçz_è®\x03(éÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?¨èÚ&£ø§~á¶iOæ¬z·vØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?I¥
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] Fix memset in NFS zap caches
[not found] ` <1346698368.3648.6.camel@lade.trondhjem.org>
@ 2012-09-03 19:02 ` Myklebust, Trond
0 siblings, 0 replies; 5+ messages in thread
From: Myklebust, Trond @ 2012-09-03 19:02 UTC (permalink / raw)
To: Andi Kleen; +Cc: linux-kernel
[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 4386 bytes --]
On Mon, 2012-09-03 at 14:52 -0400, Trond Myklebust wrote:
> On Mon, 2012-09-03 at 20:47 +0200, Andi Kleen wrote:
> > > 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.
>
This should do the right thing:
8<------------------------------------------------------------------
>From c8879cbdf7c4697e450c4a001f24b88e04b70857 Mon Sep 17 00:00:00 2001
From: Trond Myklebust <Trond.Myklebust@netapp.com>
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 <andi@firstfloor.org>
Signed-off-by: Trond Myklebust <Trond.Myklebust@netapp.com>
---
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(-)
diff --git a/fs/nfs/inode.c b/fs/nfs/inode.c
index c6e895f..9b47610 100644
--- a/fs/nfs/inode.c
+++ b/fs/nfs/inode.c
@@ -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
diff --git a/fs/nfs/nfs3proc.c b/fs/nfs/nfs3proc.c
index d6b3b5f..6932209 100644
--- a/fs/nfs/nfs3proc.c
+++ b/fs/nfs/nfs3proc.c
@@ -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,
diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
index 74f5c26..1e50326 100644
--- a/fs/nfs/nfs4proc.c
+++ b/fs/nfs/nfs4proc.c
@@ -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;
}
diff --git a/include/linux/nfs_fs.h b/include/linux/nfs_fs.h
index 1f8fc7f..4b03f56 100644
--- a/include/linux/nfs_fs.h
+++ b/include/linux/nfs_fs.h
@@ -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);
--
1.7.11.4
--
Trond Myklebust
Linux NFS client maintainer
NetApp
Trond.Myklebust@netapp.com
www.netapp.com
ÿôèº{.nÇ+·®+%Ëÿ±éݶ\x17¥wÿº{.nÇ+·¥{±þG«éÿ{ayº\x1dÊÚë,j\a¢f£¢·hïêÿêçz_è®\x03(éÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?¨èÚ&£ø§~á¶iOæ¬z·vØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?I¥
^ permalink raw reply related [flat|nested] 5+ messages in thread
end of thread, other threads:[~2012-09-03 19:02 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2012-09-03 18:35 [PATCH] Fix memset in NFS zap caches Andi Kleen
2012-09-03 18:39 ` Myklebust, Trond
2012-09-03 18:47 ` Andi Kleen
2012-09-03 18:52 ` Myklebust, Trond
[not found] ` <1346698368.3648.6.camel@lade.trondhjem.org>
2012-09-03 19:02 ` Myklebust, Trond
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).