[PATCH] nfs: Fix ugly referral attributes

[PATCH] nfs: Fix ugly referral attributes

[Chuck Lever]

Before traversing a referral and performing a mount, the mounted-on
directory looks strange:

dr-xr-xr-x. 2 4294967294 4294967294 0 Dec 31  1969 dir.0

nfs4_get_referral is wiping out any cached attributes with what was
returned via GETATTR(fs_locations), but the bit mask for that
operation does not request any file attributes.

Set the default bits in the GETATTR bitmask to retrieve the usual
set of object attributes so that the memcpy in nfs4_get_referral can
fill the attributes in properly.

Fixes: 6b97fd3da1ea ("NFSv4: Follow a referral")
Suggested-by: Pradeep Thomas <pradeepthomas@gmail.com>
Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
Cc: stable@vger.kernel.org
---
 fs/nfs/nfs4proc.c |    8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
index 6c61e2b..ec3c525 100644
--- a/fs/nfs/nfs4proc.c
+++ b/fs/nfs/nfs4proc.c
@@ -6763,9 +6763,7 @@ static int _nfs4_proc_fs_locations(struct rpc_clnt *client, struct inode *dir,
 				   struct page *page)
 {
 	struct nfs_server *server = NFS_SERVER(dir);
-	u32 bitmask[3] = {
-		[0] = FATTR4_WORD0_FSID | FATTR4_WORD0_FS_LOCATIONS,
-	};
+	u32 bitmask[3];
 	struct nfs4_fs_locations_arg args = {
 		.dir_fh = NFS_FH(dir),
 		.name = name,
@@ -6784,6 +6782,10 @@ static int _nfs4_proc_fs_locations(struct rpc_clnt *client, struct inode *dir,
 
 	dprintk("%s: start\n", __func__);
 
+	bitmask[0] = nfs4_fattr_bitmap[0] | FATTR4_WORD0_FS_LOCATIONS;
+	bitmask[1] = nfs4_fattr_bitmap[1];
+	bitmask[2] = nfs4_fattr_bitmap[2] & ~FATTR4_WORD2_SECURITY_LABEL;
+
 	/* Ask for the fileid of the absent filesystem if mounted_on_fileid
 	 * is not supported */
 	if (NFS_SERVER(dir)->attr_bitmask[1] & FATTR4_WORD1_MOUNTED_ON_FILEID)

Re: [PATCH] nfs: Fix ugly referral attributes

[Trond Myklebust]

T24gRnJpLCAyMDE3LTExLTAzIGF0IDEzOjQ2IC0wNDAwLCBDaHVjayBMZXZlciB3cm90ZToNCj4g
QmVmb3JlIHRyYXZlcnNpbmcgYSByZWZlcnJhbCBhbmQgcGVyZm9ybWluZyBhIG1vdW50LCB0aGUg
bW91bnRlZC1vbg0KPiBkaXJlY3RvcnkgbG9va3Mgc3RyYW5nZToNCj4gDQo+IGRyLXhyLXhyLXgu
IDIgNDI5NDk2NzI5NCA0Mjk0OTY3Mjk0IDAgRGVjIDMxICAxOTY5IGRpci4wDQo+IA0KPiBuZnM0
X2dldF9yZWZlcnJhbCBpcyB3aXBpbmcgb3V0IGFueSBjYWNoZWQgYXR0cmlidXRlcyB3aXRoIHdo
YXQgd2FzDQo+IHJldHVybmVkIHZpYSBHRVRBVFRSKGZzX2xvY2F0aW9ucyksIGJ1dCB0aGUgYml0
IG1hc2sgZm9yIHRoYXQNCj4gb3BlcmF0aW9uIGRvZXMgbm90IHJlcXVlc3QgYW55IGZpbGUgYXR0
cmlidXRlcy4NCj4gDQo+IFNldCB0aGUgZGVmYXVsdCBiaXRzIGluIHRoZSBHRVRBVFRSIGJpdG1h
c2sgdG8gcmV0cmlldmUgdGhlIHVzdWFsDQo+IHNldCBvZiBvYmplY3QgYXR0cmlidXRlcyBzbyB0
aGF0IHRoZSBtZW1jcHkgaW4gbmZzNF9nZXRfcmVmZXJyYWwgY2FuDQo+IGZpbGwgdGhlIGF0dHJp
YnV0ZXMgaW4gcHJvcGVybHkuDQo+IA0KDQpIbW0uLi4gU28gSSBiZWxpZXZlIHRoZSByZWFzb24g
d2h5IHdlIGRpZG4ndCBkbyB0aGlzIGluIHRoZSBvcmlnaW5hbA0KaW1wbGVtZW50YXRpb24gaXMg
YmVjYXVzZSBvZiBSRkM3NTMwLCBTZWN0aW9uIDguMy4xLCB3aGljaCBzYXlzOg0KDQogICBPdGhl
ciBhdHRyaWJ1dGVzIFNIT1VMRCBOT1QgYmUgbWFkZSBhdmFpbGFibGUgZm9yIGFic2VudCBmaWxl
DQogICBzeXN0ZW1zLCBldmVuIHdoZW4gaXQgaXMgcG9zc2libGUgdG8gcHJvdmlkZSB0aGVtLiBU
aGUgc2VydmVyIHNob3VsZA0KICAgbm90IGFzc3VtZSB0aGF0IG1vcmUgaW5mb3JtYXRpb24gaXMg
YWx3YXlzIGJldHRlciBhbmQgc2hvdWxkIGF2b2lkDQogICBncmF0dWl0b3VzbHkgcHJvdmlkaW5n
IGFkZGl0aW9uYWwgaW5mb3JtYXRpb24uDQoNCldoZXJlICJvdGhlciBhdHRyaWJ1dGVzIiBtZWFu
cyBhbnl0aGluZyBvdGhlciB0aGFuIGZzX2xvY2F0aW9ucywgZnNpZA0KYW5kIG1vdW50ZWQtb24t
ZmlsZWlkLg0KDQpJT1c6IHRoZSBwcm90b2NvbCBzcGVjIGhhcyBub3JtYXRpdmUgbGFuZ3VhZ2Ug
c3RhdGluZyB0aGF0IHdlIHNob3VsZA0Kbm90IGV4cGVjdCB0byBiZSBhYmxlIHRvIHJldHJpZXZl
IG90aGVyIGF0dHJpYnV0ZXMsIGFuZCB0aGF0IHRoZSBzZXJ2ZXINCnNob3VsZCBub3QgcmV0dXJu
IHRoZW0uDQoNCi0tIA0KVHJvbmQgTXlrbGVidXN0DQpMaW51eCBORlMgY2xpZW50IG1haW50YWlu
ZXIsIFByaW1hcnlEYXRhDQp0cm9uZC5teWtsZWJ1c3RAcHJpbWFyeWRhdGEuY29tDQo=

Re: [PATCH] nfs: Fix ugly referral attributes

[Trond Myklebust]

T24gRnJpLCAyMDE3LTExLTAzIGF0IDE4OjE3ICswMDAwLCBUcm9uZCBNeWtsZWJ1c3Qgd3JvdGU6
DQo+IE9uIEZyaSwgMjAxNy0xMS0wMyBhdCAxMzo0NiAtMDQwMCwgQ2h1Y2sgTGV2ZXIgd3JvdGU6
DQo+ID4gQmVmb3JlIHRyYXZlcnNpbmcgYSByZWZlcnJhbCBhbmQgcGVyZm9ybWluZyBhIG1vdW50
LCB0aGUgbW91bnRlZC1vbg0KPiA+IGRpcmVjdG9yeSBsb29rcyBzdHJhbmdlOg0KPiA+IA0KPiA+
IGRyLXhyLXhyLXguIDIgNDI5NDk2NzI5NCA0Mjk0OTY3Mjk0IDAgRGVjIDMxICAxOTY5IGRpci4w
DQo+ID4gDQo+ID4gbmZzNF9nZXRfcmVmZXJyYWwgaXMgd2lwaW5nIG91dCBhbnkgY2FjaGVkIGF0
dHJpYnV0ZXMgd2l0aCB3aGF0IHdhcw0KPiA+IHJldHVybmVkIHZpYSBHRVRBVFRSKGZzX2xvY2F0
aW9ucyksIGJ1dCB0aGUgYml0IG1hc2sgZm9yIHRoYXQNCj4gPiBvcGVyYXRpb24gZG9lcyBub3Qg
cmVxdWVzdCBhbnkgZmlsZSBhdHRyaWJ1dGVzLg0KPiA+IA0KPiA+IFNldCB0aGUgZGVmYXVsdCBi
aXRzIGluIHRoZSBHRVRBVFRSIGJpdG1hc2sgdG8gcmV0cmlldmUgdGhlIHVzdWFsDQo+ID4gc2V0
IG9mIG9iamVjdCBhdHRyaWJ1dGVzIHNvIHRoYXQgdGhlIG1lbWNweSBpbiBuZnM0X2dldF9yZWZl
cnJhbA0KPiA+IGNhbg0KPiA+IGZpbGwgdGhlIGF0dHJpYnV0ZXMgaW4gcHJvcGVybHkuDQo+ID4g
DQo+IA0KPiBIbW0uLi4gU28gSSBiZWxpZXZlIHRoZSByZWFzb24gd2h5IHdlIGRpZG4ndCBkbyB0
aGlzIGluIHRoZSBvcmlnaW5hbA0KPiBpbXBsZW1lbnRhdGlvbiBpcyBiZWNhdXNlIG9mIFJGQzc1
MzAsIFNlY3Rpb24gOC4zLjEsIHdoaWNoIHNheXM6DQo+IA0KPiAgICBPdGhlciBhdHRyaWJ1dGVz
IFNIT1VMRCBOT1QgYmUgbWFkZSBhdmFpbGFibGUgZm9yIGFic2VudCBmaWxlDQo+ICAgIHN5c3Rl
bXMsIGV2ZW4gd2hlbiBpdCBpcyBwb3NzaWJsZSB0byBwcm92aWRlIHRoZW0uIFRoZSBzZXJ2ZXIN
Cj4gc2hvdWxkDQo+ICAgIG5vdCBhc3N1bWUgdGhhdCBtb3JlIGluZm9ybWF0aW9uIGlzIGFsd2F5
cyBiZXR0ZXIgYW5kIHNob3VsZCBhdm9pZA0KPiAgICBncmF0dWl0b3VzbHkgcHJvdmlkaW5nIGFk
ZGl0aW9uYWwgaW5mb3JtYXRpb24uDQo+IA0KPiBXaGVyZSAib3RoZXIgYXR0cmlidXRlcyIgbWVh
bnMgYW55dGhpbmcgb3RoZXIgdGhhbiBmc19sb2NhdGlvbnMsIGZzaWQNCj4gYW5kIG1vdW50ZWQt
b24tZmlsZWlkLg0KPiANCj4gSU9XOiB0aGUgcHJvdG9jb2wgc3BlYyBoYXMgbm9ybWF0aXZlIGxh
bmd1YWdlIHN0YXRpbmcgdGhhdCB3ZSBzaG91bGQNCj4gbm90IGV4cGVjdCB0byBiZSBhYmxlIHRv
IHJldHJpZXZlIG90aGVyIGF0dHJpYnV0ZXMsIGFuZCB0aGF0IHRoZQ0KPiBzZXJ2ZXINCj4gc2hv
dWxkIG5vdCByZXR1cm4gdGhlbS4NCg0KU29ycnkuIEluIGNhc2UgaXQgd2Fzbid0IGNsZWFyLCB0
aGUgYWJvdmUgd2FzIG5vdCBpbnRlbmRlZCBhcyBhDQpyZWplY3Rpb24gb2YgdGhlIHBhdGNoLiBU
aGUgc2FtZSBzZWN0aW9uIGFsc28gc2F5czoNCg0KICAgV2hlbiBhIEdFVEFUVFIgb3BlcmF0aW9u
IGluY2x1ZGVzIGEgYml0bWFzayBmb3IgdGhlIGF0dHJpYnV0ZQ0KICAgZnNfbG9jYXRpb25zLCBi
dXQgd2hlcmUgdGhlIGJpdG1hc2sgaW5jbHVkZXMgYXR0cmlidXRlcyB0aGF0IGFyZSBub3QNCiAg
IHN1cHBvcnRlZCwgR0VUQVRUUiB3aWxsIG5vdCByZXR1cm4gYW4gZXJyb3IgYnV0IHdpbGwgcmV0
dXJuIHRoZSBtYXNrDQogICBvZiB0aGUgYWN0dWFsIGF0dHJpYnV0ZXMgc3VwcG9ydGVkIHdpdGgg
dGhlIHJlc3VsdHMNCg0KU28gdGhlIG5vcm1hdGl2ZSBsYW5ndWFnZSBpcyBwdXR0aW5nIHRoZSBv
bnVzIG9uIHRoZSBzZXJ2ZXIgdG8gYmUNCmNvbnNlcnZhdGl2ZSBpbiB3aGF0IGl0IHJldHVybnMs
IGFuZCBkb2VzIG5vdCByZXF1aXJlIHRoZSBjbGllbnQgdG8gYmUNCmNvbnNlcnZhdGl2ZSBpbiB3
aGF0IGl0IGFza3MgZm9yLi4uDQoNCi0tIA0KVHJvbmQgTXlrbGVidXN0DQpMaW51eCBORlMgY2xp
ZW50IG1haW50YWluZXIsIFByaW1hcnlEYXRhDQp0cm9uZC5teWtsZWJ1c3RAcHJpbWFyeWRhdGEu
Y29tDQo=

Re: [PATCH] nfs: Fix ugly referral attributes

[Chuck Lever]

> On Nov 3, 2017, at 2:32 PM, Trond Myklebust <trondmy@primarydata.com> wrote:
> 
> On Fri, 2017-11-03 at 18:17 +0000, Trond Myklebust wrote:
>> On Fri, 2017-11-03 at 13:46 -0400, Chuck Lever wrote:
>>> Before traversing a referral and performing a mount, the mounted-on
>>> directory looks strange:
>>> 
>>> dr-xr-xr-x. 2 4294967294 4294967294 0 Dec 31  1969 dir.0
>>> 
>>> nfs4_get_referral is wiping out any cached attributes with what was
>>> returned via GETATTR(fs_locations), but the bit mask for that
>>> operation does not request any file attributes.
>>> 
>>> Set the default bits in the GETATTR bitmask to retrieve the usual
>>> set of object attributes so that the memcpy in nfs4_get_referral
>>> can
>>> fill the attributes in properly.
>>> 
>> 
>> Hmm... So I believe the reason why we didn't do this in the original
>> implementation is because of RFC7530, Section 8.3.1, which says:
>> 
>>   Other attributes SHOULD NOT be made available for absent file
>>   systems, even when it is possible to provide them. The server
>> should
>>   not assume that more information is always better and should avoid
>>   gratuitously providing additional information.
>> 
>> Where "other attributes" means anything other than fs_locations, fsid
>> and mounted-on-fileid.
>> 
>> IOW: the protocol spec has normative language stating that we should
>> not expect to be able to retrieve other attributes, and that the
>> server
>> should not return them.
> 
> Sorry. In case it wasn't clear, the above was not intended as a
> rejection of the patch. The same section also says:
> 
>   When a GETATTR operation includes a bitmask for the attribute
>   fs_locations, but where the bitmask includes attributes that are not
>   supported, GETATTR will not return an error but will return the mask
>   of the actual attributes supported with the results
> 
> So the normative language is putting the onus on the server to be
> conservative in what it returns, and does not require the client to be
> conservative in what it asks for...

Following up, the display of the referral object's attributes is
best effort, IMO, which seems to agree with both sections of text.

And, I don't think the client requests more from the target server
than it has on hand. I could be mistaken about that, and it would
be simple to trim out some of the bits in the GETATTR bitmask if
the client is making an unreasonable request.

The concern seems to apply mostly to the MOUNTED_ON_FSID bit. I
noticed the original code always asserted that bit, even though
a later patch added the conditional use of MOUNTED_ON_FSID, but
the FSID bit was not cleared in that case. The new patch will
send either of those bits, but not both at once. If that's
incorrect, I can send an updated patch.


--
Chuck Lever

Re: [PATCH] nfs: Fix ugly referral attributes

[Chuck Lever]

> On Nov 3, 2017, at 2:48 PM, Chuck Lever <chuck.lever@oracle.com> wrote:
> 
>> 
>> On Nov 3, 2017, at 2:32 PM, Trond Myklebust <trondmy@primarydata.com> wrote:
>> 
>> On Fri, 2017-11-03 at 18:17 +0000, Trond Myklebust wrote:
>>> On Fri, 2017-11-03 at 13:46 -0400, Chuck Lever wrote:
>>>> Before traversing a referral and performing a mount, the mounted-on
>>>> directory looks strange:
>>>> 
>>>> dr-xr-xr-x. 2 4294967294 4294967294 0 Dec 31  1969 dir.0
>>>> 
>>>> nfs4_get_referral is wiping out any cached attributes with what was
>>>> returned via GETATTR(fs_locations), but the bit mask for that
>>>> operation does not request any file attributes.
>>>> 
>>>> Set the default bits in the GETATTR bitmask to retrieve the usual
>>>> set of object attributes so that the memcpy in nfs4_get_referral
>>>> can
>>>> fill the attributes in properly.
>>>> 
>>> 
>>> Hmm... So I believe the reason why we didn't do this in the original
>>> implementation is because of RFC7530, Section 8.3.1, which says:
>>> 
>>>  Other attributes SHOULD NOT be made available for absent file
>>>  systems, even when it is possible to provide them. The server
>>> should
>>>  not assume that more information is always better and should avoid
>>>  gratuitously providing additional information.
>>> 
>>> Where "other attributes" means anything other than fs_locations, fsid
>>> and mounted-on-fileid.
>>> 
>>> IOW: the protocol spec has normative language stating that we should
>>> not expect to be able to retrieve other attributes, and that the
>>> server
>>> should not return them.
>> 
>> Sorry. In case it wasn't clear, the above was not intended as a
>> rejection of the patch. The same section also says:
>> 
>>  When a GETATTR operation includes a bitmask for the attribute
>>  fs_locations, but where the bitmask includes attributes that are not
>>  supported, GETATTR will not return an error but will return the mask
>>  of the actual attributes supported with the results
>> 
>> So the normative language is putting the onus on the server to be
>> conservative in what it returns, and does not require the client to be
>> conservative in what it asks for...
> 
> Following up, the display of the referral object's attributes is
> best effort, IMO, which seems to agree with both sections of text.
> 
> And, I don't think the client requests more from the target server
> than it has on hand. I could be mistaken about that, and it would
> be simple to trim out some of the bits in the GETATTR bitmask if
> the client is making an unreasonable request.
> 
> The concern seems to apply mostly to the MOUNTED_ON_FSID bit. I
> noticed the original code always asserted that bit, even though
> a later patch added the conditional use of MOUNTED_ON_FSID, but
> the FSID bit was not cleared in that case.

Editing mistake. The original code always asserted FSID, and
the later patch made the use of MOUNTED_ON_FSID conditional
but does not clear FSID when MOUNTED_ON_FSID is used.


> The new patch will
> send either of those bits, but not both at once. If that's
> incorrect, I can send an updated patch.
> 
> 
> --
> Chuck Lever
> 
> 
> 
> --
> 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

--
Chuck Lever