[PATCH] misc: copy extended attributes in populate_fs

[PATCH] misc: copy extended attributes in populate_fs

[Ross Burton]

Hi,

When using mkefs with -d to create a file system image from a pre-constructed
root filesystem directory, any extended attributes set are not copied into the
filesystem.

This is a problem when the file system needs them, for example it uses SELinux.
Following is a patch to copy any extended attributes to the target file system
in populate_fs().

Regards,
Ross

[PATCH] misc: copy extended attributes in populate_fs

[Ross Burton]

When creating a file system using a source directory, also copy any extended
attributes that have been set.

Signed-off-by: Ross Burton <ross.burton@intel.com>
---
 misc/create_inode.c |   56 +++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 56 insertions(+)

diff --git a/misc/create_inode.c b/misc/create_inode.c
index c9c99b5..2795f9e 100644
--- a/misc/create_inode.c
+++ b/misc/create_inode.c
@@ -103,6 +103,55 @@ static errcode_t set_inode_extra(ext2_filsys fs, ext2_ino_t cwd,
 	return retval;
 }
 
+static errcode_t set_inode_xattr(ext2_filsys fs, ext2_ino_t ino, const char *filename)
+{
+	errcode_t			retval;
+	struct ext2_inode		inode;
+	struct ext2_xattr_handle	*handle;
+	ssize_t				size, value_size;
+	char				*list;
+	int				i;
+
+	size = llistxattr(filename, NULL, 0);
+	if (size == -1) {
+		com_err(__func__, errno, "listxattr failed on %s", filename);
+		return errno;
+	} else if (size == 0) {
+		return 0;
+	}
+
+	retval = ext2fs_xattrs_open(fs, ino, &handle);
+	if (retval) {
+		com_err(__func__, errno, "while opening inode %u", ino);
+		return errno;
+	}
+
+	list = malloc(size);
+	size = llistxattr(filename, list, size);
+
+	for (i = 0; i < size; i += strlen(&list[i]) + 1) {
+		const char *name = &list[i];
+		char *value;
+
+		value_size = getxattr(filename, name, NULL, 0);
+		value = malloc(value_size);
+		value_size = getxattr(filename, name, value, value_size);
+
+		retval = ext2fs_xattr_set(handle, name, value, value_size);
+		if (retval)
+			com_err(__func__, retval, "while writing xattr %u", ino);
+
+		free (value);
+	}
+	free(list);
+
+	retval = ext2fs_xattrs_close(&handle);
+	if (retval)
+		com_err(__func__, errno, "while closing inode %u", ino);
+
+	return retval;
+}
+
 /* Make a special files (block and character devices), fifo's, and sockets  */
 errcode_t do_mknod_internal(ext2_filsys fs, ext2_ino_t cwd, const char *name,
 			    struct stat *st)
@@ -615,6 +664,13 @@ static errcode_t __populate_fs(ext2_filsys fs, ext2_ino_t parent_ino,
 			goto out;
 		}
 
+		retval = set_inode_xattr(fs, ino, name);
+		if (retval) {
+			com_err(__func__, retval,
+				_("while setting xattrs for \"%s\""), name);
+			goto out;
+		}
+
 		/* Save the hardlink ino */
 		if (save_inode) {
 			/*
-- 
1.7.10.4

Re: [PATCH] misc: copy extended attributes in populate_fs

[Darrick J. Wong]

On Fri, Jun 27, 2014 at 10:02:17PM +0100, Ross Burton wrote:
> When creating a file system using a source directory, also copy any extended
> attributes that have been set.
> 
> Signed-off-by: Ross Burton <ross.burton@intel.com>
> ---
>  misc/create_inode.c |   56 +++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 56 insertions(+)
> 
> diff --git a/misc/create_inode.c b/misc/create_inode.c
> index c9c99b5..2795f9e 100644
> --- a/misc/create_inode.c
> +++ b/misc/create_inode.c
> @@ -103,6 +103,55 @@ static errcode_t set_inode_extra(ext2_filsys fs, ext2_ino_t cwd,
>  	return retval;
>  }
>  
> +static errcode_t set_inode_xattr(ext2_filsys fs, ext2_ino_t ino, const char *filename)
> +{
> +	errcode_t			retval;
> +	struct ext2_inode		inode;
> +	struct ext2_xattr_handle	*handle;
> +	ssize_t				size, value_size;
> +	char				*list;
> +	int				i;
> +
> +	size = llistxattr(filename, NULL, 0);
> +	if (size == -1) {
> +		com_err(__func__, errno, "listxattr failed on %s", filename);
> +		return errno;
> +	} else if (size == 0) {
> +		return 0;
> +	}
> +
> +	retval = ext2fs_xattrs_open(fs, ino, &handle);

If the FS does not have at least one of the inline_data or ext_attr features
turned on, the ext2fs_xattrs_open call returns EXT2_ET_MISSING_EA_FEATURE,
which aborts the whole operation.  Is that ok?

> +	if (retval) {
> +		com_err(__func__, errno, "while opening inode %u", ino);

retval, not errno.

> +		return errno;
> +	}
> +
> +	list = malloc(size);

What happens if malloc fails?

> +	size = llistxattr(filename, list, size);

What if this second call should fail for some reason?  Shouldn't we stop?

> +
> +	for (i = 0; i < size; i += strlen(&list[i]) + 1) {
> +		const char *name = &list[i];
> +		char *value;
> +
> +		value_size = getxattr(filename, name, NULL, 0);

What if getxattr returns -1?

> +		value = malloc(value_size);
> +		value_size = getxattr(filename, name, value, value_size);

Same complaints about not checking malloc/getxattr return values.

> +
> +		retval = ext2fs_xattr_set(handle, name, value, value_size);
> +		if (retval)
> +			com_err(__func__, retval, "while writing xattr %u", ino);
> +
> +		free (value);
> +	}
> +	free(list);
> +
> +	retval = ext2fs_xattrs_close(&handle);
> +	if (retval)
> +		com_err(__func__, errno, "while closing inode %u", ino);

retval, not errno.

--D
> +
> +	return retval;
> +}
> +
>  /* Make a special files (block and character devices), fifo's, and sockets  */
>  errcode_t do_mknod_internal(ext2_filsys fs, ext2_ino_t cwd, const char *name,
>  			    struct stat *st)
> @@ -615,6 +664,13 @@ static errcode_t __populate_fs(ext2_filsys fs, ext2_ino_t parent_ino,
>  			goto out;
>  		}
>  
> +		retval = set_inode_xattr(fs, ino, name);
> +		if (retval) {
> +			com_err(__func__, retval,
> +				_("while setting xattrs for \"%s\""), name);
> +			goto out;
> +		}
> +
>  		/* Save the hardlink ino */
>  		if (save_inode) {
>  			/*
> -- 
> 1.7.10.4
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] misc: copy extended attributes in populate_fs

[Burton, Ross]

On 30 June 2014 19:43, Darrick J. Wong <darrick.wong@oracle.com> wrote:
>> +     retval = ext2fs_xattrs_open(fs, ino, &handle);
>
> If the FS does not have at least one of the inline_data or ext_attr features
> turned on, the ext2fs_xattrs_open call returns EXT2_ET_MISSING_EA_FEATURE,
> which aborts the whole operation.  Is that ok?

Good point.

I'm currently dithering over whether this should silently do nothing,
or warn per file, or have an already-warned boolean to avoid spamming
the user.

>> +     if (retval) {
>> +             com_err(__func__, errno, "while opening inode %u", ino);
>
> retval, not errno.

Literally just squashing some commits for these error paths which went wrong.

>> +             return errno;
>> +     }
>> +
>> +     list = malloc(size);
>
> What happens if malloc fails?

You're doomed?

What's e2fsprog's policy on OOM - I see some instances of checked
malloc() that exit() promptly, others that simply return 0 and hope
for the best, and some that use fputs() and gettext() - but if we're
OOM then that's even more likely to fail.

I'll follow the official line, but what that line is isn't clear from
a sample of malloc() calls in the source.

>> +     size = llistxattr(filename, list, size);
>
> What if this second call should fail for some reason?  Shouldn't we stop?

Yes, also fixed locally.

>> +
>> +     for (i = 0; i < size; i += strlen(&list[i]) + 1) {
>> +             const char *name = &list[i];
>> +             char *value;
>> +
>> +             value_size = getxattr(filename, name, NULL, 0);
>
> What if getxattr returns -1?

Should bail, will fix.

>> +             value = malloc(value_size);
>> +             value_size = getxattr(filename, name, value, value_size);
>
> Same complaints about not checking malloc/getxattr return values.

Agreed.

>> +     retval = ext2fs_xattrs_close(&handle);
>> +     if (retval)
>> +             com_err(__func__, errno, "while closing inode %u", ino);
>
> retval, not errno.

[sags head in shame]

Next time I'm patching at 10pm I'll remember to review the patch the
next morning before sending...

Ross

Re: [PATCH] misc: copy extended attributes in populate_fs

[Darrick J. Wong]

On Mon, Jun 30, 2014 at 07:59:03PM +0100, Burton, Ross wrote:
> On 30 June 2014 19:43, Darrick J. Wong <darrick.wong@oracle.com> wrote:
> >> +     retval = ext2fs_xattrs_open(fs, ino, &handle);
> >
> > If the FS does not have at least one of the inline_data or ext_attr features
> > turned on, the ext2fs_xattrs_open call returns EXT2_ET_MISSING_EA_FEATURE,
> > which aborts the whole operation.  Is that ok?
> 
> Good point.
> 
> I'm currently dithering over whether this should silently do nothing,
> or warn per file, or have an already-warned boolean to avoid spamming
> the user.
> 
> >> +     if (retval) {
> >> +             com_err(__func__, errno, "while opening inode %u", ino);
> >
> > retval, not errno.
> 
> Literally just squashing some commits for these error paths which went wrong.
> 
> >> +             return errno;
> >> +     }
> >> +
> >> +     list = malloc(size);
> >
> > What happens if malloc fails?
> 
> You're doomed?
> 
> What's e2fsprog's policy on OOM - I see some instances of checked
> malloc() that exit() promptly, others that simply return 0 and hope
> for the best, and some that use fputs() and gettext() - but if we're
> OOM then that's even more likely to fail.
> 
> I'll follow the official line, but what that line is isn't clear from
> a sample of malloc() calls in the source.

if (!list) com_err(__func__, ENOMEM, "while _______");

as a starting point.  Better to print a nice error and exit than to simply
crash on the null pointer dereference.

I suppose you could also do the retval = ext2fs_get_mem(size, &list);
construction as well.

--D

> 
> >> +     size = llistxattr(filename, list, size);
> >
> > What if this second call should fail for some reason?  Shouldn't we stop?
> 
> Yes, also fixed locally.
> 
> >> +
> >> +     for (i = 0; i < size; i += strlen(&list[i]) + 1) {
> >> +             const char *name = &list[i];
> >> +             char *value;
> >> +
> >> +             value_size = getxattr(filename, name, NULL, 0);
> >
> > What if getxattr returns -1?
> 
> Should bail, will fix.
> 
> >> +             value = malloc(value_size);
> >> +             value_size = getxattr(filename, name, value, value_size);
> >
> > Same complaints about not checking malloc/getxattr return values.
> 
> Agreed.
> 
> >> +     retval = ext2fs_xattrs_close(&handle);
> >> +     if (retval)
> >> +             com_err(__func__, errno, "while closing inode %u", ino);
> >
> > retval, not errno.
> 
> [sags head in shame]
> 
> Next time I'm patching at 10pm I'll remember to review the patch the
> next morning before sending...
> 
> Ross