[PATCH v4 0/3] ceph: don't NULL terminate virtual xattrs

[PATCH v4 0/3] ceph: don't NULL terminate virtual xattrs

[Jeff Layton]

v4: resurrect snprintf_noterm as static function that uses a
    fixed-size intermediate buffer.
    Return -E2BIG and WARN if the formatted string exceeds temp buffer.
    make getxattr_cb callbacks return ssize_t.
v3: switch to using an intermediate buffer for snprintf destination
    add patch to fix ceph_vxattrcb_layout return value
v2: drop bogus EXPORT_SYMBOL of static function

This is the 4th posting of this patchset. In this variant, we add a new
variatic static function that uses an internal buffer and calls
vsnprintf to do the formatting, and then memcpys the result into the
buffer. This also adds a bit of type-sanity cleanup of the vxattr
handling in general.

Most of the rationale for this set is in the description of the last
patch of the series.

Jeff Layton (3):
  ceph: make getxattr_cb return ssize_t
  ceph: return -ERANGE if virtual xattr value didn't fit in buffer
  ceph: don't NULL terminate virtual xattrs

 fs/ceph/xattr.c | 182 ++++++++++++++++++++++++++++--------------------
 1 file changed, 108 insertions(+), 74 deletions(-)

-- 
2.21.0

[PATCH v4 1/3] ceph: make getxattr_cb return ssize_t

[Jeff Layton]

The getxattr_cb functions return size_t, which is unsigned and then
cast that value to int and then ssize_t before returning it. While all
of this works, it relies on implicit casting rules for signed/unsigned
conversions.

Change getxattr_cb to return ssize_t to better conform with what the
caller actually wants. Also, remove some suspicious casts.

Signed-off-by: Jeff Layton <jlayton@kernel.org>
---
 fs/ceph/xattr.c | 90 ++++++++++++++++++++++++-------------------------
 1 file changed, 45 insertions(+), 45 deletions(-)

diff --git a/fs/ceph/xattr.c b/fs/ceph/xattr.c
index 6621d27e64f5..e90e19e9660b 100644
--- a/fs/ceph/xattr.c
+++ b/fs/ceph/xattr.c
@@ -32,8 +32,8 @@ static bool ceph_is_valid_xattr(const char *name)
 struct ceph_vxattr {
 	char *name;
 	size_t name_size;	/* strlen(name) + 1 (for '\0') */
-	size_t (*getxattr_cb)(struct ceph_inode_info *ci, char *val,
-			      size_t size);
+	ssize_t (*getxattr_cb)(struct ceph_inode_info *ci, char *val,
+			       size_t size);
 	bool (*exists_cb)(struct ceph_inode_info *ci);
 	unsigned int flags;
 };
@@ -52,8 +52,8 @@ static bool ceph_vxattrcb_layout_exists(struct ceph_inode_info *ci)
 		rcu_dereference_raw(fl->pool_ns) != NULL);
 }
 
-static size_t ceph_vxattrcb_layout(struct ceph_inode_info *ci, char *val,
-				   size_t size)
+static ssize_t ceph_vxattrcb_layout(struct ceph_inode_info *ci, char *val,
+				    size_t size)
 {
 	struct ceph_fs_client *fsc = ceph_sb_to_client(ci->vfs_inode.i_sb);
 	struct ceph_osd_client *osdc = &fsc->client->osdc;
@@ -80,7 +80,7 @@ static size_t ceph_vxattrcb_layout(struct ceph_inode_info *ci, char *val,
 		len = snprintf(buf, sizeof(buf),
 		"stripe_unit=%u stripe_count=%u object_size=%u pool=%lld",
 		ci->i_layout.stripe_unit, ci->i_layout.stripe_count,
-	        ci->i_layout.object_size, (unsigned long long)pool);
+		ci->i_layout.object_size, pool);
 		total_len = len;
 	}
 
@@ -112,28 +112,28 @@ static size_t ceph_vxattrcb_layout(struct ceph_inode_info *ci, char *val,
 	return ret;
 }
 
-static size_t ceph_vxattrcb_layout_stripe_unit(struct ceph_inode_info *ci,
-					       char *val, size_t size)
+static ssize_t ceph_vxattrcb_layout_stripe_unit(struct ceph_inode_info *ci,
+						char *val, size_t size)
 {
 	return snprintf(val, size, "%u", ci->i_layout.stripe_unit);
 }
 
-static size_t ceph_vxattrcb_layout_stripe_count(struct ceph_inode_info *ci,
-						char *val, size_t size)
+static ssize_t ceph_vxattrcb_layout_stripe_count(struct ceph_inode_info *ci,
+						 char *val, size_t size)
 {
 	return snprintf(val, size, "%u", ci->i_layout.stripe_count);
 }
 
-static size_t ceph_vxattrcb_layout_object_size(struct ceph_inode_info *ci,
-					       char *val, size_t size)
+static ssize_t ceph_vxattrcb_layout_object_size(struct ceph_inode_info *ci,
+						char *val, size_t size)
 {
 	return snprintf(val, size, "%u", ci->i_layout.object_size);
 }
 
-static size_t ceph_vxattrcb_layout_pool(struct ceph_inode_info *ci,
-					char *val, size_t size)
+static ssize_t ceph_vxattrcb_layout_pool(struct ceph_inode_info *ci,
+					 char *val, size_t size)
 {
-	int ret;
+	ssize_t ret;
 	struct ceph_fs_client *fsc = ceph_sb_to_client(ci->vfs_inode.i_sb);
 	struct ceph_osd_client *osdc = &fsc->client->osdc;
 	s64 pool = ci->i_layout.pool_id;
@@ -144,18 +144,18 @@ static size_t ceph_vxattrcb_layout_pool(struct ceph_inode_info *ci,
 	if (pool_name)
 		ret = snprintf(val, size, "%s", pool_name);
 	else
-		ret = snprintf(val, size, "%lld", (unsigned long long)pool);
+		ret = snprintf(val, size, "%lld", pool);
 	up_read(&osdc->lock);
 	return ret;
 }
 
-static size_t ceph_vxattrcb_layout_pool_namespace(struct ceph_inode_info *ci,
-						  char *val, size_t size)
+static ssize_t ceph_vxattrcb_layout_pool_namespace(struct ceph_inode_info *ci,
+						   char *val, size_t size)
 {
 	int ret = 0;
 	struct ceph_string *ns = ceph_try_get_string(ci->i_layout.pool_ns);
 	if (ns) {
-		ret = snprintf(val, size, "%.*s", (int)ns->len, ns->str);
+		ret = snprintf(val, size, "%.*s", ns->len, ns->str);
 		ceph_put_string(ns);
 	}
 	return ret;
@@ -163,50 +163,50 @@ static size_t ceph_vxattrcb_layout_pool_namespace(struct ceph_inode_info *ci,
 
 /* directories */
 
-static size_t ceph_vxattrcb_dir_entries(struct ceph_inode_info *ci, char *val,
-					size_t size)
+static ssize_t ceph_vxattrcb_dir_entries(struct ceph_inode_info *ci, char *val,
+					 size_t size)
 {
 	return snprintf(val, size, "%lld", ci->i_files + ci->i_subdirs);
 }
 
-static size_t ceph_vxattrcb_dir_files(struct ceph_inode_info *ci, char *val,
-				      size_t size)
+static ssize_t ceph_vxattrcb_dir_files(struct ceph_inode_info *ci, char *val,
+				       size_t size)
 {
 	return snprintf(val, size, "%lld", ci->i_files);
 }
 
-static size_t ceph_vxattrcb_dir_subdirs(struct ceph_inode_info *ci, char *val,
-					size_t size)
+static ssize_t ceph_vxattrcb_dir_subdirs(struct ceph_inode_info *ci, char *val,
+					 size_t size)
 {
 	return snprintf(val, size, "%lld", ci->i_subdirs);
 }
 
-static size_t ceph_vxattrcb_dir_rentries(struct ceph_inode_info *ci, char *val,
-					 size_t size)
+static ssize_t ceph_vxattrcb_dir_rentries(struct ceph_inode_info *ci, char *val,
+					  size_t size)
 {
 	return snprintf(val, size, "%lld", ci->i_rfiles + ci->i_rsubdirs);
 }
 
-static size_t ceph_vxattrcb_dir_rfiles(struct ceph_inode_info *ci, char *val,
-				       size_t size)
+static ssize_t ceph_vxattrcb_dir_rfiles(struct ceph_inode_info *ci, char *val,
+					size_t size)
 {
 	return snprintf(val, size, "%lld", ci->i_rfiles);
 }
 
-static size_t ceph_vxattrcb_dir_rsubdirs(struct ceph_inode_info *ci, char *val,
-					 size_t size)
+static ssize_t ceph_vxattrcb_dir_rsubdirs(struct ceph_inode_info *ci, char *val,
+					  size_t size)
 {
 	return snprintf(val, size, "%lld", ci->i_rsubdirs);
 }
 
-static size_t ceph_vxattrcb_dir_rbytes(struct ceph_inode_info *ci, char *val,
-				       size_t size)
+static ssize_t ceph_vxattrcb_dir_rbytes(struct ceph_inode_info *ci, char *val,
+					size_t size)
 {
 	return snprintf(val, size, "%lld", ci->i_rbytes);
 }
 
-static size_t ceph_vxattrcb_dir_rctime(struct ceph_inode_info *ci, char *val,
-				       size_t size)
+static ssize_t ceph_vxattrcb_dir_rctime(struct ceph_inode_info *ci, char *val,
+					size_t size)
 {
 	return snprintf(val, size, "%lld.%09ld", ci->i_rctime.tv_sec,
 			ci->i_rctime.tv_nsec);
@@ -218,8 +218,8 @@ static bool ceph_vxattrcb_dir_pin_exists(struct ceph_inode_info *ci)
 	return ci->i_dir_pin != -ENODATA;
 }
 
-static size_t ceph_vxattrcb_dir_pin(struct ceph_inode_info *ci, char *val,
-                                    size_t size)
+static ssize_t ceph_vxattrcb_dir_pin(struct ceph_inode_info *ci, char *val,
+				     size_t size)
 {
 	return snprintf(val, size, "%d", (int)ci->i_dir_pin);
 }
@@ -238,21 +238,21 @@ static bool ceph_vxattrcb_quota_exists(struct ceph_inode_info *ci)
 	return ret;
 }
 
-static size_t ceph_vxattrcb_quota(struct ceph_inode_info *ci, char *val,
-				  size_t size)
+static ssize_t ceph_vxattrcb_quota(struct ceph_inode_info *ci, char *val,
+				   size_t size)
 {
 	return snprintf(val, size, "max_bytes=%llu max_files=%llu",
 			ci->i_max_bytes, ci->i_max_files);
 }
 
-static size_t ceph_vxattrcb_quota_max_bytes(struct ceph_inode_info *ci,
-					    char *val, size_t size)
+static ssize_t ceph_vxattrcb_quota_max_bytes(struct ceph_inode_info *ci,
+					     char *val, size_t size)
 {
 	return snprintf(val, size, "%llu", ci->i_max_bytes);
 }
 
-static size_t ceph_vxattrcb_quota_max_files(struct ceph_inode_info *ci,
-					    char *val, size_t size)
+static ssize_t ceph_vxattrcb_quota_max_files(struct ceph_inode_info *ci,
+					     char *val, size_t size)
 {
 	return snprintf(val, size, "%llu", ci->i_max_files);
 }
@@ -263,8 +263,8 @@ static bool ceph_vxattrcb_snap_btime_exists(struct ceph_inode_info *ci)
 	return (ci->i_snap_btime.tv_sec != 0 || ci->i_snap_btime.tv_nsec != 0);
 }
 
-static size_t ceph_vxattrcb_snap_btime(struct ceph_inode_info *ci, char *val,
-				       size_t size)
+static ssize_t ceph_vxattrcb_snap_btime(struct ceph_inode_info *ci, char *val,
+					size_t size)
 {
 	return snprintf(val, size, "%lld.%09ld", ci->i_snap_btime.tv_sec,
 			ci->i_snap_btime.tv_nsec);
@@ -791,7 +791,7 @@ ssize_t __ceph_getxattr(struct inode *inode, const char *name, void *value,
 	struct ceph_inode_xattr *xattr;
 	struct ceph_vxattr *vxattr = NULL;
 	int req_mask;
-	int err;
+	ssize_t err;
 
 	/* let's see if a virtual xattr was requested */
 	vxattr = ceph_match_vxattr(inode, name);
-- 
2.21.0

[PATCH v4 2/3] ceph: return -ERANGE if virtual xattr value didn't fit in buffer

[Jeff Layton]

The getxattr manpage states that we should return ERANGE if the
destination buffer size is too small to hold the value.
ceph_vxattrcb_layout does this internally, but we should be doing
this for all vxattrs.

Fix the only caller of getxattr_cb to check the returned size
against the buffer length and return -ERANGE if it doesn't fit.
Drop the same check in ceph_vxattrcb_layout and just rely on the
caller to handle it.

Signed-off-by: Jeff Layton <jlayton@kernel.org>
---
 fs/ceph/xattr.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/fs/ceph/xattr.c b/fs/ceph/xattr.c
index e90e19e9660b..9b77dca0b786 100644
--- a/fs/ceph/xattr.c
+++ b/fs/ceph/xattr.c
@@ -63,7 +63,7 @@ static ssize_t ceph_vxattrcb_layout(struct ceph_inode_info *ci, char *val,
 	const char *ns_field = " pool_namespace=";
 	char buf[128];
 	size_t len, total_len = 0;
-	int ret;
+	ssize_t ret;
 
 	pool_ns = ceph_try_get_string(ci->i_layout.pool_ns);
 
@@ -87,11 +87,8 @@ static ssize_t ceph_vxattrcb_layout(struct ceph_inode_info *ci, char *val,
 	if (pool_ns)
 		total_len += strlen(ns_field) + pool_ns->len;
 
-	if (!size) {
-		ret = total_len;
-	} else if (total_len > size) {
-		ret = -ERANGE;
-	} else {
+	ret = total_len;
+	if (size >= total_len) {
 		memcpy(val, buf, len);
 		ret = len;
 		if (pool_name) {
@@ -803,8 +800,11 @@ ssize_t __ceph_getxattr(struct inode *inode, const char *name, void *value,
 		if (err)
 			return err;
 		err = -ENODATA;
-		if (!(vxattr->exists_cb && !vxattr->exists_cb(ci)))
+		if (!(vxattr->exists_cb && !vxattr->exists_cb(ci))) {
 			err = vxattr->getxattr_cb(ci, value, size);
+			if (size && size < err)
+				err = -ERANGE;
+		}
 		return err;
 	}
 
-- 
2.21.0

[PATCH v4 3/3] ceph: don't NULL terminate virtual xattrs

[Jeff Layton]

The convention with xattrs is to not store the termination with string
data, given that it returns the length. This is how setfattr/getfattr
operate.

Most of ceph's virtual xattr routines use snprintf to plop the string
directly into the destination buffer, but snprintf always NULL
terminates the string. This means that if we send the kernel a buffer
that is the exact length needed to hold the string, it'll end up
truncated.

Add a ceph_fmt_xattr helper function to format the string into an
on-stack buffer that is should always be large enough to hold the whole
thing and then memcpy the result into the destination buffer. If it does
turn out that the formatted string won't fit in the on-stack buffer,
then return -E2BIG and do a WARN_ONCE().

Change over most of the virtual xattr routines to use the new helper. A
couple of the xattrs are sourced from strings however, and it's
difficult to know how long they'll be. Just have those memcpy the result
in place after verifying the length.

Signed-off-by: Jeff Layton <jlayton@kernel.org>
---
 fs/ceph/xattr.c | 84 ++++++++++++++++++++++++++++++++++---------------
 1 file changed, 59 insertions(+), 25 deletions(-)

diff --git a/fs/ceph/xattr.c b/fs/ceph/xattr.c
index 9b77dca0b786..37b458a9af3a 100644
--- a/fs/ceph/xattr.c
+++ b/fs/ceph/xattr.c
@@ -109,22 +109,49 @@ static ssize_t ceph_vxattrcb_layout(struct ceph_inode_info *ci, char *val,
 	return ret;
 }
 
+/*
+ * The convention with strings in xattrs is that they should not be NULL
+ * terminated, since we're returning the length with them. snprintf always
+ * NULL terminates however, so call it on a temporary buffer and then memcpy
+ * the result into place.
+ */
+static int ceph_fmt_xattr(char *val, size_t size, const char *fmt, ...)
+{
+	int ret;
+	va_list args;
+	char buf[96]; /* NB: reevaluate size if new vxattrs are added */
+
+	va_start(args, fmt);
+	ret = vsnprintf(buf, size ? sizeof(buf) : 0, fmt, args);
+	va_end(args);
+
+	/* Sanity check */
+	if (size && ret + 1 > sizeof(buf)) {
+		WARN_ONCE(true, "Returned length too big (%d)", ret);
+		return -E2BIG;
+	}
+
+	if (ret <= size)
+		memcpy(val, buf, ret);
+	return ret;
+}
+
 static ssize_t ceph_vxattrcb_layout_stripe_unit(struct ceph_inode_info *ci,
 						char *val, size_t size)
 {
-	return snprintf(val, size, "%u", ci->i_layout.stripe_unit);
+	return ceph_fmt_xattr(val, size, "%u", ci->i_layout.stripe_unit);
 }
 
 static ssize_t ceph_vxattrcb_layout_stripe_count(struct ceph_inode_info *ci,
 						 char *val, size_t size)
 {
-	return snprintf(val, size, "%u", ci->i_layout.stripe_count);
+	return ceph_fmt_xattr(val, size, "%u", ci->i_layout.stripe_count);
 }
 
 static ssize_t ceph_vxattrcb_layout_object_size(struct ceph_inode_info *ci,
 						char *val, size_t size)
 {
-	return snprintf(val, size, "%u", ci->i_layout.object_size);
+	return ceph_fmt_xattr(val, size, "%u", ci->i_layout.object_size);
 }
 
 static ssize_t ceph_vxattrcb_layout_pool(struct ceph_inode_info *ci,
@@ -138,10 +165,13 @@ static ssize_t ceph_vxattrcb_layout_pool(struct ceph_inode_info *ci,
 
 	down_read(&osdc->lock);
 	pool_name = ceph_pg_pool_name_by_id(osdc->osdmap, pool);
-	if (pool_name)
-		ret = snprintf(val, size, "%s", pool_name);
-	else
-		ret = snprintf(val, size, "%lld", pool);
+	if (pool_name) {
+		ret = strlen(pool_name);
+		if (ret <= size)
+			memcpy(val, pool_name, ret);
+	} else {
+		ret = ceph_fmt_xattr(val, size, "%lld", pool);
+	}
 	up_read(&osdc->lock);
 	return ret;
 }
@@ -149,10 +179,13 @@ static ssize_t ceph_vxattrcb_layout_pool(struct ceph_inode_info *ci,
 static ssize_t ceph_vxattrcb_layout_pool_namespace(struct ceph_inode_info *ci,
 						   char *val, size_t size)
 {
-	int ret = 0;
+	ssize_t ret = 0;
 	struct ceph_string *ns = ceph_try_get_string(ci->i_layout.pool_ns);
+
 	if (ns) {
-		ret = snprintf(val, size, "%.*s", ns->len, ns->str);
+		ret = ns->len;
+		if (ret <= size)
+			memcpy(val, ns->str, ret);
 		ceph_put_string(ns);
 	}
 	return ret;
@@ -163,50 +196,51 @@ static ssize_t ceph_vxattrcb_layout_pool_namespace(struct ceph_inode_info *ci,
 static ssize_t ceph_vxattrcb_dir_entries(struct ceph_inode_info *ci, char *val,
 					 size_t size)
 {
-	return snprintf(val, size, "%lld", ci->i_files + ci->i_subdirs);
+	return ceph_fmt_xattr(val, size, "%lld", ci->i_files + ci->i_subdirs);
 }
 
 static ssize_t ceph_vxattrcb_dir_files(struct ceph_inode_info *ci, char *val,
 				       size_t size)
 {
-	return snprintf(val, size, "%lld", ci->i_files);
+	return ceph_fmt_xattr(val, size, "%lld", ci->i_files);
 }
 
 static ssize_t ceph_vxattrcb_dir_subdirs(struct ceph_inode_info *ci, char *val,
 					 size_t size)
 {
-	return snprintf(val, size, "%lld", ci->i_subdirs);
+	return ceph_fmt_xattr(val, size, "%lld", ci->i_subdirs);
 }
 
 static ssize_t ceph_vxattrcb_dir_rentries(struct ceph_inode_info *ci, char *val,
 					  size_t size)
 {
-	return snprintf(val, size, "%lld", ci->i_rfiles + ci->i_rsubdirs);
+	return ceph_fmt_xattr(val, size, "%lld",
+				ci->i_rfiles + ci->i_rsubdirs);
 }
 
 static ssize_t ceph_vxattrcb_dir_rfiles(struct ceph_inode_info *ci, char *val,
 					size_t size)
 {
-	return snprintf(val, size, "%lld", ci->i_rfiles);
+	return ceph_fmt_xattr(val, size, "%lld", ci->i_rfiles);
 }
 
 static ssize_t ceph_vxattrcb_dir_rsubdirs(struct ceph_inode_info *ci, char *val,
 					  size_t size)
 {
-	return snprintf(val, size, "%lld", ci->i_rsubdirs);
+	return ceph_fmt_xattr(val, size, "%lld", ci->i_rsubdirs);
 }
 
 static ssize_t ceph_vxattrcb_dir_rbytes(struct ceph_inode_info *ci, char *val,
 					size_t size)
 {
-	return snprintf(val, size, "%lld", ci->i_rbytes);
+	return ceph_fmt_xattr(val, size, "%lld", ci->i_rbytes);
 }
 
 static ssize_t ceph_vxattrcb_dir_rctime(struct ceph_inode_info *ci, char *val,
 					size_t size)
 {
-	return snprintf(val, size, "%lld.%09ld", ci->i_rctime.tv_sec,
-			ci->i_rctime.tv_nsec);
+	return ceph_fmt_xattr(val, size, "%lld.%09ld", ci->i_rctime.tv_sec,
+				ci->i_rctime.tv_nsec);
 }
 
 /* dir pin */
@@ -218,7 +252,7 @@ static bool ceph_vxattrcb_dir_pin_exists(struct ceph_inode_info *ci)
 static ssize_t ceph_vxattrcb_dir_pin(struct ceph_inode_info *ci, char *val,
 				     size_t size)
 {
-	return snprintf(val, size, "%d", (int)ci->i_dir_pin);
+	return ceph_fmt_xattr(val, size, "%d", (int)ci->i_dir_pin);
 }
 
 /* quotas */
@@ -238,20 +272,20 @@ static bool ceph_vxattrcb_quota_exists(struct ceph_inode_info *ci)
 static ssize_t ceph_vxattrcb_quota(struct ceph_inode_info *ci, char *val,
 				   size_t size)
 {
-	return snprintf(val, size, "max_bytes=%llu max_files=%llu",
-			ci->i_max_bytes, ci->i_max_files);
+	return ceph_fmt_xattr(val, size, "max_bytes=%llu max_files=%llu",
+				ci->i_max_bytes, ci->i_max_files);
 }
 
 static ssize_t ceph_vxattrcb_quota_max_bytes(struct ceph_inode_info *ci,
 					     char *val, size_t size)
 {
-	return snprintf(val, size, "%llu", ci->i_max_bytes);
+	return ceph_fmt_xattr(val, size, "%llu", ci->i_max_bytes);
 }
 
 static ssize_t ceph_vxattrcb_quota_max_files(struct ceph_inode_info *ci,
 					     char *val, size_t size)
 {
-	return snprintf(val, size, "%llu", ci->i_max_files);
+	return ceph_fmt_xattr(val, size, "%llu", ci->i_max_files);
 }
 
 /* snapshots */
@@ -263,8 +297,8 @@ static bool ceph_vxattrcb_snap_btime_exists(struct ceph_inode_info *ci)
 static ssize_t ceph_vxattrcb_snap_btime(struct ceph_inode_info *ci, char *val,
 					size_t size)
 {
-	return snprintf(val, size, "%lld.%09ld", ci->i_snap_btime.tv_sec,
-			ci->i_snap_btime.tv_nsec);
+	return ceph_fmt_xattr(val, size, "%lld.%09ld", ci->i_snap_btime.tv_sec,
+				ci->i_snap_btime.tv_nsec);
 }
 
 #define CEPH_XATTR_NAME(_type, _name)	XATTR_CEPH_PREFIX #_type "." #_name
-- 
2.21.0

Re: [PATCH v4 3/3] ceph: don't NULL terminate virtual xattrs

[Yan, Zheng]

On Tue, Jun 25, 2019 at 4:18 AM Jeff Layton <jlayton@kernel.org> wrote:
>
> The convention with xattrs is to not store the termination with string
> data, given that it returns the length. This is how setfattr/getfattr
> operate.
>
> Most of ceph's virtual xattr routines use snprintf to plop the string
> directly into the destination buffer, but snprintf always NULL
> terminates the string. This means that if we send the kernel a buffer
> that is the exact length needed to hold the string, it'll end up
> truncated.
>
> Add a ceph_fmt_xattr helper function to format the string into an
> on-stack buffer that is should always be large enough to hold the whole
> thing and then memcpy the result into the destination buffer. If it does
> turn out that the formatted string won't fit in the on-stack buffer,
> then return -E2BIG and do a WARN_ONCE().
>
> Change over most of the virtual xattr routines to use the new helper. A
> couple of the xattrs are sourced from strings however, and it's
> difficult to know how long they'll be. Just have those memcpy the result
> in place after verifying the length.
>
> Signed-off-by: Jeff Layton <jlayton@kernel.org>
> ---
>  fs/ceph/xattr.c | 84 ++++++++++++++++++++++++++++++++++---------------
>  1 file changed, 59 insertions(+), 25 deletions(-)
>
> diff --git a/fs/ceph/xattr.c b/fs/ceph/xattr.c
> index 9b77dca0b786..37b458a9af3a 100644
> --- a/fs/ceph/xattr.c
> +++ b/fs/ceph/xattr.c
> @@ -109,22 +109,49 @@ static ssize_t ceph_vxattrcb_layout(struct ceph_inode_info *ci, char *val,
>         return ret;
>  }
>
> +/*
> + * The convention with strings in xattrs is that they should not be NULL
> + * terminated, since we're returning the length with them. snprintf always
> + * NULL terminates however, so call it on a temporary buffer and then memcpy
> + * the result into place.
> + */
> +static int ceph_fmt_xattr(char *val, size_t size, const char *fmt, ...)
> +{
> +       int ret;
> +       va_list args;
> +       char buf[96]; /* NB: reevaluate size if new vxattrs are added */
> +
> +       va_start(args, fmt);
> +       ret = vsnprintf(buf, size ? sizeof(buf) : 0, fmt, args);
> +       va_end(args);
> +
> +       /* Sanity check */
> +       if (size && ret + 1 > sizeof(buf)) {
> +               WARN_ONCE(true, "Returned length too big (%d)", ret);
> +               return -E2BIG;
> +       }
> +
> +       if (ret <= size)
> +               memcpy(val, buf, ret);
> +       return ret;
> +}
> +
>  static ssize_t ceph_vxattrcb_layout_stripe_unit(struct ceph_inode_info *ci,
>                                                 char *val, size_t size)
>  {
> -       return snprintf(val, size, "%u", ci->i_layout.stripe_unit);
> +       return ceph_fmt_xattr(val, size, "%u", ci->i_layout.stripe_unit);
>  }
>
>  static ssize_t ceph_vxattrcb_layout_stripe_count(struct ceph_inode_info *ci,
>                                                  char *val, size_t size)
>  {
> -       return snprintf(val, size, "%u", ci->i_layout.stripe_count);
> +       return ceph_fmt_xattr(val, size, "%u", ci->i_layout.stripe_count);
>  }
>
>  static ssize_t ceph_vxattrcb_layout_object_size(struct ceph_inode_info *ci,
>                                                 char *val, size_t size)
>  {
> -       return snprintf(val, size, "%u", ci->i_layout.object_size);
> +       return ceph_fmt_xattr(val, size, "%u", ci->i_layout.object_size);
>  }
>
>  static ssize_t ceph_vxattrcb_layout_pool(struct ceph_inode_info *ci,
> @@ -138,10 +165,13 @@ static ssize_t ceph_vxattrcb_layout_pool(struct ceph_inode_info *ci,
>
>         down_read(&osdc->lock);
>         pool_name = ceph_pg_pool_name_by_id(osdc->osdmap, pool);
> -       if (pool_name)
> -               ret = snprintf(val, size, "%s", pool_name);
> -       else
> -               ret = snprintf(val, size, "%lld", pool);
> +       if (pool_name) {
> +               ret = strlen(pool_name);
> +               if (ret <= size)
> +                       memcpy(val, pool_name, ret);
> +       } else {
> +               ret = ceph_fmt_xattr(val, size, "%lld", pool);
> +       }
>         up_read(&osdc->lock);
>         return ret;
>  }
> @@ -149,10 +179,13 @@ static ssize_t ceph_vxattrcb_layout_pool(struct ceph_inode_info *ci,
>  static ssize_t ceph_vxattrcb_layout_pool_namespace(struct ceph_inode_info *ci,
>                                                    char *val, size_t size)
>  {
> -       int ret = 0;
> +       ssize_t ret = 0;
>         struct ceph_string *ns = ceph_try_get_string(ci->i_layout.pool_ns);
> +
>         if (ns) {
> -               ret = snprintf(val, size, "%.*s", ns->len, ns->str);
> +               ret = ns->len;
> +               if (ret <= size)
> +                       memcpy(val, ns->str, ret);
>                 ceph_put_string(ns);
>         }
>         return ret;
> @@ -163,50 +196,51 @@ static ssize_t ceph_vxattrcb_layout_pool_namespace(struct ceph_inode_info *ci,
>  static ssize_t ceph_vxattrcb_dir_entries(struct ceph_inode_info *ci, char *val,
>                                          size_t size)
>  {
> -       return snprintf(val, size, "%lld", ci->i_files + ci->i_subdirs);
> +       return ceph_fmt_xattr(val, size, "%lld", ci->i_files + ci->i_subdirs);
>  }
>
>  static ssize_t ceph_vxattrcb_dir_files(struct ceph_inode_info *ci, char *val,
>                                        size_t size)
>  {
> -       return snprintf(val, size, "%lld", ci->i_files);
> +       return ceph_fmt_xattr(val, size, "%lld", ci->i_files);
>  }
>
>  static ssize_t ceph_vxattrcb_dir_subdirs(struct ceph_inode_info *ci, char *val,
>                                          size_t size)
>  {
> -       return snprintf(val, size, "%lld", ci->i_subdirs);
> +       return ceph_fmt_xattr(val, size, "%lld", ci->i_subdirs);
>  }
>
>  static ssize_t ceph_vxattrcb_dir_rentries(struct ceph_inode_info *ci, char *val,
>                                           size_t size)
>  {
> -       return snprintf(val, size, "%lld", ci->i_rfiles + ci->i_rsubdirs);
> +       return ceph_fmt_xattr(val, size, "%lld",
> +                               ci->i_rfiles + ci->i_rsubdirs);
>  }
>
>  static ssize_t ceph_vxattrcb_dir_rfiles(struct ceph_inode_info *ci, char *val,
>                                         size_t size)
>  {
> -       return snprintf(val, size, "%lld", ci->i_rfiles);
> +       return ceph_fmt_xattr(val, size, "%lld", ci->i_rfiles);
>  }
>
>  static ssize_t ceph_vxattrcb_dir_rsubdirs(struct ceph_inode_info *ci, char *val,
>                                           size_t size)
>  {
> -       return snprintf(val, size, "%lld", ci->i_rsubdirs);
> +       return ceph_fmt_xattr(val, size, "%lld", ci->i_rsubdirs);
>  }
>
>  static ssize_t ceph_vxattrcb_dir_rbytes(struct ceph_inode_info *ci, char *val,
>                                         size_t size)
>  {
> -       return snprintf(val, size, "%lld", ci->i_rbytes);
> +       return ceph_fmt_xattr(val, size, "%lld", ci->i_rbytes);
>  }
>
>  static ssize_t ceph_vxattrcb_dir_rctime(struct ceph_inode_info *ci, char *val,
>                                         size_t size)
>  {
> -       return snprintf(val, size, "%lld.%09ld", ci->i_rctime.tv_sec,
> -                       ci->i_rctime.tv_nsec);
> +       return ceph_fmt_xattr(val, size, "%lld.%09ld", ci->i_rctime.tv_sec,
> +                               ci->i_rctime.tv_nsec);
>  }
>
>  /* dir pin */
> @@ -218,7 +252,7 @@ static bool ceph_vxattrcb_dir_pin_exists(struct ceph_inode_info *ci)
>  static ssize_t ceph_vxattrcb_dir_pin(struct ceph_inode_info *ci, char *val,
>                                      size_t size)
>  {
> -       return snprintf(val, size, "%d", (int)ci->i_dir_pin);
> +       return ceph_fmt_xattr(val, size, "%d", (int)ci->i_dir_pin);
>  }
>
>  /* quotas */
> @@ -238,20 +272,20 @@ static bool ceph_vxattrcb_quota_exists(struct ceph_inode_info *ci)
>  static ssize_t ceph_vxattrcb_quota(struct ceph_inode_info *ci, char *val,
>                                    size_t size)
>  {
> -       return snprintf(val, size, "max_bytes=%llu max_files=%llu",
> -                       ci->i_max_bytes, ci->i_max_files);
> +       return ceph_fmt_xattr(val, size, "max_bytes=%llu max_files=%llu",
> +                               ci->i_max_bytes, ci->i_max_files);
>  }
>
>  static ssize_t ceph_vxattrcb_quota_max_bytes(struct ceph_inode_info *ci,
>                                              char *val, size_t size)
>  {
> -       return snprintf(val, size, "%llu", ci->i_max_bytes);
> +       return ceph_fmt_xattr(val, size, "%llu", ci->i_max_bytes);
>  }
>
>  static ssize_t ceph_vxattrcb_quota_max_files(struct ceph_inode_info *ci,
>                                              char *val, size_t size)
>  {
> -       return snprintf(val, size, "%llu", ci->i_max_files);
> +       return ceph_fmt_xattr(val, size, "%llu", ci->i_max_files);
>  }
>
>  /* snapshots */
> @@ -263,8 +297,8 @@ static bool ceph_vxattrcb_snap_btime_exists(struct ceph_inode_info *ci)
>  static ssize_t ceph_vxattrcb_snap_btime(struct ceph_inode_info *ci, char *val,
>                                         size_t size)
>  {
> -       return snprintf(val, size, "%lld.%09ld", ci->i_snap_btime.tv_sec,
> -                       ci->i_snap_btime.tv_nsec);
> +       return ceph_fmt_xattr(val, size, "%lld.%09ld", ci->i_snap_btime.tv_sec,
> +                               ci->i_snap_btime.tv_nsec);
>  }
>
>  #define CEPH_XATTR_NAME(_type, _name)  XATTR_CEPH_PREFIX #_type "." #_name
> --
> 2.21.0
>

series reviewed-by.

Re: [PATCH v4 3/3] ceph: don't NULL terminate virtual xattrs

[Ilya Dryomov]

On Mon, Jun 24, 2019 at 6:27 PM Jeff Layton <jlayton@kernel.org> wrote:
>
> The convention with xattrs is to not store the termination with string
> data, given that it returns the length. This is how setfattr/getfattr
> operate.
>
> Most of ceph's virtual xattr routines use snprintf to plop the string
> directly into the destination buffer, but snprintf always NULL
> terminates the string. This means that if we send the kernel a buffer
> that is the exact length needed to hold the string, it'll end up
> truncated.
>
> Add a ceph_fmt_xattr helper function to format the string into an
> on-stack buffer that is should always be large enough to hold the whole
> thing and then memcpy the result into the destination buffer. If it does
> turn out that the formatted string won't fit in the on-stack buffer,
> then return -E2BIG and do a WARN_ONCE().
>
> Change over most of the virtual xattr routines to use the new helper. A
> couple of the xattrs are sourced from strings however, and it's
> difficult to know how long they'll be. Just have those memcpy the result
> in place after verifying the length.
>
> Signed-off-by: Jeff Layton <jlayton@kernel.org>
> ---
>  fs/ceph/xattr.c | 84 ++++++++++++++++++++++++++++++++++---------------
>  1 file changed, 59 insertions(+), 25 deletions(-)
>
> diff --git a/fs/ceph/xattr.c b/fs/ceph/xattr.c
> index 9b77dca0b786..37b458a9af3a 100644
> --- a/fs/ceph/xattr.c
> +++ b/fs/ceph/xattr.c
> @@ -109,22 +109,49 @@ static ssize_t ceph_vxattrcb_layout(struct ceph_inode_info *ci, char *val,
>         return ret;
>  }
>
> +/*
> + * The convention with strings in xattrs is that they should not be NULL
> + * terminated, since we're returning the length with them. snprintf always
> + * NULL terminates however, so call it on a temporary buffer and then memcpy
> + * the result into place.
> + */
> +static int ceph_fmt_xattr(char *val, size_t size, const char *fmt, ...)
> +{
> +       int ret;
> +       va_list args;
> +       char buf[96]; /* NB: reevaluate size if new vxattrs are added */
> +
> +       va_start(args, fmt);
> +       ret = vsnprintf(buf, size ? sizeof(buf) : 0, fmt, args);
> +       va_end(args);
> +
> +       /* Sanity check */
> +       if (size && ret + 1 > sizeof(buf)) {
> +               WARN_ONCE(true, "Returned length too big (%d)", ret);
> +               return -E2BIG;
> +       }
> +
> +       if (ret <= size)
> +               memcpy(val, buf, ret);
> +       return ret;
> +}

Nit: perhaps check size at the top and bail early instead of checking
it at every step?

Thanks,

                Ilya

Re: [PATCH v4 3/3] ceph: don't NULL terminate virtual xattrs

[Jeff Layton]

On Tue, 2019-06-25 at 16:35 +0200, Ilya Dryomov wrote:
> On Mon, Jun 24, 2019 at 6:27 PM Jeff Layton <jlayton@kernel.org> wrote:
> > The convention with xattrs is to not store the termination with string
> > data, given that it returns the length. This is how setfattr/getfattr
> > operate.
> > 
> > Most of ceph's virtual xattr routines use snprintf to plop the string
> > directly into the destination buffer, but snprintf always NULL
> > terminates the string. This means that if we send the kernel a buffer
> > that is the exact length needed to hold the string, it'll end up
> > truncated.
> > 
> > Add a ceph_fmt_xattr helper function to format the string into an
> > on-stack buffer that is should always be large enough to hold the whole
> > thing and then memcpy the result into the destination buffer. If it does
> > turn out that the formatted string won't fit in the on-stack buffer,
> > then return -E2BIG and do a WARN_ONCE().
> > 
> > Change over most of the virtual xattr routines to use the new helper. A
> > couple of the xattrs are sourced from strings however, and it's
> > difficult to know how long they'll be. Just have those memcpy the result
> > in place after verifying the length.
> > 
> > Signed-off-by: Jeff Layton <jlayton@kernel.org>
> > ---
> >  fs/ceph/xattr.c | 84 ++++++++++++++++++++++++++++++++++---------------
> >  1 file changed, 59 insertions(+), 25 deletions(-)
> > 
> > diff --git a/fs/ceph/xattr.c b/fs/ceph/xattr.c
> > index 9b77dca0b786..37b458a9af3a 100644
> > --- a/fs/ceph/xattr.c
> > +++ b/fs/ceph/xattr.c
> > @@ -109,22 +109,49 @@ static ssize_t ceph_vxattrcb_layout(struct ceph_inode_info *ci, char *val,
> >         return ret;
> >  }
> > 
> > +/*
> > + * The convention with strings in xattrs is that they should not be NULL
> > + * terminated, since we're returning the length with them. snprintf always
> > + * NULL terminates however, so call it on a temporary buffer and then memcpy
> > + * the result into place.
> > + */
> > +static int ceph_fmt_xattr(char *val, size_t size, const char *fmt, ...)
> > +{
> > +       int ret;
> > +       va_list args;
> > +       char buf[96]; /* NB: reevaluate size if new vxattrs are added */
> > +
> > +       va_start(args, fmt);
> > +       ret = vsnprintf(buf, size ? sizeof(buf) : 0, fmt, args);
> > +       va_end(args);
> > +
> > +       /* Sanity check */
> > +       if (size && ret + 1 > sizeof(buf)) {
> > +               WARN_ONCE(true, "Returned length too big (%d)", ret);
> > +               return -E2BIG;
> > +       }
> > +
> > +       if (ret <= size)
> > +               memcpy(val, buf, ret);
> > +       return ret;
> > +}
> 
> Nit: perhaps check size at the top and bail early instead of checking
> it at every step?
> 
> Thanks,
> 
>                 Ilya

We don't know how much space we'll need until vsnprintf is called. Note
that both of these checks involve "ret", and that isn't set until
vsnprintf returns.
-- 
Jeff Layton <jlayton@kernel.org>