Re: [NFS] [PATCH 006 of 8] knfsd: nfsd4: fslocations data structures

["J. Bruce Fields" <bfields@fieldses.org>]

On Thu, Sep 28, 2006 at 11:45:40PM -0700, Andrew Morton wrote:
> On Fri, 29 Sep 2006 13:09:13 +1000
> NeilBrown <neilb@suse.de> wrote:
> 
> > 
> > From: Manoj Naik <manoj@almaden.ibm.com>
> > 
> > Define FS locations structures, some functions to manipulate them, and add
> > code to parse FS locations in downcall and add to the exports structure.
...
> > +	if (fsloc->locations_count < 0)
> 
> this is unsigned.

Yes, thanks.

> > +	fsloc->locations = kzalloc(fsloc->locations_count
> > +			* sizeof(struct nfsd4_fs_location), GFP_KERNEL);
> 
> This is subject to multiplication overflow.  If it's a privileged operation
> and isn't dependent on stuff coming in over the wire then ok..

This is data provided locally by root.  But there's no reason to allow
this to be arbitrarily large, and I'd still prefer to have some
sanity-checking, so I'll replace the bogus "<0" check above by a
check for something "too large".

> > +out_free_all:
> > +	nfsd4_fslocs_free(fsloc);
> 
> This call to nfsd4_fslocs_free() can end up kfreeing members of
> fsloc->locations[] which haven't been initialised here.  Are we sure the
> caller set them all to zero?

Yes.  There will only ever be one caller, and it has to initialize
these to zero.  I agree that this could be more obvious....

> > +	return err;
> > +}
> > +
> > +#else /* CONFIG_NFSD_V4 */
> > +static int fsloc_parse(char **, char *, struct svc_export *) { return 0; }
> 
> static inline
> 
> This has a different prototype from the other version of fsloc_parse()
> 
> This ain't C++ - function arguments need identifiers as well as types.
> 
> Someone needs to read Documentation/SubmitChecklist..

That would be me, sorry, yes.

Bryce Harrington is also setting up automatic compile-testing for us
with the obvious config options turned on and off, so hopefully that
will help save me from myself....

Patches addressing the above follow.

--b.