Re: [PATCH 07/10] NFSv4: Introduce new label structure

[David P. Quigley]

On Wed, 2010-07-07 at 09:21 -0700, Casey Schaufler wrote:
> Chuck Lever wrote:
> > My comments below apply to the other NFS client patches as well.  This
> > seemed like the right one to use for code examples.
> >
> > On 07/ 7/10 10:31 AM, David P. Quigley wrote:
> >> 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@xxxxxxxxxx>
> >> Signed-off-by: David P. Quigley<dpquigl@xxxxxxxxxxxxx>
> >> ---
> >>   fs/nfs/nfs4proc.c       |   26 ++++++++++++++++++++++++++
> >>   fs/nfsd/xdr4.h          |    3 +++
> >>   include/linux/nfs4.h    |    7 +++++++
> >>   include/linux/nfs_fs.h  |    7 +++++++
> >>   include/linux/nfs_xdr.h |   22 ++++++++++++++++++++++
> >>   5 files changed, 65 insertions(+), 0 deletions(-)
> >>
> >> diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
> >> index 071fced..71bb8da 100644
> >> --- a/fs/nfs/nfs4proc.c
> >> +++ b/fs/nfs/nfs4proc.c
> >> @@ -148,6 +148,32 @@ const u32 nfs4_fs_locations_bitmap[2] = {
> >>       | FATTR4_WORD1_MOUNTED_ON_FILEID
> >>   };
> >>
> >> +#ifdef CONFIG_NFS_V4_SECURITY_LABEL
> >> +
> >> +struct nfs4_label * nfs4_label_alloc (gfp_t flags)
> >
> > Style: have you run these patches through scripts/checkpatch.pl? 
> > Kernel coding style likes "struct foo *function(args)", without the
> > spaces.
> >
> >> +{
> >> +    struct nfs4_label *label = NULL;
> >> +
> >> +    label = kzalloc(sizeof(struct nfs4_label) + NFS4_MAXLABELLEN,
> >> flags);
> >> +    if (label == NULL)
> >> +        return NULL;
> >
> > Instead of NULL, you could return an ERR_PTR.  NULL could then be used
> > to signal that labels aren't supported.  See below.
> >
> >> +
> >> +    label->label = (void *)(label + 1);
> >> +    label->len = NFS4_MAXLABELLEN;
> >> +    /* 0 is the null format meaning that the data is not to be
> >> translated */
> >> +     label->lfs = 0;
> >> +    return label;
> >> +}
> >> +
> >> +void nfs4_label_free (struct nfs4_label *label)
> >> +{
> >> +    if (label)
> >> +        kfree(label);
> >
> > Style: kfree() already checks for a NULL pointer, so you shouldn't. 
> > At one point recently, all such checks were removed from the kernel in
> > favor of using the one already in kfree().
> >
> > Also, you check the server capabilities before calling this function,
> > nearly every time.  Is that really needed?  If there's a label data
> > structure, it should be freed whether the server supports it or not.
> >
> > That capabilities check is probably going to be more complex if you
> > want to have NFSv3 label support as well.  Would it make sense to
> > provide a function that can check for NFSv3 (eventually) or NFSv4
> > label support?
> >
> > Or, fold such checks into your allocator?  Hiding the capabilities
> > check here would allow easy expansion in the future to include other
> > NFS versions, and cause less clutter in all callers.
> >
> >> +    return;
> >> +}
> >> +
> >> +#endif
> >
> > Style: Generally speaking, we like to keep "#ifdef CONFIG_BAR" noise
> > down to a dull roar.  So, this is the right place to keep it, and the
> > generic functions (like nfs_lookup() and nfs_readdir()) is generally not.
> >
> > Usually we accomplish this by having functions like nfs4_label_free()
> > always be available, but if CONFIG_NFS_V4_SECURITY_LABEL isn't set,
> > the function call is a no-op.
> >
> >> +
> >>   static void nfs4_setup_readdir(u64 cookie, __be32 *verifier, struct
> >> dentry *dentry,
> >>           struct nfs4_readdir_arg *readdir)
> >>   {
> >> diff --git a/fs/nfsd/xdr4.h b/fs/nfsd/xdr4.h
> >> index efa3377..c217277 100644
> >> --- a/fs/nfsd/xdr4.h
> >> +++ b/fs/nfsd/xdr4.h
> >> @@ -108,6 +108,7 @@ struct nfsd4_create {
> >>       struct iattr    cr_iattr;           /* request */
> >>       struct nfsd4_change_info  cr_cinfo; /* response */
> >>       struct nfs4_acl *cr_acl;
> >> +    struct nfs4_label *cr_label;
> >>   };
> >>   #define cr_linklen    u.link.namelen
> >>   #define cr_linkname    u.link.name
> >> @@ -235,6 +236,7 @@ struct nfsd4_open {
> >>       int        op_truncate;        /* used during processing */
> >>       struct nfs4_stateowner *op_stateowner; /* used during
> >> processing */
> >>       struct nfs4_acl *op_acl;
> >> +    struct nfs4_label *op_label;
> >>   };
> >>   #define op_iattr    iattr
> >>   #define op_verf        verf
> >> @@ -316,6 +318,7 @@ struct nfsd4_setattr {
> >>       u32        sa_bmval[3];        /* request */
> >>       struct iattr    sa_iattr;           /* request */
> >>       struct nfs4_acl *sa_acl;
> >> +    struct nfs4_label *sa_label;
> >>   };
> >>
> >>   struct nfsd4_setclientid {
> >> diff --git a/include/linux/nfs4.h b/include/linux/nfs4.h
> >> index a2abd1a..c512a0c 100644
> >> --- a/include/linux/nfs4.h
> >> +++ b/include/linux/nfs4.h
> >> @@ -167,6 +167,13 @@ struct nfs4_acl {
> >>       struct nfs4_ace    aces[0];
> >>   };
> >>
> >> +struct nfs4_label {
> >> +    void        *label;
> >> +    u32        len;
> >> +    uint32_t    lfs;
> >> +};
> >
> > If we have support for NFS labels in NFSv3, would we also use struct
> > nfs4_label?
> >
> > It seems to me you want something more generic, just like nfs_fh or
> > nfs_fattr, to represent these.  Over time, I'm guessing label support
> > won't be tied to a specific NFS version.  And, you are passing these
> > around in the generic NFS functions (for post-op updates and inode
> > revalidation, and what not).
> >
> > Can I recommend "struct nfs_seclabel" instead?  Then have
> > nfs_seclabel_alloc() and nfs_seclabel_free().
> 
> Security has been the primary consumer of labels to date, but
> the xattr concept has always been envisioned as useful in many
> ways. That, and people have so many different views on what is
> and isn't security and whether it is good or evil that you are
> asking to limit the value if you tie "security" to the names.
> Plus, it adds unnecessary characters.

I agree that xattrs are useful in other ways but this NFSv4 attribute's
purpose is security labels. This is definitely not meant to be anything
like an xattr.

> 
> >
> > Does it make sense to deduplicate these in the client's memory?  It
> > seems to me that you could have hundreds or thousands that all contain
> > the same label information.
> 
> That would be easy enough to do. Look at smack_import() for a
> worked example.
> 

I'm not sure its worth it. These structures don't stay around for long.
Its purpose is just to get the info up the stack to a point where we can
put it in the inode proper. 

> >
> >> +
> >> +
> >>   typedef struct { char data[NFS4_VERIFIER_SIZE]; } nfs4_verifier;
> >>   typedef struct { char data[NFS4_STATEID_SIZE]; } nfs4_stateid;
> >>
> >> diff --git a/include/linux/nfs_fs.h b/include/linux/nfs_fs.h
> >> index 07ce460..2813b71 100644
> >> --- a/include/linux/nfs_fs.h
> >> +++ b/include/linux/nfs_fs.h
> >> @@ -454,6 +454,13 @@ extern int nfs_mountpoint_expiry_timeout;
> >>   extern void nfs_release_automount_timer(void);
> >>
> >>   /*
> >> + * linux/fs/nfs/nfs4proc.c
> >> + */
> >> +
> >> +struct nfs4_label *    nfs4_label_alloc (gfp_t flags);
> >> +void            nfs4_label_free (struct nfs4_label *);
> >> +
> >> +/*
> >>    * linux/fs/nfs/unlink.c
> >>    */
> >>   extern int  nfs_async_unlink(struct inode *dir, struct dentry
> >> *dentry);
> >> diff --git a/include/linux/nfs_xdr.h b/include/linux/nfs_xdr.h
> >> index 28cde54..dc505e4 100644
> >> --- a/include/linux/nfs_xdr.h
> >> +++ b/include/linux/nfs_xdr.h
> >> @@ -207,6 +207,7 @@ struct nfs_openargs {
> >>       const struct nfs_server *server;     /* Needed for ID mapping */
> >>       const u32 *        bitmask;
> >>       __u32            claim;
> >> +    const struct nfs4_label *label;
> >>       struct nfs4_sequence_args    seq_args;
> >>   };
> >>
> >> @@ -216,7 +217,9 @@ struct nfs_openres {
> >>       struct nfs4_change_info    cinfo;
> >>       __u32                   rflags;
> >>       struct nfs_fattr *      f_attr;
> >> +    struct nfs4_label *    f_label;
> >>       struct nfs_fattr *      dir_attr;
> >> +    struct nfs4_label *    dir_label;
> >>       struct nfs_seqid *    seqid;
> >>       const struct nfs_server *server;
> >>       fmode_t            delegation_type;
> >> @@ -256,6 +259,7 @@ struct nfs_closeargs {
> >>   struct nfs_closeres {
> >>       nfs4_stateid            stateid;
> >>       struct nfs_fattr *    fattr;
> >> +    struct nfs4_label *    label;
> >>       struct nfs_seqid *    seqid;
> >>       const struct nfs_server *server;
> >>       struct nfs4_sequence_res    seq_res;
> >> @@ -324,6 +328,7 @@ struct nfs4_delegreturnargs {
> >>
> >>   struct nfs4_delegreturnres {
> >>       struct nfs_fattr * fattr;
> >> +    struct nfs4_label * label;
> >>       const struct nfs_server *server;
> >>       struct nfs4_sequence_res    seq_res;
> >>   };
> >> @@ -343,6 +348,7 @@ struct nfs_readargs {
> >>
> >>   struct nfs_readres {
> >>       struct nfs_fattr *    fattr;
> >> +    struct nfs4_label *    label;
> >>       __u32            count;
> >>       int                     eof;
> >>       struct nfs4_sequence_res    seq_res;
> >> @@ -390,6 +396,7 @@ struct nfs_removeres {
> >>       const struct nfs_server *server;
> >>       struct nfs4_change_info    cinfo;
> >>       struct nfs_fattr    dir_attr;
> >> +    struct nfs4_label    *dir_label;
> >>       struct nfs4_sequence_res     seq_res;
> >>   };
> >>
> >> @@ -405,6 +412,7 @@ struct nfs_entry {
> >>       int            eof;
> >>       struct nfs_fh *        fh;
> >>       struct nfs_fattr *    fattr;
> >> +    struct nfs4_label *    label;
> >>   };
> >>
> >>   /*
> >> @@ -443,6 +451,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 nfs4_sequence_args     seq_args;
> >>   };
> >>
> >> @@ -473,6 +482,7 @@ struct nfs_getaclres {
> >>
> >>   struct nfs_setattrres {
> >>       struct nfs_fattr *              fattr;
> >> +    struct nfs4_label *        label;
> >>       const struct nfs_server *    server;
> >>       struct nfs4_sequence_res    seq_res;
> >>   };
> >> @@ -662,6 +672,7 @@ struct nfs4_accessargs {
> >>   struct nfs4_accessres {
> >>       const struct nfs_server *    server;
> >>       struct nfs_fattr *        fattr;
> >> +    struct nfs4_label *        label;
> >>       u32                supported;
> >>       u32                access;
> >>       struct nfs4_sequence_res    seq_res;
> >> @@ -684,6 +695,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_sequence_args     seq_args;
> >>   };
> >>
> >> @@ -691,8 +703,10 @@ 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;
> >>       struct nfs_fattr *        dir_fattr;
> >> +    struct nfs4_label *        dir_label;
> >>       struct nfs4_sequence_res    seq_res;
> >>   };
> >>
> >> @@ -717,6 +731,7 @@ struct nfs4_getattr_res {
> >>       const struct nfs_server *    server;
> >>       struct nfs_fattr *        fattr;
> >>       struct nfs4_sequence_res    seq_res;
> >> +    struct nfs4_label *        label;
> >>   };
> >>
> >>   struct nfs4_link_arg {
> >> @@ -730,8 +745,10 @@ struct nfs4_link_arg {
> >>   struct nfs4_link_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;
> >>       struct nfs4_sequence_res    seq_res;
> >>   };
> >>
> >> @@ -747,6 +764,7 @@ struct nfs4_lookup_res {
> >>       const struct nfs_server *    server;
> >>       struct nfs_fattr *        fattr;
> >>       struct nfs_fh *            fh;
> >> +    struct nfs4_label *        label;
> >>       struct nfs4_sequence_res    seq_res;
> >>   };
> >>
> >> @@ -800,6 +818,8 @@ struct nfs4_rename_arg {
> >>       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;
> >>       const u32 *            bitmask;
> >>       struct nfs4_sequence_args    seq_args;
> >>   };
> >> @@ -808,8 +828,10 @@ struct nfs4_rename_res {
> >>       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;
> >>       struct nfs4_sequence_res    seq_res;
> >>   };
> >>
> >
> >

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/