Skip to contentSkip to conversation

Make nfs_file_cred more robust.

PATCH/RFC Neil Brown 2 participants 3 messages Oct 15, 2008 – Oct 16, 2008 1 message not in the archive

Subsystems (2)
NFS client NFS server
lore
Share & tools
lore

Neil Brown

author
#0 lore raw

Hi Trond,
I wonder if you would consider including the following patch which makes nfs_file_cred a little more robust.

It isn't strictly needed with current mainline. However we have a patch set which (among other things) passes a 'struct file' down (via ATTR_FILE) for a chmod call.
If chmod is called on e.g. a device special file, this will cause nfs3_proc_setattr to call nfs_file_cred on a 'struct file' which does not have an associated open context or credential. That goes Oops.

Thanks for your consideration,
NeilBrown

--------------

From: NeilBrown <[email protected]>

As not all files have an associated open_context (e.g. device special files), it is safest to test for the existence of the open context before de-referencing it.

Signed-off-by NeilBrown <[email protected]>
include/linux/nfs_fs.h +6 -2
index 78a5922..63bf8f0 100644
@@ -372,8 +372,12 @@ static inline struct nfs_open_context *nfs_file_open_context(struct file *filp)
 
 static inline struct rpc_cred *nfs_file_cred(struct file *file)
 {
-	if (file != NULL)
-		return nfs_file_open_context(file)->cred;
+	if (file != NULL) {
+		struct nfs_open_context *ctx =
+			nfs_file_open_context(file);
+		if (ctx)
+			return ctx->cred;
+	}
 	return NULL;
 }

A message that is not in the archive (sent off-list or not yet received): 18678.28273.285015.747371-wvvUuzkyo1EYVZTmpyfIwg@public.gmane.org

#2 lore raw
Quoted 11 lines · Neil Brown

Hi Trond,
I wonder if you would consider including the following patch which makes nfs_file_cred a little more robust.

It isn't strictly needed with current mainline. However we have a patch set which (among other things) passes a 'struct file' down (via ATTR_FILE) for a chmod call.
If chmod is called on e.g. a device special file, this will cause nfs3_proc_setattr to call nfs_file_cred on a 'struct file' which does not have an associated open context or credential. That goes Oops.

Quoted 2 lines

Thanks for your consideration,
NeilBrown

Hi Neil,

Won't this still end up exploding in nfs4_proc_setattr?

Cheers
Trond

Quoted 30 lines

--------------

From: NeilBrown <[email protected]>

As not all files have an associated open_context (e.g. device special files), it is safest to test for the existence of the open context before de-referencing it.

Signed-off-by NeilBrown <[email protected]>
include/linux/nfs_fs.h +6 -2
index 78a5922..63bf8f0 100644
@@ -372,8 +372,12 @@ static inline struct nfs_open_context *nfs_file_open_context(struct file *filp)
 
 static inline struct rpc_cred *nfs_file_cred(struct file *file)
 {
-	if (file != NULL)
-		return nfs_file_open_context(file)->cred;
+	if (file != NULL) {
+		struct nfs_open_context *ctx =
+			nfs_file_open_context(file);
+		if (ctx)
+			return ctx->cred;
+	}
 	return NULL;
 }
Signature · 6 lines
Trond Myklebust
Linux NFS client maintainer

NetApp
[email protected]
www.netapp.com
author
#3 lore raw
Quoted 12 lines · On Wednesday October 15, [email protected] wrote:
Quoted 11 lines · Neil Brown

Hi Trond,
I wonder if you would consider including the following patch which makes nfs_file_cred a little more robust.

It isn't strictly needed with current mainline. However we have a patch set which (among other things) passes a 'struct file' down (via ATTR_FILE) for a chmod call.
If chmod is called on e.g. a device special file, this will cause nfs3_proc_setattr to call nfs_file_cred on a 'struct file' which does not have an associated open context or credential. That goes Oops.

Quoted 7 lines
Quoted 2 lines

Thanks for your consideration,
NeilBrown

Hi Neil,

Won't this still end up exploding in nfs4_proc_setattr?

Yes, of course. I guess NFSv4 wasn't tested.

How's this?

Thanks,
NeilBrown

From: Neil Brown <[email protected]>
Subject: [PATCH] Make nfs_file_cred more robust.

As not all files have an associated open_context (e.g. device special files), it is safest to test for the existence of the open context before de-referencing it.

Signed-off-by NeilBrown <[email protected]>
---
 fs/nfs/nfs4proc.c      |    6 ++++--
 include/linux/nfs_fs.h |    8 ++++++--
 2 files changed, 10 insertions(+), 4 deletions(-)
fs/nfs/nfs4proc.c +4 -2
index c910413..83e700a 100644
@@ -1659,8 +1659,10 @@ nfs4_proc_setattr(struct dentry *dentry, struct nfs_fattr *fattr,
 		struct nfs_open_context *ctx;
 
 		ctx = nfs_file_open_context(sattr->ia_file);
-		cred = ctx->cred;
-		state = ctx->state;
+		if (ctx) {
+			cred = ctx->cred;
+			state = ctx->state;
+		}
 	}
 
 	status = nfs4_do_setattr(inode, cred, fattr, sattr, state);
include/linux/nfs_fs.h +6 -2
index 78a5922..63bf8f0 100644
@@ -372,8 +372,12 @@ static inline struct nfs_open_context *nfs_file_open_context(struct file *filp)
 
 static inline struct rpc_cred *nfs_file_cred(struct file *file)
 {
-	if (file != NULL)
-		return nfs_file_open_context(file)->cred;
+	if (file != NULL) {
+		struct nfs_open_context *ctx =
+			nfs_file_open_context(file);
+		if (ctx)
+			return ctx->cred;
+	}
 	return NULL;
 }
Signature · 1 line
1.5.6.5