[v4,1/3] xattr: add simple initxattrs function
diff mbox

Message ID 1483653823-22018-2-git-send-email-david.graziano@rockwellcollins.com
State New
Headers show

Commit Message

David Graziano Jan. 5, 2017, 10:03 p.m. UTC
Adds new simple_xattr_initxattrs() initialization function for
initializing the extended attributes via LSM callback. Based
on callback function used by tmpfs/shmem. This is allows for
consolidation and avoiding code duplication when other filesystem
need to implement a simple initxattrs LSM callback function.

Signed-off-by: David Graziano <david.graziano@rockwellcollins.com>
---
 fs/xattr.c            | 39 +++++++++++++++++++++++++++++++++++++++
 include/linux/xattr.h |  3 +++
 2 files changed, 42 insertions(+)

Comments

Christoph Hellwig Jan. 8, 2017, 9:55 a.m. UTC | #1
> +/*
> + * Callback for security_inode_init_security() for acquiring xattrs.
> + */
> +int simple_xattr_initxattrs(struct inode *inode,
> +			    const struct xattr *xattr_array,
> +			    void *fs_info)
> +{
> +	struct simple_xattrs *xattrs;
> +	const struct xattr *xattr;
> +	struct simple_xattr *new_xattr;
> +	size_t len;
> +
> +	if (!fs_info)
> +		return -ENOMEM;

This probablt should be an EINVAL, and also a WARN_ON_ONCE.

> +	xattrs = (struct simple_xattrs *) fs_info;

No need for the cast.  In fact we should probably just declarate it
as struct simple_xattrs *xattrs in the protoype and thus be type safe.

> +
> +	for (xattr = xattr_array; xattr->name != NULL; xattr++) {
> +		new_xattr = simple_xattr_alloc(xattr->value, xattr->value_len);
> +		if (!new_xattr)
> +			return -ENOMEM;

We'll need to unwind the previous allocations here.
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
David Graziano Jan. 9, 2017, 3:41 p.m. UTC | #2
On Sun, Jan 8, 2017 at 3:55 AM, Christoph Hellwig <hch@infradead.org> wrote:
>> +/*
>> + * Callback for security_inode_init_security() for acquiring xattrs.
>> + */
>> +int simple_xattr_initxattrs(struct inode *inode,
>> +                         const struct xattr *xattr_array,
>> +                         void *fs_info)
>> +{
>> +     struct simple_xattrs *xattrs;
>> +     const struct xattr *xattr;
>> +     struct simple_xattr *new_xattr;
>> +     size_t len;
>> +
>> +     if (!fs_info)
>> +             return -ENOMEM;
>
> This probablt should be an EINVAL, and also a WARN_ON_ONCE.

I will change the return value to -EINVAL and add the WARN_ON_ONCE.
In the next version of the patchset.

>
>> +     xattrs = (struct simple_xattrs *) fs_info;
>
> No need for the cast.  In fact we should probably just declarate it
> as struct simple_xattrs *xattrs in the protoype and thus be type safe.

I don't think the prototype can be changed to "struct simple_xattrs" as the
security_inode_init_security() function in security/security.c which calls
this is asumming an initxattrs function with following prototype
int (*initxattrs)  (struct inode *inode, const struct xattr
*xattr_array, void *fs_data)

>
>> +
>> +     for (xattr = xattr_array; xattr->name != NULL; xattr++) {
>> +             new_xattr = simple_xattr_alloc(xattr->value, xattr->value_len);
>> +             if (!new_xattr)
>> +                     return -ENOMEM;
>
> We'll need to unwind the previous allocations here.

This patchset essentially relocates the shmem_initxattrs() function from
mm/shmem.c and uses the relocated function for both tmpfs and mqueuefs.
That inital function didn't attempt to unwind the previous allocations. If the
consensus is to unwind any allocations made by this function I can look
at adding it.
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch
diff mbox

diff --git a/fs/xattr.c b/fs/xattr.c
index c243905..69dd142 100644
--- a/fs/xattr.c
+++ b/fs/xattr.c
@@ -994,3 +994,42 @@  void simple_xattr_list_add(struct simple_xattrs *xattrs,
 	list_add(&new_xattr->list, &xattrs->head);
 	spin_unlock(&xattrs->lock);
 }
+
+/*
+ * Callback for security_inode_init_security() for acquiring xattrs.
+ */
+int simple_xattr_initxattrs(struct inode *inode,
+			    const struct xattr *xattr_array,
+			    void *fs_info)
+{
+	struct simple_xattrs *xattrs;
+	const struct xattr *xattr;
+	struct simple_xattr *new_xattr;
+	size_t len;
+
+	if (!fs_info)
+		return -ENOMEM;
+	xattrs = (struct simple_xattrs *) fs_info;
+
+	for (xattr = xattr_array; xattr->name != NULL; xattr++) {
+		new_xattr = simple_xattr_alloc(xattr->value, xattr->value_len);
+		if (!new_xattr)
+			return -ENOMEM;
+		len = strlen(xattr->name) + 1;
+		new_xattr->name = kmalloc(XATTR_SECURITY_PREFIX_LEN + len,
+					  GFP_KERNEL);
+		if (!new_xattr->name) {
+			kfree(new_xattr);
+			return -ENOMEM;
+		}
+
+		memcpy(new_xattr->name, XATTR_SECURITY_PREFIX,
+		       XATTR_SECURITY_PREFIX_LEN);
+		memcpy(new_xattr->name + XATTR_SECURITY_PREFIX_LEN,
+		       xattr->name, len);
+
+		simple_xattr_list_add(xattrs, new_xattr);
+	}
+
+	return 0;
+}
diff --git a/include/linux/xattr.h b/include/linux/xattr.h
index 94079ba..a787d1a 100644
--- a/include/linux/xattr.h
+++ b/include/linux/xattr.h
@@ -108,5 +108,8 @@  ssize_t simple_xattr_list(struct inode *inode, struct simple_xattrs *xattrs, cha
 			  size_t size);
 void simple_xattr_list_add(struct simple_xattrs *xattrs,
 			   struct simple_xattr *new_xattr);
+int simple_xattr_initxattrs(struct inode *inode,
+			    const struct xattr *xattr_array,
+			    void *fs_info);
 
 #endif	/* _LINUX_XATTR_H */