Re: [PATCH v3 3/7] fsstress: add operation for setting xattrs on files and directories

[Eryu Guan <guaneryu@gmail.com>]

On Thu, Apr 04, 2019 at 05:30:18PM +0100, fdmanana@kernel.org wrote:
> From: Filipe Manana <fdmanana@suse.com>
> 
> Currently fsstress does not exercise creating, reading or deleting xattrs
> on files or directories. This change adds support for setting xattrs on
> files and directories, using only the xattr user namespace (the other
> namespaces are not general purpose and are used for security, capabilities,
> ACLs, etc). This adds a counter for each file entry structure that keeps
> track of the number of xattrs set for the file entry, and each new xattr
> has a name that includes the counter's value (example: "user.x4").
> Values for the xattrs have at most 100 bytes, which is more than the
> maximum size supported for all major filesystems.
> 
> Signed-off-by: Filipe Manana <fdmanana@suse.com>

I've went through the xattr patches, and they look good to me overall,
thanks! Just some minor comments in this patch below.

> ---
> 
> V2: Use a different name for the operation (setfattr instead of the already
>     taken setxattr) and make use of the helper functions for opening and
>     closing files or directories, introduced in the first patch of this
>     series.
> V3: Simplified implementation to not need to open a file descriptor and
>     use a path string instead.
> 
>  ltp/fsstress.c | 140 +++++++++++++++++++++++++++++++++++++++++++++++++++++----
>  1 file changed, 131 insertions(+), 9 deletions(-)
> 
> diff --git a/ltp/fsstress.c b/ltp/fsstress.c
> index 3ec19143..d1db19da 100644
> --- a/ltp/fsstress.c
> +++ b/ltp/fsstress.c
> @@ -27,6 +27,7 @@
>  io_context_t	io_ctx;
>  #endif
>  #include <sys/syscall.h>
> +#include <sys/xattr.h>
>  
>  #ifndef FS_IOC_GETFLAGS
>  #define FS_IOC_GETFLAGS                 _IOR('f', 1, long)
> @@ -84,6 +85,7 @@ typedef enum {
>  	OP_RESVSP,
>  	OP_RMDIR,
>  	OP_SETATTR,
> +	OP_SETFATTR,
>  	OP_SETXATTR,

I think it'd be better to document above three *ATTR operations in
comments.

>  	OP_SPLICE,
>  	OP_STAT,
> @@ -110,6 +112,7 @@ typedef struct opdesc {
>  typedef struct fent {
>  	int	id;
>  	int	parent;
> +	int	xattr_counter;
>  } fent_t;
>  
>  typedef struct flist {
> @@ -156,6 +159,8 @@ struct print_string {
>  #define	MAXFSIZE	((1ULL << 63) - 1ULL)
>  #define	MAXFSIZE32	((1ULL << 40) - 1ULL)
>  
> +#define XATTR_NAME_BUF_SIZE 18
> +
>  void	afsync_f(int, long);
>  void	allocsp_f(int, long);
>  void	aread_f(int, long);
> @@ -176,6 +181,7 @@ void	fdatasync_f(int, long);
>  void	fiemap_f(int, long);
>  void	freesp_f(int, long);
>  void	fsync_f(int, long);
> +char	*gen_random_string(int);
>  void	getattr_f(int, long);
>  void	getdents_f(int, long);
>  void	link_f(int, long);
> @@ -194,6 +200,7 @@ void	rename_f(int, long);
>  void	resvsp_f(int, long);
>  void	rmdir_f(int, long);
>  void	setattr_f(int, long);
> +void	setfattr_f(int, long);
>  void	setxattr_f(int, long);
>  void	splice_f(int, long);
>  void	stat_f(int, long);
> @@ -204,6 +211,7 @@ void	unlink_f(int, long);
>  void	unresvsp_f(int, long);
>  void	write_f(int, long);
>  void	writev_f(int, long);
> +char	*xattr_flag_to_string(int);
>  
>  opdesc_t	ops[] = {
>       /* { OP_ENUM, "name", function, freq, iswrite }, */
> @@ -245,6 +253,7 @@ opdesc_t	ops[] = {
>  	{ OP_RESVSP, "resvsp", resvsp_f, 1, 1 },
>  	{ OP_RMDIR, "rmdir", rmdir_f, 1, 1 },
>  	{ OP_SETATTR, "setattr", setattr_f, 0, 1 },
> +	{ OP_SETFATTR, "setfattr", setfattr_f, 4, 1 },

The frequency of setfattr looks a bit high to me, a buffer "write" op
has 4. Maybe 2 is more reasonable and change get/delfattr op's freq to 1?

(And rename delfattr to removefattr to match the removexattr syscall
name?)

>  	{ OP_SETXATTR, "setxattr", setxattr_f, 1, 1 },
>  	{ OP_SPLICE, "splice", splice_f, 1, 1 },
>  	{ OP_STAT, "stat", stat_f, 1, 0 },
> @@ -296,7 +305,7 @@ char		*execute_cmd = NULL;
>  int		execute_freq = 1;
>  struct print_string	flag_str = {0};
>  
> -void	add_to_flist(int, int, int);
> +void	add_to_flist(int, int, int, int);
>  void	append_pathname(pathname_t *, char *);
>  int	attr_list_path(pathname_t *, char *, const int, int, attrlist_cursor_t *);
>  int	attr_remove_path(pathname_t *, const char *, int);
> @@ -315,6 +324,7 @@ int	fent_to_name(pathname_t *, flist_t *, fent_t *);
>  void	fix_parent(int, int);
>  void	free_pathname(pathname_t *);
>  int	generate_fname(fent_t *, int, pathname_t *, int *, int *);
> +void	generate_xattr_name(int, char *);
>  int	get_fname(int, long, pathname_t *, flist_t **, fent_t **, int *);
>  void	init_pathname(pathname_t *);
>  int	lchown_path(pathname_t *, uid_t, gid_t);
> @@ -716,7 +726,7 @@ translate_flags(int flags, const char *delim,
>  }
>  
>  void
> -add_to_flist(int ft, int id, int parent)
> +add_to_flist(int ft, int id, int parent, int xattr_counter)
>  {
>  	fent_t	*fep;
>  	flist_t	*ftp;
> @@ -729,6 +739,7 @@ add_to_flist(int ft, int id, int parent)
>  	fep = &ftp->fents[ftp->nfiles++];
>  	fep->id = id;
>  	fep->parent = parent;
> +	fep->xattr_counter = xattr_counter;
>  }
>  
>  void
> @@ -1122,6 +1133,12 @@ generate_fname(fent_t *fep, int ft, pathname_t *name, int *idp, int *v)
>  	return 1;
>  }
>  
> +void generate_xattr_name(int xattr_num, char *buffer)
> +{
> +	memcpy(buffer, "user.x", 6);
> +	sprintf(buffer + 6, "%d", xattr_num);

I think it's better to pass the buffer length too and check against it
to avoid buffer overflow.

Thanks,
Eryu

> +}
> +
>  /*
>   * Get file 
>   * Input: "which" to choose the file-types eg. non-directory
> @@ -3030,7 +3047,7 @@ creat_f(int opno, long r)
>  			if (xfsctl(f.path, fd, XFS_IOC_FSSETXATTR, &a) < 0)
>  				e1 = errno;
>  		}
> -		add_to_flist(type, id, parid);
> +		add_to_flist(type, id, parid, 0);
>  		close(fd);
>  	}
>  	if (v) {
> @@ -3495,6 +3512,28 @@ fsync_f(int opno, long r)
>  	close(fd);
>  }
>  
> +char *
> +gen_random_string(int len)
> +{
> +	static const char charset[] = "0123456789"
> +		"abcdefghijklmnopqrstuvwxyz"
> +		"ABCDEFGHIJKLMNOPQRSTUVWXYZ";
> +	int i;
> +	char *s;
> +
> +	if (len == 0)
> +		return NULL;
> +
> +	s = malloc(len);
> +	if (!s)
> +		return NULL;
> +
> +	for (i = 0; i < len; i++)
> +		s[i] = charset[random() % sizeof(charset)];
> +
> +	return s;
> +}
> +
>  void
>  getattr_f(int opno, long r)
>  {
> @@ -3551,6 +3590,7 @@ link_f(int opno, long r)
>  	int		e;
>  	pathname_t	f;
>  	fent_t		*fep;
> +	fent_t		*fep_src;
>  	flist_t		*flp;
>  	int		id;
>  	pathname_t	l;
> @@ -3559,7 +3599,7 @@ link_f(int opno, long r)
>  	int		v1;
>  
>  	init_pathname(&f);
> -	if (!get_fname(FT_NOTDIR, r, &f, &flp, NULL, &v1)) {
> +	if (!get_fname(FT_NOTDIR, r, &f, &flp, &fep_src, &v1)) {
>  		if (v1)
>  			printf("%d/%d: link - no file\n", procid, opno);
>  		free_pathname(&f);
> @@ -3586,7 +3626,7 @@ link_f(int opno, long r)
>  	e = link_path(&f, &l) < 0 ? errno : 0;
>  	check_cwd();
>  	if (e == 0)
> -		add_to_flist(flp - flist, id, parid);
> +		add_to_flist(flp - flist, id, parid, fep_src->xattr_counter);
>  	if (v) {
>  		printf("%d/%d: link %s %s %d\n", procid, opno, f.path, l.path,
>  			e);
> @@ -3626,7 +3666,7 @@ mkdir_f(int opno, long r)
>  	e = mkdir_path(&f, 0777) < 0 ? errno : 0;
>  	check_cwd();
>  	if (e == 0)
> -		add_to_flist(FT_DIR, id, parid);
> +		add_to_flist(FT_DIR, id, parid, 0);
>  	if (v) {
>  		printf("%d/%d: mkdir %s %d\n", procid, opno, f.path, e);
>  		printf("%d/%d: mkdir add id=%d,parent=%d\n", procid, opno, id, parid);
> @@ -3664,7 +3704,7 @@ mknod_f(int opno, long r)
>  	e = mknod_path(&f, S_IFCHR|0444, 0) < 0 ? errno : 0;
>  	check_cwd();
>  	if (e == 0)
> -		add_to_flist(FT_DEV, id, parid);
> +		add_to_flist(FT_DEV, id, parid, 0);
>  	if (v) {
>  		printf("%d/%d: mknod %s %d\n", procid, opno, f.path, e);
>  		printf("%d/%d: mknod add id=%d,parent=%d\n", procid, opno, id, parid);
> @@ -4040,12 +4080,14 @@ rename_f(int opno, long r)
>  	e = rename_path(&f, &newf) < 0 ? errno : 0;
>  	check_cwd();
>  	if (e == 0) {
> +		int xattr_counter = fep->xattr_counter;
> +
>  		if (flp - flist == FT_DIR) {
>  			oldid = fep->id;
>  			fix_parent(oldid, id);
>  		}
>  		del_from_flist(flp - flist, fep - flp->fents);
> -		add_to_flist(flp - flist, id, parid);
> +		add_to_flist(flp - flist, id, parid, xattr_counter);
>  	}
>  	if (v) {
>  		printf("%d/%d: rename %s to %s %d\n", procid, opno, f.path,
> @@ -4168,6 +4210,76 @@ setattr_f(int opno, long r)
>  }
>  
>  void
> +setfattr_f(int opno, long r)
> +{
> +	int		e;
> +	pathname_t	f;
> +	fent_t	        *fep;
> +	int		v;
> +	int             value_len;
> +	char            name[XATTR_NAME_BUF_SIZE];
> +	char            *value = NULL;
> +	int             flag = 0;
> +	int             xattr_num;
> +
> +	init_pathname(&f);
> +	if (!get_fname(FT_REGFILE | FT_DIRm, r, &f, NULL, &fep, &v)) {
> +		if (v)
> +			printf("%d/%d: setfattr - no filename\n", procid, opno);
> +		goto out;
> +	}
> +	check_cwd();
> +
> +	if ((fep->xattr_counter > 0) && (random() % 2)) {
> +		/*
> +		 * Use an existing xattr name for replacing its value or
> +		 * create again a xattr that was previously deleted.
> +		 */
> +		xattr_num = (random() % fep->xattr_counter) + 1;
> +		if (random() % 2)
> +			flag = XATTR_REPLACE;
> +	} else {
> +		/* Use a new xattr name. */
> +		xattr_num = fep->xattr_counter + 1;
> +		/*
> +		 * Don't always use the create flag because even if our xattr
> +		 * counter is 0, we may still have xattrs associated to this
> +		 * file (this happens when xattrs are added to a file through
> +		 * one of its other hard links), so we can end up updating an
> +		 * existing xattr too.
> +		 */
> +		if (random() % 2)
> +			flag = XATTR_CREATE;
> +	}
> +
> +	/*
> +	 * The maximum supported value size depends on the filesystem
> +	 * implementation, but 100 bytes is a safe value for most filesystems
> +	 * at least.
> +	 */
> +	value_len = random() % 101;
> +	value = gen_random_string(value_len);
> +	if (!value && value_len > 0) {
> +		if (v)
> +			printf("%d/%d: setfattr - file %s failed to allocate value with %d bytes\n",
> +			       procid, opno, f.path, value_len);
> +		goto out;
> +	}
> +	generate_xattr_name(xattr_num, name);
> +
> +	e = setxattr(f.path, name, value, value_len, flag) < 0 ? errno : 0;
> +	if (e == 0)
> +		fep->xattr_counter++;
> +	if (v)
> +		printf("%d/%d: setfattr file %s name %s flag %s value length %d: %d\n",
> +		       procid, opno, f.path, name, xattr_flag_to_string(flag),
> +		       value_len, e);
> +out:
> +	free(value);
> +	free_pathname(&f);
> +}
> +
> +void
>  stat_f(int opno, long r)
>  {
>  	int		e;
> @@ -4229,7 +4341,7 @@ symlink_f(int opno, long r)
>  	e = symlink_path(val, &f) < 0 ? errno : 0;
>  	check_cwd();
>  	if (e == 0)
> -		add_to_flist(FT_SYM, id, parid);
> +		add_to_flist(FT_SYM, id, parid, 0);
>  	free(val);
>  	if (v) {
>  		printf("%d/%d: symlink %s %d\n", procid, opno, f.path, e);
> @@ -4498,3 +4610,13 @@ writev_f(int opno, long r)
>  	free_pathname(&f);
>  	close(fd);
>  }
> +
> +char *
> +xattr_flag_to_string(int flag)
> +{
> +	if (flag == XATTR_CREATE)
> +		return "create";
> +	if (flag == XATTR_REPLACE)
> +		return "replace";
> +	return "none";
> +}
> -- 
> 2.11.0
>