NFS: Detect loops in a readdir due to bad cookies
Subsystems (2)
PATCH 3/3 NFS: Detect loops in a readdir due to bad cookies
Some filesystems (such as ext4) can return the same cookie value for multiple files. If we try to start a readdir with one of these cookies, the server will return the first file found with a cookie of the same value. This can cause the client to enter an infinite loop.
--- fs/nfs/dir.c | 23 ++++++++++++++++++++++- include/linux/nfs_fs.h | 2 ++ 2 files changed, 24 insertions(+), 1 deletions(-)
fs/nfs/dir.c +22 -1
@@ -139,7 +139,9 @@ static struct nfs_open_dir_context *alloc_nfs_open_dir_context(struct rpc_cred *struct nfs_open_dir_context *ctx; ctx =kmalloc (sizeof (*ctx),GFP_KERNEL );if (ctx !=NULL ) { + ctx->duped =0 ; ctx->dir_cookie =0 ; + ctx->dup_cookie =0 ; ctx->cred =get_rpccred (cred); }return ctx;
@@ -342,11 +344,18 @@ staticint nfs_readdir_search_for_cookie (struct nfs_cache_array *array,nfs_readdir_descriptor_t *desc) {int i; +int new_pos;int status = -EAGAIN ; +struct nfs_open_dir_context *ctx = desc->file->private_data;for (i =0 ; i < array->size; i++) {if (array->array[i].cookie == *desc->dir_cookie) { - desc->file->f_pos = desc->current_index + i; + new_pos = desc->current_index + i; +if (new_pos < desc->file->f_pos) { + ctx->dup_cookie = *desc->dir_cookie; + ctx->duped =1 ; + } + desc->file->f_pos = new_pos; desc->cache_entry_index = i;return 0 ; }
@@ -731,6 +740,18 @@ int nfs_do_filldir(nfs_readdir_descriptor_t *desc, void *dirent,int i =0 ;int res =0 ;struct nfs_cache_array *array =NULL ; +struct nfs_open_dir_context *ctx = file->private_data; + +if (ctx->duped ==1 && ctx->dup_cookie == *desc->dir_cookie) { +if (printk_ratelimit ()) { +pr_notice ("NFS: directory %s/%s contains a readdir loop. " +"Please contact your server vendor." , + file->f_dentry->d_parent->d_name.name, + file->f_dentry->d_name.name); + } + res = -ELOOP ; + goto out; + } array =nfs_readdir_get_array (desc->page);if (IS_ERR (array)) {
include/linux/nfs_fs.h +2 -0
@@ -97,7 +97,9 @@ struct nfs_open_context {struct nfs_open_dir_context {struct rpc_cred *cred; +int duped;__u64 dir_cookie; +__u64 dup_cookie; };/*
Signature · 1 line
1.7.4.1
Quoted 26 lines · Bryan Schumaker
Some filesystems (such as ext4) can return the same cookie value for multiple files. If we try to start a readdir with one of these cookies, the server will return the first file found with a cookie of the same value. This can cause the client to enter an infinite loop.
Signed-off-by Bryan Schumaker <[email protected]>--- fs/nfs/dir.c | 23 ++++++++++++++++++++++- include/linux/nfs_fs.h | 2 ++ 2 files changed, 24 insertions(+), 1 deletions(-)fs/nfs/dir.c +2
-0@@ -139,7 +139,9 @@ static struct nfs_open_dir_context *alloc_nfs_open_dir_context(struct rpc_cred *struct nfs_open_dir_context *ctx; ctx =kmalloc (sizeof (*ctx),GFP_KERNEL );if (ctx !=NULL ) { + ctx->duped =0 ; ctx->dir_cookie =0 ; + ctx->dup_cookie =0 ; ctx->cred =get_rpccred (cred); }return ctx;
fs/nfs/dir.c static · line 347
Quoted 4 lines
fs/nfs/dir.c +1
-0@@ -342,11 +344,18 @@ staticint nfs_readdir_search_for_cookie (struct nfs_cache_array *array,nfs_readdir_descriptor_t *desc) {int i; +int new_pos;
'new_pos' needs to be an loff_t in order to match the types of desc->current_index and file->f_pos. Those are not bounded to 'int' values (unlike the value of 'i' which is < array->size).
Quoted 16 lines
fs/nfs/dir.c +7
-1int status = -EAGAIN ; +struct nfs_open_dir_context *ctx = desc->file->private_data;for (i =0 ; i < array->size; i++) {if (array->array[i].cookie == *desc->dir_cookie) { - desc->file->f_pos = desc->current_index + i; + new_pos = desc->current_index + i; +if (new_pos < desc->file->f_pos) { + ctx->dup_cookie = *desc->dir_cookie; + ctx->duped =1 ; + } + desc->file->f_pos = new_pos; desc->cache_entry_index = i;return 0 ; }
fs/nfs/dir.c int nfs_do_filldir(nfs_readdir_descriptor_t *desc, void *dirent, · line 745
Quoted 6 lines
fs/nfs/dir.c +3
-0@@ -731,6 +740,18 @@ int nfs_do_filldir(nfs_readdir_descriptor_t *desc, void *dirent,int i =0 ;int res =0 ;struct nfs_cache_array *array =NULL ; +struct nfs_open_dir_context *ctx = file->private_data; + +if (ctx->duped ==1 && ctx->dup_cookie == *desc->dir_cookie) {
^^^^^^^^^^^^^^^^
nit: it is usually slightly more efficient to test for ctx->duped != 0.
Quoted 17 lines
fs/nfs/dir.c +9
-0+if (printk_ratelimit ()) { +pr_notice ("NFS: directory %s/%s contains a readdir loop. " +"Please contact your server vendor." , + file->f_dentry->d_parent->d_name.name, + file->f_dentry->d_name.name); + } + res = -ELOOP ; + goto out; + } array =nfs_readdir_get_array (desc->page);if (IS_ERR (array)) {include/linux/nfs_fs.h +0
-0
include/linux/nfs_fs.h struct nfs_open_context { · line 100
Quoted 4 lines
include/linux/nfs_fs.h +1
-0@@ -97,7 +97,9 @@ struct nfs_open_context {struct nfs_open_dir_context {struct rpc_cred *cred; +int duped;
nit: Can you move this to the end of the context struct (together with dup_cookie).
include/linux/nfs_fs.h struct nfs_open_context { · line 105
Quoted 5 lines
include/linux/nfs_fs.h +1
-0__u64 dir_cookie; +__u64 dup_cookie; };/*
We probably always want to clear ctx->duped in nfs_llseek_dir(), and also on a successful nfs_readdir_search_for_pos(). In both those cases we can end up deliberately looping backwards in the stream of cookies.
Signature · 6 lines
Trond Myklebust Linux NFS client maintainer NetApp [email protected] www.netapp.com
Doesn't returning identical cookies for multiple, different entries in the same directory constitute a protocol violation?
ps
-----Original Message-----
From: [email protected] [mailto:[email protected]] On Behalf Of Bryan Schumaker
Sent: Wednesday, March 23, 2011 1:40 PM
To: Trond >> "Myklebust, Trond"
Cc: [email protected]
Subject: [PATCH 3/3] NFS: Detect loops in a readdir due to bad cookies
Some filesystems (such as ext4) can return the same cookie value for multiple files. If we try to start a readdir with one of these cookies, the server will return the first file found with a cookie of the same value. This can cause the client to enter an infinite loop.
--- fs/nfs/dir.c | 23 ++++++++++++++++++++++- include/linux/nfs_fs.h | 2 ++ 2 files changed, 24 insertions(+), 1 deletions(-)
fs/nfs/dir.c +22 -1
@@ -139,7 +139,9 @@ static struct nfs_open_dir_context *alloc_nfs_open_dir_context(struct rpc_cred *struct nfs_open_dir_context *ctx; ctx =kmalloc (sizeof (*ctx),GFP_KERNEL );if (ctx !=NULL ) { + ctx->duped =0 ; ctx->dir_cookie =0 ; + ctx->dup_cookie =0 ; ctx->cred =get_rpccred (cred); }return ctx;
@@ -342,11 +344,18 @@ staticint nfs_readdir_search_for_cookie (struct nfs_cache_array *array,nfs_readdir_descriptor_t *desc) {int i; +int new_pos;int status = -EAGAIN ; +struct nfs_open_dir_context *ctx = desc->file->private_data;for (i =0 ; i < array->size; i++) {if (array->array[i].cookie == *desc->dir_cookie) { - desc->file->f_pos = desc->current_index + i; + new_pos = desc->current_index + i; +if (new_pos < desc->file->f_pos) { + ctx->dup_cookie = *desc->dir_cookie; + ctx->duped =1 ; + } + desc->file->f_pos = new_pos; desc->cache_entry_index = i;return 0 ; }
@@ -731,6 +740,18 @@ int nfs_do_filldir(nfs_readdir_descriptor_t *desc, void *dirent,int i =0 ;int res =0 ;struct nfs_cache_array *array =NULL ; +struct nfs_open_dir_context *ctx = file->private_data; + +if (ctx->duped ==1 && ctx->dup_cookie == *desc->dir_cookie) { +if (printk_ratelimit ()) { +pr_notice ("NFS: directory %s/%s contains a readdir loop. " +"Please contact your server vendor." , + file->f_dentry->d_parent->d_name.name, + file->f_dentry->d_name.name); + } + res = -ELOOP ; + goto out; + } array =nfs_readdir_get_array (desc->page);if (IS_ERR (array)) {
include/linux/nfs_fs.h +2 -0
@@ -97,7 +97,9 @@ struct nfs_open_context {struct nfs_open_dir_context {struct rpc_cred *cred; +int duped;__u64 dir_cookie; +__u64 dup_cookie; };/*
Signature · 6 lines
1.7.4.1 -- 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
Quoted 1 line · On Wed, 2011-03-23 at 14:10 -0400, [email protected] wrote:
Doesn't returning identical cookies for multiple, different entries in the same directory constitute a protocol violation?
Yes. Unfortunately quite a few common filesystems seem quite happy to do so.
Most of the time they get away with it, but occasionally we do get support requests from people asking 'why is my NFS client looping forever?'.
With Bryan's patches, we now have a way to have the client error out instead of looping, and notify the admin that their server is exporting a broken underlying filesystem.
Signature · 6 lines
Trond Myklebust Linux NFS client maintainer NetApp [email protected] www.netapp.com
Quoted 1 line · Bryan Schumaker
Some filesystems (such as ext4) can return the same cookie value for multiple
Do the ext4 people know about this? (And what's the easiest way to reproduce it?)
--b.
Quoted 84 lines
files. If we try to start a readdir with one of these cookies, the server will return the first file found with a cookie of the same value. This can cause the client to enter an infinite loop.
Signed-off-by Bryan Schumaker <[email protected]>--- fs/nfs/dir.c | 23 ++++++++++++++++++++++- include/linux/nfs_fs.h | 2 ++ 2 files changed, 24 insertions(+), 1 deletions(-)fs/nfs/dir.c +22
-1@@ -139,7 +139,9 @@ static struct nfs_open_dir_context *alloc_nfs_open_dir_context(struct rpc_cred *struct nfs_open_dir_context *ctx; ctx =kmalloc (sizeof (*ctx),GFP_KERNEL );if (ctx !=NULL ) { + ctx->duped =0 ; ctx->dir_cookie =0 ; + ctx->dup_cookie =0 ; ctx->cred =get_rpccred (cred); }return ctx;@@ -342,11 +344,18 @@ staticint nfs_readdir_search_for_cookie (struct nfs_cache_array *array,nfs_readdir_descriptor_t *desc) {int i; +int new_pos;int status = -EAGAIN ; +struct nfs_open_dir_context *ctx = desc->file->private_data;for (i =0 ; i < array->size; i++) {if (array->array[i].cookie == *desc->dir_cookie) { - desc->file->f_pos = desc->current_index + i; + new_pos = desc->current_index + i; +if (new_pos < desc->file->f_pos) { + ctx->dup_cookie = *desc->dir_cookie; + ctx->duped =1 ; + } + desc->file->f_pos = new_pos; desc->cache_entry_index = i;return 0 ; }@@ -731,6 +740,18 @@ int nfs_do_filldir(nfs_readdir_descriptor_t *desc, void *dirent,int i =0 ;int res =0 ;struct nfs_cache_array *array =NULL ; +struct nfs_open_dir_context *ctx = file->private_data; + +if (ctx->duped ==1 && ctx->dup_cookie == *desc->dir_cookie) { +if (printk_ratelimit ()) { +pr_notice ("NFS: directory %s/%s contains a readdir loop. " +"Please contact your server vendor." , + file->f_dentry->d_parent->d_name.name, + file->f_dentry->d_name.name); + } + res = -ELOOP ; + goto out; + } array =nfs_readdir_get_array (desc->page);if (IS_ERR (array)) {include/linux/nfs_fs.h +2
-0@@ -97,7 +97,9 @@ struct nfs_open_context {struct nfs_open_dir_context {struct rpc_cred *cred; +int duped;__u64 dir_cookie; +__u64 dup_cookie; };/* Signature · 6 lines
1.7.4.1 -- 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
Quoted 5 lines · J. Bruce Fields
Quoted 1 line · Bryan Schumaker
Some filesystems (such as ext4) can return the same cookie value for multiple
Do the ext4 people know about this? (And what's the easiest way to reproduce it?)
They are aware of it. I don't know if they are aware how easy it is to reproduce.
Bryan saw several 10s of instances of duplicate cookies when he was doing performance testing of readdir on a directory with 10^6 entries. Most of the time it didn't cause looping on the client since the looping behaviour depends on the distribution of those cookies in the client's readdir cache. However on occasion he did see looping in the client, and at least once he saw looping on the server too (in which a duplicate cookie caused the server to return files that it had already sent to the client).
Signature · 6 lines
Trond Myklebust Linux NFS client maintainer NetApp [email protected] www.netapp.com
QWx0aG91Z2ggSSB0aGluayB0aGF0IGl0IGlzIGEgZ29vZCB0aGluZyB0byBwcm90ZWN0IHRoZSBj bGllbnQgYWdhaW5zdCBicm9rZW4gc2VydmVycywgaXQgZG9lcyBzZWVtIGxpa2UgdGhlIHJpZ2h0 IHNvbHV0aW9uIGlzIHRvIGdldCB0aGUgc2VydmVyIGZpeGVkLi4uDQoNCgkJcHMNCg0KDQotLS0t LU9yaWdpbmFsIE1lc3NhZ2UtLS0tLQ0KRnJvbTogbGludXgtbmZzLW93bmVyQHZnZXIua2VybmVs Lm9yZyBbbWFpbHRvOmxpbnV4LW5mcy1vd25lckB2Z2VyLmtlcm5lbC5vcmddIE9uIEJlaGFsZiBP ZiBUcm9uZCBNeWtsZWJ1c3QNClNlbnQ6IFdlZG5lc2RheSwgTWFyY2ggMjMsIDIwMTEgMjozNCBQ TQ0KVG86IEouIEJydWNlIEZpZWxkcw0KQ2M6IEJyeWFuIFNjaHVtYWtlcjsgbGludXgtbmZzQHZn ZXIua2VybmVsLm9yZw0KU3ViamVjdDogUmU6IFtQQVRDSCAzLzNdIE5GUzogRGV0ZWN0IGxvb3Bz IGluIGEgcmVhZGRpciBkdWUgdG8gYmFkIGNvb2tpZXMNCg0KT24gV2VkLCAyMDExLTAzLTIzIGF0 IDE0OjI1IC0wNDAwLCBKLiBCcnVjZSBGaWVsZHMgd3JvdGU6DQo+IE9uIFdlZCwgTWFyIDIzLCAy MDExIGF0IDAxOjM5OjQ0UE0gLTA0MDAsIEJyeWFuIFNjaHVtYWtlciB3cm90ZToNCj4gPiBTb21l IGZpbGVzeXN0ZW1zIChzdWNoIGFzIGV4dDQpIGNhbiByZXR1cm4gdGhlIHNhbWUgY29va2llIHZh bHVlIGZvciBtdWx0aXBsZQ0KPiANCj4gRG8gdGhlIGV4dDQgcGVvcGxlIGtub3cgYWJvdXQgdGhp cz8gIChBbmQgd2hhdCdzIHRoZSBlYXNpZXN0IHdheSB0bw0KPiByZXByb2R1Y2UgaXQ/KQ0KDQpU aGV5IGFyZSBhd2FyZSBvZiBpdC4gSSBkb24ndCBrbm93IGlmIHRoZXkgYXJlIGF3YXJlIGhvdyBl YXN5IGl0IGlzIHRvDQpyZXByb2R1Y2UuDQoNCkJyeWFuIHNhdyBzZXZlcmFsIDEwcyBvZiBpbnN0 YW5jZXMgb2YgZHVwbGljYXRlIGNvb2tpZXMgd2hlbiBoZSB3YXMNCmRvaW5nIHBlcmZvcm1hbmNl IHRlc3Rpbmcgb2YgcmVhZGRpciBvbiBhIGRpcmVjdG9yeSB3aXRoIDEwXjYgZW50cmllcy4NCk1v c3Qgb2YgdGhlIHRpbWUgaXQgZGlkbid0IGNhdXNlIGxvb3Bpbmcgb24gdGhlIGNsaWVudCBzaW5j ZSB0aGUgbG9vcGluZw0KYmVoYXZpb3VyIGRlcGVuZHMgb24gdGhlIGRpc3RyaWJ1dGlvbiBvZiB0 aG9zZSBjb29raWVzIGluIHRoZSBjbGllbnQncw0KcmVhZGRpciBjYWNoZS4gSG93ZXZlciBvbiBv Y2Nhc2lvbiBoZSBkaWQgc2VlIGxvb3BpbmcgaW4gdGhlIGNsaWVudCwgYW5kDQphdCBsZWFzdCBv bmNlIGhlIHNhdyBsb29waW5nIG9uIHRoZSBzZXJ2ZXIgdG9vIChpbiB3aGljaCBhIGR1cGxpY2F0 ZQ0KY29va2llIGNhdXNlZCB0aGUgc2VydmVyIHRvIHJldHVybiBmaWxlcyB0aGF0IGl0IGhhZCBh bHJlYWR5IHNlbnQgdG8gdGhlDQpjbGllbnQpLg0KDQotLSANClRyb25kIE15a2xlYnVzdA0KTGlu dXggTkZTIGNsaWVudCBtYWludGFpbmVyDQoNCk5ldEFwcA0KVHJvbmQuTXlrbGVidXN0QG5ldGFw cC5jb20NCnd3dy5uZXRhcHAuY29tDQoNCi0tDQpUbyB1bnN1YnNjcmliZSBmcm9tIHRoaXMgbGlz dDogc2VuZCB0aGUgbGluZSAidW5zdWJzY3JpYmUgbGludXgtbmZzIiBpbg0KdGhlIGJvZHkgb2Yg YSBtZXNzYWdlIHRvIG1ham9yZG9tb0B2Z2VyLmtlcm5lbC5vcmcNCk1vcmUgbWFqb3Jkb21vIGlu Zm8gYXQgIGh0dHA6Ly92Z2VyLmtlcm5lbC5vcmcvbWFqb3Jkb21vLWluZm8uaHRtbA0KDQoT77+9 77+97Lm7HO+/vSbvv71+77+9Ju+/vRjvv73vv70rLe+/ve+/vd22F++/ve+/vXfvv73vv73Lm++/ ve+/ve+/vW3vv71i77+977+9Z37Ip++/vRfvv73vv73cqH3vv73vv73vv73GoHrvv70majordu+/ ve+/ve+/vQfvv73vv73vv73vv716Wivvv73vv70rembvv73vv73vv71o77+977+977+9fu+/ve+/ ve+/ve+/vWnvv73vv73vv71677+9Hu+/vXfvv73vv73vv70/77+977+977+977+9Ju+/vSnfohtm
Quoted 1 line · On Wed, 2011-03-23 at 14:42 -0400, [email protected] wrote:
Although I think that it is a good thing to protect the client against broken servers, it does seem like the right solution is to get the server fixed...
Don't get me wrong: I fully agree with that!
The reason for wanting this code is to cut down on the amount of debugging we have to do in order to detect those broken servers. Detecting duplicate cookies in a wireshark trace can be very time-consuming if you are dealing with a 10^6-entry directory.
Signature · 6 lines
Trond Myklebust Linux NFS client maintainer NetApp [email protected] www.netapp.com
SG93IGRvZXMgbG9jYWwgcmVhZGRpciB3b3JrIG9uIHN1Y2ggZmlsZSBzeXN0ZW1zPyAgOi0pDQoN CgkJcHMNCg0KDQotLS0tLU9yaWdpbmFsIE1lc3NhZ2UtLS0tLQ0KRnJvbTogbGludXgtbmZzLW93 bmVyQHZnZXIua2VybmVsLm9yZyBbbWFpbHRvOmxpbnV4LW5mcy1vd25lckB2Z2VyLmtlcm5lbC5v cmddIE9uIEJlaGFsZiBPZiBUcm9uZCBNeWtsZWJ1c3QNClNlbnQ6IFdlZG5lc2RheSwgTWFyY2gg MjMsIDIwMTEgMjozNCBQTQ0KVG86IEouIEJydWNlIEZpZWxkcw0KQ2M6IEJyeWFuIFNjaHVtYWtl cjsgbGludXgtbmZzQHZnZXIua2VybmVsLm9yZw0KU3ViamVjdDogUmU6IFtQQVRDSCAzLzNdIE5G UzogRGV0ZWN0IGxvb3BzIGluIGEgcmVhZGRpciBkdWUgdG8gYmFkIGNvb2tpZXMNCg0KT24gV2Vk LCAyMDExLTAzLTIzIGF0IDE0OjI1IC0wNDAwLCBKLiBCcnVjZSBGaWVsZHMgd3JvdGU6DQo+IE9u IFdlZCwgTWFyIDIzLCAyMDExIGF0IDAxOjM5OjQ0UE0gLTA0MDAsIEJyeWFuIFNjaHVtYWtlciB3 cm90ZToNCj4gPiBTb21lIGZpbGVzeXN0ZW1zIChzdWNoIGFzIGV4dDQpIGNhbiByZXR1cm4gdGhl IHNhbWUgY29va2llIHZhbHVlIGZvciBtdWx0aXBsZQ0KPiANCj4gRG8gdGhlIGV4dDQgcGVvcGxl IGtub3cgYWJvdXQgdGhpcz8gIChBbmQgd2hhdCdzIHRoZSBlYXNpZXN0IHdheSB0bw0KPiByZXBy b2R1Y2UgaXQ/KQ0KDQpUaGV5IGFyZSBhd2FyZSBvZiBpdC4gSSBkb24ndCBrbm93IGlmIHRoZXkg YXJlIGF3YXJlIGhvdyBlYXN5IGl0IGlzIHRvDQpyZXByb2R1Y2UuDQoNCkJyeWFuIHNhdyBzZXZl cmFsIDEwcyBvZiBpbnN0YW5jZXMgb2YgZHVwbGljYXRlIGNvb2tpZXMgd2hlbiBoZSB3YXMNCmRv aW5nIHBlcmZvcm1hbmNlIHRlc3Rpbmcgb2YgcmVhZGRpciBvbiBhIGRpcmVjdG9yeSB3aXRoIDEw XjYgZW50cmllcy4NCk1vc3Qgb2YgdGhlIHRpbWUgaXQgZGlkbid0IGNhdXNlIGxvb3Bpbmcgb24g dGhlIGNsaWVudCBzaW5jZSB0aGUgbG9vcGluZw0KYmVoYXZpb3VyIGRlcGVuZHMgb24gdGhlIGRp c3RyaWJ1dGlvbiBvZiB0aG9zZSBjb29raWVzIGluIHRoZSBjbGllbnQncw0KcmVhZGRpciBjYWNo ZS4gSG93ZXZlciBvbiBvY2Nhc2lvbiBoZSBkaWQgc2VlIGxvb3BpbmcgaW4gdGhlIGNsaWVudCwg YW5kDQphdCBsZWFzdCBvbmNlIGhlIHNhdyBsb29waW5nIG9uIHRoZSBzZXJ2ZXIgdG9vIChpbiB3 aGljaCBhIGR1cGxpY2F0ZQ0KY29va2llIGNhdXNlZCB0aGUgc2VydmVyIHRvIHJldHVybiBmaWxl cyB0aGF0IGl0IGhhZCBhbHJlYWR5IHNlbnQgdG8gdGhlDQpjbGllbnQpLg0KDQotLSANClRyb25k IE15a2xlYnVzdA0KTGludXggTkZTIGNsaWVudCBtYWludGFpbmVyDQoNCk5ldEFwcA0KVHJvbmQu TXlrbGVidXN0QG5ldGFwcC5jb20NCnd3dy5uZXRhcHAuY29tDQoNCi0tDQpUbyB1bnN1YnNjcmli ZSBmcm9tIHRoaXMgbGlzdDogc2VuZCB0aGUgbGluZSAidW5zdWJzY3JpYmUgbGludXgtbmZzIiBp bg0KdGhlIGJvZHkgb2YgYSBtZXNzYWdlIHRvIG1ham9yZG9tb0B2Z2VyLmtlcm5lbC5vcmcNCk1v cmUgbWFqb3Jkb21vIGluZm8gYXQgIGh0dHA6Ly92Z2VyLmtlcm5lbC5vcmcvbWFqb3Jkb21vLWlu Zm8uaHRtbA0KDQoT77+977+97Lm7HO+/vSbvv71+77+9Ju+/vRjvv73vv70rLe+/ve+/vd22F++/ ve+/vXfvv73vv73Lm++/ve+/ve+/vW3vv71i77+977+9Z37Ip++/vRfvv73vv73cqH3vv73vv73v v73GoHrvv70majordu+/ve+/ve+/vQfvv73vv73vv73vv716Wivvv73vv70rembvv73vv73vv71o 77+977+977+9fu+/ve+/ve+/ve+/vWnvv73vv73vv71677+9Hu+/vXfvv73vv73vv70/77+977+9 77+977+9Ju+/vSnfohtm
Quoted 4 lines · On Wed, 23 Mar 2011 14:48:35 -0400 Trond Myklebust…
Quoted 1 line · On Wed, 2011-03-23 at 14:42 -0400, [email protected] wrote:
Although I think that it is a good thing to protect the client against broken servers, it does seem like the right solution is to get the server fixed...
Don't get me wrong: I fully agree with that!
Ack.
As I understand it, ext4 (and ext3) use the hash of the basename as the cookie. This hash has a per-directory random seed which is hidden so it is not possible to deliberately create files with the same hash, but thanks to the birthday paradox it isn't too hard to get collisions in a suitably large directory.
For 'readdir' ext4 keeps extra state in the 'struct file' so that it knows where it was up to in a list of names with the same hash and so won't return duplicates. However if you telldir/seekdir you will restart at the start of the sequence (I think).
Assuming that the fs always delivers names-with-the-same-cookie in a contiguous set (which seems likely), NFSD could mitigate this problem by treating them as a unit.
i.e. to a given READDIR request, it either returns all names with a particular cookie, or none of them.
This could only work if the set of names with the one cookie fits inside a single reply, but that should be likely unless the rsize is tiny.
nfsd should also, after lseeking to the given address, skip over any subsequent entries that have the same address.
This would ensure no looping happened, and the first step would make it unlikely that names were missed.
It would be nice if filesystems would get this right, but I think it is reasonable for NFSD to take steps like this to guard against unfortunate filesystem design.
NeilBrown
Quoted 34 lines
-----Original Message-----
From: [email protected] [mailto:[email protected]] On Behalf Of NeilBrown
Sent: Wednesday, March 23, 2011 2:28 PM
To: Trond Myklebust
Cc: Staubach, Peter; [email protected]; [email protected]; [email protected]
Subject: Re: [PATCH 3/3] NFS: Detect loops in a readdir due to bad cookiesQuoted 2 lines · On Wed, 23 Mar 2011 14:48:35 -0400 Trond Myklebust…
Quoted 1 line · On Wed, 2011-03-23 at 14:42 -0400, [email protected] wrote:
Although I think that it is a good thing to protect the
client against broken servers, it does seem like the right solution is to get the server fixed...
Quoted 2 lines
Don't get me wrong: I fully agree with that!
Ack.
As I understand it, ext4 (and ext3) use the hash of the basename as the
cookie. This hash has a per-directory random seed which is hidden so it is
not possible to deliberately create files with the same hash, but thanks to
the birthday paradox it isn't too hard to get collisions in a suitably large
directory.
Quoted 5 lines
For 'readdir' ext4 keeps extra state in the 'struct file' so that it knows
where it was up to in a list of names with the same hash and so won't return
duplicates.
Would it be possible to use this "extra state" to uniqify the cookie used by NFS? Having no idea what this extra state is, I'm just wildly speculating.
Quoted 34 lines
However if you telldir/seekdir you will restart
at the start of
the sequence (I think).Assuming that the fs always delivers names-with-the-same-cookie in a contiguous set (which seems likely), NFSD could mitigate this problem by
treating them as a unit.
i.e. to a given READDIR request, it either returns all names with a particular cookie, or none of them.
This could only work if the set of names with the one cookie fits inside a
single reply, but that should be likely unless the rsize is tiny.nfsd should also, after lseeking to the given address, skip over any subsequent entries that have the same address.
This would ensure no looping happened, and the first step would make it
unlikely that names were missed.It would be nice if filesystems would get this right, but I think it is
reasonable for NFSD to take steps like this to guard against unfortunate
filesystem design.NeilBrown
--
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
Quoted 34 lines · On Wed, 23 Mar 2011 17:36:38 -0400 <[email protected]> wrote:
Quoted 34 lines
-----Original Message-----
From: [email protected] [mailto:[email protected]] On Behalf Of NeilBrown
Sent: Wednesday, March 23, 2011 2:28 PM
To: Trond Myklebust
Cc: Staubach, Peter; [email protected]; [email protected]; [email protected]
Subject: Re: [PATCH 3/3] NFS: Detect loops in a readdir due to bad cookiesQuoted 2 lines · On Wed, 23 Mar 2011 14:48:35 -0400 Trond Myklebust…
Quoted 1 line · On Wed, 2011-03-23 at 14:42 -0400, [email protected] wrote:
Although I think that it is a good thing to protect the
client against broken servers, it does seem like the right solution is to get the server fixed...
Quoted 2 lines
Don't get me wrong: I fully agree with that!
Ack.
As I understand it, ext4 (and ext3) use the hash of the basename as the
cookie. This hash has a per-directory random seed which is hidden so it is
not possible to deliberately create files with the same hash, but thanks to
the birthday paradox it isn't too hard to get collisions in a suitably large
directory.
Quoted 8 lines
Quoted 5 lines
For 'readdir' ext4 keeps extra state in the 'struct file' so that it knows
where it was up to in a list of names with the same hash and so won't return
duplicates.Would it be possible to use this "extra state" to uniqify the cookie used by NFS? Having no idea what this extra state is, I'm just wildly speculating.
No, it is all hidden inside filp->private_data. It is just a linked-list of file names (see 'extra_fname' in fs/ext4/dir.c).
The best you could do would be to shift the cookie up a few bits (ext4 cookies are 62 bit already!) and add a sequence counter.
But I don't think that approach will really give more reliability than just trying to hide cookie-runs by making sure they don't cross from one packet to the next.
NeilBrown