On 12/02/13 17:54, J. Bruce Fields wrote:
> On Fri, Feb 08, 2013 at 07:39:22AM -0500, Steve Dickson wrote:
>> @@ -242,7 +247,8 @@ nfsd4_decode_bitmap(struct nfsd4_compoundargs *argp, u32 *bmval)
>>
>> 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)
>> {
>> int expected_len, len = 0;
>> u32 dummy32;
>> @@ -386,6 +392,50 @@ nfsd4_decode_fattr(struct nfsd4_compoundargs *argp, u32 *bmval,
>> goto xdr_error;
>> }
>> }
>> +#ifdef CONFIG_NFSD_V4_SECURITY_LABEL
>> + if (bmval[2] & FATTR4_WORD2_SECURITY_LABEL) {
>> + uint32_t pi;
>> + uint32_t lfs;
>> +
>> + READ_BUF(4);
>> + len += 4;
>> + READ32(lfs);
>> + READ_BUF(4);
>> + len += 4;
>> + READ32(pi);
>> + 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);
>
> So we don't want to allocate these the same way we did on the client?
These allocations are slightly different.. One is for incoming labels
and the other is for out going...
>
>> + if (*label == NULL) {
>> + host_err = -ENOMEM;
>> + goto out_nfserr;
>> + }
>> +
>> + (*label)->label = kmalloc(dummy32 + 1, GFP_KERNEL);
>> + if ((*label)->label == NULL) {
>> + host_err = -ENOMEM;
>> + kfree(*label);
>> + goto out_nfserr;
>> + }
>> +
>> + (*label)->len = dummy32;
>> + memcpy((*label)->label, buf, dummy32);
>> + ((char *)(*label)->label)[dummy32] = '\0';
>> + (*label)->pi = pi;
>> + (*label)->lfs = lfs;
>> +
>> + defer_free(argp, kfree, (*label)->label);
>> + defer_free(argp, kfree, *label);
>
> Hm: looking at defer_free and nfsd4_release_compoundargs: looks like
> argp->to_free is effectively a stack, so these will get freed in the
> reverse order: *label first, then (*label)->label. That's trouble.
>
> So, I think we want the order of those two defer_free()'s reversed.
>
> Or we could just allocate the whole things as one chunk as on the client
> and not have to worry about this kind of thing. I think I'd prefer that
> to trying to keep the allocation as small as possible. We're only using
> this memory temporarily so it's not as though we need to have tons of
> struct nfs4_labels allocated at the same time anyway.
I reworked this in the upcoming release to be more chunky... ;-)
>> +
>> static __be32 fattr_handle_absent_fs(u32 *bmval0, u32 *bmval1, u32 *rdattr_err)
>> {
>> /* As per referral draft: */
>> @@ -2111,6 +2205,10 @@ nfsd4_encode_fattr(struct svc_fh *fhp, struct svc_export *exp,
>>
>> if (!aclsupport)
>> word0 &= ~FATTR4_WORD0_ACL;
>> +#ifdef CONFIG_NFSD_V4_SECURITY_LABEL
>> + if (word2)
>> + word2 |= FATTR4_WORD2_SECURITY_LABEL;
>> +#endif
>
> Please just fix the definition of NFSD4_1_SUPPORTED_ATTRS_WORD2. (The
> ifdef can go in that definition.)
Right...
>
> Although: don't we need to check whether the exported filesystem
> supports security labels?
I took a quick look around and didn't see any interface to do that.
Is it even possible to do this?
>
>> if (!word2) {
>> if ((buflen -= 12) < 0)
>> goto out_resource;
>> @@ -2423,6 +2521,14 @@ out_acl:
>> get_parent_attributes(exp, &stat);
>> WRITE64(stat.ino);
>> }
>> + if (bmval2 & FATTR4_WORD2_SECURITY_LABEL) {
>> + status = nfsd4_encode_security_label(rqstp, dentry,
>> + &p, &buflen);
>> + if (status == nfserr_resource)
>> + goto out_resource;
>
> Just remove that, there's no real point to the out_resource goto.
Gone...
>
>> + if (status)
>> + goto out;
>
>> + }
>> if (bmval2 & FATTR4_WORD2_SUPPATTR_EXCLCREAT) {
>> WRITE32(3);
>> WRITE32(NFSD_SUPPATTR_EXCLCREAT_WORD0);
>> diff --git a/fs/nfsd/nfsd.h b/fs/nfsd/nfsd.h
>> index de23db2..727fe44 100644
>> --- a/fs/nfsd/nfsd.h
>> +++ b/fs/nfsd/nfsd.h
>> @@ -308,10 +308,10 @@ void nfsd_lockd_shutdown(void);
>> | FATTR4_WORD1_OWNER | FATTR4_WORD1_OWNER_GROUP | FATTR4_WORD1_RAWDEV \
>> | FATTR4_WORD1_SPACE_AVAIL | FATTR4_WORD1_SPACE_FREE | FATTR4_WORD1_SPACE_TOTAL \
>> | FATTR4_WORD1_SPACE_USED | FATTR4_WORD1_TIME_ACCESS | FATTR4_WORD1_TIME_ACCESS_SET \
>> - | FATTR4_WORD1_TIME_DELTA | FATTR4_WORD1_TIME_METADATA \
>> - | FATTR4_WORD1_TIME_MODIFY | FATTR4_WORD1_TIME_MODIFY_SET | FATTR4_WORD1_MOUNTED_ON_FILEID)
>> + | FATTR4_WORD1_TIME_DELTA | FATTR4_WORD1_TIME_METADATA | FATTR4_WORD1_TIME_MODIFY \
>> + | FATTR4_WORD1_TIME_MODIFY_SET | FATTR4_WORD1_MOUNTED_ON_FILEID)
>
> Did that actually change anything? If not, just leave the formatting
> alone.
Gone...
>
>>
>> -#define NFSD4_SUPPORTED_ATTRS_WORD2 0
>> +#define NFSD4_SUPPORTED_ATTRS_WORD2 FATTR4_WORD2_SECURITY_LABEL
>
> That should be conditional on CONFIG_NFSD_V4_SECURITY_LABEL, shouldn't
> it?
Yes..
>
>>
>> #define NFSD4_1_SUPPORTED_ATTRS_WORD0 \
>> NFSD4_SUPPORTED_ATTRS_WORD0
>> @@ -350,7 +350,7 @@ static inline u32 nfsd_suppattrs2(u32 minorversion)
>> #define NFSD_WRITEABLE_ATTRS_WORD1 \
>> (FATTR4_WORD1_MODE | FATTR4_WORD1_OWNER | FATTR4_WORD1_OWNER_GROUP \
>> | FATTR4_WORD1_TIME_ACCESS_SET | FATTR4_WORD1_TIME_MODIFY_SET)
>> -#define NFSD_WRITEABLE_ATTRS_WORD2 0
>> +#define NFSD_WRITEABLE_ATTRS_WORD2 FATTR4_WORD2_SECURITY_LABEL
>>
>> #define NFSD_SUPPATTR_EXCLCREAT_WORD0 \
>> NFSD_WRITEABLE_ATTRS_WORD0
>> diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c
>> index d586117..0567fdc 100644
>> --- a/fs/nfsd/vfs.c
>> +++ b/fs/nfsd/vfs.c
>> @@ -28,6 +28,7 @@
>> #include <asm/uaccess.h>
>> #include <linux/exportfs.h>
>> #include <linux/writeback.h>
>> +#include <linux/security.h>
>>
>> #ifdef CONFIG_NFSD_V3
>> #include "xdr3.h"
>> @@ -621,6 +622,35 @@ int nfsd4_is_junction(struct dentry *dentry)
>> return 0;
>> return 1;
>> }
>> +#ifdef CONFIG_NFSD_V4_SECURITY_LABEL
>> +__be32 nfsd4_set_nfs4_label(struct svc_rqst *rqstp, struct svc_fh *fhp,
>> + struct nfs4_label *label)
>> +{
>> + __be32 error;
>> + int host_error;
>> + struct dentry *dentry;
>> +
>> + /* Get inode */
>
> That comment's unnecessary.
Gone...
>
>> + /* XXX: should we have a MAY_SSECCTX? */
>
> I don't know, should we?
I vote no! ;-) What is that?
steved.
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
