* [PATCH 1/2] NFS:Prevent infinite loop in decode_attr_fs_locations.
@ 2008-10-17 22:52 Dean Hildebrand
2008-10-17 22:52 ` [PATCH 2/2] NFS: Cleanup decode_attr_fs_locations function Dean Hildebrand
0 siblings, 1 reply; 8+ messages in thread
From: Dean Hildebrand @ 2008-10-17 22:52 UTC (permalink / raw)
To: linux-nfs; +Cc: Dean Hildebrand
An infinite loop could occur if n > NFS4_FS_LOCATIONS_MAXENTRIES.
or if m > NFS4_FS_LOCATION_MAXSERVERS.
Signed-off-by: Dean Hildebrand <dhildeb@us.ibm.com>
---
fs/nfs/nfs4xdr.c | 49 ++++++++++++++++++++++++++++++-------------------
1 files changed, 30 insertions(+), 19 deletions(-)
diff --git a/fs/nfs/nfs4xdr.c b/fs/nfs/nfs4xdr.c
index b916297..0b4c565 100644
--- a/fs/nfs/nfs4xdr.c
+++ b/fs/nfs/nfs4xdr.c
@@ -2577,14 +2577,31 @@ static int decode_attr_fs_locations(struct xdr_stream *xdr, uint32_t *bitmap, st
READ32(n);
if (n <= 0)
goto out_eio;
+
+ if (n > NFS4_FS_LOCATIONS_MAXENTRIES) {
+ dprintk("\n%s: Using first %u of %d fs locations\n",
+ __func__, NFS4_FS_LOCATIONS_MAXENTRIES, n);
+ n = NFS4_FS_LOCATIONS_MAXENTRIES;
+ }
+
res->nlocations = 0;
while (res->nlocations < n) {
u32 m;
+ unsigned int totalserv, i;
struct nfs4_fs_location *loc = &res->locations[res->nlocations];
READ_BUF(4);
READ32(m);
+ totalserv = m;
+ if (m > NFS4_FS_LOCATION_MAXSERVERS) {
+ dprintk("\n%s: Using first %u of %u servers "
+ "returned for location %u\n",
+ __func__, NFS4_FS_LOCATION_MAXSERVERS,
+ m, res->nlocations);
+ m = NFS4_FS_LOCATION_MAXSERVERS;
+ }
+
loc->nservers = 0;
dprintk("%s: servers ", __func__);
while (loc->nservers < m) {
@@ -2593,29 +2610,23 @@ static int decode_attr_fs_locations(struct xdr_stream *xdr, uint32_t *bitmap, st
if (unlikely(status != 0))
goto out_eio;
dprintk("%s ", server->data);
- if (loc->nservers < NFS4_FS_LOCATION_MAXSERVERS)
- loc->nservers++;
- else {
- unsigned int i;
- dprintk("%s: using first %u of %u servers "
- "returned for location %u\n",
- __func__,
- NFS4_FS_LOCATION_MAXSERVERS,
- m, res->nlocations);
- for (i = loc->nservers; i < m; i++) {
- unsigned int len;
- char *data;
- status = decode_opaque_inline(xdr, &len, &data);
- if (unlikely(status != 0))
- goto out_eio;
- }
- }
+ loc->nservers++;
}
+
+ /* Decode and ignore overflow servers */
+ for (i = loc->nservers; i < totalserv; i++) {
+ unsigned int len;
+ char *data;
+ status = decode_opaque_inline(xdr, &len, &data);
+ if (unlikely(status != 0))
+ goto out_eio;
+ }
+
status = decode_pathname(xdr, &loc->rootpath);
if (unlikely(status != 0))
goto out_eio;
- if (res->nlocations < NFS4_FS_LOCATIONS_MAXENTRIES)
- res->nlocations++;
+
+ res->nlocations++;
}
out:
dprintk("%s: fs_locations done, error = %d\n", __func__, status);
--
1.5.3.3
^ permalink raw reply related [flat|nested] 8+ messages in thread* [PATCH 2/2] NFS: Cleanup decode_attr_fs_locations function.
2008-10-17 22:52 [PATCH 1/2] NFS:Prevent infinite loop in decode_attr_fs_locations Dean Hildebrand
@ 2008-10-17 22:52 ` Dean Hildebrand
2008-10-19 10:38 ` Benny Halevy
0 siblings, 1 reply; 8+ messages in thread
From: Dean Hildebrand @ 2008-10-17 22:52 UTC (permalink / raw)
To: linux-nfs; +Cc: Dean Hildebrand
a) Use correct data types.
b) Use nloc and nserv instead of n and m variable names.
c) Try to clean up formatting of debugging statements.
d) Move while loops to for loops.
Signed-off-by: Dean Hildebrand <dhildeb@us.ibm.com>
---
fs/nfs/nfs4xdr.c | 38 ++++++++++++++++++++------------------
1 files changed, 20 insertions(+), 18 deletions(-)
diff --git a/fs/nfs/nfs4xdr.c b/fs/nfs/nfs4xdr.c
index 0b4c565..b7de923 100644
--- a/fs/nfs/nfs4xdr.c
+++ b/fs/nfs/nfs4xdr.c
@@ -2560,7 +2560,8 @@ out_eio:
static int decode_attr_fs_locations(struct xdr_stream *xdr, uint32_t *bitmap, struct nfs4_fs_locations *res)
{
- int n;
+ u32 nloc;
+ unsigned int i;
__be32 *p;
int status = -EIO;
@@ -2574,37 +2575,37 @@ static int decode_attr_fs_locations(struct xdr_stream *xdr, uint32_t *bitmap, st
if (unlikely(status != 0))
goto out;
READ_BUF(4);
- READ32(n);
- if (n <= 0)
+ READ32(nloc);
+ if (nloc <= 0)
goto out_eio;
- if (n > NFS4_FS_LOCATIONS_MAXENTRIES) {
- dprintk("\n%s: Using first %u of %d fs locations\n",
- __func__, NFS4_FS_LOCATIONS_MAXENTRIES, n);
- n = NFS4_FS_LOCATIONS_MAXENTRIES;
+ if (nloc > NFS4_FS_LOCATIONS_MAXENTRIES) {
+ dprintk("\n%s: Using first %u of %u fs locations\n",
+ __func__, NFS4_FS_LOCATIONS_MAXENTRIES, nloc);
+ nloc = NFS4_FS_LOCATIONS_MAXENTRIES;
}
res->nlocations = 0;
- while (res->nlocations < n) {
- u32 m;
- unsigned int totalserv, i;
- struct nfs4_fs_location *loc = &res->locations[res->nlocations];
+ for (i = 0; i < nloc; i++) {
+ u32 nserv;
+ unsigned int totalserv, j;
+ struct nfs4_fs_location *loc = &res->locations[i];
READ_BUF(4);
- READ32(m);
+ READ32(nserv);
- totalserv = m;
- if (m > NFS4_FS_LOCATION_MAXSERVERS) {
+ totalserv = nserv;
+ if (nserv > NFS4_FS_LOCATION_MAXSERVERS) {
dprintk("\n%s: Using first %u of %u servers "
"returned for location %u\n",
__func__, NFS4_FS_LOCATION_MAXSERVERS,
- m, res->nlocations);
- m = NFS4_FS_LOCATION_MAXSERVERS;
+ nserv, i);
+ nserv = NFS4_FS_LOCATION_MAXSERVERS;
}
loc->nservers = 0;
dprintk("%s: servers ", __func__);
- while (loc->nservers < m) {
+ for (j = 0; j < nserv; j++) {
struct nfs4_string *server = &loc->servers[loc->nservers];
status = decode_opaque_inline(xdr, &server->len, &server->data);
if (unlikely(status != 0))
@@ -2612,9 +2613,10 @@ static int decode_attr_fs_locations(struct xdr_stream *xdr, uint32_t *bitmap, st
dprintk("%s ", server->data);
loc->nservers++;
}
+ dprintk("\n");
/* Decode and ignore overflow servers */
- for (i = loc->nservers; i < totalserv; i++) {
+ for (j = loc->nservers; j < totalserv; j++) {
unsigned int len;
char *data;
status = decode_opaque_inline(xdr, &len, &data);
--
1.5.3.3
^ permalink raw reply related [flat|nested] 8+ messages in thread* Re: [PATCH 2/2] NFS: Cleanup decode_attr_fs_locations function.
2008-10-17 22:52 ` [PATCH 2/2] NFS: Cleanup decode_attr_fs_locations function Dean Hildebrand
@ 2008-10-19 10:38 ` Benny Halevy
2008-10-24 17:09 ` Dean Hildebrand
0 siblings, 1 reply; 8+ messages in thread
From: Benny Halevy @ 2008-10-19 10:38 UTC (permalink / raw)
To: Dean Hildebrand; +Cc: linux-nfs, Dean Hildebrand
On Oct. 18, 2008, 0:52 +0200, Dean Hildebrand <seattleplus@gmail.com> wrote:
> a) Use correct data types.
> b) Use nloc and nserv instead of n and m variable names.
> c) Try to clean up formatting of debugging statements.
> d) Move while loops to for loops.
>
> Signed-off-by: Dean Hildebrand <dhildeb@us.ibm.com>
> ---
> fs/nfs/nfs4xdr.c | 38 ++++++++++++++++++++------------------
> 1 files changed, 20 insertions(+), 18 deletions(-)
>
> diff --git a/fs/nfs/nfs4xdr.c b/fs/nfs/nfs4xdr.c
> index 0b4c565..b7de923 100644
> --- a/fs/nfs/nfs4xdr.c
> +++ b/fs/nfs/nfs4xdr.c
> @@ -2560,7 +2560,8 @@ out_eio:
>
> static int decode_attr_fs_locations(struct xdr_stream *xdr, uint32_t *bitmap, struct nfs4_fs_locations *res)
> {
> - int n;
> + u32 nloc;
> + unsigned int i;
> __be32 *p;
> int status = -EIO;
>
> @@ -2574,37 +2575,37 @@ static int decode_attr_fs_locations(struct xdr_stream *xdr, uint32_t *bitmap, st
> if (unlikely(status != 0))
> goto out;
> READ_BUF(4);
> - READ32(n);
> - if (n <= 0)
> + READ32(nloc);
> + if (nloc <= 0)
> goto out_eio;
>
> - if (n > NFS4_FS_LOCATIONS_MAXENTRIES) {
> - dprintk("\n%s: Using first %u of %d fs locations\n",
> - __func__, NFS4_FS_LOCATIONS_MAXENTRIES, n);
> - n = NFS4_FS_LOCATIONS_MAXENTRIES;
> + if (nloc > NFS4_FS_LOCATIONS_MAXENTRIES) {
> + dprintk("\n%s: Using first %u of %u fs locations\n",
> + __func__, NFS4_FS_LOCATIONS_MAXENTRIES, nloc);
> + nloc = NFS4_FS_LOCATIONS_MAXENTRIES;
> }
>
> res->nlocations = 0;
> - while (res->nlocations < n) {
> - u32 m;
> - unsigned int totalserv, i;
> - struct nfs4_fs_location *loc = &res->locations[res->nlocations];
> + for (i = 0; i < nloc; i++) {
you could also keep using res->nlocations as iterator, e.g.
- res->nlocations = 0;
- while (res->nlocations < n) {
+ for (res->nlocations = 0; res->nlocations < nloc; res->nlocations++) {
Since it is incremented every time we go through the loop
anyway using the auxiliary variable is useless.
(It could possibly improve performance a bit for long
arrays if the compiler would've used a register for the local
variable, but then you should assign its final value
to res->nlocations, not increment it every iteration.
However, I don't think it's worth it in this case)
> + u32 nserv;
> + unsigned int totalserv, j;
> + struct nfs4_fs_location *loc = &res->locations[i];
>
> READ_BUF(4);
> - READ32(m);
> + READ32(nserv);
>
> - totalserv = m;
> - if (m > NFS4_FS_LOCATION_MAXSERVERS) {
> + totalserv = nserv;
> + if (nserv > NFS4_FS_LOCATION_MAXSERVERS) {
> dprintk("\n%s: Using first %u of %u servers "
> "returned for location %u\n",
> __func__, NFS4_FS_LOCATION_MAXSERVERS,
> - m, res->nlocations);
> - m = NFS4_FS_LOCATION_MAXSERVERS;
> + nserv, i);
> + nserv = NFS4_FS_LOCATION_MAXSERVERS;
> }
>
> loc->nservers = 0;
> dprintk("%s: servers ", __func__);
> - while (loc->nservers < m) {
> + for (j = 0; j < nserv; j++) {
ditto for loc->nservers.
Benny
> struct nfs4_string *server = &loc->servers[loc->nservers];
> status = decode_opaque_inline(xdr, &server->len, &server->data);
> if (unlikely(status != 0))
> @@ -2612,9 +2613,10 @@ static int decode_attr_fs_locations(struct xdr_stream *xdr, uint32_t *bitmap, st
> dprintk("%s ", server->data);
> loc->nservers++;
> }
> + dprintk("\n");
>
> /* Decode and ignore overflow servers */
> - for (i = loc->nservers; i < totalserv; i++) {
> + for (j = loc->nservers; j < totalserv; j++) {
> unsigned int len;
> char *data;
> status = decode_opaque_inline(xdr, &len, &data);
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH 2/2] NFS: Cleanup decode_attr_fs_locations function.
2008-10-19 10:38 ` Benny Halevy
@ 2008-10-24 17:09 ` Dean Hildebrand
0 siblings, 0 replies; 8+ messages in thread
From: Dean Hildebrand @ 2008-10-24 17:09 UTC (permalink / raw)
To: Benny Halevy; +Cc: linux-nfs
Benny Halevy wrote:
> On Oct. 18, 2008, 0:52 +0200, Dean Hildebrand <seattleplus@gmail.com> wrote:
>
>> a) Use correct data types.
>> b) Use nloc and nserv instead of n and m variable names.
>> c) Try to clean up formatting of debugging statements.
>> d) Move while loops to for loops.
>>
>> Signed-off-by: Dean Hildebrand <dhildeb@us.ibm.com>
>> ---
>> fs/nfs/nfs4xdr.c | 38 ++++++++++++++++++++------------------
>> 1 files changed, 20 insertions(+), 18 deletions(-)
>>
>> diff --git a/fs/nfs/nfs4xdr.c b/fs/nfs/nfs4xdr.c
>> index 0b4c565..b7de923 100644
>> --- a/fs/nfs/nfs4xdr.c
>> +++ b/fs/nfs/nfs4xdr.c
>> @@ -2560,7 +2560,8 @@ out_eio:
>>
>> static int decode_attr_fs_locations(struct xdr_stream *xdr, uint32_t *bitmap, struct nfs4_fs_locations *res)
>> {
>> - int n;
>> + u32 nloc;
>> + unsigned int i;
>> __be32 *p;
>> int status = -EIO;
>>
>> @@ -2574,37 +2575,37 @@ static int decode_attr_fs_locations(struct xdr_stream *xdr, uint32_t *bitmap, st
>> if (unlikely(status != 0))
>> goto out;
>> READ_BUF(4);
>> - READ32(n);
>> - if (n <= 0)
>> + READ32(nloc);
>> + if (nloc <= 0)
>> goto out_eio;
>>
>> - if (n > NFS4_FS_LOCATIONS_MAXENTRIES) {
>> - dprintk("\n%s: Using first %u of %d fs locations\n",
>> - __func__, NFS4_FS_LOCATIONS_MAXENTRIES, n);
>> - n = NFS4_FS_LOCATIONS_MAXENTRIES;
>> + if (nloc > NFS4_FS_LOCATIONS_MAXENTRIES) {
>> + dprintk("\n%s: Using first %u of %u fs locations\n",
>> + __func__, NFS4_FS_LOCATIONS_MAXENTRIES, nloc);
>> + nloc = NFS4_FS_LOCATIONS_MAXENTRIES;
>> }
>>
>> res->nlocations = 0;
>> - while (res->nlocations < n) {
>> - u32 m;
>> - unsigned int totalserv, i;
>> - struct nfs4_fs_location *loc = &res->locations[res->nlocations];
>> + for (i = 0; i < nloc; i++) {
>>
>
> you could also keep using res->nlocations as iterator, e.g.
>
> - res->nlocations = 0;
> - while (res->nlocations < n) {
> + for (res->nlocations = 0; res->nlocations < nloc; res->nlocations++) {
>
Sounds reasonable, although I don't want to hear any flak for exceeding
80 chars.
Dean
> Since it is incremented every time we go through the loop
> anyway using the auxiliary variable is useless.
> (It could possibly improve performance a bit for long
> arrays if the compiler would've used a register for the local
> variable, but then you should assign its final value
> to res->nlocations, not increment it every iteration.
> However, I don't think it's worth it in this case)
>
>
>> + u32 nserv;
>> + unsigned int totalserv, j;
>> + struct nfs4_fs_location *loc = &res->locations[i];
>>
>> READ_BUF(4);
>> - READ32(m);
>> + READ32(nserv);
>>
>> - totalserv = m;
>> - if (m > NFS4_FS_LOCATION_MAXSERVERS) {
>> + totalserv = nserv;
>> + if (nserv > NFS4_FS_LOCATION_MAXSERVERS) {
>> dprintk("\n%s: Using first %u of %u servers "
>> "returned for location %u\n",
>> __func__, NFS4_FS_LOCATION_MAXSERVERS,
>> - m, res->nlocations);
>> - m = NFS4_FS_LOCATION_MAXSERVERS;
>> + nserv, i);
>> + nserv = NFS4_FS_LOCATION_MAXSERVERS;
>> }
>>
>> loc->nservers = 0;
>> dprintk("%s: servers ", __func__);
>> - while (loc->nservers < m) {
>> + for (j = 0; j < nserv; j++) {
>>
>
> ditto for loc->nservers.
>
> Benny
>
>
>> struct nfs4_string *server = &loc->servers[loc->nservers];
>> status = decode_opaque_inline(xdr, &server->len, &server->data);
>> if (unlikely(status != 0))
>> @@ -2612,9 +2613,10 @@ static int decode_attr_fs_locations(struct xdr_stream *xdr, uint32_t *bitmap, st
>> dprintk("%s ", server->data);
>> loc->nservers++;
>> }
>> + dprintk("\n");
>>
>> /* Decode and ignore overflow servers */
>> - for (i = loc->nservers; i < totalserv; i++) {
>> + for (j = loc->nservers; j < totalserv; j++) {
>> unsigned int len;
>> char *data;
>> status = decode_opaque_inline(xdr, &len, &data);
>>
^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH 2/2] NFS: Cleanup decode_attr_fs_locations function.
@ 2008-10-28 17:32 Dean Hildebrand
0 siblings, 0 replies; 8+ messages in thread
From: Dean Hildebrand @ 2008-10-28 17:32 UTC (permalink / raw)
To: linux-nfs; +Cc: Dean Hildebrand
a) Use correct data types.
b) Use nloc and nserv instead of n and m variable names.
c) Try to clean up formatting of debugging statements.
d) Move while loops to for loops.
Signed-off-by: Dean Hildebrand <dhildeb@us.ibm.com>
---
fs/nfs/nfs4xdr.c | 40 ++++++++++++++++++----------------------
1 files changed, 18 insertions(+), 22 deletions(-)
diff --git a/fs/nfs/nfs4xdr.c b/fs/nfs/nfs4xdr.c
index 0b4c565..421609f 100644
--- a/fs/nfs/nfs4xdr.c
+++ b/fs/nfs/nfs4xdr.c
@@ -2560,7 +2560,7 @@ out_eio:
static int decode_attr_fs_locations(struct xdr_stream *xdr, uint32_t *bitmap, struct nfs4_fs_locations *res)
{
- int n;
+ u32 nloc;
__be32 *p;
int status = -EIO;
@@ -2574,47 +2574,45 @@ static int decode_attr_fs_locations(struct xdr_stream *xdr, uint32_t *bitmap, st
if (unlikely(status != 0))
goto out;
READ_BUF(4);
- READ32(n);
- if (n <= 0)
+ READ32(nloc);
+ if (nloc <= 0)
goto out_eio;
- if (n > NFS4_FS_LOCATIONS_MAXENTRIES) {
- dprintk("\n%s: Using first %u of %d fs locations\n",
- __func__, NFS4_FS_LOCATIONS_MAXENTRIES, n);
- n = NFS4_FS_LOCATIONS_MAXENTRIES;
+ if (nloc > NFS4_FS_LOCATIONS_MAXENTRIES) {
+ dprintk("\n%s: Using first %u of %u fs locations\n",
+ __func__, NFS4_FS_LOCATIONS_MAXENTRIES, nloc);
+ nloc = NFS4_FS_LOCATIONS_MAXENTRIES;
}
- res->nlocations = 0;
- while (res->nlocations < n) {
- u32 m;
- unsigned int totalserv, i;
+ for (res->nlocations = 0; res->nlocations < nloc; res->nlocations++) {
+ u32 nserv;
+ unsigned int totalserv, j;
struct nfs4_fs_location *loc = &res->locations[res->nlocations];
READ_BUF(4);
- READ32(m);
+ READ32(nserv);
- totalserv = m;
- if (m > NFS4_FS_LOCATION_MAXSERVERS) {
+ totalserv = nserv;
+ if (nserv > NFS4_FS_LOCATION_MAXSERVERS) {
dprintk("\n%s: Using first %u of %u servers "
"returned for location %u\n",
__func__, NFS4_FS_LOCATION_MAXSERVERS,
- m, res->nlocations);
- m = NFS4_FS_LOCATION_MAXSERVERS;
+ nserv, res->nlocations);
+ nserv = NFS4_FS_LOCATION_MAXSERVERS;
}
- loc->nservers = 0;
dprintk("%s: servers ", __func__);
- while (loc->nservers < m) {
+ for (loc->nservers = 0; loc->nservers < nserv; loc->nservers++) {
struct nfs4_string *server = &loc->servers[loc->nservers];
status = decode_opaque_inline(xdr, &server->len, &server->data);
if (unlikely(status != 0))
goto out_eio;
dprintk("%s ", server->data);
- loc->nservers++;
}
+ dprintk("\n");
/* Decode and ignore overflow servers */
- for (i = loc->nservers; i < totalserv; i++) {
+ for (j = loc->nservers; j < totalserv; j++) {
unsigned int len;
char *data;
status = decode_opaque_inline(xdr, &len, &data);
@@ -2625,8 +2623,6 @@ static int decode_attr_fs_locations(struct xdr_stream *xdr, uint32_t *bitmap, st
status = decode_pathname(xdr, &loc->rootpath);
if (unlikely(status != 0))
goto out_eio;
-
- res->nlocations++;
}
out:
dprintk("%s: fs_locations done, error = %d\n", __func__, status);
--
1.5.4.5
^ permalink raw reply related [flat|nested] 8+ messages in thread* [PATCH 1/2] NFS:Prevent infinite loop in decode_attr_fs_locations.
@ 2008-10-17 18:17 Dean Hildebrand
2008-10-17 18:17 ` [PATCH 2/2] NFS: Cleanup decode_attr_fs_locations function Dean Hildebrand
0 siblings, 1 reply; 8+ messages in thread
From: Dean Hildebrand @ 2008-10-17 18:17 UTC (permalink / raw)
To: linux-nfs; +Cc: Dean Hildebrand
An infinite loop could occur if n > NFS4_FS_LOCATIONS_MAXENTRIES.
Signed-off-by: Dean Hildebrand <dhildeb@us.ibm.com>
---
fs/nfs/nfs4xdr.c | 14 ++++++++++++--
1 files changed, 12 insertions(+), 2 deletions(-)
diff --git a/fs/nfs/nfs4xdr.c b/fs/nfs/nfs4xdr.c
index b916297..5e59481 100644
--- a/fs/nfs/nfs4xdr.c
+++ b/fs/nfs/nfs4xdr.c
@@ -2577,6 +2577,16 @@ static int decode_attr_fs_locations(struct xdr_stream *xdr, uint32_t *bitmap, st
READ32(n);
if (n <= 0)
goto out_eio;
+
+ if (n > NFS4_FS_LOCATIONS_MAXENTRIES) {
+ dprintk("%s: using first %u of %d fs locations\n",
+ __func__, NFS4_FS_LOCATIONS_MAXENTRIES, n);
+ n = NFS4_FS_LOCATIONS_MAXENTRIES;
+ } else {
+ dprintk("%s: using %d fs locations\n",
+ __func__, n);
+ }
+
res->nlocations = 0;
while (res->nlocations < n) {
u32 m;
@@ -2614,8 +2624,8 @@ static int decode_attr_fs_locations(struct xdr_stream *xdr, uint32_t *bitmap, st
status = decode_pathname(xdr, &loc->rootpath);
if (unlikely(status != 0))
goto out_eio;
- if (res->nlocations < NFS4_FS_LOCATIONS_MAXENTRIES)
- res->nlocations++;
+
+ res->nlocations++;
}
out:
dprintk("%s: fs_locations done, error = %d\n", __func__, status);
--
1.5.3.3
^ permalink raw reply related [flat|nested] 8+ messages in thread* [PATCH 2/2] NFS: Cleanup decode_attr_fs_locations function.
2008-10-17 18:17 [PATCH 1/2] NFS:Prevent infinite loop in decode_attr_fs_locations Dean Hildebrand
@ 2008-10-17 18:17 ` Dean Hildebrand
2008-10-17 18:55 ` J. Bruce Fields
0 siblings, 1 reply; 8+ messages in thread
From: Dean Hildebrand @ 2008-10-17 18:17 UTC (permalink / raw)
To: linux-nfs; +Cc: Dean Hildebrand
a) Reduce nesting level of loops.
b) Use correct data types.
c) Use nloc and nserv instead of n and m variable names.
d) Try to clean up formatting of debugging statements.
e) Move while loops to for loops.
Signed-off-by: Dean Hildebrand <dhildeb@us.ibm.com>
---
fs/nfs/nfs4xdr.c | 71 +++++++++++++++++++++++++++++-------------------------
1 files changed, 38 insertions(+), 33 deletions(-)
diff --git a/fs/nfs/nfs4xdr.c b/fs/nfs/nfs4xdr.c
index 5e59481..0e78fbd 100644
--- a/fs/nfs/nfs4xdr.c
+++ b/fs/nfs/nfs4xdr.c
@@ -2560,7 +2560,8 @@ out_eio:
static int decode_attr_fs_locations(struct xdr_stream *xdr, uint32_t *bitmap, struct nfs4_fs_locations *res)
{
- int n;
+ u32 nloc;
+ unsigned int i;
__be32 *p;
int status = -EIO;
@@ -2574,53 +2575,57 @@ static int decode_attr_fs_locations(struct xdr_stream *xdr, uint32_t *bitmap, st
if (unlikely(status != 0))
goto out;
READ_BUF(4);
- READ32(n);
- if (n <= 0)
+ READ32(nloc);
+ if (nloc <= 0)
goto out_eio;
-
- if (n > NFS4_FS_LOCATIONS_MAXENTRIES) {
- dprintk("%s: using first %u of %d fs locations\n",
- __func__, NFS4_FS_LOCATIONS_MAXENTRIES, n);
- n = NFS4_FS_LOCATIONS_MAXENTRIES;
+ if (nloc > NFS4_FS_LOCATIONS_MAXENTRIES) {
+ dprintk("%s: using first %u of %u fs locations\n",
+ __func__, NFS4_FS_LOCATIONS_MAXENTRIES, nloc);
+ nloc = NFS4_FS_LOCATIONS_MAXENTRIES;
} else {
- dprintk("%s: using %d fs locations\n",
- __func__, n);
+ dprintk("%s: using %u fs locations\n",
+ __func__, nloc);
}
res->nlocations = 0;
- while (res->nlocations < n) {
- u32 m;
- struct nfs4_fs_location *loc = &res->locations[res->nlocations];
+ for (i = 0; i < nloc; i++) {
+ u32 nserv;
+ unsigned int totalserv, j;
+ struct nfs4_fs_location *loc = &res->locations[i];
READ_BUF(4);
- READ32(m);
+ READ32(nserv);
+
+ totalserv = nserv;
+ if (nserv > NFS4_FS_LOCATION_MAXSERVERS) {
+ dprintk("%s: Using first %u of %u servers "
+ "returned for location %u\n",
+ __func__, NFS4_FS_LOCATION_MAXSERVERS,
+ nserv, i);
+ nserv = NFS4_FS_LOCATION_MAXSERVERS;
+ }
loc->nservers = 0;
- dprintk("%s: servers ", __func__);
- while (loc->nservers < m) {
+ dprintk("%s: Servers ", __func__);
+ for (j = 0; j < nserv; j++) {
struct nfs4_string *server = &loc->servers[loc->nservers];
status = decode_opaque_inline(xdr, &server->len, &server->data);
if (unlikely(status != 0))
goto out_eio;
dprintk("%s ", server->data);
- if (loc->nservers < NFS4_FS_LOCATION_MAXSERVERS)
- loc->nservers++;
- else {
- unsigned int i;
- dprintk("%s: using first %u of %u servers "
- "returned for location %u\n",
- __func__,
- NFS4_FS_LOCATION_MAXSERVERS,
- m, res->nlocations);
- for (i = loc->nservers; i < m; i++) {
- unsigned int len;
- char *data;
- status = decode_opaque_inline(xdr, &len, &data);
- if (unlikely(status != 0))
- goto out_eio;
- }
- }
+ loc->nservers++;
+ }
+ dprintk("\n");
+
+ /* Decode and ignore overflow servers */
+ for (j = loc->nservers; j < totalserv; j++) {
+ unsigned int len;
+ char *data;
+ status = decode_opaque_inline(xdr, &len, &data);
+ if (unlikely(status != 0))
+ goto out_eio;
}
+
status = decode_pathname(xdr, &loc->rootpath);
if (unlikely(status != 0))
goto out_eio;
--
1.5.3.3
^ permalink raw reply related [flat|nested] 8+ messages in thread* Re: [PATCH 2/2] NFS: Cleanup decode_attr_fs_locations function.
2008-10-17 18:17 ` [PATCH 2/2] NFS: Cleanup decode_attr_fs_locations function Dean Hildebrand
@ 2008-10-17 18:55 ` J. Bruce Fields
2008-10-17 22:01 ` Dean Hildebrand
0 siblings, 1 reply; 8+ messages in thread
From: J. Bruce Fields @ 2008-10-17 18:55 UTC (permalink / raw)
To: Dean Hildebrand; +Cc: linux-nfs, Dean Hildebrand
On Fri, Oct 17, 2008 at 11:17:48AM -0700, Dean Hildebrand wrote:
> a) Reduce nesting level of loops.
> b) Use correct data types.
> c) Use nloc and nserv instead of n and m variable names.
> d) Try to clean up formatting of debugging statements.
> e) Move while loops to for loops.
Does this also fix a possible infinite loop in the inner loop?
--b.
>
> Signed-off-by: Dean Hildebrand <dhildeb@us.ibm.com>
> ---
> fs/nfs/nfs4xdr.c | 71 +++++++++++++++++++++++++++++-------------------------
> 1 files changed, 38 insertions(+), 33 deletions(-)
>
> diff --git a/fs/nfs/nfs4xdr.c b/fs/nfs/nfs4xdr.c
> index 5e59481..0e78fbd 100644
> --- a/fs/nfs/nfs4xdr.c
> +++ b/fs/nfs/nfs4xdr.c
> @@ -2560,7 +2560,8 @@ out_eio:
>
> static int decode_attr_fs_locations(struct xdr_stream *xdr, uint32_t *bitmap, struct nfs4_fs_locations *res)
> {
> - int n;
> + u32 nloc;
> + unsigned int i;
> __be32 *p;
> int status = -EIO;
>
> @@ -2574,53 +2575,57 @@ static int decode_attr_fs_locations(struct xdr_stream *xdr, uint32_t *bitmap, st
> if (unlikely(status != 0))
> goto out;
> READ_BUF(4);
> - READ32(n);
> - if (n <= 0)
> + READ32(nloc);
> + if (nloc <= 0)
> goto out_eio;
> -
> - if (n > NFS4_FS_LOCATIONS_MAXENTRIES) {
> - dprintk("%s: using first %u of %d fs locations\n",
> - __func__, NFS4_FS_LOCATIONS_MAXENTRIES, n);
> - n = NFS4_FS_LOCATIONS_MAXENTRIES;
> + if (nloc > NFS4_FS_LOCATIONS_MAXENTRIES) {
> + dprintk("%s: using first %u of %u fs locations\n",
> + __func__, NFS4_FS_LOCATIONS_MAXENTRIES, nloc);
> + nloc = NFS4_FS_LOCATIONS_MAXENTRIES;
> } else {
> - dprintk("%s: using %d fs locations\n",
> - __func__, n);
> + dprintk("%s: using %u fs locations\n",
> + __func__, nloc);
> }
>
> res->nlocations = 0;
> - while (res->nlocations < n) {
> - u32 m;
> - struct nfs4_fs_location *loc = &res->locations[res->nlocations];
> + for (i = 0; i < nloc; i++) {
> + u32 nserv;
> + unsigned int totalserv, j;
> + struct nfs4_fs_location *loc = &res->locations[i];
>
> READ_BUF(4);
> - READ32(m);
> + READ32(nserv);
> +
> + totalserv = nserv;
> + if (nserv > NFS4_FS_LOCATION_MAXSERVERS) {
> + dprintk("%s: Using first %u of %u servers "
> + "returned for location %u\n",
> + __func__, NFS4_FS_LOCATION_MAXSERVERS,
> + nserv, i);
> + nserv = NFS4_FS_LOCATION_MAXSERVERS;
> + }
>
> loc->nservers = 0;
> - dprintk("%s: servers ", __func__);
> - while (loc->nservers < m) {
> + dprintk("%s: Servers ", __func__);
> + for (j = 0; j < nserv; j++) {
> struct nfs4_string *server = &loc->servers[loc->nservers];
> status = decode_opaque_inline(xdr, &server->len, &server->data);
> if (unlikely(status != 0))
> goto out_eio;
> dprintk("%s ", server->data);
> - if (loc->nservers < NFS4_FS_LOCATION_MAXSERVERS)
> - loc->nservers++;
> - else {
> - unsigned int i;
> - dprintk("%s: using first %u of %u servers "
> - "returned for location %u\n",
> - __func__,
> - NFS4_FS_LOCATION_MAXSERVERS,
> - m, res->nlocations);
> - for (i = loc->nservers; i < m; i++) {
> - unsigned int len;
> - char *data;
> - status = decode_opaque_inline(xdr, &len, &data);
> - if (unlikely(status != 0))
> - goto out_eio;
> - }
> - }
> + loc->nservers++;
> + }
> + dprintk("\n");
> +
> + /* Decode and ignore overflow servers */
> + for (j = loc->nservers; j < totalserv; j++) {
> + unsigned int len;
> + char *data;
> + status = decode_opaque_inline(xdr, &len, &data);
> + if (unlikely(status != 0))
> + goto out_eio;
> }
> +
> status = decode_pathname(xdr, &loc->rootpath);
> if (unlikely(status != 0))
> goto out_eio;
> --
> 1.5.3.3
>
> --
> 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
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH 2/2] NFS: Cleanup decode_attr_fs_locations function.
2008-10-17 18:55 ` J. Bruce Fields
@ 2008-10-17 22:01 ` Dean Hildebrand
0 siblings, 0 replies; 8+ messages in thread
From: Dean Hildebrand @ 2008-10-17 22:01 UTC (permalink / raw)
To: J. Bruce Fields; +Cc: linux-nfs, Dean Hildebrand
J. Bruce Fields wrote:
> On Fri, Oct 17, 2008 at 11:17:48AM -0700, Dean Hildebrand wrote:
>
>> a) Reduce nesting level of loops.
>> b) Use correct data types.
>> c) Use nloc and nserv instead of n and m variable names.
>> d) Try to clean up formatting of debugging statements.
>> e) Move while loops to for loops.
>>
>
> Does this also fix a possible infinite loop in the inner loop?
>
Doh, I put that fix in this patch instead of the first. I will move it
to the first. and apply your comments from
the other email.
Dean
> 1
>> Signed-off-by: Dean Hildebrand <dhildeb@us.ibm.com>
>> ---
>> fs/nfs/nfs4xdr.c | 71 +++++++++++++++++++++++++++++-------------------------
>> 1 files changed, 38 insertions(+), 33 deletions(-)
>>
>> diff --git a/fs/nfs/nfs4xdr.c b/fs/nfs/nfs4xdr.c
>> index 5e59481..0e78fbd 100644
>> --- a/fs/nfs/nfs4xdr.c
>> +++ b/fs/nfs/nfs4xdr.c
>> @@ -2560,7 +2560,8 @@ out_eio:
>>
>> static int decode_attr_fs_locations(struct xdr_stream *xdr, uint32_t *bitmap, struct nfs4_fs_locations *res)
>> {
>> - int n;
>> + u32 nloc;
>> + unsigned int i;
>> __be32 *p;
>> int status = -EIO;
>>
>> @@ -2574,53 +2575,57 @@ static int decode_attr_fs_locations(struct xdr_stream *xdr, uint32_t *bitmap, st
>> if (unlikely(status != 0))
>> goto out;
>> READ_BUF(4);
>> - READ32(n);
>> - if (n <= 0)
>> + READ32(nloc);
>> + if (nloc <= 0)
>> goto out_eio;
>> -
>> - if (n > NFS4_FS_LOCATIONS_MAXENTRIES) {
>> - dprintk("%s: using first %u of %d fs locations\n",
>> - __func__, NFS4_FS_LOCATIONS_MAXENTRIES, n);
>> - n = NFS4_FS_LOCATIONS_MAXENTRIES;
>> + if (nloc > NFS4_FS_LOCATIONS_MAXENTRIES) {
>> + dprintk("%s: using first %u of %u fs locations\n",
>> + __func__, NFS4_FS_LOCATIONS_MAXENTRIES, nloc);
>> + nloc = NFS4_FS_LOCATIONS_MAXENTRIES;
>> } else {
>> - dprintk("%s: using %d fs locations\n",
>> - __func__, n);
>> + dprintk("%s: using %u fs locations\n",
>> + __func__, nloc);
>> }
>>
>> res->nlocations = 0;
>> - while (res->nlocations < n) {
>> - u32 m;
>> - struct nfs4_fs_location *loc = &res->locations[res->nlocations];
>> + for (i = 0; i < nloc; i++) {
>> + u32 nserv;
>> + unsigned int totalserv, j;
>> + struct nfs4_fs_location *loc = &res->locations[i];
>>
>> READ_BUF(4);
>> - READ32(m);
>> + READ32(nserv);
>> +
>> + totalserv = nserv;
>> + if (nserv > NFS4_FS_LOCATION_MAXSERVERS) {
>> + dprintk("%s: Using first %u of %u servers "
>> + "returned for location %u\n",
>> + __func__, NFS4_FS_LOCATION_MAXSERVERS,
>> + nserv, i);
>> + nserv = NFS4_FS_LOCATION_MAXSERVERS;
>> + }
>>
>> loc->nservers = 0;
>> - dprintk("%s: servers ", __func__);
>> - while (loc->nservers < m) {
>> + dprintk("%s: Servers ", __func__);
>> + for (j = 0; j < nserv; j++) {
>> struct nfs4_string *server = &loc->servers[loc->nservers];
>> status = decode_opaque_inline(xdr, &server->len, &server->data);
>> if (unlikely(status != 0))
>> goto out_eio;
>> dprintk("%s ", server->data);
>> - if (loc->nservers < NFS4_FS_LOCATION_MAXSERVERS)
>> - loc->nservers++;
>> - else {
>> - unsigned int i;
>> - dprintk("%s: using first %u of %u servers "
>> - "returned for location %u\n",
>> - __func__,
>> - NFS4_FS_LOCATION_MAXSERVERS,
>> - m, res->nlocations);
>> - for (i = loc->nservers; i < m; i++) {
>> - unsigned int len;
>> - char *data;
>> - status = decode_opaque_inline(xdr, &len, &data);
>> - if (unlikely(status != 0))
>> - goto out_eio;
>> - }
>> - }
>> + loc->nservers++;
>> + }
>> + dprintk("\n");
>> +
>> + /* Decode and ignore overflow servers */
>> + for (j = loc->nservers; j < totalserv; j++) {
>> + unsigned int len;
>> + char *data;
>> + status = decode_opaque_inline(xdr, &len, &data);
>> + if (unlikely(status != 0))
>> + goto out_eio;
>> }
>> +
>> status = decode_pathname(xdr, &loc->rootpath);
>> if (unlikely(status != 0))
>> goto out_eio;
>> --
>> 1.5.3.3
>>
>> --
>> 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
>>
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2008-10-28 17:32 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2008-10-17 22:52 [PATCH 1/2] NFS:Prevent infinite loop in decode_attr_fs_locations Dean Hildebrand
2008-10-17 22:52 ` [PATCH 2/2] NFS: Cleanup decode_attr_fs_locations function Dean Hildebrand
2008-10-19 10:38 ` Benny Halevy
2008-10-24 17:09 ` Dean Hildebrand
-- strict thread matches above, loose matches on Subject: below --
2008-10-28 17:32 Dean Hildebrand
2008-10-17 18:17 [PATCH 1/2] NFS:Prevent infinite loop in decode_attr_fs_locations Dean Hildebrand
2008-10-17 18:17 ` [PATCH 2/2] NFS: Cleanup decode_attr_fs_locations function Dean Hildebrand
2008-10-17 18:55 ` J. Bruce Fields
2008-10-17 22:01 ` Dean Hildebrand
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox