diff mbox

[4/4] nfsd: vfs_llseek() with 32 or 64 bit offsets (hashes)

Message ID 20110808153813.1872437.44997.stgit@fsdevel3 (mailing list archive)
State New, archived
Headers show

Commit Message

Bernd Schubert Aug. 8, 2011, 3:38 p.m. UTC 
Use 32-bit or 64-bit llseek() hashes for directory offsets depending on
the NFS version. NFSv2 gets 32-bit hashes only.

NOTE: This patch got rather complex as Christoph asked to set the
filp->f_mode flag in the open call or immediatly after dentry_open()
in nfsd_open() to avoid races.
Personally I still do not see a reason for that and in my opinion
FMODE_32BITHASH/FMODE_64BITHASH flags could be set nfsd_readdir(), as it
follows directly after nfsd_open() without a chance of races.


Signed-off-by: Bernd Schubert <bernd.schubert@itwm.fraunhofer.de>
---
 fs/nfsd/vfs.c |   33 +++++++++++++++++++++++----------
 fs/nfsd/vfs.h |    2 ++
 2 files changed, 25 insertions(+), 10 deletions(-)


--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Comments

J. Bruce Fields Aug. 9, 2011, 5:33 p.m. UTC | #1 
On Mon, Aug 08, 2011 at 05:38:13PM +0200, Bernd Schubert wrote:
> Use 32-bit or 64-bit llseek() hashes for directory offsets depending on
> the NFS version. NFSv2 gets 32-bit hashes only.
> 
> NOTE: This patch got rather complex as Christoph asked to set the
> filp->f_mode flag in the open call or immediatly after dentry_open()
> in nfsd_open() to avoid races.
> Personally I still do not see a reason for that and in my opinion
> FMODE_32BITHASH/FMODE_64BITHASH flags could be set nfsd_readdir(), as it
> follows directly after nfsd_open() without a chance of races.

The bulk of the patch seems to be just an access->may_flags rename.
Could you please split that into a separate patch?

--b.

> 
> 
> Signed-off-by: Bernd Schubert <bernd.schubert@itwm.fraunhofer.de>
> ---
>  fs/nfsd/vfs.c |   33 +++++++++++++++++++++++----------
>  fs/nfsd/vfs.h |    2 ++
>  2 files changed, 25 insertions(+), 10 deletions(-)
> 
> diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c
> index fd0acca..4bb517f 100644
> --- a/fs/nfsd/vfs.c
> +++ b/fs/nfsd/vfs.c
> @@ -708,12 +708,13 @@ static int nfsd_open_break_lease(struct inode *inode, int access)
>  
>  /*
>   * Open an existing file or directory.
> - * The access argument indicates the type of open (read/write/lock)
> + * The may_flags argument indicates the type of open (read/write/lock)
> + * and additional flags.
>   * N.B. After this call fhp needs an fh_put
>   */
>  __be32
>  nfsd_open(struct svc_rqst *rqstp, struct svc_fh *fhp, int type,
> -			int access, struct file **filp)
> +			int may_flags, struct file **filp)
>  {
>  	struct dentry	*dentry;
>  	struct inode	*inode;
> @@ -728,7 +729,7 @@ nfsd_open(struct svc_rqst *rqstp, struct svc_fh *fhp, int type,
>  	 * and (hopefully) checked permission - so allow OWNER_OVERRIDE
>  	 * in case a chmod has now revoked permission.
>  	 */
> -	err = fh_verify(rqstp, fhp, type, access | NFSD_MAY_OWNER_OVERRIDE);
> +	err = fh_verify(rqstp, fhp, type, may_flags | NFSD_MAY_OWNER_OVERRIDE);
>  	if (err)
>  		goto out;
>  
> @@ -739,7 +740,7 @@ nfsd_open(struct svc_rqst *rqstp, struct svc_fh *fhp, int type,
>  	 * or any access when mandatory locking enabled
>  	 */
>  	err = nfserr_perm;
> -	if (IS_APPEND(inode) && (access & NFSD_MAY_WRITE))
> +	if (IS_APPEND(inode) && (may_flags & NFSD_MAY_WRITE))
>  		goto out;
>  	/*
>  	 * We must ignore files (but only files) which might have mandatory
> @@ -752,12 +753,12 @@ nfsd_open(struct svc_rqst *rqstp, struct svc_fh *fhp, int type,
>  	if (!inode->i_fop)
>  		goto out;
>  
> -	host_err = nfsd_open_break_lease(inode, access);
> +	host_err = nfsd_open_break_lease(inode, may_flags);
>  	if (host_err) /* NOMEM or WOULDBLOCK */
>  		goto out_nfserr;
>  
> -	if (access & NFSD_MAY_WRITE) {
> -		if (access & NFSD_MAY_READ)
> +	if (may_flags & NFSD_MAY_WRITE) {
> +		if (may_flags & NFSD_MAY_READ)
>  			flags = O_RDWR|O_LARGEFILE;
>  		else
>  			flags = O_WRONLY|O_LARGEFILE;
> @@ -766,8 +767,15 @@ nfsd_open(struct svc_rqst *rqstp, struct svc_fh *fhp, int type,
>  			    flags, current_cred());
>  	if (IS_ERR(*filp))
>  		host_err = PTR_ERR(*filp);
> -	else
> -		host_err = ima_file_check(*filp, access);
> +	else {
> +		host_err = ima_file_check(*filp, may_flags);
> +
> +		if (may_flags & NFSD_MAY_64BIT_COOKIE)
> +			(*filp)->f_mode |= FMODE_64BITHASH;
> +		else
> +			(*filp)->f_mode |= FMODE_32BITHASH;
> +	}
> +
>  out_nfserr:
>  	err = nfserrno(host_err);
>  out:
> @@ -1989,8 +1997,13 @@ nfsd_readdir(struct svc_rqst *rqstp, struct svc_fh *fhp, loff_t *offsetp,
>  	__be32		err;
>  	struct file	*file;
>  	loff_t		offset = *offsetp;
> +	int		flags = NFSD_MAY_READ;
> +
> +	/* NFSv2 only supports 32 bit cookies */
> +	if (rqstp->rq_vers > 2)
> +		flags |= NFSD_MAY_64BIT_COOKIE;
>  
> -	err = nfsd_open(rqstp, fhp, S_IFDIR, NFSD_MAY_READ, &file);
> +	err = nfsd_open(rqstp, fhp, S_IFDIR, flags, &file);
>  	if (err)
>  		goto out;
>  
> diff --git a/fs/nfsd/vfs.h b/fs/nfsd/vfs.h
> index e0bbac0..ecd00e1 100644
> --- a/fs/nfsd/vfs.h
> +++ b/fs/nfsd/vfs.h
> @@ -26,6 +26,8 @@
>  #define NFSD_MAY_NOT_BREAK_LEASE 512
>  #define NFSD_MAY_BYPASS_GSS	1024
>  
> +#define NFSD_MAY_64BIT_COOKIE	2048 /* 64 bit readdir cookies for >= NFSv3 */
> +
>  #define NFSD_MAY_CREATE		(NFSD_MAY_EXEC|NFSD_MAY_WRITE)
>  #define NFSD_MAY_REMOVE		(NFSD_MAY_EXEC|NFSD_MAY_WRITE|NFSD_MAY_TRUNC)
>  
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Bernd Schubert Aug. 10, 2011, 7:13 p.m. UTC | #2 
On 08/09/2011 07:33 PM, J. Bruce Fields wrote:
> On Mon, Aug 08, 2011 at 05:38:13PM +0200, Bernd Schubert wrote:
>> Use 32-bit or 64-bit llseek() hashes for directory offsets depending on
>> the NFS version. NFSv2 gets 32-bit hashes only.
>>
>> NOTE: This patch got rather complex as Christoph asked to set the
>> filp->f_mode flag in the open call or immediatly after dentry_open()
>> in nfsd_open() to avoid races.
>> Personally I still do not see a reason for that and in my opinion
>> FMODE_32BITHASH/FMODE_64BITHASH flags could be set nfsd_readdir(), as it
>> follows directly after nfsd_open() without a chance of races.
>
> The bulk of the patch seems to be just an access->may_flags rename.
> Could you please split that into a separate patch?

Ok, shall I resend the entire patch series, but already remove the 
32-bit nfsd_readdir() cookie patch? Or only just this patch split into 
to parts?


Thanks,
Bernd
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
J. Bruce Fields Aug. 10, 2011, 7:35 p.m. UTC | #3 
On Wed, Aug 10, 2011 at 09:13:09PM +0200, Bernd Schubert wrote:
> On 08/09/2011 07:33 PM, J. Bruce Fields wrote:
> >On Mon, Aug 08, 2011 at 05:38:13PM +0200, Bernd Schubert wrote:
> >>Use 32-bit or 64-bit llseek() hashes for directory offsets depending on
> >>the NFS version. NFSv2 gets 32-bit hashes only.
> >>
> >>NOTE: This patch got rather complex as Christoph asked to set the
> >>filp->f_mode flag in the open call or immediatly after dentry_open()
> >>in nfsd_open() to avoid races.
> >>Personally I still do not see a reason for that and in my opinion
> >>FMODE_32BITHASH/FMODE_64BITHASH flags could be set nfsd_readdir(), as it
> >>follows directly after nfsd_open() without a chance of races.
> >
> >The bulk of the patch seems to be just an access->may_flags rename.
> >Could you please split that into a separate patch?
> 
> Ok, shall I resend the entire patch series, but already remove the
> 32-bit nfsd_readdir() cookie patch? Or only just this patch split
> into to parts?

Probably best to resend.  Who's going to take these patches?

(Looked like it would probably make the most sense for an ext4 tree, as
that looked like the trickiest part?  But I'll take the nfsd4 fix.)

--b.
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c
index fd0acca..4bb517f 100644
--- a/fs/nfsd/vfs.c
+++ b/fs/nfsd/vfs.c
@@ -708,12 +708,13 @@  static int nfsd_open_break_lease(struct inode *inode, int access)
 
 /*
  * Open an existing file or directory.
- * The access argument indicates the type of open (read/write/lock)
+ * The may_flags argument indicates the type of open (read/write/lock)
+ * and additional flags.
  * N.B. After this call fhp needs an fh_put
  */
 __be32
 nfsd_open(struct svc_rqst *rqstp, struct svc_fh *fhp, int type,
-			int access, struct file **filp)
+			int may_flags, struct file **filp)
 {
 	struct dentry	*dentry;
 	struct inode	*inode;
@@ -728,7 +729,7 @@  nfsd_open(struct svc_rqst *rqstp, struct svc_fh *fhp, int type,
 	 * and (hopefully) checked permission - so allow OWNER_OVERRIDE
 	 * in case a chmod has now revoked permission.
 	 */
-	err = fh_verify(rqstp, fhp, type, access | NFSD_MAY_OWNER_OVERRIDE);
+	err = fh_verify(rqstp, fhp, type, may_flags | NFSD_MAY_OWNER_OVERRIDE);
 	if (err)
 		goto out;
 
@@ -739,7 +740,7 @@  nfsd_open(struct svc_rqst *rqstp, struct svc_fh *fhp, int type,
 	 * or any access when mandatory locking enabled
 	 */
 	err = nfserr_perm;
-	if (IS_APPEND(inode) && (access & NFSD_MAY_WRITE))
+	if (IS_APPEND(inode) && (may_flags & NFSD_MAY_WRITE))
 		goto out;
 	/*
 	 * We must ignore files (but only files) which might have mandatory
@@ -752,12 +753,12 @@  nfsd_open(struct svc_rqst *rqstp, struct svc_fh *fhp, int type,
 	if (!inode->i_fop)
 		goto out;
 
-	host_err = nfsd_open_break_lease(inode, access);
+	host_err = nfsd_open_break_lease(inode, may_flags);
 	if (host_err) /* NOMEM or WOULDBLOCK */
 		goto out_nfserr;
 
-	if (access & NFSD_MAY_WRITE) {
-		if (access & NFSD_MAY_READ)
+	if (may_flags & NFSD_MAY_WRITE) {
+		if (may_flags & NFSD_MAY_READ)
 			flags = O_RDWR|O_LARGEFILE;
 		else
 			flags = O_WRONLY|O_LARGEFILE;
@@ -766,8 +767,15 @@  nfsd_open(struct svc_rqst *rqstp, struct svc_fh *fhp, int type,
 			    flags, current_cred());
 	if (IS_ERR(*filp))
 		host_err = PTR_ERR(*filp);
-	else
-		host_err = ima_file_check(*filp, access);
+	else {
+		host_err = ima_file_check(*filp, may_flags);
+
+		if (may_flags & NFSD_MAY_64BIT_COOKIE)
+			(*filp)->f_mode |= FMODE_64BITHASH;
+		else
+			(*filp)->f_mode |= FMODE_32BITHASH;
+	}
+
 out_nfserr:
 	err = nfserrno(host_err);
 out:
@@ -1989,8 +1997,13 @@  nfsd_readdir(struct svc_rqst *rqstp, struct svc_fh *fhp, loff_t *offsetp,
 	__be32		err;
 	struct file	*file;
 	loff_t		offset = *offsetp;
+	int		flags = NFSD_MAY_READ;
+
+	/* NFSv2 only supports 32 bit cookies */
+	if (rqstp->rq_vers > 2)
+		flags |= NFSD_MAY_64BIT_COOKIE;
 
-	err = nfsd_open(rqstp, fhp, S_IFDIR, NFSD_MAY_READ, &file);
+	err = nfsd_open(rqstp, fhp, S_IFDIR, flags, &file);
 	if (err)
 		goto out;
 
diff --git a/fs/nfsd/vfs.h b/fs/nfsd/vfs.h
index e0bbac0..ecd00e1 100644
--- a/fs/nfsd/vfs.h
+++ b/fs/nfsd/vfs.h
@@ -26,6 +26,8 @@ 
 #define NFSD_MAY_NOT_BREAK_LEASE 512
 #define NFSD_MAY_BYPASS_GSS	1024
 
+#define NFSD_MAY_64BIT_COOKIE	2048 /* 64 bit readdir cookies for >= NFSv3 */
+
 #define NFSD_MAY_CREATE		(NFSD_MAY_EXEC|NFSD_MAY_WRITE)
 #define NFSD_MAY_REMOVE		(NFSD_MAY_EXEC|NFSD_MAY_WRITE|NFSD_MAY_TRUNC)