[PATCH v2 0/3] ceph: don't NULL terminate virtual xattr values

[PATCH v2 0/3] ceph: don't NULL terminate virtual xattr values

[Jeff Layton]

v2: drop bogus EXPORT_SYMBOL of static function

The only real difference between this set and the one I sent originally
is the removal of a spurious EXPORT_SYMBOL in the snprintf patch.

I'm mostly sending this with a wider cc list in an effort to get a
review from the maintainers of the printf code. Basically ceph needs a
snprintf variant that does not NULL terminate in order to handle its
virtual xattrs.

Joe Perches had expressed some concerns about stack usage in vsnprintf
with this, but I'm not sure I really understand the basis of that
concern. If it is problematic, then I could use suggestions as to how
best to fix that up.

----------------------------8<-----------------------------

kcephfs has several "virtual" xattrs that return strings that are
currently populated using snprintf(), which always NULL terminates the
string.

This leads to the string being truncated when we use a buffer length
acquired by calling getxattr with a 0 size first. The last character
of the string ends up being clobbered by the termination.

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

This patch makes ceph's virtual xattrs not include NULL termination
when formatting their values. In order to handle this, a new
snprintf_noterm function is added, and ceph is changed over to use
this to populate the xattr value buffer. Finally, we fix ceph to
return -ERANGE properly when the string didn't fit in the buffer.

Jeff Layton (3):
  lib/vsprintf: add snprintf_noterm
  ceph: don't NULL terminate virtual xattr strings
  ceph: return -ERANGE if virtual xattr value didn't fit in buffer

 fs/ceph/xattr.c        |  49 +++++++-------
 include/linux/kernel.h |   2 +
 lib/vsprintf.c         | 144 ++++++++++++++++++++++++++++-------------
 3 files changed, 129 insertions(+), 66 deletions(-)

-- 
2.21.0

[PATCH v2 1/3] lib/vsprintf: add snprintf_noterm

[Jeff Layton]

The getxattr interface returns a length after filling out the value
buffer, and the convention with xattrs is to not NULL terminate string
data.

CephFS implements some virtual xattrs by using snprintf to fill the
buffer, but that always NULL terminates the string. If userland sends
down a buffer that is just the right length to hold the text without
termination then we end up truncating the value.

Factor the formatting piece of vsnprintf into a separate helper
function, and have vsnprintf call that and then do the NULL termination
afterward. Then add a snprintf_noterm function that calls the new helper
to populate the string but skips the termination.

Signed-off-by: Jeff Layton <jlayton@kernel.org>
---
 include/linux/kernel.h |   2 +
 lib/vsprintf.c         | 144 ++++++++++++++++++++++++++++-------------
 2 files changed, 102 insertions(+), 44 deletions(-)

diff --git a/include/linux/kernel.h b/include/linux/kernel.h
index 2d14e21c16c0..2f305a347482 100644
--- a/include/linux/kernel.h
+++ b/include/linux/kernel.h
@@ -462,6 +462,8 @@ extern int num_to_str(char *buf, int size,
 extern __printf(2, 3) int sprintf(char *buf, const char * fmt, ...);
 extern __printf(2, 0) int vsprintf(char *buf, const char *, va_list);
 extern __printf(3, 4)
+int snprintf_noterm(char *buf, size_t size, const char *fmt, ...);
+extern __printf(3, 4)
 int snprintf(char *buf, size_t size, const char *fmt, ...);
 extern __printf(3, 0)
 int vsnprintf(char *buf, size_t size, const char *fmt, va_list args);
diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index 791b6fa36905..9360a9933bf8 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -2296,53 +2296,24 @@ set_precision(struct printf_spec *spec, int prec)
 }
 
 /**
- * vsnprintf - Format a string and place it in a buffer
+ * vsnprintf_noterm - Format a string and place it in a buffer without NULL
+ *		      terminating it
  * @buf: The buffer to place the result into
- * @size: The size of the buffer, including the trailing null space
+ * @end: The end of the buffer
  * @fmt: The format string to use
  * @args: Arguments for the format string
  *
- * This function generally follows C99 vsnprintf, but has some
- * extensions and a few limitations:
- *
- *  - ``%n`` is unsupported
- *  - ``%p*`` is handled by pointer()
- *
- * See pointer() or Documentation/core-api/printk-formats.rst for more
- * extensive description.
- *
- * **Please update the documentation in both places when making changes**
- *
- * The return value is the number of characters which would
- * be generated for the given input, excluding the trailing
- * '\0', as per ISO C99. If you want to have the exact
- * number of characters written into @buf as return value
- * (not including the trailing '\0'), use vscnprintf(). If the
- * return is greater than or equal to @size, the resulting
- * string is truncated.
- *
- * If you're not already dealing with a va_list consider using snprintf().
+ * See the documentation over vsnprintf. This function does NOT add any NULL
+ * termination to the buffer. The caller must do that if necessary.
  */
-int vsnprintf(char *buf, size_t size, const char *fmt, va_list args)
+static int vsnprintf_noterm(char *buf, char *end, const char *fmt,
+			    va_list args)
 {
 	unsigned long long num;
-	char *str, *end;
+	char *str;
 	struct printf_spec spec = {0};
 
-	/* Reject out-of-range values early.  Large positive sizes are
-	   used for unknown buffer sizes. */
-	if (WARN_ON_ONCE(size > INT_MAX))
-		return 0;
-
 	str = buf;
-	end = buf + size;
-
-	/* Make sure end is always >= buf */
-	if (end < buf) {
-		end = ((void *)-1);
-		size = end - buf;
-	}
-
 	while (*fmt) {
 		const char *old_fmt = fmt;
 		int read = format_decode(fmt, &spec);
@@ -2462,18 +2433,68 @@ int vsnprintf(char *buf, size_t size, const char *fmt, va_list args)
 			str = number(str, end, num, spec);
 		}
 	}
-
 out:
+	/* the trailing null byte doesn't count towards the total */
+	return str-buf;
+}
+
+/**
+ * vsnprintf - Format a string and place it in a buffer
+ * @buf: The buffer to place the result into
+ * @size: The size of the buffer, including the trailing null space
+ * @fmt: The format string to use
+ * @args: Arguments for the format string
+ *
+ * This function generally follows C99 vsnprintf, but has some
+ * extensions and a few limitations:
+ *
+ *  - ``%n`` is unsupported
+ *  - ``%p*`` is handled by pointer()
+ *
+ * See pointer() or Documentation/core-api/printk-formats.rst for more
+ * extensive description.
+ *
+ * **Please update the documentation in both places when making changes**
+ *
+ * The return value is the number of characters which would
+ * be generated for the given input, excluding the trailing
+ * '\0', as per ISO C99. If you want to have the exact
+ * number of characters written into @buf as return value
+ * (not including the trailing '\0'), use vscnprintf(). If the
+ * return is greater than or equal to @size, the resulting
+ * string is truncated.
+ *
+ * If you're not already dealing with a va_list consider using snprintf().
+ */
+int vsnprintf(char *buf, size_t size, const char *fmt, va_list args)
+{
+	int ret;
+	char *end;
+
+	/* Reject out-of-range values early.  Large positive sizes are
+	   used for unknown buffer sizes. */
+	if (WARN_ON_ONCE(size > INT_MAX))
+		return 0;
+
+	end = buf + size;
+
+	/* Make sure end is always >= buf */
+	if (end < buf) {
+		end = ((void *)-1);
+		size = end - buf;
+	}
+
+	ret = vsnprintf_noterm(buf, end, fmt, args);
+
+	/* NULL terminate the result */
 	if (size > 0) {
-		if (str < end)
-			*str = '\0';
+		if (ret < size)
+			buf[ret] = '\0';
 		else
-			end[-1] = '\0';
+			buf[size - 1] = '\0';
 	}
 
-	/* the trailing null byte doesn't count towards the total */
-	return str-buf;
-
+	return ret;
 }
 EXPORT_SYMBOL(vsnprintf);
 
@@ -2506,6 +2527,41 @@ int vscnprintf(char *buf, size_t size, const char *fmt, va_list args)
 }
 EXPORT_SYMBOL(vscnprintf);
 
+/**
+ * snprintf_noterm - Format a string and place it in a buffer
+ * @buf: The buffer to place the result into
+ * @size: The size of the buffer, including the trailing null space
+ * @fmt: The format string to use
+ * @...: Arguments for the format string
+ *
+ * Same as snprintf, but don't NULL terminate the result.
+ */
+int snprintf_noterm(char *buf, size_t size, const char *fmt, ...)
+{
+	va_list args;
+	int ret;
+	char *end;
+
+	/* Reject out-of-range values early.  Large positive sizes are
+	   used for unknown buffer sizes. */
+	if (WARN_ON_ONCE(size > INT_MAX))
+		return 0;
+
+	/* Make sure end is always >= buf */
+	end = buf + size;
+	if (end < buf) {
+		end = ((void *)-1);
+		size = end - buf;
+	}
+
+	va_start(args, fmt);
+	ret = vsnprintf_noterm(buf, end, fmt, args);
+	va_end(args);
+
+	return ret;
+}
+EXPORT_SYMBOL(snprintf_noterm);
+
 /**
  * snprintf - Format a string and place it in a buffer
  * @buf: The buffer to place the result into
-- 
2.21.0

[PATCH v2 2/3] ceph: don't NULL terminate virtual xattr strings

[Jeff Layton]

The convention with xattrs is to not NULL terminate string data in the
value. Have ceph use snprintf_noterm to populate virtual xattrs.

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

diff --git a/fs/ceph/xattr.c b/fs/ceph/xattr.c
index 6621d27e64f5..a1cd9613be98 100644
--- a/fs/ceph/xattr.c
+++ b/fs/ceph/xattr.c
@@ -71,13 +71,13 @@ static size_t ceph_vxattrcb_layout(struct ceph_inode_info *ci, char *val,
 	down_read(&osdc->lock);
 	pool_name = ceph_pg_pool_name_by_id(osdc->osdmap, pool);
 	if (pool_name) {
-		len = snprintf(buf, sizeof(buf),
+		len = snprintf_noterm(buf, sizeof(buf),
 		"stripe_unit=%u stripe_count=%u object_size=%u pool=",
 		ci->i_layout.stripe_unit, ci->i_layout.stripe_count,
 	        ci->i_layout.object_size);
 		total_len = len + strlen(pool_name);
 	} else {
-		len = snprintf(buf, sizeof(buf),
+		len = snprintf_noterm(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);
@@ -115,19 +115,19 @@ static size_t ceph_vxattrcb_layout(struct ceph_inode_info *ci, char *val,
 static size_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 snprintf_noterm(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)
 {
-	return snprintf(val, size, "%u", ci->i_layout.stripe_count);
+	return snprintf_noterm(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)
 {
-	return snprintf(val, size, "%u", ci->i_layout.object_size);
+	return snprintf_noterm(val, size, "%u", ci->i_layout.object_size);
 }
 
 static size_t ceph_vxattrcb_layout_pool(struct ceph_inode_info *ci,
@@ -142,9 +142,10 @@ static size_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);
+		ret = snprintf_noterm(val, size, "%s", pool_name);
 	else
-		ret = snprintf(val, size, "%lld", (unsigned long long)pool);
+		ret = snprintf_noterm(val, size, "%lld",
+					(unsigned long long)pool);
 	up_read(&osdc->lock);
 	return ret;
 }
@@ -155,7 +156,7 @@ static size_t ceph_vxattrcb_layout_pool_namespace(struct ceph_inode_info *ci,
 	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_noterm(val, size, "%.*s", (int)ns->len, ns->str);
 		ceph_put_string(ns);
 	}
 	return ret;
@@ -166,49 +167,50 @@ static size_t ceph_vxattrcb_layout_pool_namespace(struct ceph_inode_info *ci,
 static size_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 snprintf_noterm(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)
 {
-	return snprintf(val, size, "%lld", ci->i_files);
+	return snprintf_noterm(val, size, "%lld", ci->i_files);
 }
 
 static size_t ceph_vxattrcb_dir_subdirs(struct ceph_inode_info *ci, char *val,
 					size_t size)
 {
-	return snprintf(val, size, "%lld", ci->i_subdirs);
+	return snprintf_noterm(val, size, "%lld", ci->i_subdirs);
 }
 
 static size_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 snprintf_noterm(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)
 {
-	return snprintf(val, size, "%lld", ci->i_rfiles);
+	return snprintf_noterm(val, size, "%lld", ci->i_rfiles);
 }
 
 static size_t ceph_vxattrcb_dir_rsubdirs(struct ceph_inode_info *ci, char *val,
 					 size_t size)
 {
-	return snprintf(val, size, "%lld", ci->i_rsubdirs);
+	return snprintf_noterm(val, size, "%lld", ci->i_rsubdirs);
 }
 
 static size_t ceph_vxattrcb_dir_rbytes(struct ceph_inode_info *ci, char *val,
 				       size_t size)
 {
-	return snprintf(val, size, "%lld", ci->i_rbytes);
+	return snprintf_noterm(val, size, "%lld", ci->i_rbytes);
 }
 
 static size_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,
+	return snprintf_noterm(val, size, "%lld.%09ld", ci->i_rctime.tv_sec,
 			ci->i_rctime.tv_nsec);
 }
 
@@ -221,7 +223,7 @@ static bool ceph_vxattrcb_dir_pin_exists(struct ceph_inode_info *ci)
 static size_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 snprintf_noterm(val, size, "%d", (int)ci->i_dir_pin);
 }
 
 /* quotas */
@@ -241,20 +243,20 @@ static bool ceph_vxattrcb_quota_exists(struct ceph_inode_info *ci)
 static size_t ceph_vxattrcb_quota(struct ceph_inode_info *ci, char *val,
 				  size_t size)
 {
-	return snprintf(val, size, "max_bytes=%llu max_files=%llu",
+	return snprintf_noterm(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)
 {
-	return snprintf(val, size, "%llu", ci->i_max_bytes);
+	return snprintf_noterm(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)
 {
-	return snprintf(val, size, "%llu", ci->i_max_files);
+	return snprintf_noterm(val, size, "%llu", ci->i_max_files);
 }
 
 /* snapshots */
@@ -266,7 +268,7 @@ static bool ceph_vxattrcb_snap_btime_exists(struct ceph_inode_info *ci)
 static size_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,
+	return snprintf_noterm(val, size, "%lld.%09ld", ci->i_snap_btime.tv_sec,
 			ci->i_snap_btime.tv_nsec);
 }
 
-- 
2.21.0

[PATCH v2 3/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.

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

diff --git a/fs/ceph/xattr.c b/fs/ceph/xattr.c
index a1cd9613be98..e3246c27f2da 100644
--- a/fs/ceph/xattr.c
+++ b/fs/ceph/xattr.c
@@ -805,8 +805,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

Re: [PATCH v2 0/3] ceph: don't NULL terminate virtual xattr values

[Andy Shevchenko]

On Wed, Jun 19, 2019 at 12:45:25PM -0400, Jeff Layton wrote:
> v2: drop bogus EXPORT_SYMBOL of static function
> 
> The only real difference between this set and the one I sent originally
> is the removal of a spurious EXPORT_SYMBOL in the snprintf patch.
> 
> I'm mostly sending this with a wider cc list in an effort to get a
> review from the maintainers of the printf code. Basically ceph needs a
> snprintf variant that does not NULL terminate in order to handle its
> virtual xattrs.
> 

> Joe Perches had expressed some concerns about stack usage in vsnprintf
> with this, but I'm not sure I really understand the basis of that
> concern. If it is problematic, then I could use suggestions as to how
> best to fix that up.

It might be problematic, since vsnprintf() can be called recursively.

> ----------------------------8<-----------------------------
> 
> kcephfs has several "virtual" xattrs that return strings that are
> currently populated using snprintf(), which always NULL terminates the
> string.
> 
> This leads to the string being truncated when we use a buffer length
> acquired by calling getxattr with a 0 size first. The last character
> of the string ends up being clobbered by the termination.

So, then don't use snprintf() for this, simple memcpy() designed for that kind
of things.

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

Fine.

> This patch makes ceph's virtual xattrs not include NULL termination
> when formatting their values. In order to handle this, a new
> snprintf_noterm function is added, and ceph is changed over to use
> this to populate the xattr value buffer.

In terms of vsnprintf(), and actually compiler point of view, it's not a string
anymore, it's a text-based data.

Personally, I don't see an advantage of a deep intrusion into vsnprintf().
The wrapper can be made to achieve this w/o touching the generic code. Thus,
you can quickly and cleanly fix the issue, while discussing this with wider
audience.

> Finally, we fix ceph to
> return -ERANGE properly when the string didn't fit in the buffer.
> 
> Jeff Layton (3):
>   lib/vsprintf: add snprintf_noterm
>   ceph: don't NULL terminate virtual xattr strings
>   ceph: return -ERANGE if virtual xattr value didn't fit in buffer
> 
>  fs/ceph/xattr.c        |  49 +++++++-------
>  include/linux/kernel.h |   2 +
>  lib/vsprintf.c         | 144 ++++++++++++++++++++++++++++-------------
>  3 files changed, 129 insertions(+), 66 deletions(-)
> 
> -- 
> 2.21.0
> 

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 0/3] ceph: don't NULL terminate virtual xattr values

[Jeff Layton]

On Thu, 2019-06-20 at 13:24 +0300, Andy Shevchenko wrote:
> On Wed, Jun 19, 2019 at 12:45:25PM -0400, Jeff Layton wrote:
> > v2: drop bogus EXPORT_SYMBOL of static function
> > 
> > The only real difference between this set and the one I sent originally
> > is the removal of a spurious EXPORT_SYMBOL in the snprintf patch.
> > 
> > I'm mostly sending this with a wider cc list in an effort to get a
> > review from the maintainers of the printf code. Basically ceph needs a
> > snprintf variant that does not NULL terminate in order to handle its
> > virtual xattrs.
> > 
> > Joe Perches had expressed some concerns about stack usage in vsnprintf
> > with this, but I'm not sure I really understand the basis of that
> > concern. If it is problematic, then I could use suggestions as to how
> > best to fix that up.
> 
> It might be problematic, since vsnprintf() can be called recursively.
> 

So the concern is that we'd have extra call/ret activity in the stack?
That seems like a lot of hand-wringing over very little, but ok if so.

> > ----------------------------8<-----------------------------
> > 
> > kcephfs has several "virtual" xattrs that return strings that are
> > currently populated using snprintf(), which always NULL terminates the
> > string.
> > 
> > This leads to the string being truncated when we use a buffer length
> > acquired by calling getxattr with a 0 size first. The last character
> > of the string ends up being clobbered by the termination.
> 
> So, then don't use snprintf() for this, simple memcpy() designed for that kind
> of things.
> 

memcpy from what? For many of these xattrs, we need to format integer
data into strings. I could roll my own routine to do this formatting,
but that's sort of what sprintf and its variants are for and I'd rather
not reimplement all of it from scratch.

> > The convention with xattrs is to not store the termination with string
> > data, given that we have the length. This is how setfattr/getfattr
> > operate.
> 
> Fine.
> 
> > This patch makes ceph's virtual xattrs not include NULL termination
> > when formatting their values. In order to handle this, a new
> > snprintf_noterm function is added, and ceph is changed over to use
> > this to populate the xattr value buffer.
> 
> In terms of vsnprintf(), and actually compiler point of view, it's not a string
> anymore, it's a text-based data.
> 
> Personally, I don't see an advantage of a deep intrusion into vsnprintf().
> The wrapper can be made to achieve this w/o touching the generic code. Thus,
> you can quickly and cleanly fix the issue, while discussing this with wider
> audience.
> 

Sorry, if I'm being dense but I'm not sure I follow here.

Are you suggesting I should just copy/paste most of vsnprintf into a new
function that just leaves off the termination at the end, and leave the
original alone? That seems like a bit of a waste, but if that's the
consensus then ok.

> > Finally, we fix ceph to
> > return -ERANGE properly when the string didn't fit in the buffer.
> > 
> > Jeff Layton (3):
> >   lib/vsprintf: add snprintf_noterm
> >   ceph: don't NULL terminate virtual xattr strings
> >   ceph: return -ERANGE if virtual xattr value didn't fit in buffer
> > 
> >  fs/ceph/xattr.c        |  49 +++++++-------
> >  include/linux/kernel.h |   2 +
> >  lib/vsprintf.c         | 144 ++++++++++++++++++++++++++++-------------
> >  3 files changed, 129 insertions(+), 66 deletions(-)
> > 
> > -- 
> > 2.21.0
> > 

-- 
Jeff Layton <jlayton@kernel.org>

Re: [PATCH v2 0/3] ceph: don't NULL terminate virtual xattr values

[Geert Uytterhoeven]

Hi Jeff,

On Thu, Jun 20, 2019 at 1:41 PM Jeff Layton <jlayton@kernel.org> wrote:
> On Thu, 2019-06-20 at 13:24 +0300, Andy Shevchenko wrote:
> > On Wed, Jun 19, 2019 at 12:45:25PM -0400, Jeff Layton wrote:
> > > v2: drop bogus EXPORT_SYMBOL of static function
> > >
> > > The only real difference between this set and the one I sent originally
> > > is the removal of a spurious EXPORT_SYMBOL in the snprintf patch.
> > >
> > > I'm mostly sending this with a wider cc list in an effort to get a
> > > review from the maintainers of the printf code. Basically ceph needs a
> > > snprintf variant that does not NULL terminate in order to handle its
> > > virtual xattrs.
> > >
> > > Joe Perches had expressed some concerns about stack usage in vsnprintf
> > > with this, but I'm not sure I really understand the basis of that
> > > concern. If it is problematic, then I could use suggestions as to how
> > > best to fix that up.
> >
> > It might be problematic, since vsnprintf() can be called recursively.
> >
>
> So the concern is that we'd have extra call/ret activity in the stack?
> That seems like a lot of hand-wringing over very little, but ok if so.
>
> > > ----------------------------8<-----------------------------
> > >
> > > kcephfs has several "virtual" xattrs that return strings that are
> > > currently populated using snprintf(), which always NULL terminates the
> > > string.
> > >
> > > This leads to the string being truncated when we use a buffer length
> > > acquired by calling getxattr with a 0 size first. The last character
> > > of the string ends up being clobbered by the termination.
> >
> > So, then don't use snprintf() for this, simple memcpy() designed for that kind
> > of things.
> >
>
> memcpy from what? For many of these xattrs, we need to format integer
> data into strings. I could roll my own routine to do this formatting,
> but that's sort of what sprintf and its variants are for and I'd rather
> not reimplement all of it from scratch.

snprintf() to a temporary buffer, and memcpy() to the final destination.
These are all fairly small buffers (most are single integer values),
so the overhead should be minimal, right?

In fact the two largest strings are already formatted in a temporary
buffer, so there is no reason ceph_vxattrcb_layout() cannot just use
snprintf() now.

Or perhaps this can use the existing seq_*() interface in some form?

BTW, while at it, please get rid of the casts when calling snprintf(), and
use the correct format specifiers instead.

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH v2 0/3] ceph: don't NULL terminate virtual xattr values

[Andy Shevchenko]

On Thu, Jun 20, 2019 at 07:41:06AM -0400, Jeff Layton wrote:
> On Thu, 2019-06-20 at 13:24 +0300, Andy Shevchenko wrote:
> > On Wed, Jun 19, 2019 at 12:45:25PM -0400, Jeff Layton wrote:

> > So, then don't use snprintf() for this, simple memcpy() designed for that kind
> > of things.
> > 
> 
> memcpy from what? For many of these xattrs, we need to format integer
> data into strings. I could roll my own routine to do this formatting,
> but that's sort of what sprintf and its variants are for and I'd rather
> not reimplement all of it from scratch.

So, use bigger temporary buffer and decide what to do with data if it doesn't
fit the destination one.

String without nul is not considered as a string, thus, memcpy() the data.

> > > This patch makes ceph's virtual xattrs not include NULL termination
> > > when formatting their values. In order to handle this, a new
> > > snprintf_noterm function is added, and ceph is changed over to use
> > > this to populate the xattr value buffer.
> > 
> > In terms of vsnprintf(), and actually compiler point of view, it's not a string
> > anymore, it's a text-based data.
> > 
> > Personally, I don't see an advantage of a deep intrusion into vsnprintf().
> > The wrapper can be made to achieve this w/o touching the generic code. Thus,
> > you can quickly and cleanly fix the issue, while discussing this with wider
> > audience.
> > 
> 
> Sorry, if I'm being dense but I'm not sure I follow here.
> 
> Are you suggesting I should just copy/paste most of vsnprintf into a new
> function that just leaves off the termination at the end, and leave the
> original alone?

Yes. The data you are expecting is not a string anymore from these functions
point of view. Even GCC nowadays complains on strncpy() when nul doesn't fit
the destination buffer.

> That seems like a bit of a waste, but if that's the
> consensus then ok.

My personal view is not a consensus, let's wait for more opinions.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 0/3] ceph: don't NULL terminate virtual xattr values

[Jeff Layton]

On Thu, 2019-06-20 at 14:22 +0200, Geert Uytterhoeven wrote:
> Hi Jeff,
> 
> On Thu, Jun 20, 2019 at 1:41 PM Jeff Layton <jlayton@kernel.org> wrote:
> > On Thu, 2019-06-20 at 13:24 +0300, Andy Shevchenko wrote:
> > > On Wed, Jun 19, 2019 at 12:45:25PM -0400, Jeff Layton wrote:
> > > > v2: drop bogus EXPORT_SYMBOL of static function
> > > > 
> > > > The only real difference between this set and the one I sent originally
> > > > is the removal of a spurious EXPORT_SYMBOL in the snprintf patch.
> > > > 
> > > > I'm mostly sending this with a wider cc list in an effort to get a
> > > > review from the maintainers of the printf code. Basically ceph needs a
> > > > snprintf variant that does not NULL terminate in order to handle its
> > > > virtual xattrs.
> > > > 
> > > > Joe Perches had expressed some concerns about stack usage in vsnprintf
> > > > with this, but I'm not sure I really understand the basis of that
> > > > concern. If it is problematic, then I could use suggestions as to how
> > > > best to fix that up.
> > > 
> > > It might be problematic, since vsnprintf() can be called recursively.
> > > 
> > 
> > So the concern is that we'd have extra call/ret activity in the stack?
> > That seems like a lot of hand-wringing over very little, but ok if so.
> > 
> > > > ----------------------------8<-----------------------------
> > > > 
> > > > kcephfs has several "virtual" xattrs that return strings that are
> > > > currently populated using snprintf(), which always NULL terminates the
> > > > string.
> > > > 
> > > > This leads to the string being truncated when we use a buffer length
> > > > acquired by calling getxattr with a 0 size first. The last character
> > > > of the string ends up being clobbered by the termination.
> > > 
> > > So, then don't use snprintf() for this, simple memcpy() designed for that kind
> > > of things.
> > > 
> > 
> > memcpy from what? For many of these xattrs, we need to format integer
> > data into strings. I could roll my own routine to do this formatting,
> > but that's sort of what sprintf and its variants are for and I'd rather
> > not reimplement all of it from scratch.
> 
> snprintf() to a temporary buffer, and memcpy() to the final destination.
> These are all fairly small buffers (most are single integer values),
> so the overhead should be minimal, right?
> 

Yeah. I was trying to avoid having to deal with a second buffer, but
this is not a performance-critical codepath, so maybe that's the best
option.

> In fact the two largest strings are already formatted in a temporary
> buffer, so there is no reason ceph_vxattrcb_layout() cannot just use
> snprintf() now.
>
> Or perhaps this can use the existing seq_*() interface in some form?
> 

Hmm. I'll have to think about that.

> BTW, while at it, please get rid of the casts when calling snprintf(), and
> use the correct format specifiers instead.
> 

Sure, will do.

Thanks for the suggestions so far (and from Andy too),
-- 
Jeff Layton <jlayton@kernel.org>

Re: [PATCH v2 0/3] ceph: don't NULL terminate virtual xattr values

[Steven Rostedt]

On Thu, 20 Jun 2019 09:54:13 -0400
Jeff Layton <jlayton@kernel.org> wrote:

> > snprintf() to a temporary buffer, and memcpy() to the final destination.
> > These are all fairly small buffers (most are single integer values),
> > so the overhead should be minimal, right?
> >   
> 
> Yeah. I was trying to avoid having to deal with a second buffer, but
> this is not a performance-critical codepath, so maybe that's the best
> option.

Yes, this looks like the better solution than adding a new library
function for other people to (ab)use.

-- Steve