diff mbox

[06/15] NFSv4: Introduce new label structure

Message ID 1360327163-20360-7-git-send-email-SteveD@redhat.com (mailing list archive)
State New, archived
Headers show

Commit Message

Steve Dickson Feb. 8, 2013, 12:39 p.m. UTC
From: David Quigley <dpquigl@davequigley.com>

In order to mimic the way that NFSv4 ACLs are implemented we have created a
structure to be used to pass label data up and down the call chain. This patch
adds the new structure and new members to the required NFSv4 call structures.

Signed-off-by: Matthew N. Dodd <Matthew.Dodd@sparta.com>
Signed-off-by: Miguel Rodel Felipe <Rodel_FM@dsi.a-star.edu.sg>
Signed-off-by: Phua Eu Gene <PHUA_Eu_Gene@dsi.a-star.edu.sg>
Signed-off-by: Khin Mi Mi Aung <Mi_Mi_AUNG@dsi.a-star.edu.sg>
---
 fs/nfs/inode.c            | 33 +++++++++++++++++++++++++++++++++
 include/linux/nfs4.h      |  7 +++++++
 include/linux/nfs_fs.h    | 17 +++++++++++++++++
 include/linux/nfs_xdr.h   | 21 +++++++++++++++++++++
 include/uapi/linux/nfs4.h |  2 +-
 5 files changed, 79 insertions(+), 1 deletion(-)

Comments

J. Bruce Fields Feb. 12, 2013, 10:07 p.m. UTC | #1
On Fri, Feb 08, 2013 at 07:39:14AM -0500, Steve Dickson wrote:
> From: David Quigley <dpquigl@davequigley.com>
> 
> In order to mimic the way that NFSv4 ACLs are implemented we have created a
> structure to be used to pass label data up and down the call chain. This patch
> adds the new structure and new members to the required NFSv4 call structures.
> 
> Signed-off-by: Matthew N. Dodd <Matthew.Dodd@sparta.com>
> Signed-off-by: Miguel Rodel Felipe <Rodel_FM@dsi.a-star.edu.sg>
> Signed-off-by: Phua Eu Gene <PHUA_Eu_Gene@dsi.a-star.edu.sg>
> Signed-off-by: Khin Mi Mi Aung <Mi_Mi_AUNG@dsi.a-star.edu.sg>
> ---
>  fs/nfs/inode.c            | 33 +++++++++++++++++++++++++++++++++
>  include/linux/nfs4.h      |  7 +++++++
>  include/linux/nfs_fs.h    | 17 +++++++++++++++++
>  include/linux/nfs_xdr.h   | 21 +++++++++++++++++++++
>  include/uapi/linux/nfs4.h |  2 +-
>  5 files changed, 79 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/nfs/inode.c b/fs/nfs/inode.c
> index ebeb94c..8d5f01b 100644
> --- a/fs/nfs/inode.c
> +++ b/fs/nfs/inode.c
> @@ -255,6 +255,39 @@ nfs_init_locked(struct inode *inode, void *opaque)
>  	return 0;
>  }
>  
> +#ifdef CONFIG_NFS_V4_SECURITY_LABEL
> +struct nfs4_label *nfs4_label_alloc(struct nfs_server *server, gfp_t flags)
> +{
> +	struct nfs4_label *label = NULL;
> +
> +	if (!(server->caps & NFS_CAP_SECURITY_LABEL))
> +		return label;
> +
> +	label = kzalloc(NFS4_MAXLABELLEN, flags);
> +	if (label == NULL)
> +		return ERR_PTR(-ENOMEM);
> +
> +	label->label = (char *)(label + 1);
> +	label->len = NFS4_MAXLABELLEN;

If you're expecting to be able to store up to NFS4_MAXLABELLEN of data
after the end of the struct, then you want:

	label = kzalloc(sizeof(struct nfs4_label) + NFS4_MAXLABELLEN, flags);

> +
> +	return label;
> +}
> +EXPORT_SYMBOL_GPL(nfs4_label_alloc);
> +
> +void nfs4_label_init(struct nfs4_label *label)
> +{
> +	if (label && label->label) {
> +		/* 0 is the null format meaning that the data is not
> +		to be translated */
> +		label->lfs = 0;
> +		label->pi = 0;
> +		label->len = NFS4_MAXLABELLEN;

This logic seems a little odd to me; why would we pass NULL to this?
Why would we skip the other initialization in the !label->label case?
Why is NFS4_MAXLABELLEN the default?  (And wasn't that just set in
nfs4_label_alloc?)

But OK maybe I need to read the rest of the patches.

> +	}
> +	return;
> +}
> +EXPORT_SYMBOL_GPL(nfs4_label_init);
> +#endif

> diff --git a/include/uapi/linux/nfs4.h b/include/uapi/linux/nfs4.h
> index 788128e..b8014a2 100644
> --- a/include/uapi/linux/nfs4.h
> +++ b/include/uapi/linux/nfs4.h
> @@ -25,7 +25,7 @@
>  #define NFS4_MAXNAMLEN		NAME_MAX
>  #define NFS4_OPAQUE_LIMIT	1024
>  #define NFS4_MAX_SESSIONID_LEN	16
> -
> +#define NFS4_MAXLABELLEN (4095 - offsetof(struct nfs4_label , label))

The way you've defined things, that should be just

			    (4095 - sizeof(struct nfs4_label))

(Is there typically some more alloc overhead that would push this
allocation over 4k?  I don't know well enough how the allocator(s) work
to answer that.)

--b.

>  #define NFS4_ACCESS_READ        0x0001
>  #define NFS4_ACCESS_LOOKUP      0x0002
>  #define NFS4_ACCESS_MODIFY      0x0004
--
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
Trond Myklebust Feb. 12, 2013, 10:28 p.m. UTC | #2
On Tue, 2013-02-12 at 17:07 -0500, J. Bruce Fields wrote:
> On Fri, Feb 08, 2013 at 07:39:14AM -0500, Steve Dickson wrote:
> > From: David Quigley <dpquigl@davequigley.com>
> > 
> > In order to mimic the way that NFSv4 ACLs are implemented we have created a
> > structure to be used to pass label data up and down the call chain. This patch
> > adds the new structure and new members to the required NFSv4 call structures.
> > 
> > Signed-off-by: Matthew N. Dodd <Matthew.Dodd@sparta.com>
> > Signed-off-by: Miguel Rodel Felipe <Rodel_FM@dsi.a-star.edu.sg>
> > Signed-off-by: Phua Eu Gene <PHUA_Eu_Gene@dsi.a-star.edu.sg>
> > Signed-off-by: Khin Mi Mi Aung <Mi_Mi_AUNG@dsi.a-star.edu.sg>
> > ---
> >  fs/nfs/inode.c            | 33 +++++++++++++++++++++++++++++++++
> >  include/linux/nfs4.h      |  7 +++++++
> >  include/linux/nfs_fs.h    | 17 +++++++++++++++++
> >  include/linux/nfs_xdr.h   | 21 +++++++++++++++++++++
> >  include/uapi/linux/nfs4.h |  2 +-
> >  5 files changed, 79 insertions(+), 1 deletion(-)
> > 
> > diff --git a/fs/nfs/inode.c b/fs/nfs/inode.c
> > index ebeb94c..8d5f01b 100644
> > --- a/fs/nfs/inode.c
> > +++ b/fs/nfs/inode.c
> > @@ -255,6 +255,39 @@ nfs_init_locked(struct inode *inode, void *opaque)
> >  	return 0;
> >  }
> >  
> > +#ifdef CONFIG_NFS_V4_SECURITY_LABEL
> > +struct nfs4_label *nfs4_label_alloc(struct nfs_server *server, gfp_t flags)
> > +{
> > +	struct nfs4_label *label = NULL;
> > +
> > +	if (!(server->caps & NFS_CAP_SECURITY_LABEL))
> > +		return label;
> > +
> > +	label = kzalloc(NFS4_MAXLABELLEN, flags);
> > +	if (label == NULL)
> > +		return ERR_PTR(-ENOMEM);
> > +
> > +	label->label = (char *)(label + 1);
> > +	label->len = NFS4_MAXLABELLEN;
> 
> If you're expecting to be able to store up to NFS4_MAXLABELLEN of data
> after the end of the struct, then you want:
> 
> 	label = kzalloc(sizeof(struct nfs4_label) + NFS4_MAXLABELLEN, flags);

Sigh... No.

I keep telling Steve that the 'label' needs to be defined as an array,
not a pointer. Having it be a pointer gives rise to a completely
unnecessary extra indirection (the CPU first needs to look up the
content of 'label->label', then dereference that pointer)...

IOW: we should just be defining struct nfs4_label as

struct nfs4_label {

	.... lots of definitions ...

	char label[NFS4_MAXLABELLEN];
};

in which case the above allocation becomes a trivial

	label = kzalloc(sizeof(*label), flags);



> > +
> > +	return label;
> > +}
> > +EXPORT_SYMBOL_GPL(nfs4_label_alloc);
> > +
> > +void nfs4_label_init(struct nfs4_label *label)
> > +{
> > +	if (label && label->label) {
> > +		/* 0 is the null format meaning that the data is not
> > +		to be translated */
> > +		label->lfs = 0;
> > +		label->pi = 0;
> > +		label->len = NFS4_MAXLABELLEN;
> 
> This logic seems a little odd to me; why would we pass NULL to this?
> Why would we skip the other initialization in the !label->label case?
> Why is NFS4_MAXLABELLEN the default?  (And wasn't that just set in
> nfs4_label_alloc?)
> 
> But OK maybe I need to read the rest of the patches.
> 
> > +	}
> > +	return;
> > +}
> > +EXPORT_SYMBOL_GPL(nfs4_label_init);
> > +#endif
> 
> > diff --git a/include/uapi/linux/nfs4.h b/include/uapi/linux/nfs4.h
> > index 788128e..b8014a2 100644
> > --- a/include/uapi/linux/nfs4.h
> > +++ b/include/uapi/linux/nfs4.h
> > @@ -25,7 +25,7 @@
> >  #define NFS4_MAXNAMLEN		NAME_MAX
> >  #define NFS4_OPAQUE_LIMIT	1024
> >  #define NFS4_MAX_SESSIONID_LEN	16
> > -
> > +#define NFS4_MAXLABELLEN (4095 - offsetof(struct nfs4_label , label))
> 
> The way you've defined things, that should be just
> 
> 			    (4095 - sizeof(struct nfs4_label))

No. See above. If the 'label' field is an array, then the offsetof() is
100% correct.

> (Is there typically some more alloc overhead that would push this
> allocation over 4k?  I don't know well enough how the allocator(s) work
> to answer that.)
> 
> --b.
> 
> >  #define NFS4_ACCESS_READ        0x0001
> >  #define NFS4_ACCESS_LOOKUP      0x0002
> >  #define NFS4_ACCESS_MODIFY      0x0004

--
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
J. Bruce Fields Feb. 12, 2013, 10:32 p.m. UTC | #3
On Tue, Feb 12, 2013 at 10:28:16PM +0000, Myklebust, Trond wrote:
> On Tue, 2013-02-12 at 17:07 -0500, J. Bruce Fields wrote:
> > On Fri, Feb 08, 2013 at 07:39:14AM -0500, Steve Dickson wrote:
> > > From: David Quigley <dpquigl@davequigley.com>
> > > 
> > > In order to mimic the way that NFSv4 ACLs are implemented we have created a
> > > structure to be used to pass label data up and down the call chain. This patch
> > > adds the new structure and new members to the required NFSv4 call structures.
> > > 
> > > Signed-off-by: Matthew N. Dodd <Matthew.Dodd@sparta.com>
> > > Signed-off-by: Miguel Rodel Felipe <Rodel_FM@dsi.a-star.edu.sg>
> > > Signed-off-by: Phua Eu Gene <PHUA_Eu_Gene@dsi.a-star.edu.sg>
> > > Signed-off-by: Khin Mi Mi Aung <Mi_Mi_AUNG@dsi.a-star.edu.sg>
> > > ---
> > >  fs/nfs/inode.c            | 33 +++++++++++++++++++++++++++++++++
> > >  include/linux/nfs4.h      |  7 +++++++
> > >  include/linux/nfs_fs.h    | 17 +++++++++++++++++
> > >  include/linux/nfs_xdr.h   | 21 +++++++++++++++++++++
> > >  include/uapi/linux/nfs4.h |  2 +-
> > >  5 files changed, 79 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/fs/nfs/inode.c b/fs/nfs/inode.c
> > > index ebeb94c..8d5f01b 100644
> > > --- a/fs/nfs/inode.c
> > > +++ b/fs/nfs/inode.c
> > > @@ -255,6 +255,39 @@ nfs_init_locked(struct inode *inode, void *opaque)
> > >  	return 0;
> > >  }
> > >  
> > > +#ifdef CONFIG_NFS_V4_SECURITY_LABEL
> > > +struct nfs4_label *nfs4_label_alloc(struct nfs_server *server, gfp_t flags)
> > > +{
> > > +	struct nfs4_label *label = NULL;
> > > +
> > > +	if (!(server->caps & NFS_CAP_SECURITY_LABEL))
> > > +		return label;
> > > +
> > > +	label = kzalloc(NFS4_MAXLABELLEN, flags);
> > > +	if (label == NULL)
> > > +		return ERR_PTR(-ENOMEM);
> > > +
> > > +	label->label = (char *)(label + 1);
> > > +	label->len = NFS4_MAXLABELLEN;
> > 
> > If you're expecting to be able to store up to NFS4_MAXLABELLEN of data
> > after the end of the struct, then you want:
> > 
> > 	label = kzalloc(sizeof(struct nfs4_label) + NFS4_MAXLABELLEN, flags);
> 
> Sigh... No.
> 
> I keep telling Steve that the 'label' needs to be defined as an array,

Yeah, I know, he said in 0/15 that he couldn't do that, so I've been
reading through these on the assumption I'll find out why not at some
point....  (Still not seeing it, though.)

--b.

> not a pointer. Having it be a pointer gives rise to a completely
> unnecessary extra indirection (the CPU first needs to look up the
> content of 'label->label', then dereference that pointer)...
> 
> IOW: we should just be defining struct nfs4_label as
> 
> struct nfs4_label {
> 
> 	.... lots of definitions ...
> 
> 	char label[NFS4_MAXLABELLEN];
> };
> 
> in which case the above allocation becomes a trivial
> 
> 	label = kzalloc(sizeof(*label), flags);
> 
> 
> 
> > > +
> > > +	return label;
> > > +}
> > > +EXPORT_SYMBOL_GPL(nfs4_label_alloc);
> > > +
> > > +void nfs4_label_init(struct nfs4_label *label)
> > > +{
> > > +	if (label && label->label) {
> > > +		/* 0 is the null format meaning that the data is not
> > > +		to be translated */
> > > +		label->lfs = 0;
> > > +		label->pi = 0;
> > > +		label->len = NFS4_MAXLABELLEN;
> > 
> > This logic seems a little odd to me; why would we pass NULL to this?
> > Why would we skip the other initialization in the !label->label case?
> > Why is NFS4_MAXLABELLEN the default?  (And wasn't that just set in
> > nfs4_label_alloc?)
> > 
> > But OK maybe I need to read the rest of the patches.
> > 
> > > +	}
> > > +	return;
> > > +}
> > > +EXPORT_SYMBOL_GPL(nfs4_label_init);
> > > +#endif
> > 
> > > diff --git a/include/uapi/linux/nfs4.h b/include/uapi/linux/nfs4.h
> > > index 788128e..b8014a2 100644
> > > --- a/include/uapi/linux/nfs4.h
> > > +++ b/include/uapi/linux/nfs4.h
> > > @@ -25,7 +25,7 @@
> > >  #define NFS4_MAXNAMLEN		NAME_MAX
> > >  #define NFS4_OPAQUE_LIMIT	1024
> > >  #define NFS4_MAX_SESSIONID_LEN	16
> > > -
> > > +#define NFS4_MAXLABELLEN (4095 - offsetof(struct nfs4_label , label))
> > 
> > The way you've defined things, that should be just
> > 
> > 			    (4095 - sizeof(struct nfs4_label))
> 
> No. See above. If the 'label' field is an array, then the offsetof() is
> 100% correct.
> 
> > (Is there typically some more alloc overhead that would push this
> > allocation over 4k?  I don't know well enough how the allocator(s) work
> > to answer that.)
> > 
> > --b.
> > 
> > >  #define NFS4_ACCESS_READ        0x0001
> > >  #define NFS4_ACCESS_LOOKUP      0x0002
> > >  #define NFS4_ACCESS_MODIFY      0x0004
> 
> --
> 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
--
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
Trond Myklebust Feb. 12, 2013, 10:40 p.m. UTC | #4
On Tue, 2013-02-12 at 17:32 -0500, J. Bruce Fields wrote:
> On Tue, Feb 12, 2013 at 10:28:16PM +0000, Myklebust, Trond wrote:
> > On Tue, 2013-02-12 at 17:07 -0500, J. Bruce Fields wrote:
> > > On Fri, Feb 08, 2013 at 07:39:14AM -0500, Steve Dickson wrote:
> > > > From: David Quigley <dpquigl@davequigley.com>
> > > > 
> > > > In order to mimic the way that NFSv4 ACLs are implemented we have created a
> > > > structure to be used to pass label data up and down the call chain. This patch
> > > > adds the new structure and new members to the required NFSv4 call structures.
> > > > 
> > > > Signed-off-by: Matthew N. Dodd <Matthew.Dodd@sparta.com>
> > > > Signed-off-by: Miguel Rodel Felipe <Rodel_FM@dsi.a-star.edu.sg>
> > > > Signed-off-by: Phua Eu Gene <PHUA_Eu_Gene@dsi.a-star.edu.sg>
> > > > Signed-off-by: Khin Mi Mi Aung <Mi_Mi_AUNG@dsi.a-star.edu.sg>
> > > > ---
> > > >  fs/nfs/inode.c            | 33 +++++++++++++++++++++++++++++++++
> > > >  include/linux/nfs4.h      |  7 +++++++
> > > >  include/linux/nfs_fs.h    | 17 +++++++++++++++++
> > > >  include/linux/nfs_xdr.h   | 21 +++++++++++++++++++++
> > > >  include/uapi/linux/nfs4.h |  2 +-
> > > >  5 files changed, 79 insertions(+), 1 deletion(-)
> > > > 
> > > > diff --git a/fs/nfs/inode.c b/fs/nfs/inode.c
> > > > index ebeb94c..8d5f01b 100644
> > > > --- a/fs/nfs/inode.c
> > > > +++ b/fs/nfs/inode.c
> > > > @@ -255,6 +255,39 @@ nfs_init_locked(struct inode *inode, void *opaque)
> > > >  	return 0;
> > > >  }
> > > >  
> > > > +#ifdef CONFIG_NFS_V4_SECURITY_LABEL
> > > > +struct nfs4_label *nfs4_label_alloc(struct nfs_server *server, gfp_t flags)
> > > > +{
> > > > +	struct nfs4_label *label = NULL;
> > > > +
> > > > +	if (!(server->caps & NFS_CAP_SECURITY_LABEL))
> > > > +		return label;
> > > > +
> > > > +	label = kzalloc(NFS4_MAXLABELLEN, flags);
> > > > +	if (label == NULL)
> > > > +		return ERR_PTR(-ENOMEM);
> > > > +
> > > > +	label->label = (char *)(label + 1);
> > > > +	label->len = NFS4_MAXLABELLEN;
> > > 
> > > If you're expecting to be able to store up to NFS4_MAXLABELLEN of data
> > > after the end of the struct, then you want:
> > > 
> > > 	label = kzalloc(sizeof(struct nfs4_label) + NFS4_MAXLABELLEN, flags);
> > 
> > Sigh... No.
> > 
> > I keep telling Steve that the 'label' needs to be defined as an array,
> 
> Yeah, I know, he said in 0/15 that he couldn't do that, so I've been
> reading through these on the assumption I'll find out why not at some
> point....  (Still not seeing it, though.)

It only makes sense to define label->label as a pointer if you need to
change the pointer value at some time. However, if that is the case,
then why allocate the struct nfs4_label and label string as a single
memory block? It would make more sense to allocate them separately so
that you can free the old label storage after you change the pointer.

Trond
--
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
J. Bruce Fields Feb. 12, 2013, 11:06 p.m. UTC | #5
On Tue, Feb 12, 2013 at 10:40:46PM +0000, Myklebust, Trond wrote:
> On Tue, 2013-02-12 at 17:32 -0500, J. Bruce Fields wrote:
> > On Tue, Feb 12, 2013 at 10:28:16PM +0000, Myklebust, Trond wrote:
> > > On Tue, 2013-02-12 at 17:07 -0500, J. Bruce Fields wrote:
> > > > On Fri, Feb 08, 2013 at 07:39:14AM -0500, Steve Dickson wrote:
> > > > > From: David Quigley <dpquigl@davequigley.com>
> > > > > 
> > > > > In order to mimic the way that NFSv4 ACLs are implemented we have created a
> > > > > structure to be used to pass label data up and down the call chain. This patch
> > > > > adds the new structure and new members to the required NFSv4 call structures.
> > > > > 
> > > > > Signed-off-by: Matthew N. Dodd <Matthew.Dodd@sparta.com>
> > > > > Signed-off-by: Miguel Rodel Felipe <Rodel_FM@dsi.a-star.edu.sg>
> > > > > Signed-off-by: Phua Eu Gene <PHUA_Eu_Gene@dsi.a-star.edu.sg>
> > > > > Signed-off-by: Khin Mi Mi Aung <Mi_Mi_AUNG@dsi.a-star.edu.sg>
> > > > > ---
> > > > >  fs/nfs/inode.c            | 33 +++++++++++++++++++++++++++++++++
> > > > >  include/linux/nfs4.h      |  7 +++++++
> > > > >  include/linux/nfs_fs.h    | 17 +++++++++++++++++
> > > > >  include/linux/nfs_xdr.h   | 21 +++++++++++++++++++++
> > > > >  include/uapi/linux/nfs4.h |  2 +-
> > > > >  5 files changed, 79 insertions(+), 1 deletion(-)
> > > > > 
> > > > > diff --git a/fs/nfs/inode.c b/fs/nfs/inode.c
> > > > > index ebeb94c..8d5f01b 100644
> > > > > --- a/fs/nfs/inode.c
> > > > > +++ b/fs/nfs/inode.c
> > > > > @@ -255,6 +255,39 @@ nfs_init_locked(struct inode *inode, void *opaque)
> > > > >  	return 0;
> > > > >  }
> > > > >  
> > > > > +#ifdef CONFIG_NFS_V4_SECURITY_LABEL
> > > > > +struct nfs4_label *nfs4_label_alloc(struct nfs_server *server, gfp_t flags)
> > > > > +{
> > > > > +	struct nfs4_label *label = NULL;
> > > > > +
> > > > > +	if (!(server->caps & NFS_CAP_SECURITY_LABEL))
> > > > > +		return label;
> > > > > +
> > > > > +	label = kzalloc(NFS4_MAXLABELLEN, flags);
> > > > > +	if (label == NULL)
> > > > > +		return ERR_PTR(-ENOMEM);
> > > > > +
> > > > > +	label->label = (char *)(label + 1);
> > > > > +	label->len = NFS4_MAXLABELLEN;
> > > > 
> > > > If you're expecting to be able to store up to NFS4_MAXLABELLEN of data
> > > > after the end of the struct, then you want:
> > > > 
> > > > 	label = kzalloc(sizeof(struct nfs4_label) + NFS4_MAXLABELLEN, flags);
> > > 
> > > Sigh... No.
> > > 
> > > I keep telling Steve that the 'label' needs to be defined as an array,
> > 
> > Yeah, I know, he said in 0/15 that he couldn't do that, so I've been
> > reading through these on the assumption I'll find out why not at some
> > point....  (Still not seeing it, though.)
> 
> It only makes sense to define label->label as a pointer if you need to
> change the pointer value at some time. However, if that is the case,
> then why allocate the struct nfs4_label and label string as a single
> memory block? It would make more sense to allocate them separately so
> that you can free the old label storage after you change the pointer.

nfs4_get_security_label() in a later patch is indeed using some
passed-in storage instead, but I'm not sure it's doing it correctly.

It would seem simpler to pick either inline or separate storage and
stick with it throughout.

I'd be inclined to favor inline even if it means larger allocations than
necessary or an extra copy.  Just because it seems harder to mess up.

--b.
--
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
Steve Dickson Feb. 13, 2013, 12:30 a.m. UTC | #6
On 12/02/13 18:06, J. Bruce Fields wrote:
> On Tue, Feb 12, 2013 at 10:40:46PM +0000, Myklebust, Trond wrote:
>> On Tue, 2013-02-12 at 17:32 -0500, J. Bruce Fields wrote:
>>> On Tue, Feb 12, 2013 at 10:28:16PM +0000, Myklebust, Trond wrote:
>>>> On Tue, 2013-02-12 at 17:07 -0500, J. Bruce Fields wrote:
>>>>> On Fri, Feb 08, 2013 at 07:39:14AM -0500, Steve Dickson wrote:
>>>>>> From: David Quigley <dpquigl@davequigley.com>
>>>>>>
>>>>>> In order to mimic the way that NFSv4 ACLs are implemented we have created a
>>>>>> structure to be used to pass label data up and down the call chain. This patch
>>>>>> adds the new structure and new members to the required NFSv4 call structures.
>>>>>>
>>>>>> Signed-off-by: Matthew N. Dodd <Matthew.Dodd@sparta.com>
>>>>>> Signed-off-by: Miguel Rodel Felipe <Rodel_FM@dsi.a-star.edu.sg>
>>>>>> Signed-off-by: Phua Eu Gene <PHUA_Eu_Gene@dsi.a-star.edu.sg>
>>>>>> Signed-off-by: Khin Mi Mi Aung <Mi_Mi_AUNG@dsi.a-star.edu.sg>
>>>>>> ---
>>>>>>  fs/nfs/inode.c            | 33 +++++++++++++++++++++++++++++++++
>>>>>>  include/linux/nfs4.h      |  7 +++++++
>>>>>>  include/linux/nfs_fs.h    | 17 +++++++++++++++++
>>>>>>  include/linux/nfs_xdr.h   | 21 +++++++++++++++++++++
>>>>>>  include/uapi/linux/nfs4.h |  2 +-
>>>>>>  5 files changed, 79 insertions(+), 1 deletion(-)
>>>>>>
>>>>>> diff --git a/fs/nfs/inode.c b/fs/nfs/inode.c
>>>>>> index ebeb94c..8d5f01b 100644
>>>>>> --- a/fs/nfs/inode.c
>>>>>> +++ b/fs/nfs/inode.c
>>>>>> @@ -255,6 +255,39 @@ nfs_init_locked(struct inode *inode, void *opaque)
>>>>>>  	return 0;
>>>>>>  }
>>>>>>  
>>>>>> +#ifdef CONFIG_NFS_V4_SECURITY_LABEL
>>>>>> +struct nfs4_label *nfs4_label_alloc(struct nfs_server *server, gfp_t flags)
>>>>>> +{
>>>>>> +	struct nfs4_label *label = NULL;
>>>>>> +
>>>>>> +	if (!(server->caps & NFS_CAP_SECURITY_LABEL))
>>>>>> +		return label;
>>>>>> +
>>>>>> +	label = kzalloc(NFS4_MAXLABELLEN, flags);
>>>>>> +	if (label == NULL)
>>>>>> +		return ERR_PTR(-ENOMEM);
>>>>>> +
>>>>>> +	label->label = (char *)(label + 1);
>>>>>> +	label->len = NFS4_MAXLABELLEN;
>>>>>
>>>>> If you're expecting to be able to store up to NFS4_MAXLABELLEN of data
>>>>> after the end of the struct, then you want:
>>>>>
>>>>> 	label = kzalloc(sizeof(struct nfs4_label) + NFS4_MAXLABELLEN, flags);
>>>>
>>>> Sigh... No.
>>>>
>>>> I keep telling Steve that the 'label' needs to be defined as an array,
>>>
>>> Yeah, I know, he said in 0/15 that he couldn't do that, so I've been
>>> reading through these on the assumption I'll find out why not at some
>>> point....  (Still not seeing it, though.)
>>
>> It only makes sense to define label->label as a pointer if you need to
>> change the pointer value at some time. However, if that is the case,
>> then why allocate the struct nfs4_label and label string as a single
>> memory block? It would make more sense to allocate them separately so
>> that you can free the old label storage after you change the pointer.
> 
> nfs4_get_security_label() in a later patch is indeed using some
> passed-in storage instead, but I'm not sure it's doing it correctly.
Exactly, this is the interface where I'm thinking it makes sense to
keep label->label a pointer.

steved.

> 
> It would seem simpler to pick either inline or separate storage and
> stick with it throughout.
> 
> I'd be inclined to favor inline even if it means larger allocations than
> necessary or an extra copy.  Just because it seems harder to mess up.
> 
> --b.
> 
--
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
diff mbox

Patch

diff --git a/fs/nfs/inode.c b/fs/nfs/inode.c
index ebeb94c..8d5f01b 100644
--- a/fs/nfs/inode.c
+++ b/fs/nfs/inode.c
@@ -255,6 +255,39 @@  nfs_init_locked(struct inode *inode, void *opaque)
 	return 0;
 }
 
+#ifdef CONFIG_NFS_V4_SECURITY_LABEL
+struct nfs4_label *nfs4_label_alloc(struct nfs_server *server, gfp_t flags)
+{
+	struct nfs4_label *label = NULL;
+
+	if (!(server->caps & NFS_CAP_SECURITY_LABEL))
+		return label;
+
+	label = kzalloc(NFS4_MAXLABELLEN, flags);
+	if (label == NULL)
+		return ERR_PTR(-ENOMEM);
+
+	label->label = (char *)(label + 1);
+	label->len = NFS4_MAXLABELLEN;
+
+	return label;
+}
+EXPORT_SYMBOL_GPL(nfs4_label_alloc);
+
+void nfs4_label_init(struct nfs4_label *label)
+{
+	if (label && label->label) {
+		/* 0 is the null format meaning that the data is not
+		to be translated */
+		label->lfs = 0;
+		label->pi = 0;
+		label->len = NFS4_MAXLABELLEN;
+	}
+	return;
+}
+EXPORT_SYMBOL_GPL(nfs4_label_init);
+#endif
+
 /*
  * This is our front-end to iget that looks up inodes by file handle
  * instead of inode number.
diff --git a/include/linux/nfs4.h b/include/linux/nfs4.h
index f9235b4..aab8bd8 100644
--- a/include/linux/nfs4.h
+++ b/include/linux/nfs4.h
@@ -28,6 +28,13 @@  struct nfs4_acl {
 	struct nfs4_ace	aces[0];
 };
 
+struct nfs4_label {
+	uint32_t	lfs;
+	uint32_t	pi;
+	u32		len;
+	char	*label;
+};
+
 typedef struct { char data[NFS4_VERIFIER_SIZE]; } nfs4_verifier;
 
 struct nfs_stateid4 {
diff --git a/include/linux/nfs_fs.h b/include/linux/nfs_fs.h
index 1cc2568..1c55373 100644
--- a/include/linux/nfs_fs.h
+++ b/include/linux/nfs_fs.h
@@ -489,6 +489,23 @@  extern int nfs_mountpoint_expiry_timeout;
 extern void nfs_release_automount_timer(void);
 
 /*
+ * linux/fs/nfs/nfs4proc.c
+ */
+#ifdef CONFIG_NFS_V4_SECURITY_LABEL
+extern struct nfs4_label *nfs4_label_alloc(struct nfs_server *server, gfp_t flags);
+extern void nfs4_label_init(struct nfs4_label *label);
+static inline void nfs4_label_free(struct nfs4_label *label)
+{
+	kfree(label);
+	return;
+}
+#else
+static inline struct nfs4_label *nfs4_label_alloc(struct nfs_server *server, gfp_t flags) { return NULL; }
+static inline void nfs4_label_init(void *label) {}
+static inline void nfs4_label_free(void *label) {}
+#endif
+
+/*
  * linux/fs/nfs/unlink.c
  */
 extern void nfs_complete_unlink(struct dentry *dentry, struct inode *);
diff --git a/include/linux/nfs_xdr.h b/include/linux/nfs_xdr.h
index ac9ba69..7e86290 100644
--- a/include/linux/nfs_xdr.h
+++ b/include/linux/nfs_xdr.h
@@ -350,6 +350,7 @@  struct nfs_openargs {
 	const u32 *		bitmask;
 	const u32 *		open_bitmap;
 	__u32			claim;
+	const struct nfs4_label *label;
 };
 
 struct nfs_openres {
@@ -359,6 +360,7 @@  struct nfs_openres {
 	struct nfs4_change_info	cinfo;
 	__u32                   rflags;
 	struct nfs_fattr *      f_attr;
+	struct nfs4_label	*f_label;
 	struct nfs_seqid *	seqid;
 	const struct nfs_server *server;
 	fmode_t			delegation_type;
@@ -403,6 +405,7 @@  struct nfs_closeres {
 	struct nfs4_sequence_res	seq_res;
 	nfs4_stateid            stateid;
 	struct nfs_fattr *	fattr;
+	struct nfs4_label	*label;
 	struct nfs_seqid *	seqid;
 	const struct nfs_server *server;
 };
@@ -476,6 +479,7 @@  struct nfs4_delegreturnargs {
 struct nfs4_delegreturnres {
 	struct nfs4_sequence_res	seq_res;
 	struct nfs_fattr * fattr;
+	struct nfs4_label	*label;
 	const struct nfs_server *server;
 };
 
@@ -496,6 +500,7 @@  struct nfs_readargs {
 struct nfs_readres {
 	struct nfs4_sequence_res	seq_res;
 	struct nfs_fattr *	fattr;
+	struct nfs4_label	*label;
 	__u32			count;
 	int                     eof;
 };
@@ -564,6 +569,7 @@  struct nfs_removeres {
 	struct nfs4_sequence_res 	seq_res;
 	const struct nfs_server *server;
 	struct nfs_fattr	*dir_attr;
+	struct nfs4_label	*dir_label;
 	struct nfs4_change_info	cinfo;
 };
 
@@ -576,6 +582,8 @@  struct nfs_renameargs {
 	const struct nfs_fh		*new_dir;
 	const struct qstr		*old_name;
 	const struct qstr		*new_name;
+	const struct nfs4_label		*old_label;
+	const struct nfs4_label		*new_label;
 };
 
 struct nfs_renameres {
@@ -583,8 +591,10 @@  struct nfs_renameres {
 	const struct nfs_server		*server;
 	struct nfs4_change_info		old_cinfo;
 	struct nfs_fattr		*old_fattr;
+	struct nfs4_label		*old_label;
 	struct nfs4_change_info		new_cinfo;
 	struct nfs_fattr		*new_fattr;
+	struct nfs4_label		*new_label;
 };
 
 /*
@@ -599,6 +609,7 @@  struct nfs_entry {
 	int			eof;
 	struct nfs_fh *		fh;
 	struct nfs_fattr *	fattr;
+	struct nfs4_label  *label;
 	unsigned char		d_type;
 	struct nfs_server *	server;
 };
@@ -631,6 +642,7 @@  struct nfs_setattrargs {
 	struct iattr *                  iap;
 	const struct nfs_server *	server; /* Needed for name mapping */
 	const u32 *			bitmask;
+	const struct nfs4_label		*label;
 };
 
 struct nfs_setaclargs {
@@ -666,6 +678,7 @@  struct nfs_getaclres {
 struct nfs_setattrres {
 	struct nfs4_sequence_res	seq_res;
 	struct nfs_fattr *              fattr;
+	struct nfs4_label		*label;
 	const struct nfs_server *	server;
 };
 
@@ -711,6 +724,7 @@  struct nfs3_setaclargs {
 struct nfs_diropok {
 	struct nfs_fh *		fh;
 	struct nfs_fattr *	fattr;
+	struct nfs4_label	*label;
 };
 
 struct nfs_readlinkargs {
@@ -841,6 +855,7 @@  struct nfs4_accessres {
 	struct nfs4_sequence_res	seq_res;
 	const struct nfs_server *	server;
 	struct nfs_fattr *		fattr;
+	struct nfs4_label		*label;
 	u32				supported;
 	u32				access;
 };
@@ -863,6 +878,7 @@  struct nfs4_create_arg {
 	const struct iattr *		attrs;
 	const struct nfs_fh *		dir_fh;
 	const u32 *			bitmask;
+	const struct nfs4_label		*label;
 };
 
 struct nfs4_create_res {
@@ -870,6 +886,7 @@  struct nfs4_create_res {
 	const struct nfs_server *	server;
 	struct nfs_fh *			fh;
 	struct nfs_fattr *		fattr;
+	struct nfs4_label		*label;
 	struct nfs4_change_info		dir_cinfo;
 };
 
@@ -894,6 +911,7 @@  struct nfs4_getattr_res {
 	struct nfs4_sequence_res	seq_res;
 	const struct nfs_server *	server;
 	struct nfs_fattr *		fattr;
+	struct nfs4_label		*label;
 };
 
 struct nfs4_link_arg {
@@ -908,8 +926,10 @@  struct nfs4_link_res {
 	struct nfs4_sequence_res	seq_res;
 	const struct nfs_server *	server;
 	struct nfs_fattr *		fattr;
+	struct nfs4_label		*label;
 	struct nfs4_change_info		cinfo;
 	struct nfs_fattr *		dir_attr;
+	struct nfs4_label		*dir_label;
 };
 
 
@@ -925,6 +945,7 @@  struct nfs4_lookup_res {
 	const struct nfs_server *	server;
 	struct nfs_fattr *		fattr;
 	struct nfs_fh *			fh;
+	struct nfs4_label		*label;
 };
 
 struct nfs4_lookup_root_arg {
diff --git a/include/uapi/linux/nfs4.h b/include/uapi/linux/nfs4.h
index 788128e..b8014a2 100644
--- a/include/uapi/linux/nfs4.h
+++ b/include/uapi/linux/nfs4.h
@@ -25,7 +25,7 @@ 
 #define NFS4_MAXNAMLEN		NAME_MAX
 #define NFS4_OPAQUE_LIMIT	1024
 #define NFS4_MAX_SESSIONID_LEN	16
-
+#define NFS4_MAXLABELLEN (4095 - offsetof(struct nfs4_label , label))
 #define NFS4_ACCESS_READ        0x0001
 #define NFS4_ACCESS_LOOKUP      0x0002
 #define NFS4_ACCESS_MODIFY      0x0004