From: "Myklebust, Trond" <Trond.Myklebust-HgOvQuBEEgTQT0dZR+AlfA@public.gmane.org> To: "J. Bruce Fields" <bfields-uC3wQj2KruNg9hUCZPvPmw@public.gmane.org> Cc: Steve Dickson <SteveD-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>, "J. Bruce Fields" <bfields-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>, "David P. Quigley" <dpquigl-+05T5uksL2qpZYMLLGbcSA@public.gmane.org>, Linux NFS list <linux-nfs-u79uwXL29TY76Z2rM5mHXA@public.gmane.org>, Linux FS devel list <linux-fsdevel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org>, Linux Security List <linux-security-module-u79uwXL29TY76Z2rM5mHXA@public.gmane.org>, SELinux List <selinux-+05T5uksL2qpZYMLLGbcSA@public.gmane.org> Subject: Re: [PATCH 06/15] NFSv4: Introduce new label structure Date: Tue, 12 Feb 2013 22:28:16 +0000 [thread overview] Message-ID: <4FA345DA4F4AE44899BD2B03EEEC2FA91F3CE55C@sacexcmbx05-prd.hq.netapp.com> (raw) In-Reply-To: <20130212220741.GJ10267-uC3wQj2KruNg9hUCZPvPmw@public.gmane.org> 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-a7DkhOHRHBuN9aS15agKxg@public.gmane.org> > > > > 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-DABiIiYg7OfQT0dZR+AlfA@public.gmane.org> > > Signed-off-by: Miguel Rodel Felipe <Rodel_FM-geVtEqcQUv4Eyxwt80+Gtti2O/JbrIOy@public.gmane.org> > > Signed-off-by: Phua Eu Gene <PHUA_Eu_Gene-geVtEqcQUv4Eyxwt80+Gtti2O/JbrIOy@public.gmane.org> > > Signed-off-by: Khin Mi Mi Aung <Mi_Mi_AUNG-geVtEqcQUv4Eyxwt80+Gtti2O/JbrIOy@public.gmane.org> > > --- > > 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-u79uwXL29TY76Z2rM5mHXA@public.gmane.org More majordomo info at http://vger.kernel.org/majordomo-info.html
WARNING: multiple messages have this Message-ID (diff)
From: "Myklebust, Trond" <Trond.Myklebust@netapp.com> To: "J. Bruce Fields" <bfields@fieldses.org> Cc: Steve Dickson <SteveD@redhat.com>, "J. Bruce Fields" <bfields@redhat.com>, "David P. Quigley" <dpquigl@tycho.nsa.gov>, Linux NFS list <linux-nfs@vger.kernel.org>, Linux FS devel list <linux-fsdevel@vger.kernel.org>, Linux Security List <linux-security-module@vger.kernel.org>, SELinux List <selinux@tycho.nsa.gov> Subject: Re: [PATCH 06/15] NFSv4: Introduce new label structure Date: Tue, 12 Feb 2013 22:28:16 +0000 [thread overview] Message-ID: <4FA345DA4F4AE44899BD2B03EEEC2FA91F3CE55C@sacexcmbx05-prd.hq.netapp.com> (raw) In-Reply-To: <20130212220741.GJ10267@fieldses.org> 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
next prev parent reply other threads:[~2013-02-12 22:28 UTC|newest] Thread overview: 58+ messages / expand[flat|nested] mbox.gz Atom feed top 2013-02-08 12:39 [PATCH 00/15] lnfs: 3.8-rc6 release Steve Dickson 2013-02-08 12:39 ` Steve Dickson 2013-02-08 12:39 ` [PATCH 01/15] Security: Add hook to calculate context based on a negative dentry Steve Dickson 2013-02-08 12:39 ` [PATCH 02/15] Security: Add Hook to test if the particular xattr is part of a MAC model Steve Dickson 2013-02-08 12:39 ` [PATCH 03/15] LSM: Add flags field to security_sb_set_mnt_opts for in kernel mount data Steve Dickson 2013-02-08 12:39 ` [PATCH 04/15] SELinux: Add new labeling type native labels Steve Dickson 2013-02-08 12:39 ` [PATCH 05/15] NFSv4: Add label recommended attribute and NFSv4 flags Steve Dickson 2013-02-08 12:39 ` [PATCH 06/15] NFSv4: Introduce new label structure Steve Dickson 2013-02-12 22:07 ` J. Bruce Fields [not found] ` <20130212220741.GJ10267-uC3wQj2KruNg9hUCZPvPmw@public.gmane.org> 2013-02-12 22:28 ` Myklebust, Trond [this message] 2013-02-12 22:28 ` Myklebust, Trond 2013-02-12 22:32 ` J. Bruce Fields 2013-02-12 22:40 ` Myklebust, Trond 2013-02-12 23:06 ` J. Bruce Fields 2013-02-13 0:30 ` Steve Dickson 2013-02-08 12:39 ` [PATCH 07/15] NFSv4: Extend fattr bitmaps to support all 3 words Steve Dickson 2013-02-08 12:39 ` [PATCH 08/15] NFS:Add labels to client function prototypes Steve Dickson 2013-02-08 12:39 ` [PATCH 09/15] NFS: Add label lifecycle management Steve Dickson 2013-02-12 22:27 ` J. Bruce Fields 2013-02-16 20:28 ` Steve Dickson [not found] ` <1360327163-20360-1-git-send-email-SteveD-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> 2013-02-08 12:39 ` [PATCH 10/15] NFS: Client implementation of Labeled-NFS Steve Dickson 2013-02-08 12:39 ` Steve Dickson 2013-02-12 23:03 ` J. Bruce Fields 2013-02-16 20:35 ` Steve Dickson [not found] ` <511FED8E.7020308-AfCzQyP5zfLQT0dZR+AlfA@public.gmane.org> 2013-02-16 22:30 ` J. Bruce Fields 2013-02-16 22:30 ` J. Bruce Fields 2013-02-17 1:24 ` Steve Dickson 2013-02-17 1:47 ` Steve Dickson 2013-02-08 12:39 ` [PATCH 11/15] NFS: Extend NFS xattr handlers to accept the security namespace Steve Dickson 2013-02-08 12:39 ` [PATCH 12/15] lnfs: Do not sleep holding the inode spin lock Steve Dickson [not found] ` <1360327163-20360-13-git-send-email-SteveD-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> 2013-02-13 15:16 ` J. Bruce Fields 2013-02-13 15:16 ` J. Bruce Fields [not found] ` <20130213151610.GI14195-uC3wQj2KruNg9hUCZPvPmw@public.gmane.org> 2013-02-16 20:36 ` Steve Dickson 2013-02-16 20:36 ` Steve Dickson 2013-02-08 12:39 ` [PATCH 13/15] Kconfig: Add Kconfig entry for Labeled NFS V4 client Steve Dickson 2013-02-08 12:39 ` [PATCH 14/15] NFSD: Server implementation of MAC Labeling Steve Dickson 2013-02-12 22:54 ` J. Bruce Fields 2013-02-12 23:07 ` J. Bruce Fields [not found] ` <20130212225425.GM10267-uC3wQj2KruNg9hUCZPvPmw@public.gmane.org> 2013-02-16 20:44 ` Steve Dickson 2013-02-16 20:44 ` Steve Dickson [not found] ` <511FEFCB.2090002-AfCzQyP5zfLQT0dZR+AlfA@public.gmane.org> 2013-02-16 22:34 ` J. Bruce Fields 2013-02-16 22:34 ` J. Bruce Fields 2013-02-08 12:39 ` [PATCH 15/15] Kconfig: Add Kconfig entry for Labeled NFS V4 server Steve Dickson 2013-02-12 21:41 ` [PATCH 00/15] lnfs: 3.8-rc6 release J. Bruce Fields 2013-02-12 22:02 ` Casey Schaufler 2013-02-12 22:02 ` Casey Schaufler 2013-02-12 22:13 ` J. Bruce Fields 2013-02-13 0:32 ` Steve Dickson 2013-02-13 0:55 ` Casey Schaufler 2013-02-13 0:55 ` Casey Schaufler 2013-02-12 23:11 ` J. Bruce Fields [not found] ` <20130212231113.GQ10267-uC3wQj2KruNg9hUCZPvPmw@public.gmane.org> 2013-02-12 23:18 ` Myklebust, Trond 2013-02-12 23:18 ` Myklebust, Trond 2013-02-13 0:11 ` J. Bruce Fields 2013-02-13 0:21 ` J. Bruce Fields 2013-02-13 0:28 ` Steve Dickson 2013-02-13 15:05 ` J. Bruce Fields 2013-02-13 15:33 ` J. Bruce Fields
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=4FA345DA4F4AE44899BD2B03EEEC2FA91F3CE55C@sacexcmbx05-prd.hq.netapp.com \ --to=trond.myklebust-hgovqubeegtqt0dzr+alfa@public.gmane.org \ --cc=SteveD-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org \ --cc=bfields-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org \ --cc=bfields-uC3wQj2KruNg9hUCZPvPmw@public.gmane.org \ --cc=dpquigl-+05T5uksL2qpZYMLLGbcSA@public.gmane.org \ --cc=linux-fsdevel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org \ --cc=linux-nfs-u79uwXL29TY76Z2rM5mHXA@public.gmane.org \ --cc=linux-security-module-u79uwXL29TY76Z2rM5mHXA@public.gmane.org \ --cc=selinux-+05T5uksL2qpZYMLLGbcSA@public.gmane.org \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.