2008-09-04 14:55:31

by Jiri Pirko

Subject: [PATCH] nfsd/nfs4acl: Number of used used array elements needs to be zeroed.

Number of used used array elements needs to be zeroed. It may cause
problems otherwise, because it's used uninitialized in find_uid().

Signed-off-by: Jiri Pirko <[email protected]>
---
fs/nfsd/nfs4acl.c | 2 ++
1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/fs/nfsd/nfs4acl.c b/fs/nfsd/nfs4acl.c
index 54b8b41..7dcd90f 100644
--- a/fs/nfsd/nfs4acl.c
+++ b/fs/nfsd/nfs4acl.c
@@ -447,11 +447,13 @@ init_state(struct posix_acl_state *state, int cnt)
state->users = kzalloc(alloc, GFP_KERNEL);
if (!state->users)
return -ENOMEM;
+ state->users->n = 0;
state->groups = kzalloc(alloc, GFP_KERNEL);
if (!state->groups) {
kfree(state->users);
return -ENOMEM;
}
+ state->groups->n = 0;
return 0;
}

--
1.5.5.1

2008-09-04 15:01:03

by J. Bruce Fields

Subject: Re: [PATCH] nfsd/nfs4acl: Number of used used array elements needs to be zeroed.

On Thu, Sep 04, 2008 at 04:55:18PM +0200, Jiri Pirko wrote:
> Number of used used array elements needs to be zeroed. It may cause
> problems otherwise, because it's used uninitialized in find_uid().
>
> Signed-off-by: Jiri Pirko <[email protected]>
> ---
> fs/nfsd/nfs4acl.c | 2 ++
> 1 files changed, 2 insertions(+), 0 deletions(-)
>
> diff --git a/fs/nfsd/nfs4acl.c b/fs/nfsd/nfs4acl.c
> index 54b8b41..7dcd90f 100644
> --- a/fs/nfsd/nfs4acl.c
> +++ b/fs/nfsd/nfs4acl.c
> @@ -447,11 +447,13 @@ init_state(struct posix_acl_state *state, int cnt)
> state->users = kzalloc(alloc, GFP_KERNEL);
> if (!state->users)
> return -ENOMEM;
> + state->users->n = 0;
> state->groups = kzalloc(alloc, GFP_KERNEL);
> if (!state->groups) {
> kfree(state->users);
> return -ENOMEM;
> }
> + state->groups->n = 0;
> return 0;
> }

Thanks for the extra eyes on this code, but: surely the kzalloc()'s are
all that's necessary? Am I missing something?

--b.

2008-09-04 16:41:28

by Benny Halevy

Subject: Re: [PATCH] nfsd/nfs4acl: Number of used used array elements needs to be zeroed.

On Sep. 04, 2008, 18:01 +0300, "J. Bruce Fields" <[email protected]> wrote:
> On Thu, Sep 04, 2008 at 04:55:18PM +0200, Jiri Pirko wrote:
>> Number of used used array elements needs to be zeroed. It may cause
>> problems otherwise, because it's used uninitialized in find_uid().
>>
>> Signed-off-by: Jiri Pirko <[email protected]>
>> ---
>> fs/nfsd/nfs4acl.c | 2 ++
>> 1 files changed, 2 insertions(+), 0 deletions(-)
>>
>> diff --git a/fs/nfsd/nfs4acl.c b/fs/nfsd/nfs4acl.c
>> index 54b8b41..7dcd90f 100644
>> --- a/fs/nfsd/nfs4acl.c
>> +++ b/fs/nfsd/nfs4acl.c
>> @@ -447,11 +447,13 @@ init_state(struct posix_acl_state *state, int cnt)
>> state->users = kzalloc(alloc, GFP_KERNEL);
>> if (!state->users)
>> return -ENOMEM;
>> + state->users->n = 0;
>> state->groups = kzalloc(alloc, GFP_KERNEL);
>> if (!state->groups) {
>> kfree(state->users);
>> return -ENOMEM;
>> }
>> + state->groups->n = 0;
>> return 0;
>> }
>
> Thanks for the extra eyes on this code, but: surely the kzalloc()'s are
> all that's necessary? Am I missing something?

quickly browsing over the code, shouldn't it be:

diff --git a/fs/nfsd/nfs4acl.c b/fs/nfsd/nfs4acl.c
index b6ed383..54b8b41 100644
--- a/fs/nfsd/nfs4acl.c
+++ b/fs/nfsd/nfs4acl.c
@@ -443,7 +443,7 @@ init_state(struct posix_acl_state *state, int cnt)
* enough space for either:
*/
alloc = sizeof(struct posix_ace_state_array)
- + cnt*sizeof(struct posix_ace_state);
+ + cnt*sizeof(struct posix_user_ace_state);
state->users = kzalloc(alloc, GFP_KERNEL);
if (!state->users)
return -ENOMEM;

Benny

>
> --b.
> --
> 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

2008-09-04 16:47:27

by david m. richter

Subject: Re: [PATCH] nfsd/nfs4acl: Number of used used array elements needs to be zeroed.

On Thu, 4 Sep 2008, Benny Halevy wrote:

> On Sep. 04, 2008, 18:01 +0300, "J. Bruce Fields" <[email protected]> wrote:
> > On Thu, Sep 04, 2008 at 04:55:18PM +0200, Jiri Pirko wrote:
> >> Number of used used array elements needs to be zeroed. It may cause
> >> problems otherwise, because it's used uninitialized in find_uid().
> >>
> >> Signed-off-by: Jiri Pirko <[email protected]>
> >> ---
> >> fs/nfsd/nfs4acl.c | 2 ++
> >> 1 files changed, 2 insertions(+), 0 deletions(-)
> >>
> >> diff --git a/fs/nfsd/nfs4acl.c b/fs/nfsd/nfs4acl.c
> >> index 54b8b41..7dcd90f 100644
> >> --- a/fs/nfsd/nfs4acl.c
> >> +++ b/fs/nfsd/nfs4acl.c
> >> @@ -447,11 +447,13 @@ init_state(struct posix_acl_state *state, int cnt)
> >> state->users = kzalloc(alloc, GFP_KERNEL);
> >> if (!state->users)
> >> return -ENOMEM;
> >> + state->users->n = 0;
> >> state->groups = kzalloc(alloc, GFP_KERNEL);
> >> if (!state->groups) {
> >> kfree(state->users);
> >> return -ENOMEM;
> >> }
> >> + state->groups->n = 0;
> >> return 0;
> >> }
> >
> > Thanks for the extra eyes on this code, but: surely the kzalloc()'s are
> > all that's necessary? Am I missing something?
>
> quickly browsing over the code, shouldn't it be:
>
> diff --git a/fs/nfsd/nfs4acl.c b/fs/nfsd/nfs4acl.c
> index b6ed383..54b8b41 100644
> --- a/fs/nfsd/nfs4acl.c
> +++ b/fs/nfsd/nfs4acl.c
> @@ -443,7 +443,7 @@ init_state(struct posix_acl_state *state, int cnt)
> * enough space for either:
> */
> alloc = sizeof(struct posix_ace_state_array)
> - + cnt*sizeof(struct posix_ace_state);
> + + cnt*sizeof(struct posix_user_ace_state);

:) heheheh, we could've used your sharp eyes last week when we
were dealing with ACL shenanigans -- i believe that this is covered in one
of the patches that bruce sent out in the last few days.

d
.

> state->users = kzalloc(alloc, GFP_KERNEL);
> if (!state->users)
> return -ENOMEM;
>
> Benny
>
> >
> > --b.
> > --
> > 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
> --
> 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
>

2008-09-04 16:51:36

by J. Bruce Fields

Subject: Re: [PATCH] nfsd/nfs4acl: Number of used used array elements needs to be zeroed.

On Thu, Sep 04, 2008 at 07:40:48PM +0300, Benny Halevy wrote:
> On Sep. 04, 2008, 18:01 +0300, "J. Bruce Fields" <[email protected]> wrote:
> > On Thu, Sep 04, 2008 at 04:55:18PM +0200, Jiri Pirko wrote:
> >> Number of used used array elements needs to be zeroed. It may cause
> >> problems otherwise, because it's used uninitialized in find_uid().
> >>
> >> Signed-off-by: Jiri Pirko <[email protected]>
> >> ---
> >> fs/nfsd/nfs4acl.c | 2 ++
> >> 1 files changed, 2 insertions(+), 0 deletions(-)
> >>
> >> diff --git a/fs/nfsd/nfs4acl.c b/fs/nfsd/nfs4acl.c
> >> index 54b8b41..7dcd90f 100644
> >> --- a/fs/nfsd/nfs4acl.c
> >> +++ b/fs/nfsd/nfs4acl.c
> >> @@ -447,11 +447,13 @@ init_state(struct posix_acl_state *state, int cnt)
> >> state->users = kzalloc(alloc, GFP_KERNEL);
> >> if (!state->users)
> >> return -ENOMEM;
> >> + state->users->n = 0;
> >> state->groups = kzalloc(alloc, GFP_KERNEL);
> >> if (!state->groups) {
> >> kfree(state->users);
> >> return -ENOMEM;
> >> }
> >> + state->groups->n = 0;
> >> return 0;
> >> }
> >
> > Thanks for the extra eyes on this code, but: surely the kzalloc()'s are
> > all that's necessary? Am I missing something?
>
> quickly browsing over the code, shouldn't it be:
>
> diff --git a/fs/nfsd/nfs4acl.c b/fs/nfsd/nfs4acl.c
> index b6ed383..54b8b41 100644
> --- a/fs/nfsd/nfs4acl.c
> +++ b/fs/nfsd/nfs4acl.c
> @@ -443,7 +443,7 @@ init_state(struct posix_acl_state *state, int cnt)
> * enough space for either:
> */
> alloc = sizeof(struct posix_ace_state_array)
> - + cnt*sizeof(struct posix_ace_state);
> + + cnt*sizeof(struct posix_user_ace_state);
> state->users = kzalloc(alloc, GFP_KERNEL);
> if (!state->users)
> return -ENOMEM;

Yep, see 91b80969ba466ba4b915a4a1d03add8c297add3f.--b.

2008-09-04 22:20:56

by Jiri Pirko

Subject: Re: [PATCH] nfsd/nfs4acl: Number of used used array elements needs to be zeroed.

On Thu, 4 Sep 2008 11:01:01 -0400
"J. Bruce Fields" <[email protected]> wrote:

> On Thu, Sep 04, 2008 at 04:55:18PM +0200, Jiri Pirko wrote:
> > Number of used used array elements needs to be zeroed. It may cause
> > problems otherwise, because it's used uninitialized in find_uid().
> >
> > Signed-off-by: Jiri Pirko <[email protected]>
> > ---
> > fs/nfsd/nfs4acl.c | 2 ++
> > 1 files changed, 2 insertions(+), 0 deletions(-)
> >
> > diff --git a/fs/nfsd/nfs4acl.c b/fs/nfsd/nfs4acl.c
> > index 54b8b41..7dcd90f 100644
> > --- a/fs/nfsd/nfs4acl.c
> > +++ b/fs/nfsd/nfs4acl.c
> > @@ -447,11 +447,13 @@ init_state(struct posix_acl_state *state, int cnt)
> > state->users = kzalloc(alloc, GFP_KERNEL);
> > if (!state->users)
> > return -ENOMEM;
> > + state->users->n = 0;
> > state->groups = kzalloc(alloc, GFP_KERNEL);
> > if (!state->groups) {
> > kfree(state->users);
> > return -ENOMEM;
> > }
> > + state->groups->n = 0;
> > return 0;
> > }
>
> Thanks for the extra eyes on this code, but: surely the kzalloc()'s are
> all that's necessary? Am I missing something?
Sure you are right. Sorry, my bad...

Jirka
>
> --b.