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(-)
diff --git a/fs/nfs/dir.c b/fs/nfs/dir.c
index b503791..dc475a7 100644
--- a/fs/nfs/dir.c
+++ b/fs/nfs/dir.c
@@ -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 @@ static
int 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)) {
diff --git a/include/linux/nfs_fs.h b/include/linux/nfs_fs.h
index 4b87c00..bbb812b 100644
--- a/include/linux/nfs_fs.h
+++ b/include/linux/nfs_fs.h
@@ -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;
};
/*
--
1.7.4.1
On Wed, 23 Mar 2011 17:36:38 -0400 <[email protected]> wrote:
>
>
> > -----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 cookies
> >
> > On Wed, 23 Mar 2011 14:48:35 -0400 Trond Myklebust
> > <[email protected]> wrote:
> >
> > > 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.
>
> 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
> -----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 cookies
>
> On Wed, 23 Mar 2011 14:48:35 -0400 Trond Myklebust
> <[email protected]> wrote:
>
> > 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.
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.
> 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
>
>
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
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
On Wed, 23 Mar 2011 14:48:35 -0400 Trond Myklebust
<[email protected]> wrote:
> 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
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.
--
Trond Myklebust
Linux NFS client maintainer
NetApp
[email protected]
http://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.
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(-)
diff --git a/fs/nfs/dir.c b/fs/nfs/dir.c
index b503791..dc475a7 100644
--- a/fs/nfs/dir.c
+++ b/fs/nfs/dir.c
@@ -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 @@ static
int 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)) {
diff --git a/include/linux/nfs_fs.h b/include/linux/nfs_fs.h
index 4b87c00..bbb812b 100644
--- a/include/linux/nfs_fs.h
+++ b/include/linux/nfs_fs.h
@@ -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;
};
/*
--
1.7.4.1
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.
--
Trond Myklebust
Linux NFS client maintainer
NetApp
[email protected]
http://www.netapp.com
On Wed, Mar 23, 2011 at 01:39:44PM -0400, Bryan Schumaker wrote:
> 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.
> 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(-)
>
> diff --git a/fs/nfs/dir.c b/fs/nfs/dir.c
> index b503791..dc475a7 100644
> --- a/fs/nfs/dir.c
> +++ b/fs/nfs/dir.c
> @@ -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 @@ static
> int 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)) {
> diff --git a/include/linux/nfs_fs.h b/include/linux/nfs_fs.h
> index 4b87c00..bbb812b 100644
> --- a/include/linux/nfs_fs.h
> +++ b/include/linux/nfs_fs.h
> @@ -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;
> };
>
> /*
> --
> 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
On Wed, 2011-03-23 at 13:39 -0400, Bryan Schumaker wrote:
> 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(-)
>
> diff --git a/fs/nfs/dir.c b/fs/nfs/dir.c
> index b503791..dc475a7 100644
> --- a/fs/nfs/dir.c
> +++ b/fs/nfs/dir.c
> @@ -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 @@ static
> int 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).
> 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) {
^^^^^^^^^^^^^^^^
nit: it is usually slightly more efficient to test for ctx->duped != 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)) {
> diff --git a/include/linux/nfs_fs.h b/include/linux/nfs_fs.h
> index 4b87c00..bbb812b 100644
> --- a/include/linux/nfs_fs.h
> +++ b/include/linux/nfs_fs.h
> @@ -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).
> __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.
--
Trond Myklebust
Linux NFS client maintainer
NetApp
[email protected]
http://www.netapp.com
On Wed, 2011-03-23 at 14:25 -0400, J. Bruce Fields wrote:
> On Wed, Mar 23, 2011 at 01:39:44PM -0400, Bryan Schumaker wrote:
> > 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).
--
Trond Myklebust
Linux NFS client maintainer
NetApp
[email protected]
http://www.netapp.com