Re: [PATCH 10/10] NFSD: Server implementation of MAC Labeling

["David P. Quigley" <dpquigl@tycho.nsa.gov>]

On Wed, 2010-07-07 at 15:24 -0400, J. Bruce Fields wrote:
> On Wed, Jul 07, 2010 at 02:03:21PM -0400, David P. Quigley wrote:
> > Comments inline
> > 
> > On Wed, 2010-07-07 at 13:21 -0400, J. Bruce Fields wrote:
> > > On Wed, Jul 07, 2010 at 10:31:26AM -0400, David P. Quigley wrote:
> > > > This patch adds the ability to encode and decode file labels on the server for
> > > >  static __be32
> > > >  nfsd4_decode_fattr(struct nfsd4_compoundargs *argp, u32 *bmval,
> > > > -		   struct iattr *iattr, struct nfs4_acl **acl)
> > > > +		   struct iattr *iattr, struct nfs4_acl **acl,
> > > > +		   struct nfs4_label **label)
> > > 
> > > As we add more arguments, I wonder if at some point it becomes worth
> > > creating something like
> > > 
> > > 	struct nfsd4_attrs {
> > > 		struct iattr iattr;
> > > 		struct nfs4_acl *acl;
> > > 		...
> > > 	}
> > > 
> > > and passing that instead?
> > 
> > I can definitely submit something like that as a stand alone patch and
> > then add our nfs4_label stuff to that instead.
> 
> Not a big deal, but welcome if such a thing looks more generally useful
> (say, if the same arguments are passed elsewhere).
> 
> > > > +#ifdef CONFIG_NFSD_V4_SECURITY_LABEL
> > > > +	if (bmval[1] & FATTR4_WORD1_SECURITY_LABEL) {
> > > > +		uint32_t lfs;
> > > > +
> > > > +		READ_BUF(4);
> > > > +		len += 4;
> > > > +		READ32(lfs);
> > > > +		READ_BUF(4);
> > > > +		len += 4;
> > > > +		READ32(dummy32);
> > > > +		READ_BUF(dummy32);
> > > > +		len += (XDR_QUADLEN(dummy32) << 2);
> > > > +		READMEM(buf, dummy32);
> > > > +
> > > > +		if (dummy32 > NFS4_MAXLABELLEN)
> > > > +			return nfserr_resource;
> > > > +
> > > > +		*label = kzalloc(sizeof(struct nfs4_label), GFP_KERNEL);
> > > 
> > > Could we allocate this some toher way (it's small, right?) and avoid the
> > > extra dynamic allocation here, just for simplicity's sake?
> > 
> > How would you suggest going about doing that?
> 
> Maybe make op_label, cr_label, etc., a struct nfs4_label instead of a
> struct nfs4_label *?

I'll look into this. Since the structure contains a void * for the label
data it might not be necessary to have to allocate the label structure
itself just make sure its zeroed out so we can check to see if its
initialized with anything.

> 
> > 
> > > 
> > > > +		if (*label == NULL) {
> > > > +			host_err = -ENOMEM;
> > > > +			goto out_nfserr;
> > > > +		}
> > > > +
> > > > +		(*label)->label = kmalloc(dummy32 + 1, GFP_KERNEL);
> > > 
> > > Might be nice to arrange NFS4_MAXLABELLEN to ensure this is never a
> > > higher-order allocation.
> > 
> > This should never be more than 4096. Under what circumstances would this
> > become a higher-order allocation?
> 
> Can't dummy32+1 be 4097?

You're right. My mistake. NFS4_MAXLABELLEN is supposed to include the
null termination. I'll fix this. I wonder if its better to just redefine
NFS4_MAXLABELLEN to be 4095 so when we use the extra null terminator it
will be a page size max.
> 
> --b.