[PATCH 2/2] nfsd41: mask out unsupported pnfs attributes

[PATCH 2/2] nfsd41: mask out unsupported pnfs attributes

[Benny Halevy]

These attributes are valid in NFSv4.1, the just doesn't support them yet.

Signed-off-by: Benny Halevy <bhalevy@panasas.com>
---
 fs/nfsd/nfs4xdr.c |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)

diff --git a/fs/nfsd/nfs4xdr.c b/fs/nfsd/nfs4xdr.c
index f48d891..bd4ce68 100644
--- a/fs/nfsd/nfs4xdr.c
+++ b/fs/nfsd/nfs4xdr.c
@@ -1761,6 +1761,10 @@ nfsd4_encode_fattr(struct svc_fh *fhp, struct svc_export *exp,
 		.dentry	= dentry,
 	};
 
+	/* mask out unsupported pNFS attributes */
+	bmval1 &= ~FATTR4_WORD1_FS_LAYOUT_TYPES;
+	bmval2 &= ~FATTR4_WORD2_LAYOUT_BLKSIZE;
+
 	BUG_ON(bmval1 & NFSD_WRITEONLY_ATTRS_WORD1);
 	BUG_ON(bmval0 & ~nfsd_suppattrs0(minorversion));
 	BUG_ON(bmval1 & ~nfsd_suppattrs1(minorversion));
-- 
1.7.2.3

[PATCH 1/2] SQUASHME: pnfsd: FATTR4_WORD2_LAYOUT_BLKSIZE is supported only under CONFIG_PNFSD

[Benny Halevy]

Signed-off-by: Benny Halevy <bhalevy@panasas.com>
---
 fs/nfsd/nfsd.h |    5 +++++
 1 files changed, 5 insertions(+), 0 deletions(-)

diff --git a/fs/nfsd/nfsd.h b/fs/nfsd/nfsd.h
index d89d30c..cef6770 100644
--- a/fs/nfsd/nfsd.h
+++ b/fs/nfsd/nfsd.h
@@ -294,9 +294,14 @@ extern time_t nfsd4_grace;
 	NFSD4_SUPPORTED_ATTRS_WORD1
 #endif /* CONFIG_PNFSD */
 
+#if defined(CONFIG_PNFSD)
 #define NFSD4_1_SUPPORTED_ATTRS_WORD2 \
 	(NFSD4_SUPPORTED_ATTRS_WORD2 | FATTR4_WORD2_SUPPATTR_EXCLCREAT | \
 	 FATTR4_WORD2_LAYOUT_BLKSIZE)
+#else /* CONFIG_PNFSD */
+#define NFSD4_1_SUPPORTED_ATTRS_WORD2 \
+	(NFSD4_SUPPORTED_ATTRS_WORD2 | FATTR4_WORD2_SUPPATTR_EXCLCREAT)
+#endif /* CONFIG_PNFSD */
 
 static inline u32 nfsd_suppattrs0(u32 minorversion)
 {
-- 
1.7.2.3

[PATCH 2/2] pnfsd: mask out unsupported pnfs attributes only when !CONFIG_PNFSD

[Benny Halevy]

Signed-off-by: Benny Halevy <bhalevy@panasas.com>
---
 fs/nfsd/nfs4xdr.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/fs/nfsd/nfs4xdr.c b/fs/nfsd/nfs4xdr.c
index f0dfa47..5e8e89d 100644
--- a/fs/nfsd/nfs4xdr.c
+++ b/fs/nfsd/nfs4xdr.c
@@ -1906,9 +1906,11 @@ nfsd4_encode_fattr(struct svc_fh *fhp, struct svc_export *exp,
 		.dentry	= dentry,
 	};
 
+#if !defined(CONFIG_PNFSD)
 	/* mask out unsupported pNFS attributes */
 	bmval1 &= ~FATTR4_WORD1_FS_LAYOUT_TYPES;
 	bmval2 &= ~FATTR4_WORD2_LAYOUT_BLKSIZE;
+#endif /* !CONFIG_PNFSD */
 
 	BUG_ON(bmval1 & NFSD_WRITEONLY_ATTRS_WORD1);
 	BUG_ON(bmval0 & ~nfsd_suppattrs0(minorversion));
-- 
1.7.2.3

Re: [PATCH 2/2] nfsd41: mask out unsupported pnfs attributes

[J. Bruce Fields]

On Thu, Sep 30, 2010 at 08:47:58PM +0200, Benny Halevy wrote:
> These attributes are valid in NFSv4.1, the just doesn't support them yet.

The existing code handles unsupported attributes in the operations
themselves.  Perhaps it makes sense to move those checks here, but if
so, explain why, and let's do this for all unsupported attributes, not
just these two.

Looking back at the spec....  I guess it's only on operations that set
attributes that we return NFS4ERR_ATTRNOTSUPP, and otherwise we silently
ignore them?

--b.

> 
> Signed-off-by: Benny Halevy <bhalevy@panasas.com>
> ---
>  fs/nfsd/nfs4xdr.c |    4 ++++
>  1 files changed, 4 insertions(+), 0 deletions(-)
> 
> diff --git a/fs/nfsd/nfs4xdr.c b/fs/nfsd/nfs4xdr.c
> index f48d891..bd4ce68 100644
> --- a/fs/nfsd/nfs4xdr.c
> +++ b/fs/nfsd/nfs4xdr.c
> @@ -1761,6 +1761,10 @@ nfsd4_encode_fattr(struct svc_fh *fhp, struct svc_export *exp,
>  		.dentry	= dentry,
>  	};
>  
> +	/* mask out unsupported pNFS attributes */
> +	bmval1 &= ~FATTR4_WORD1_FS_LAYOUT_TYPES;
> +	bmval2 &= ~FATTR4_WORD2_LAYOUT_BLKSIZE;
> +
>  	BUG_ON(bmval1 & NFSD_WRITEONLY_ATTRS_WORD1);
>  	BUG_ON(bmval0 & ~nfsd_suppattrs0(minorversion));
>  	BUG_ON(bmval1 & ~nfsd_suppattrs1(minorversion));
> -- 
> 1.7.2.3
> 

Re: [PATCH 2/2] nfsd41: mask out unsupported pnfs attributes

[Benny Halevy]

On 2010-10-01 16:48,  J. Bruce Fields wrote:
> On Thu, Sep 30, 2010 at 08:47:58PM +0200, Benny Halevy wrote:
>> These attributes are valid in NFSv4.1, the just doesn't support them yet.
> 
> The existing code handles unsupported attributes in the operations
> themselves.  Perhaps it makes sense to move those checks here, but if
> so, explain why, and let's do this for all unsupported attributes, not
> just these two.

The client can run a DOS attack on the server by requesting invalid attributes
and tripping the BUG_ONs in nfsd4_encode_fattr.

We can/should also change the BUG_ONs to either report invalid
attribute or just silently ignore them, but the client is
perfectly entitled to get attrs we don't support :)

> 
> Looking back at the spec....  I guess it's only on operations that set
> attributes that we return NFS4ERR_ATTRNOTSUPP, and otherwise we silently
> ignore them?

For the GETATTR case, we just return the attrmask for the attrs we support.
IOW:
   The server returns an attribute bitmap that
   indicates the attribute values that it was able to return, which will
   include all attributes requested by the client that are attributes
   supported by the server for the target file system.

Benny

> 
> --b.
> 
>>
>> Signed-off-by: Benny Halevy <bhalevy@panasas.com>
>> ---
>>  fs/nfsd/nfs4xdr.c |    4 ++++
>>  1 files changed, 4 insertions(+), 0 deletions(-)
>>
>> diff --git a/fs/nfsd/nfs4xdr.c b/fs/nfsd/nfs4xdr.c
>> index f48d891..bd4ce68 100644
>> --- a/fs/nfsd/nfs4xdr.c
>> +++ b/fs/nfsd/nfs4xdr.c
>> @@ -1761,6 +1761,10 @@ nfsd4_encode_fattr(struct svc_fh *fhp, struct svc_export *exp,
>>  		.dentry	= dentry,
>>  	};
>>  
>> +	/* mask out unsupported pNFS attributes */
>> +	bmval1 &= ~FATTR4_WORD1_FS_LAYOUT_TYPES;
>> +	bmval2 &= ~FATTR4_WORD2_LAYOUT_BLKSIZE;
>> +
>>  	BUG_ON(bmval1 & NFSD_WRITEONLY_ATTRS_WORD1);
>>  	BUG_ON(bmval0 & ~nfsd_suppattrs0(minorversion));
>>  	BUG_ON(bmval1 & ~nfsd_suppattrs1(minorversion));
>> -- 
>> 1.7.2.3
>>

Re: [PATCH 2/2] nfsd41: mask out unsupported pnfs attributes

[J. Bruce Fields]

On Fri, Oct 01, 2010 at 05:17:51PM +0200, Benny Halevy wrote:
> On 2010-10-01 16:48,  J. Bruce Fields wrote:
> > On Thu, Sep 30, 2010 at 08:47:58PM +0200, Benny Halevy wrote:
> >> These attributes are valid in NFSv4.1, the just doesn't support them yet.
> > 
> > The existing code handles unsupported attributes in the operations
> > themselves.  Perhaps it makes sense to move those checks here, but if
> > so, explain why, and let's do this for all unsupported attributes, not
> > just these two.
> 
> The client can run a DOS attack on the server by requesting invalid attributes
> and tripping the BUG_ONs in nfsd4_encode_fattr.

How can they do that?  getattr and readdir, for example, both handle
this.  But I may well be missing something!

> We can/should also change the BUG_ONs to either report invalid
> attribute or just silently ignore them, but the client is
> perfectly entitled to get attrs we don't support :)

Sure.

> > Looking back at the spec....  I guess it's only on operations that set
> > attributes that we return NFS4ERR_ATTRNOTSUPP, and otherwise we silently
> > ignore them?
> 
> For the GETATTR case, we just return the attrmask for the attrs we support.
> IOW:
>    The server returns an attribute bitmap that
>    indicates the attribute values that it was able to return, which will
>    include all attributes requested by the client that are attributes
>    supported by the server for the target file system.

OK, makes sense.

--b.

Re: [PATCH 2/2] nfsd41: mask out unsupported pnfs attributes

[Benny Halevy]

On 2010-10-01 17:20, J. Bruce Fields wrote:
> On Fri, Oct 01, 2010 at 05:17:51PM +0200, Benny Halevy wrote:
>> On 2010-10-01 16:48,  J. Bruce Fields wrote:
>>> On Thu, Sep 30, 2010 at 08:47:58PM +0200, Benny Halevy wrote:
>>>> These attributes are valid in NFSv4.1, the just doesn't support them yet.
>>>
>>> The existing code handles unsupported attributes in the operations
>>> themselves.  Perhaps it makes sense to move those checks here, but if
>>> so, explain why, and let's do this for all unsupported attributes, not
>>> just these two.
>>
>> The client can run a DOS attack on the server by requesting invalid attributes
>> and tripping the BUG_ONs in nfsd4_encode_fattr.
> 
> How can they do that?  getattr and readdir, for example, both handle
> this.  But I may well be missing something!
> 

You're right.  I got confused by the pnfsd specific bug I fixed here:
http://marc.info/?l=linux-nfs&m=128587444310102&w=2

In this case LAYOUT_BLKSIZE was marked as supported but nothing
really supported it so we ended up with an invalid result sent
to the client.

So let's drop this patch. sorry.

Benny

>> We can/should also change the BUG_ONs to either report invalid
>> attribute or just silently ignore them, but the client is
>> perfectly entitled to get attrs we don't support :)
> 
> Sure.
> 
>>> Looking back at the spec....  I guess it's only on operations that set
>>> attributes that we return NFS4ERR_ATTRNOTSUPP, and otherwise we silently
>>> ignore them?
>>
>> For the GETATTR case, we just return the attrmask for the attrs we support.
>> IOW:
>>    The server returns an attribute bitmap that
>>    indicates the attribute values that it was able to return, which will
>>    include all attributes requested by the client that are attributes
>>    supported by the server for the target file system.
> 
> OK, makes sense.
> 
> --b.