[PATCH v2] Modify mtd-utils intgck utility to test extended attribute set/get for UBIFS

[PATCH v2] Modify mtd-utils intgck utility to test extended attribute set/get for UBIFS

[Subodh Nijsure]

Changes since v1:
        Randomize value of extended attribute.
        Update dir_entry structure to keep track of extended attribute and
        check it during verification phase, as suggested during v1 review.
	Now compiling and running integck requires libattr.

Signed-off-by: Subodh Nijsure <snijsure@grid-net.com>
---
 tests/fs-tests/integrity/Makefile  |    2 +-
 tests/fs-tests/integrity/integck.c |   92 +++++++++++++++++++++++++++++++++---
 tests/ubi-tests/Makefile           |    3 +-
 3 files changed, 88 insertions(+), 9 deletions(-)

diff --git a/tests/fs-tests/integrity/Makefile b/tests/fs-tests/integrity/Makefile
index 4d6fc7d..b814f77 100644
--- a/tests/fs-tests/integrity/Makefile
+++ b/tests/fs-tests/integrity/Makefile
@@ -25,7 +25,7 @@ $(TARGETS): libubi.a
 # Disable optimizations to make it possible to use gdb comfortably
 # Use -rdynamic to have stack backtraces
 debug: libubi.a
-	gcc $(CFLAGS) -O0 -D INTEGCK_DEBUG -rdynamic integck.c libubi.a -o integck
+	gcc $(CFLAGS) -O0 -D INTEGCK_DEBUG -rdynamic integck.c libubi.a -o integck -lattr
 
 clean:
 	rm -f *.o $(TARGETS) libubi.a
diff --git a/tests/fs-tests/integrity/integck.c b/tests/fs-tests/integrity/integck.c
index 30322cd..7bd73bf 100644
--- a/tests/fs-tests/integrity/integck.c
+++ b/tests/fs-tests/integrity/integck.c
@@ -91,6 +91,7 @@
 		normsg(fmt " (line %d)", ##__VA_ARGS__, __LINE__);           \
 } while(0)
 
+static int xattr_count;
 /* The variables below are set by command line arguments */
 static struct {
 	long repeat_cnt;
@@ -198,6 +199,7 @@ struct dir_entry_info /* Each entry in a directory has one of these */
 	struct dir_entry_info *next_link; /* List of hard links for same file */
 	struct dir_entry_info *prev_link; /* List of hard links for same file */
 	char *name;
+	char *xattr_value; /* Extended attribute on this directory */
 	struct dir_info *parent; /* Parent directory */
 	union {
 		struct file_info *file;
@@ -360,6 +362,60 @@ static char *cat_paths(const char *a, const char *b)
 }
 
 /*
+ * Verify security.selinux extendend attribute for given path
+ */
+void xattr_check(char *path, char *xattr_value)
+{
+	int ret;
+	char buf[255];
+	int attrLen;
+	/* Check if path actually exists */
+	if (access(path, F_OK) != 0)
+		return;
+	if (xattr_value == NULL)
+		return;
+	attrLen = strlen(xattr_value) + 1;
+	v("retrieve extended attribute for  %s", path);
+	getxattr(path, "security.selinux", buf, attrLen);
+	CHECK(ret == 0);
+	ret = strncmp(buf, xattr_value, attrLen);
+	if (ret != 0) {
+		printf("ret != 0 PATH %s\n", path);
+		printf("Expected value %s and retrived val %s\n",
+			xattr_value, buf);
+		CHECK(ret == 0);
+	} else {
+		CHECK(ret == 0);
+	}
+}
+
+/*
+ * Assign security.selinux extendend attribute for this path
+ */
+char *assign_xattr(const char *path)
+{
+	int ret;
+	char value[255], *xattr_value;
+	int attrLen;
+
+	sprintf(value, "root:object_r:bin_t%d", xattr_count);
+	xattr_count++;
+	xattr_value = (char *)malloc(strlen(value)+1);
+	if (xattr_value == NULL)
+		return NULL;
+	strcpy(xattr_value, value);
+	attrLen = strlen(xattr_value) + 1;
+	v("assign extended attribute to %s", path);
+	/* printf("%s assigned %s\n", path, xattr_value); */
+	ret = setxattr(path, "security.selinux", xattr_value, attrLen, 0x0);
+	if (ret == 0)
+		return xattr_value;
+	else {
+		free(xattr_value);
+		return NULL;
+	}
+}
+/*
  * Get the free space for the tested file system.
  */
 static void get_fs_space(uint64_t *total, uint64_t *free)
@@ -450,13 +506,17 @@ static void free_writes_info(struct file_info *file)
 }
 
 static void *add_dir_entry(struct dir_info *parent, char type, const char *name,
-			   void *target)
+			   const char *xattr_value, void *target)
 {
 	struct dir_entry_info *entry;
 
 	entry = zalloc(sizeof(struct dir_entry_info));
 	entry->type = type;
 	entry->name = dup_string(name);
+	if (xattr_value != NULL)
+		entry->xattr_value = dup_string(xattr_value);
+	else
+		entry->xattr_value = NULL;
 	entry->parent = parent;
 
 	entry->next = parent->first;
@@ -540,6 +600,7 @@ static void remove_dir_entry(struct dir_entry_info *entry, int free_target)
 	}
 
 	free(entry->name);
+	free(entry->xattr_value);
 	free(entry);
 }
 
@@ -551,6 +612,7 @@ static void remove_dir_entry(struct dir_entry_info *entry, int free_target)
 static int dir_new(struct dir_info *parent, const char *name)
 {
 	char *path;
+	char *xattr_value;
 
 	assert(parent);
 
@@ -573,9 +635,11 @@ static int dir_new(struct dir_info *parent, const char *name)
 		CHECK(lstat(path, &st) == 0);
 		CHECK(S_ISDIR(st.st_mode));
 	}
+	xattr_value = assign_xattr(path);
+	CHECK(xattr_value != NULL);
+	add_dir_entry(parent, 'd', name, xattr_value, NULL);
+	free(xattr_value);
 	free(path);
-
-	add_dir_entry(parent, 'd', name, NULL);
 	return 0;
 }
 
@@ -630,6 +694,7 @@ static int dir_remove(struct dir_info *dir)
 static int file_new(struct dir_info *parent, const char *name)
 {
 	char *path;
+	char *xattr_value;
 	mode_t mode;
 	int fd;
 
@@ -657,7 +722,10 @@ static int file_new(struct dir_info *parent, const char *name)
 		CHECK(S_ISREG(st.st_mode));
 	}
 
-	add_dir_entry(parent, 'f', name, NULL);
+	xattr_value = assign_xattr(path);
+	CHECK(xattr_value != NULL);
+	add_dir_entry(parent, 'f', name, xattr_value, NULL);
+	free(xattr_value);
 
 	close(fd);
 	free(path);
@@ -669,6 +737,7 @@ static int link_new(struct dir_info *parent, const char *name,
 {
 	struct dir_entry_info *entry;
 	char *path, *target;
+	char *xattr_value = NULL;
 	int ret;
 	struct stat st1, st2;
 
@@ -704,7 +773,10 @@ static int link_new(struct dir_info *parent, const char *name,
 		CHECK(st2.st_nlink == st1.st_nlink + 1);
 	}
 
-	add_dir_entry(parent, 'f', name, file);
+	xattr_value = assign_xattr(path);
+	CHECK(xattr_value != NULL);
+	add_dir_entry(parent, 'f', name, xattr_value, file);
+	free(xattr_value);
 
 	free(target);
 	free(path);
@@ -1753,6 +1825,10 @@ static void dir_check(struct dir_info *dir)
 	/* Now check each entry */
 	entry = dir->first;
 	while (entry) {
+		char buf[2048];
+		sprintf(buf, "%s/%s", path, entry->name);
+		if (entry->type == 'd' || entry->type == 'f')
+			xattr_check(buf, entry->xattr_value);
 		if (entry->type == 'd') {
 			dir_check(entry->dir);
 			link_count += 1; /* <subdir>/.. */
@@ -1927,6 +2003,7 @@ static int rename_entry(struct dir_entry_info *entry)
 	struct dir_entry_info *rename_entry = NULL;
 	struct dir_info *parent;
 	char *path, *to, *name;
+	char *xattr_value = NULL;
 	int ret, isdir, retry;
 	struct stat st1, st2;
 
@@ -2000,7 +2077,8 @@ static int rename_entry(struct dir_entry_info *entry)
 		return 0;
 	}
 
-	add_dir_entry(parent, entry->type, name, entry->target);
+	add_dir_entry(parent, entry->type, name, xattr_value, entry->target);
+	free(xattr_value);
 	if (rename_entry)
 		remove_dir_entry(rename_entry, 1);
 	remove_dir_entry(entry, 0);
@@ -2132,7 +2210,7 @@ static int symlink_new(struct dir_info *dir, const char *nm)
 	if (args.verify_ops)
 		verify_symlink(target, path);
 
-	s = add_dir_entry(dir, 's', name, NULL);
+	s = add_dir_entry(dir, 's', name, NULL, NULL);
 	s->target_pathname = target;
 
 	free(path);
diff --git a/tests/ubi-tests/Makefile b/tests/ubi-tests/Makefile
index 2c47a9f..ba8e7e0 100644
--- a/tests/ubi-tests/Makefile
+++ b/tests/ubi-tests/Makefile
@@ -8,7 +8,8 @@ LIBS = libubi
 TARGETS=io_update volrefcnt integ io_paral io_read io_basic \
           mkvol_basic mkvol_bad mkvol_paral rsvol
 
-CFLAGS += -I$(LIBUBI_HEADER_PATH) -I $(KERNELHDR) -lpthread
+CFLAGS += -I$(LIBUBI_HEADER_PATH) -I$(KERNELHDR) -lpthread
+LDFLAGS += -lpthread
 
 include ../../common.mk
 
-- 
1.7.5.4

Re: [PATCH v2] Modify mtd-utils intgck utility to test extended attribute set/get for UBIFS

[Artem Bityutskiy]

[-- Attachment #1: Type: text/plain, Size: 1153 bytes --]

On Thu, 2012-05-10 at 16:05 -0700, Subodh Nijsure wrote:
> + * Verify security.selinux extendend attribute for given path
> + */
> +void xattr_check(char *path, char *xattr_value)
> +{
> +	int ret;
> +	char buf[255];
> +	int attrLen;
> +	/* Check if path actually exists */
> +	if (access(path, F_OK) != 0)
> +		return;
> +	if (xattr_value == NULL)
> +		return;
> +	attrLen = strlen(xattr_value) + 1;
> +	v("retrieve extended attribute for  %s", path);
> +	getxattr(path, "security.selinux", buf, attrLen);
> +	CHECK(ret == 0);
> +	ret = strncmp(buf, xattr_value, attrLen);
> +	if (ret != 0) {
> +		printf("ret != 0 PATH %s\n", path);
> +		printf("Expected value %s and retrived val %s\n",
> +			xattr_value, buf);
> +		CHECK(ret == 0);
> +	} else {
> +		CHECK(ret == 0);
> +	}

If the file-system does not support xattrs and returns ENOTSUPP, you
should not fail. Instead, you should set a "xattrs_supported" variable,
set it to zero, and have "fi (!xattrs_supported) return" check at the
top level of this function. Or something like this which handles the
situation gracefully.

-- 
Best Regards,
Artem Bityutskiy

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH v2] Modify mtd-utils intgck utility to test extended attribute set/get for UBIFS

[Artem Bityutskiy]

[-- Attachment #1: Type: text/plain, Size: 5542 bytes --]

Hi,

On Thu, 2012-05-10 at 16:05 -0700, Subodh Nijsure wrote:
> Changes since v1:
>         Randomize value of extended attribute.
>         Update dir_entry structure to keep track of extended attribute and
>         check it during verification phase, as suggested during v1 review.
> 	Now compiling and running integck requires libattr.
> 
> Signed-off-by: Subodh Nijsure <snijsure@grid-net.com>

Please, fix these compiler warnings:

gcc  -Wall -g -O2 -I../../../include -I../../../ubi-utils//include -c ../../../ubi-utils//libubi.c -o libubi.o
ar cr libubi.a libubi.o
gcc  -Wall -g -O2 -I../../../include -I../../../ubi-utils//include    integck.c libubi.a   -o integck
integck.c: In function ‘xattr_check’:
integck.c:379:2: warning: implicit declaration of function ‘getxattr’ [-Wimplicit-function-declaration]
integck.c: In function ‘assign_xattr’:
integck.c:410:2: warning: implicit declaration of function ‘setxattr’ [-Wimplicit-function-declaration]
integck.c: In function ‘xattr_check’:
integck.c:380:2: warning: ‘ret’ may be used uninitialized in this function [-Wuninitialized]

Below are some comments - some of them apply to more places, so do not
forget to check other places in your patch for the same things.

> diff --git a/tests/fs-tests/integrity/Makefile b/tests/fs-tests/integrity/Makefile
> index 4d6fc7d..b814f77 100644
> --- a/tests/fs-tests/integrity/Makefile
> +++ b/tests/fs-tests/integrity/Makefile
> @@ -25,7 +25,7 @@ $(TARGETS): libubi.a
>  # Disable optimizations to make it possible to use gdb comfortably
>  # Use -rdynamic to have stack backtraces
>  debug: libubi.a
> -	gcc $(CFLAGS) -O0 -D INTEGCK_DEBUG -rdynamic integck.c libubi.a -o integck
> +	gcc $(CFLAGS) -O0 -D INTEGCK_DEBUG -rdynamic integck.c libubi.a -o integck -lattr
>  
>  clean:
>  	rm -f *.o $(TARGETS) libubi.a
> diff --git a/tests/fs-tests/integrity/integck.c b/tests/fs-tests/integrity/integck.c
> index 30322cd..7bd73bf 100644
> --- a/tests/fs-tests/integrity/integck.c
> +++ b/tests/fs-tests/integrity/integck.c
> @@ -91,6 +91,7 @@
>  		normsg(fmt " (line %d)", ##__VA_ARGS__, __LINE__);           \
>  } while(0)
>  
> +static int xattr_count;
>  /* The variables below are set by command line arguments */
>  static struct {
>  	long repeat_cnt;
> @@ -198,6 +199,7 @@ struct dir_entry_info /* Each entry in a directory has one of these */
>  	struct dir_entry_info *next_link; /* List of hard links for same file */
>  	struct dir_entry_info *prev_link; /* List of hard links for same file */
>  	char *name;
> +	char *xattr_value; /* Extended attribute on this directory */
>  	struct dir_info *parent; /* Parent directory */
>  	union {
>  		struct file_info *file;
> @@ -360,6 +362,60 @@ static char *cat_paths(const char *a, const char *b)
>  }
>  
>  /*
> + * Verify security.selinux extendend attribute for given path
> + */
> +void xattr_check(char *path, char *xattr_value)
> +{
> +	int ret;
> +	char buf[255];

Introduce a macro for max xattr length instead of a magic number.

> +	int attrLen;
> +	/* Check if path actually exists */
> +	if (access(path, F_OK) != 0)
> +		return;
> +	if (xattr_value == NULL)
> +		return;

It is faster to first check xattr_value, then make the "access" syscall.

Also, please the style this file uses - "if (!xattr)".

> +	attrLen = strlen(xattr_value) + 1;
> +	v("retrieve extended attribute for  %s", path);
> +	getxattr(path, "security.selinux", buf, attrLen);
> +	CHECK(ret == 0);
> +	ret = strncmp(buf, xattr_value, attrLen);
> +	if (ret != 0) {
> +		printf("ret != 0 PATH %s\n", path);
> +		printf("Expected value %s and retrived val %s\n",
> +			xattr_value, buf);

Use errmsg().

> +		CHECK(ret == 0);
> +	} else {
> +		CHECK(ret == 0);
> +	}
> +}
> +
> +/*
> + * Assign security.selinux extendend attribute for this path
> + */
> +char *assign_xattr(const char *path)
> +{
> +	int ret;
> +	char value[255], *xattr_value;
> +	int attrLen;
> +
> +	sprintf(value, "root:object_r:bin_t%d", xattr_count);
> +	xattr_count++;
> +	xattr_value = (char *)malloc(strlen(value)+1);

sizefo(value) instead of strlen.
Remove the cast after malloc.

> +	if (xattr_value == NULL)
> +		return NULL;
> +	strcpy(xattr_value, value);
> +	attrLen = strlen(xattr_value) + 1;
> +	v("assign extended attribute to %s", path);
> +	/* printf("%s assigned %s\n", path, xattr_value); */
Kill this comment.

> +	ret = setxattr(path, "security.selinux", xattr_value, attrLen, 0x0);
> +	if (ret == 0)
> +		return xattr_value;
> +	else {
> +		free(xattr_value);
> +		return NULL;
> +	}
> +}

Please, do not assign an xattr to all files and directories - better a
give it a 20% chance or something like this. This is better for test
coverage I thing. Add something like:

if (random_no(5) != 0)
	return

at the beginning of this function.


> diff --git a/tests/ubi-tests/Makefile b/tests/ubi-tests/Makefile
> index 2c47a9f..ba8e7e0 100644
> --- a/tests/ubi-tests/Makefile
> +++ b/tests/ubi-tests/Makefile
> @@ -8,7 +8,8 @@ LIBS = libubi
>  TARGETS=io_update volrefcnt integ io_paral io_read io_basic \
>            mkvol_basic mkvol_bad mkvol_paral rsvol
>  
> -CFLAGS += -I$(LIBUBI_HEADER_PATH) -I $(KERNELHDR) -lpthread
> +CFLAGS += -I$(LIBUBI_HEADER_PATH) -I$(KERNELHDR) -lpthread
> +LDFLAGS += -lpthread

This change seems to have nothing to do with integck - please, kill this
chunk.

-- 
Best Regards,
Artem Bityutskiy

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]