make BTRFS, UDF, NILFS2 work with NFSv2.

PATCH Superseded NeilBrown 5 participants 12 messages May 8, 2015 – Oct 5, 2015

Subsystems (6)

VFS Btrfs file system NFS client NFS server +2 more subsystems

lore

Share & tools

lore

NeilBrown

author

2015-05-08 00:16 UTC #0 lore raw

cover make BTRFS, UDF, NILFS2 work with NFSv2.

Hi filesystem folks,
I just had a report that BTRFS doesn't work reliably with NFSv2.

The problem is that NFSv2 doesn't encode filehandle size so it may report to the filesystem a longer handle that is being expected.

Filesystems should not require a specific length, only at least that length.

A code audit shows that NILFS2 and UDF suffer the same problem.

Following patches should fix it... well, they compile and look good.

Please consider for inclusion is respective trees.

Admittedly NFSv2 is a bit "last century", but while it is easy to support, we may as well.

Thanks,
NeilBrown

---

NeilBrown (3):
BTRFS: support NFSv2 export
NILFS2: support NFSv2 export
UDF: support NFSv2 export

 fs/btrfs/export.c |   10 +++++-----
 fs/nilfs2/namei.c |    6 +++---
 fs/udf/namei.c    |   16 ++++++++++++----
 3 files changed, 20 insertions(+), 12 deletions(-)

--
Signature

NeilBrown

author

2015-05-08 00:16 UTC #1 lore raw

PATCH 1/3 BTRFS: support NFSv2 export

The "fh_len" passed to ->fh_to_* is not guaranteed to be that same as that returned by encode_fh - it may be larger.

With NFSv2, the filehandle is fixed length, so it may appear longer than expected and be zero-padded.

So we must test that fh_len is at least some value, not exactly equal to it.

Signed-off-by NeilBrown <[email protected]>

---
 fs/btrfs/export.c |   10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

fs/btrfs/export.c +5 -5

index 8d052209f473..2513a7f53334 100644
@@ -112,11 +112,11 @@ static struct dentry *btrfs_fh_to_parent(struct super_block *sb, struct fid *fh,
 	u32 generation;
 
 	if (fh_type == FILEID_BTRFS_WITH_PARENT) {
-		if (fh_len !=  BTRFS_FID_SIZE_CONNECTABLE)
+		if (fh_len <  BTRFS_FID_SIZE_CONNECTABLE)
 			return NULL;
 		root_objectid = fid->root_objectid;
 	} else if (fh_type == FILEID_BTRFS_WITH_PARENT_ROOT) {
-		if (fh_len != BTRFS_FID_SIZE_CONNECTABLE_ROOT)
+		if (fh_len < BTRFS_FID_SIZE_CONNECTABLE_ROOT)
 			return NULL;
 		root_objectid = fid->parent_root_objectid;
 	} else

@@ -136,11 +136,11 @@ static struct dentry *btrfs_fh_to_dentry(struct super_block *sb, struct fid *fh,
 	u32 generation;
 
 	if ((fh_type != FILEID_BTRFS_WITH_PARENT ||
-	     fh_len != BTRFS_FID_SIZE_CONNECTABLE) &&
+	     fh_len < BTRFS_FID_SIZE_CONNECTABLE) &&
 	    (fh_type != FILEID_BTRFS_WITH_PARENT_ROOT ||
-	     fh_len != BTRFS_FID_SIZE_CONNECTABLE_ROOT) &&
+	     fh_len < BTRFS_FID_SIZE_CONNECTABLE_ROOT) &&
 	    (fh_type != FILEID_BTRFS_WITHOUT_PARENT ||
-	     fh_len != BTRFS_FID_SIZE_NON_CONNECTABLE))
+	     fh_len < BTRFS_FID_SIZE_NON_CONNECTABLE))
 		return NULL;
 
 	objectid = fid->objectid;

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

David Sterba

Acked-by on v1 1/3

2015-05-11 08:13 UTC #2 lore raw

Re: [PATCH 1/3] BTRFS: support NFSv2 export

Quoted 10 lines · NeilBrown

The "fh_len" passed to ->fh_to_* is not guaranteed to be that same as that returned by encode_fh - it may be larger.
With NFSv2, the filehandle is fixed length, so it may appear longer than expected and be zero-padded.
So we must test that fh_len is at least some value, not exactly equal to it.
Signed-off-by NeilBrown <[email protected]>

Acked-by David Sterba <[email protected]>

Neil Brown

author

2015-09-24 01:59 UTC #3 lore raw

Re: [PATCH 1/3] BTRFS: support NFSv2 export

Quoted 7 lines · David Sterba

Quoted 6 lines · NeilBrown
The "fh_len" passed to ->fh_to_* is not guaranteed to be that same as that returned by encode_fh - it may be larger.
With NFSv2, the filehandle is fixed length, so it may appear longer than expected and be zero-padded.

Quoted 6 lines

Quoted 4 lines
So we must test that fh_len is at least some value, not exactly equal to it.
Signed-off-by NeilBrown <[email protected]>
Acked-by David Sterba <[email protected]>

Thanks. However I just checked mainline and this still hasn't been applied.
Should I resend it to someone? Who?

Thanks,
NeilBrown

Chris Mason

2015-10-05 14:32 UTC #4 lore raw

Re: [PATCH 1/3] BTRFS: support NFSv2 export

Quoted 12 lines · Neil Brown

Quoted 10 lines · David Sterba
Quoted 9 lines · NeilBrown
The "fh_len" passed to ->fh_to_* is not guaranteed to be that same as that returned by encode_fh - it may be larger.
With NFSv2, the filehandle is fixed length, so it may appear longer than expected and be zero-padded.
So we must test that fh_len is at least some value, not exactly equal to it.

Quoted 7 lines

Quoted 3 lines
Quoted 1 line
Signed-off-by NeilBrown <[email protected]>
Acked-by David Sterba <[email protected]>
Thanks. However I just checked mainline and this still hasn't been applied.
Should I resend it to someone? Who?

Sorry Neil, I thought you were pushing these after it was ack'd. I'll put it in my pull this week.

-chris

NeilBrown

author

2015-05-08 00:16 UTC #5 lore raw

PATCH 2/3 NILFS2: support NFSv2 export

In reply to NeilBrown

The "fh_len" passed to ->fh_to_* is not guaranteed to be that same as that returned by encode_fh - it may be larger.

With NFSv2, the filehandle is fixed length, so it may appear longer than expected and be zero-padded.

So we must test that fh_len is at least some value, not exactly equal to it.

Signed-off-by NeilBrown <[email protected]>

---
 fs/nilfs2/namei.c |    6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

fs/nilfs2/namei.c +3 -3

index 22180836ec22..b65fb79d16fd 100644
@@ -496,8 +496,8 @@ static struct dentry *nilfs_fh_to_dentry(struct super_block *sb, struct fid *fh,
 {
 	struct nilfs_fid *fid = (struct nilfs_fid *)fh;
 
-	if ((fh_len != NILFS_FID_SIZE_NON_CONNECTABLE &&
-	     fh_len != NILFS_FID_SIZE_CONNECTABLE) ||
+	if ((fh_len < NILFS_FID_SIZE_NON_CONNECTABLE &&
+	     fh_len < NILFS_FID_SIZE_CONNECTABLE) ||
 	    (fh_type != FILEID_NILFS_WITH_PARENT &&
 	     fh_type != FILEID_NILFS_WITHOUT_PARENT))
 		return NULL;

@@ -510,7 +510,7 @@ static struct dentry *nilfs_fh_to_parent(struct super_block *sb, struct fid *fh,
 {
 	struct nilfs_fid *fid = (struct nilfs_fid *)fh;
 
-	if (fh_len != NILFS_FID_SIZE_CONNECTABLE ||
+	if (fh_len < NILFS_FID_SIZE_CONNECTABLE ||
 	    fh_type != FILEID_NILFS_WITH_PARENT)
 		return NULL;

Ryusuke Konishi

2015-05-10 16:31 UTC #6 lore raw

Re: [PATCH 2/3] NILFS2: support NFSv2 export

Quoted 29 lines · NeilBrown

The "fh_len" passed to ->fh_to_* is not guaranteed to be that same as that returned by encode_fh - it may be larger.
With NFSv2, the filehandle is fixed length, so it may appear longer than expected and be zero-padded.
So we must test that fh_len is at least some value, not exactly equal to it.
Signed-off-by NeilBrown <[email protected]>
---
 fs/nilfs2/namei.c |    6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)
fs/nilfs2/namei.c +2 -2
index 22180836ec22..b65fb79d16fd 100644
@@ -496,8 +496,8 @@ static struct dentry *nilfs_fh_to_dentry(struct super_block *sb, struct fid *fh,
 {
 	struct nilfs_fid *fid = (struct nilfs_fid *)fh;
 
-	if ((fh_len != NILFS_FID_SIZE_NON_CONNECTABLE &&
-	     fh_len != NILFS_FID_SIZE_CONNECTABLE) ||
 
+	if ((fh_len < NILFS_FID_SIZE_NON_CONNECTABLE &&
+	     fh_len < NILFS_FID_SIZE_CONNECTABLE) ||
 	    (fh_type != FILEID_NILFS_WITH_PARENT &&
 	     fh_type != FILEID_NILFS_WITHOUT_PARENT))

Quoted 1 line

return NULL;

A bit weird. "fh_len < NILFS_FID_SIZE_CONNECTABLE" implies "fh_len < NILFS_FID_SIZE_NON_CONNECTABLE".

How about the following fix ?

	if ((fh_type != FILEID_NILFS_WITH_PARENT ||
	     fh_len < NILFS_FID_SIZE_CONNECTABLE) &&
	    (fh_type != FILEID_NILFS_WITHOUT_PARENT ||
	     fh_len < NILFS_FID_SIZE_NON_CONNECTABLE))
		return NULL;

Regards,
Ryusuke Konishi

Quoted 12 lines

fs/nilfs2/namei.c +1 -2

@@ -510,7 +510,7 @@ static struct dentry *nilfs_fh_to_parent(struct super_block *sb, struct fid *fh,
 {
 	struct nilfs_fid *fid = (struct nilfs_fid *)fh;
 
-	if (fh_len != NILFS_FID_SIZE_CONNECTABLE ||
+	if (fh_len < NILFS_FID_SIZE_CONNECTABLE ||
 	    fh_type != FILEID_NILFS_WITH_PARENT)
 		return NULL;
 
 
 
--

Quoted 3 lines

To unsubscribe from this list: send the line "unsubscribe linux-nilfs" in the body of a message to [email protected] 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 [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

NeilBrown

author

2015-05-11 07:02 UTC #7 lore raw

Re: [PATCH 2/3] NILFS2: support NFSv2 export

Quoted 35 lines · On Mon, 11 May 2015 01:31:43 +0900 (JST) Ryusuke Konishi…

Quoted 29 lines · NeilBrown
The "fh_len" passed to ->fh_to_* is not guaranteed to be that same as that returned by encode_fh - it may be larger.
With NFSv2, the filehandle is fixed length, so it may appear longer than expected and be zero-padded.
So we must test that fh_len is at least some value, not exactly equal to it.
Signed-off-by NeilBrown <[email protected]>
---
 fs/nilfs2/namei.c |    6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)
fs/nilfs2/namei.c +2 -2
index 22180836ec22..b65fb79d16fd 100644
@@ -496,8 +496,8 @@ static struct dentry *nilfs_fh_to_dentry(struct super_block *sb, struct fid *fh,
 {
 	struct nilfs_fid *fid = (struct nilfs_fid *)fh;
 
-	if ((fh_len != NILFS_FID_SIZE_NON_CONNECTABLE &&
-	     fh_len != NILFS_FID_SIZE_CONNECTABLE) ||
 
+	if ((fh_len < NILFS_FID_SIZE_NON_CONNECTABLE &&
+	     fh_len < NILFS_FID_SIZE_CONNECTABLE) ||
 	    (fh_type != FILEID_NILFS_WITH_PARENT &&
 	     fh_type != FILEID_NILFS_WITHOUT_PARENT))
Quoted 1 line
return NULL;
A bit weird. "fh_len < NILFS_FID_SIZE_CONNECTABLE" implies "fh_len < NILFS_FID_SIZE_NON_CONNECTABLE".

Quoted 8 lines

How about the following fix ?

	if ((fh_type != FILEID_NILFS_WITH_PARENT ||
	     fh_len < NILFS_FID_SIZE_CONNECTABLE) &&
	    (fh_type != FILEID_NILFS_WITHOUT_PARENT ||
	     fh_len < NILFS_FID_SIZE_NON_CONNECTABLE))
		return NULL;

Yes, weird. The code only uses the early parts of the filehandle, so we only need to complain if the fh_len is less than FILEID_NILFS_WITHOUT_PARENT.

So I'd prefer:

@@ -496,8 +496,7 @@ static struct dentry *nilfs_fh_to_dentry(struct super_block *sb, struct fid *fh,

 {
        struct nilfs_fid *fid = (struct nilfs_fid *)fh;

- if ((fh_len != NILFS_FID_SIZE_NON_CONNECTABLE &&
- fh_len != NILFS_FID_SIZE_CONNECTABLE) ||
+ if (fh_len < NILFS_FID_SIZE_NON_CONNECTABLE ||

            (fh_type != FILEID_NILFS_WITH_PARENT &&
             fh_type != FILEID_NILFS_WITHOUT_PARENT))
                return NULL;

Would you be OK with that? If so I'll resend.

Thanks,
NeilBrown

Ryusuke Konishi

2015-05-11 09:54 UTC #8 lore raw

Re: [PATCH 2/3] NILFS2: support NFSv2 export

Quoted 64 lines · NeilBrown

Quoted 35 lines · On Mon, 11 May 2015 01:31:43 +0900 (JST) Ryusuke Konishi…
Quoted 29 lines · NeilBrown
The "fh_len" passed to ->fh_to_* is not guaranteed to be that same as that returned by encode_fh - it may be larger.
With NFSv2, the filehandle is fixed length, so it may appear longer than expected and be zero-padded.
So we must test that fh_len is at least some value, not exactly equal to it.
Signed-off-by NeilBrown <[email protected]>
---
 fs/nilfs2/namei.c |    6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)
fs/nilfs2/namei.c +2 -2
index 22180836ec22..b65fb79d16fd 100644
@@ -496,8 +496,8 @@ static struct dentry *nilfs_fh_to_dentry(struct super_block *sb, struct fid *fh,
 {
 	struct nilfs_fid *fid = (struct nilfs_fid *)fh;
 
-	if ((fh_len != NILFS_FID_SIZE_NON_CONNECTABLE &&
-	     fh_len != NILFS_FID_SIZE_CONNECTABLE) ||
 
+	if ((fh_len < NILFS_FID_SIZE_NON_CONNECTABLE &&
+	     fh_len < NILFS_FID_SIZE_CONNECTABLE) ||
 	    (fh_type != FILEID_NILFS_WITH_PARENT &&
 	     fh_type != FILEID_NILFS_WITHOUT_PARENT))
Quoted 1 line
return NULL;
A bit weird. "fh_len < NILFS_FID_SIZE_CONNECTABLE" implies "fh_len < NILFS_FID_SIZE_NON_CONNECTABLE".
Quoted 8 lines
How about the following fix ?
	if ((fh_type != FILEID_NILFS_WITH_PARENT ||
	     fh_len < NILFS_FID_SIZE_CONNECTABLE) &&
	    (fh_type != FILEID_NILFS_WITHOUT_PARENT ||
	     fh_len < NILFS_FID_SIZE_NON_CONNECTABLE))
		return NULL;
Yes, weird. The code only uses the early parts of the filehandle, so we only need to complain if the fh_len is less than FILEID_NILFS_WITHOUT_PARENT.
So I'd prefer:
fs/nilfs2/namei.c +1 -2
@@ -496,8 +496,7 @@ static struct dentry *nilfs_fh_to_dentry(struct super_block *sb, struct fid *fh,
 {
        struct nilfs_fid *fid = (struct nilfs_fid *)fh;
 
-       if ((fh_len != NILFS_FID_SIZE_NON_CONNECTABLE &&
-            fh_len != NILFS_FID_SIZE_CONNECTABLE) ||
+       if (fh_len < NILFS_FID_SIZE_NON_CONNECTABLE ||
            (fh_type != FILEID_NILFS_WITH_PARENT &&
             fh_type != FILEID_NILFS_WITHOUT_PARENT))
                return NULL;

Quoted 4 lines

Would you be OK with that? If so I'll resend.
Thanks,
NeilBrown

Thanks. This looks OK to me.
I'll apply it if you will resend.

Regards,
Ryusuke Konishi

NeilBrown

author

2015-05-11 11:13 UTC #9 lore raw

PATCH v2 1/1 NILFS2: support NFSv2 export

The "fh_len" passed to ->fh_to_* is not guaranteed to be that same as that returned by encode_fh - it may be larger.

With NFSv2, the filehandle is fixed length, so it may appear longer than expected and be zero-padded.

So we must test that fh_len is at least some value, not exactly equal to it.

Signed-off-by NeilBrown <[email protected]>

fs/nilfs2/namei.c +2 -3

index 22180836ec22..37dd6b05b1b5 100644
@@ -496,8 +496,7 @@ static struct dentry *nilfs_fh_to_dentry(struct super_block *sb, struct fid *fh,
 {
 	struct nilfs_fid *fid = (struct nilfs_fid *)fh;
 
-	if ((fh_len != NILFS_FID_SIZE_NON_CONNECTABLE &&
-	     fh_len != NILFS_FID_SIZE_CONNECTABLE) ||
+	if (fh_len < NILFS_FID_SIZE_NON_CONNECTABLE ||
 	    (fh_type != FILEID_NILFS_WITH_PARENT &&
 	     fh_type != FILEID_NILFS_WITHOUT_PARENT))
 		return NULL;

@@ -510,7 +509,7 @@ static struct dentry *nilfs_fh_to_parent(struct super_block *sb, struct fid *fh,
 {
 	struct nilfs_fid *fid = (struct nilfs_fid *)fh;
 
-	if (fh_len != NILFS_FID_SIZE_CONNECTABLE ||
+	if (fh_len < NILFS_FID_SIZE_CONNECTABLE ||
 	    fh_type != FILEID_NILFS_WITH_PARENT)
 		return NULL;

NeilBrown

author

2015-05-08 00:16 UTC #10 lore raw

PATCH 3/3 UDF: support NFSv2 export

In reply to NeilBrown

The "fh_len" passed to ->fh_to_* is not guaranteed to be that same as that returned by encode_fh - it may be larger.

With NFSv2, the filehandle is fixed length, so it may appear longer than expected and be zero-padded.

So we must test that fh_len is at least some value, not exactly equal to it.

Signed-off-by NeilBrown <[email protected]>

---
 fs/udf/namei.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

fs/udf/namei.c +2 -2

index 5c03f0dfb98b..facc2a840f7b 100644
@@ -1221,7 +1221,7 @@ static struct dentry *udf_nfs_get_inode(struct super_block *sb, u32 block,
 static struct dentry *udf_fh_to_dentry(struct super_block *sb,
 				       struct fid *fid, int fh_len, int fh_type)
 {
-	if ((fh_len != 3 && fh_len != 5) ||
+	if (fh_len < 3 ||
 	    (fh_type != FILEID_UDF_WITH_PARENT &&
 	     fh_type != FILEID_UDF_WITHOUT_PARENT))
 		return NULL;

@@ -1233,7 +1233,7 @@ static struct dentry *udf_fh_to_dentry(struct super_block *sb,
 static struct dentry *udf_fh_to_parent(struct super_block *sb,
 				       struct fid *fid, int fh_len, int fh_type)
 {
-	if (fh_len != 5 || fh_type != FILEID_UDF_WITH_PARENT)
+	if (fh_len < 5 || fh_type != FILEID_UDF_WITH_PARENT)
 		return NULL;
 
 	return udf_nfs_get_inode(sb, fid->udf.parent_block,

Jan Kara

reviewer

2015-05-11 08:57 UTC #11 lore raw

Re: [PATCH 3/3] UDF: support NFSv2 export

Quoted 10 lines · NeilBrown

The "fh_len" passed to ->fh_to_* is not guaranteed to be that same as that returned by encode_fh - it may be larger.
With NFSv2, the filehandle is fixed length, so it may appear longer than expected and be zero-padded.
So we must test that fh_len is at least some value, not exactly equal to it.
Signed-off-by NeilBrown <[email protected]>

Thanks. The patch looks good to me. I've added it to my tree.

Honza

Quoted 28 lines

---
 fs/udf/namei.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

fs/udf/namei.c +2 -2

index 5c03f0dfb98b..facc2a840f7b 100644
@@ -1221,7 +1221,7 @@ static struct dentry *udf_nfs_get_inode(struct super_block *sb, u32 block,
 static struct dentry *udf_fh_to_dentry(struct super_block *sb,
 				       struct fid *fid, int fh_len, int fh_type)
 {
-	if ((fh_len != 3 && fh_len != 5) ||
+	if (fh_len < 3 ||
 	    (fh_type != FILEID_UDF_WITH_PARENT &&
 	     fh_type != FILEID_UDF_WITHOUT_PARENT))
 		return NULL;

@@ -1233,7 +1233,7 @@ static struct dentry *udf_fh_to_dentry(struct super_block *sb,
 static struct dentry *udf_fh_to_parent(struct super_block *sb,
 				       struct fid *fid, int fh_len, int fh_type)
 {
-	if (fh_len != 5 || fh_type != FILEID_UDF_WITH_PARENT)
+	if (fh_len < 5 || fh_type != FILEID_UDF_WITH_PARENT)
 		return NULL;
 
 	return udf_nfs_get_inode(sb, fid->udf.parent_block,

Signature · 6 lines

Jan Kara <[email protected]>
SUSE Labs, CR
--
To unsubscribe from this list: send the line "unsubscribe linux-nilfs" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html