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

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

[Jeff Layton]

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         | 145 ++++++++++++++++++++++++++++-------------
 3 files changed, 130 insertions(+), 66 deletions(-)

-- 
2.21.0

[PATCH 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         | 145 ++++++++++++++++++++++++++++-------------
 2 files changed, 103 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..ad5f4990eda3 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,69 @@ 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;
+}
+EXPORT_SYMBOL(vsnprintf_noterm);
+
+/**
+ * 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 +2528,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 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 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 0/3] ceph: don't NULL terminate virtual xattr values

[Andreas Gruenbacher]

On Fri, 14 Jun 2019 at 15:46, Jeff Layton <jlayton@kernel.org> wrote:
> 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.

This looks reasonable from an xattr point of view.

Thanks,
Andreas

> 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         | 145 ++++++++++++++++++++++++++++-------------
>  3 files changed, 130 insertions(+), 66 deletions(-)
>
> --
> 2.21.0
>

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

[Yan, Zheng]

On Fri, Jun 14, 2019 at 9:48 PM Jeff Layton <jlayton@kernel.org> wrote:
>
> 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         | 145 ++++++++++++++++++++++++++++-------------
>  2 files changed, 103 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..ad5f4990eda3 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,69 @@ 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;
> +}
> +EXPORT_SYMBOL(vsnprintf_noterm);

export static function?

> +
> +/**
> + * 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 +2528,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
>

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

[Joe Perches]

On Sat, 2019-06-15 at 10:41 +0800, Yan, Zheng wrote:
> On Fri, Jun 14, 2019 at 9:48 PM Jeff Layton <jlayton@kernel.org> wrote:
> > 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.

Is this function really necessary enough to add
the additional stack use to the generic case?

Why not add have this function call vsnprintf
and then terminate the string separately?

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

[Jeff Layton]

On Sat, 2019-06-15 at 10:41 +0800, Yan, Zheng wrote:
> On Fri, Jun 14, 2019 at 9:48 PM Jeff Layton <jlayton@kernel.org> wrote:
> > 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         | 145 ++++++++++++++++++++++++++++-------------
> >  2 files changed, 103 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..ad5f4990eda3 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,69 @@ 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;
> > +}
> > +EXPORT_SYMBOL(vsnprintf_noterm);
> 
> export static function?
> 

Good catch! I had originally had this function as an exported helper,
but made it static because there were no callers. Will fix.

> > +
> > +/**
> > + * 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 +2528,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
> > 

-- 
Jeff Layton <jlayton@kernel.org>

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

[Jeff Layton]

On Fri, 2019-06-14 at 19:58 -0700, Joe Perches wrote:
> On Sat, 2019-06-15 at 10:41 +0800, Yan, Zheng wrote:
> > On Fri, Jun 14, 2019 at 9:48 PM Jeff Layton <jlayton@kernel.org> wrote:
> > > 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.
> 
> Is this function really necessary enough to add
> the additional stack use to the generic case?
> 

The only alternative I saw was to allocate an extra buffer in the
callers, call snprintf to populate that and then copy the result into
the destination buffer sans termination. I really would like to avoid
that here.

Does breaking this code out into a helper add any significant stack
usage? I didn't see it that way, but I am quite concerned about not
slowing down the generic vsnprintf routine.

> Why not add have this function call vsnprintf
> and then terminate the string separately?
> 

I don't quite follow what you're suggesting here. vsnprintf is what does
the termination today, and we need a function that doesn't do that.

-- 
Jeff Layton <jlayton@kernel.org>