An infinite loop could occur if n > NFS4_FS_LOCATIONS_MAXENTRIES.
or if m > NFS4_FS_LOCATION_MAXSERVERS.
Signed-off-by: Dean Hildebrand <[email protected]>
---
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
Benny Halevy wrote:
> On Oct. 18, 2008, 0:52 +0200, Dean Hildebrand <[email protected]> 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 <[email protected]>
>> ---
>> 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);
>>
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 <[email protected]>
---
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
On Oct. 18, 2008, 0:52 +0200, Dean Hildebrand <[email protected]> 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 <[email protected]>
> ---
> 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);