[v3,1/2] ceph: fix buffer length handling in virtual xattrs
diff mbox series

Message ID 20190621141833.17551-2-jlayton@kernel.org
State New
Headers show
Series
  • ceph: don't NULL terminate virtual xattr values
Related show

Commit Message

Jeff Layton June 21, 2019, 2:18 p.m. UTC
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 new routines to format the string into an on-stack buffer that is
always large enough to hold the whole thing and then memcpy the result
into the destination buffer. Then, change over the virtual xattr
routines to use the new helper functions as appropriate.

Finally, make the code return ERANGE if the destination buffer size was
too small to hold the returned value.

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

Comments

Ilya Dryomov June 24, 2019, 10 a.m. UTC | #1
On Fri, Jun 21, 2019 at 4:18 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 new routines to format the string into an on-stack buffer that is
> always large enough to hold the whole thing and then memcpy the result
> into the destination buffer. Then, change over the virtual xattr
> routines to use the new helper functions as appropriate.
>
> Finally, make the code return ERANGE if the destination buffer size was
> too small to hold the returned value.
>
> Signed-off-by: Jeff Layton <jlayton@kernel.org>
> ---
>  fs/ceph/xattr.c | 103 ++++++++++++++++++++++++++++++++++++------------
>  1 file changed, 78 insertions(+), 25 deletions(-)
>
> diff --git a/fs/ceph/xattr.c b/fs/ceph/xattr.c
> index 6621d27e64f5..359d3cbbb37b 100644
> --- a/fs/ceph/xattr.c
> +++ b/fs/ceph/xattr.c
> @@ -112,22 +112,47 @@ static size_t ceph_vxattrcb_layout(struct ceph_inode_info *ci, char *val,
>         return ret;
>  }
>
> +/* Enough to hold any possible expression of integer TYPE in base 10 */
> +#define INT_STR_SIZE(_type)    3*sizeof(_type)+2
> +
> +/*
> + * snprintf always NULL terminates, but we need for xattrs to not be. For
> + * the integer vxattrs, just create an on-stack buffer for snprintf's
> + * destination, and just don't copy the termination to the actual buffer.
> + */
> +#define GENERATE_XATTR_INT_FORMATTER(_lbl, _format, _type)                  \
> +static size_t format_ ## _lbl ## _xattr(char *val, size_t size, _type src)   \
> +{                                                                           \
> +       size_t ret;                                                          \
> +       char buf[INT_STR_SIZE(_type)];                                       \
> +                                                                            \
> +       ret = snprintf(buf, size ? sizeof(buf) : 0, _format, src);           \
> +       if (ret <= size)                                                     \
> +               memcpy(val, buf, ret);                                       \
> +       return ret;                                                          \
> +}
> +
> +GENERATE_XATTR_INT_FORMATTER(u, "%u", unsigned int)
> +GENERATE_XATTR_INT_FORMATTER(d, "%d", int)
> +GENERATE_XATTR_INT_FORMATTER(lld, "%lld", long long)
> +GENERATE_XATTR_INT_FORMATTER(llu, "%llu", unsigned long long)
> +
>  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 format_u_xattr(val, size, 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 format_u_xattr(val, size, 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 format_u_xattr(val, size, ci->i_layout.object_size);
>  }
>
>  static size_t ceph_vxattrcb_layout_pool(struct ceph_inode_info *ci,
> @@ -141,10 +166,14 @@ 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);
> -       else
> -               ret = snprintf(val, size, "%lld", (unsigned long long)pool);
> +       if (pool_name) {
> +               ret = strlen(pool_name);
> +
> +               if (ret <= size)
> +                       memcpy(val, pool_name, ret);
> +       } else {
> +               ret = format_lld_xattr(val, size, pool);
> +       }
>         up_read(&osdc->lock);
>         return ret;
>  }
> @@ -155,7 +184,11 @@ 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 = ns->len;
> +
> +               if (ret <= size)
> +                       memcpy(val, ns->str, ns->len);
> +
>                 ceph_put_string(ns);
>         }
>         return ret;
> @@ -166,50 +199,61 @@ 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 format_lld_xattr(val, size, 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 format_lld_xattr(val, size, 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 format_lld_xattr(val, size, 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 format_lld_xattr(val, size, 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 format_lld_xattr(val, size, 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 format_lld_xattr(val, size, 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 format_lld_xattr(val, size, ci->i_rbytes);
> +}
> +
> +static size_t format_ts64_xattr(char *val, size_t size, struct timespec64 *src)
> +{
> +       size_t ret;
> +       char buf[INT_STR_SIZE(long long) + 1 + 9];
> +
> +       ret = snprintf(buf, size ? sizeof(buf) : 0, "%lld.%09ld", src->tv_sec,
> +                      src->tv_nsec);
> +       if (ret <= size)
> +               memcpy(val, buf, ret);
> +       return ret;
>  }
>
>  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,
> -                       ci->i_rctime.tv_nsec);
> +       return format_ts64_xattr(val, size, &ci->i_rctime);
>  }
>
>  /* dir pin */
> @@ -221,7 +265,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 format_d_xattr(val, size, ci->i_dir_pin);
>  }
>
>  /* quotas */
> @@ -241,20 +285,27 @@ 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",
> -                       ci->i_max_bytes, ci->i_max_files);
> +       size_t ret;
> +       char buf[(2*INT_STR_SIZE(unsigned long long)) + 10 + 11];
> +
> +       ret = snprintf(buf, size ? sizeof(buf) : 0,
> +                      "max_bytes=%llu max_files=%llu",
> +                      ci->i_max_bytes, ci->i_max_files);
> +       if (ret <= size)
> +               memcpy(val, buf, ret);
> +       return ret;
>  }
>
>  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 format_llu_xattr(val, size, 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 format_llu_xattr(val, size, ci->i_max_files);
>  }
>
>  /* snapshots */
> @@ -266,8 +317,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,
> -                       ci->i_snap_btime.tv_nsec);
> +       return format_ts64_xattr(val, size, &ci->i_snap_btime);
>  }
>
>  #define CEPH_XATTR_NAME(_type, _name)  XATTR_CEPH_PREFIX #_type "." #_name

Hi Jeff,

This seems over-engineered to me.  You have four functions just for
ints, two more for ts64 and quota and several ad-hoc %s memcpys.  Why
not define a single function with a generously-sized buffer, BUG in
case it's too small and take a format string?

Regardless, I would much prefer

  char buf[64];

over

  #define INT_STR_SIZE(_type)    3*sizeof(_type)+2
  char buf[(2*INT_STR_SIZE(unsigned long long)) + 10 + 11];

Thanks,

                Ilya
Jeff Layton June 24, 2019, 10:26 a.m. UTC | #2
On Mon, 2019-06-24 at 12:00 +0200, Ilya Dryomov wrote:
> On Fri, Jun 21, 2019 at 4:18 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 new routines to format the string into an on-stack buffer that is
> > always large enough to hold the whole thing and then memcpy the result
> > into the destination buffer. Then, change over the virtual xattr
> > routines to use the new helper functions as appropriate.
> > 
> > Finally, make the code return ERANGE if the destination buffer size was
> > too small to hold the returned value.
> > 
> > Signed-off-by: Jeff Layton <jlayton@kernel.org>
> > ---
> >  fs/ceph/xattr.c | 103 ++++++++++++++++++++++++++++++++++++------------
> >  1 file changed, 78 insertions(+), 25 deletions(-)
> > 
> > diff --git a/fs/ceph/xattr.c b/fs/ceph/xattr.c
> > index 6621d27e64f5..359d3cbbb37b 100644
> > --- a/fs/ceph/xattr.c
> > +++ b/fs/ceph/xattr.c
> > @@ -112,22 +112,47 @@ static size_t ceph_vxattrcb_layout(struct ceph_inode_info *ci, char *val,
> >         return ret;
> >  }
> > 
> > +/* Enough to hold any possible expression of integer TYPE in base 10 */
> > +#define INT_STR_SIZE(_type)    3*sizeof(_type)+2
> > +
> > +/*
> > + * snprintf always NULL terminates, but we need for xattrs to not be. For
> > + * the integer vxattrs, just create an on-stack buffer for snprintf's
> > + * destination, and just don't copy the termination to the actual buffer.
> > + */
> > +#define GENERATE_XATTR_INT_FORMATTER(_lbl, _format, _type)                  \
> > +static size_t format_ ## _lbl ## _xattr(char *val, size_t size, _type src)   \
> > +{                                                                           \
> > +       size_t ret;                                                          \
> > +       char buf[INT_STR_SIZE(_type)];                                       \
> > +                                                                            \
> > +       ret = snprintf(buf, size ? sizeof(buf) : 0, _format, src);           \
> > +       if (ret <= size)                                                     \
> > +               memcpy(val, buf, ret);                                       \
> > +       return ret;                                                          \
> > +}
> > +
> > +GENERATE_XATTR_INT_FORMATTER(u, "%u", unsigned int)
> > +GENERATE_XATTR_INT_FORMATTER(d, "%d", int)
> > +GENERATE_XATTR_INT_FORMATTER(lld, "%lld", long long)
> > +GENERATE_XATTR_INT_FORMATTER(llu, "%llu", unsigned long long)
> > +
> >  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 format_u_xattr(val, size, 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 format_u_xattr(val, size, 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 format_u_xattr(val, size, ci->i_layout.object_size);
> >  }
> > 
> >  static size_t ceph_vxattrcb_layout_pool(struct ceph_inode_info *ci,
> > @@ -141,10 +166,14 @@ 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);
> > -       else
> > -               ret = snprintf(val, size, "%lld", (unsigned long long)pool);
> > +       if (pool_name) {
> > +               ret = strlen(pool_name);
> > +
> > +               if (ret <= size)
> > +                       memcpy(val, pool_name, ret);
> > +       } else {
> > +               ret = format_lld_xattr(val, size, pool);
> > +       }
> >         up_read(&osdc->lock);
> >         return ret;
> >  }
> > @@ -155,7 +184,11 @@ 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 = ns->len;
> > +
> > +               if (ret <= size)
> > +                       memcpy(val, ns->str, ns->len);
> > +
> >                 ceph_put_string(ns);
> >         }
> >         return ret;
> > @@ -166,50 +199,61 @@ 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 format_lld_xattr(val, size, 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 format_lld_xattr(val, size, 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 format_lld_xattr(val, size, 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 format_lld_xattr(val, size, 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 format_lld_xattr(val, size, 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 format_lld_xattr(val, size, 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 format_lld_xattr(val, size, ci->i_rbytes);
> > +}
> > +
> > +static size_t format_ts64_xattr(char *val, size_t size, struct timespec64 *src)
> > +{
> > +       size_t ret;
> > +       char buf[INT_STR_SIZE(long long) + 1 + 9];
> > +
> > +       ret = snprintf(buf, size ? sizeof(buf) : 0, "%lld.%09ld", src->tv_sec,
> > +                      src->tv_nsec);
> > +       if (ret <= size)
> > +               memcpy(val, buf, ret);
> > +       return ret;
> >  }
> > 
> >  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,
> > -                       ci->i_rctime.tv_nsec);
> > +       return format_ts64_xattr(val, size, &ci->i_rctime);
> >  }
> > 
> >  /* dir pin */
> > @@ -221,7 +265,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 format_d_xattr(val, size, ci->i_dir_pin);
> >  }
> > 
> >  /* quotas */
> > @@ -241,20 +285,27 @@ 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",
> > -                       ci->i_max_bytes, ci->i_max_files);
> > +       size_t ret;
> > +       char buf[(2*INT_STR_SIZE(unsigned long long)) + 10 + 11];
> > +
> > +       ret = snprintf(buf, size ? sizeof(buf) : 0,
> > +                      "max_bytes=%llu max_files=%llu",
> > +                      ci->i_max_bytes, ci->i_max_files);
> > +       if (ret <= size)
> > +               memcpy(val, buf, ret);
> > +       return ret;
> >  }
> > 
> >  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 format_llu_xattr(val, size, 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 format_llu_xattr(val, size, ci->i_max_files);
> >  }
> > 
> >  /* snapshots */
> > @@ -266,8 +317,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,
> > -                       ci->i_snap_btime.tv_nsec);
> > +       return format_ts64_xattr(val, size, &ci->i_snap_btime);
> >  }
> > 
> >  #define CEPH_XATTR_NAME(_type, _name)  XATTR_CEPH_PREFIX #_type "." #_name
> 
> Hi Jeff,
> 
> This seems over-engineered to me.  You have four functions just for
> ints, two more for ts64 and quota and several ad-hoc %s memcpys.  Why
> not define a single function with a generously-sized buffer, BUG in
> case it's too small and take a format string?

Having to declare a 64 byte (or 128 byte or whatever) buffer on the
stack for an int seemed a bit wasteful, but ok. We can certainly do it
that way instead.

> Regardless, I would much prefer
> 
>   char buf[64];
> 
> over
> 
>   #define INT_STR_SIZE(_type)    3*sizeof(_type)+2
>   char buf[(2*INT_STR_SIZE(unsigned long long)) + 10 + 11];
> 

We'll have to agree to disagree here. I rather like seeing the math for
the buffer length calculations expanded out, particularly in the event
that they need to be changed in the future.
Ilya Dryomov June 24, 2019, 11:03 a.m. UTC | #3
On Mon, Jun 24, 2019 at 12:26 PM Jeff Layton <jlayton@kernel.org> wrote:
>
> On Mon, 2019-06-24 at 12:00 +0200, Ilya Dryomov wrote:
> > On Fri, Jun 21, 2019 at 4:18 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 new routines to format the string into an on-stack buffer that is
> > > always large enough to hold the whole thing and then memcpy the result
> > > into the destination buffer. Then, change over the virtual xattr
> > > routines to use the new helper functions as appropriate.
> > >
> > > Finally, make the code return ERANGE if the destination buffer size was
> > > too small to hold the returned value.
> > >
> > > Signed-off-by: Jeff Layton <jlayton@kernel.org>
> > > ---
> > >  fs/ceph/xattr.c | 103 ++++++++++++++++++++++++++++++++++++------------
> > >  1 file changed, 78 insertions(+), 25 deletions(-)
> > >
> > > diff --git a/fs/ceph/xattr.c b/fs/ceph/xattr.c
> > > index 6621d27e64f5..359d3cbbb37b 100644
> > > --- a/fs/ceph/xattr.c
> > > +++ b/fs/ceph/xattr.c
> > > @@ -112,22 +112,47 @@ static size_t ceph_vxattrcb_layout(struct ceph_inode_info *ci, char *val,
> > >         return ret;
> > >  }
> > >
> > > +/* Enough to hold any possible expression of integer TYPE in base 10 */
> > > +#define INT_STR_SIZE(_type)    3*sizeof(_type)+2
> > > +
> > > +/*
> > > + * snprintf always NULL terminates, but we need for xattrs to not be. For
> > > + * the integer vxattrs, just create an on-stack buffer for snprintf's
> > > + * destination, and just don't copy the termination to the actual buffer.
> > > + */
> > > +#define GENERATE_XATTR_INT_FORMATTER(_lbl, _format, _type)                  \
> > > +static size_t format_ ## _lbl ## _xattr(char *val, size_t size, _type src)   \
> > > +{                                                                           \
> > > +       size_t ret;                                                          \
> > > +       char buf[INT_STR_SIZE(_type)];                                       \
> > > +                                                                            \
> > > +       ret = snprintf(buf, size ? sizeof(buf) : 0, _format, src);           \
> > > +       if (ret <= size)                                                     \
> > > +               memcpy(val, buf, ret);                                       \
> > > +       return ret;                                                          \
> > > +}
> > > +
> > > +GENERATE_XATTR_INT_FORMATTER(u, "%u", unsigned int)
> > > +GENERATE_XATTR_INT_FORMATTER(d, "%d", int)
> > > +GENERATE_XATTR_INT_FORMATTER(lld, "%lld", long long)
> > > +GENERATE_XATTR_INT_FORMATTER(llu, "%llu", unsigned long long)
> > > +
> > >  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 format_u_xattr(val, size, 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 format_u_xattr(val, size, 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 format_u_xattr(val, size, ci->i_layout.object_size);
> > >  }
> > >
> > >  static size_t ceph_vxattrcb_layout_pool(struct ceph_inode_info *ci,
> > > @@ -141,10 +166,14 @@ 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);
> > > -       else
> > > -               ret = snprintf(val, size, "%lld", (unsigned long long)pool);
> > > +       if (pool_name) {
> > > +               ret = strlen(pool_name);
> > > +
> > > +               if (ret <= size)
> > > +                       memcpy(val, pool_name, ret);
> > > +       } else {
> > > +               ret = format_lld_xattr(val, size, pool);
> > > +       }
> > >         up_read(&osdc->lock);
> > >         return ret;
> > >  }
> > > @@ -155,7 +184,11 @@ 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 = ns->len;
> > > +
> > > +               if (ret <= size)
> > > +                       memcpy(val, ns->str, ns->len);
> > > +
> > >                 ceph_put_string(ns);
> > >         }
> > >         return ret;
> > > @@ -166,50 +199,61 @@ 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 format_lld_xattr(val, size, 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 format_lld_xattr(val, size, 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 format_lld_xattr(val, size, 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 format_lld_xattr(val, size, 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 format_lld_xattr(val, size, 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 format_lld_xattr(val, size, 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 format_lld_xattr(val, size, ci->i_rbytes);
> > > +}
> > > +
> > > +static size_t format_ts64_xattr(char *val, size_t size, struct timespec64 *src)
> > > +{
> > > +       size_t ret;
> > > +       char buf[INT_STR_SIZE(long long) + 1 + 9];
> > > +
> > > +       ret = snprintf(buf, size ? sizeof(buf) : 0, "%lld.%09ld", src->tv_sec,
> > > +                      src->tv_nsec);
> > > +       if (ret <= size)
> > > +               memcpy(val, buf, ret);
> > > +       return ret;
> > >  }
> > >
> > >  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,
> > > -                       ci->i_rctime.tv_nsec);
> > > +       return format_ts64_xattr(val, size, &ci->i_rctime);
> > >  }
> > >
> > >  /* dir pin */
> > > @@ -221,7 +265,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 format_d_xattr(val, size, ci->i_dir_pin);
> > >  }
> > >
> > >  /* quotas */
> > > @@ -241,20 +285,27 @@ 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",
> > > -                       ci->i_max_bytes, ci->i_max_files);
> > > +       size_t ret;
> > > +       char buf[(2*INT_STR_SIZE(unsigned long long)) + 10 + 11];
> > > +
> > > +       ret = snprintf(buf, size ? sizeof(buf) : 0,
> > > +                      "max_bytes=%llu max_files=%llu",
> > > +                      ci->i_max_bytes, ci->i_max_files);
> > > +       if (ret <= size)
> > > +               memcpy(val, buf, ret);
> > > +       return ret;
> > >  }
> > >
> > >  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 format_llu_xattr(val, size, 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 format_llu_xattr(val, size, ci->i_max_files);
> > >  }
> > >
> > >  /* snapshots */
> > > @@ -266,8 +317,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,
> > > -                       ci->i_snap_btime.tv_nsec);
> > > +       return format_ts64_xattr(val, size, &ci->i_snap_btime);
> > >  }
> > >
> > >  #define CEPH_XATTR_NAME(_type, _name)  XATTR_CEPH_PREFIX #_type "." #_name
> >
> > Hi Jeff,
> >
> > This seems over-engineered to me.  You have four functions just for
> > ints, two more for ts64 and quota and several ad-hoc %s memcpys.  Why
> > not define a single function with a generously-sized buffer, BUG in
> > case it's too small and take a format string?
>
> Having to declare a 64 byte (or 128 byte or whatever) buffer on the
> stack for an int seemed a bit wasteful, but ok. We can certainly do it
> that way instead.

This is a leaf function, unlikely to be called deep in the stack.
I would be fine with pretty much any (reasonable) buffer here.

Treating all xattrs the same is actually good IMO.  A hypothetical
situation where printing e.g. pin succeeds but printing quota overflows
the stack would seem rather weird.

Thanks,

                Ilya

Patch
diff mbox series

diff --git a/fs/ceph/xattr.c b/fs/ceph/xattr.c
index 6621d27e64f5..359d3cbbb37b 100644
--- a/fs/ceph/xattr.c
+++ b/fs/ceph/xattr.c
@@ -112,22 +112,47 @@  static size_t ceph_vxattrcb_layout(struct ceph_inode_info *ci, char *val,
 	return ret;
 }
 
+/* Enough to hold any possible expression of integer TYPE in base 10 */
+#define INT_STR_SIZE(_type)	3*sizeof(_type)+2
+
+/*
+ * snprintf always NULL terminates, but we need for xattrs to not be. For
+ * the integer vxattrs, just create an on-stack buffer for snprintf's
+ * destination, and just don't copy the termination to the actual buffer.
+ */
+#define GENERATE_XATTR_INT_FORMATTER(_lbl, _format, _type)		     \
+static size_t format_ ## _lbl ## _xattr(char *val, size_t size, _type src)   \
+{									     \
+	size_t ret;							     \
+	char buf[INT_STR_SIZE(_type)];					     \
+									     \
+	ret = snprintf(buf, size ? sizeof(buf) : 0, _format, src);	     \
+	if (ret <= size)						     \
+		memcpy(val, buf, ret);					     \
+	return ret;							     \
+}
+
+GENERATE_XATTR_INT_FORMATTER(u, "%u", unsigned int)
+GENERATE_XATTR_INT_FORMATTER(d, "%d", int)
+GENERATE_XATTR_INT_FORMATTER(lld, "%lld", long long)
+GENERATE_XATTR_INT_FORMATTER(llu, "%llu", unsigned long long)
+
 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 format_u_xattr(val, size, 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 format_u_xattr(val, size, 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 format_u_xattr(val, size, ci->i_layout.object_size);
 }
 
 static size_t ceph_vxattrcb_layout_pool(struct ceph_inode_info *ci,
@@ -141,10 +166,14 @@  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);
-	else
-		ret = snprintf(val, size, "%lld", (unsigned long long)pool);
+	if (pool_name) {
+		ret = strlen(pool_name);
+
+		if (ret <= size)
+			memcpy(val, pool_name, ret);
+	} else {
+		ret = format_lld_xattr(val, size, pool);
+	}
 	up_read(&osdc->lock);
 	return ret;
 }
@@ -155,7 +184,11 @@  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 = ns->len;
+
+		if (ret <= size)
+			memcpy(val, ns->str, ns->len);
+
 		ceph_put_string(ns);
 	}
 	return ret;
@@ -166,50 +199,61 @@  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 format_lld_xattr(val, size, 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 format_lld_xattr(val, size, 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 format_lld_xattr(val, size, 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 format_lld_xattr(val, size, 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 format_lld_xattr(val, size, 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 format_lld_xattr(val, size, 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 format_lld_xattr(val, size, ci->i_rbytes);
+}
+
+static size_t format_ts64_xattr(char *val, size_t size, struct timespec64 *src)
+{
+	size_t ret;
+	char buf[INT_STR_SIZE(long long) + 1 + 9];
+
+	ret = snprintf(buf, size ? sizeof(buf) : 0, "%lld.%09ld", src->tv_sec,
+		       src->tv_nsec);
+	if (ret <= size)
+		memcpy(val, buf, ret);
+	return ret;
 }
 
 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,
-			ci->i_rctime.tv_nsec);
+	return format_ts64_xattr(val, size, &ci->i_rctime);
 }
 
 /* dir pin */
@@ -221,7 +265,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 format_d_xattr(val, size, ci->i_dir_pin);
 }
 
 /* quotas */
@@ -241,20 +285,27 @@  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",
-			ci->i_max_bytes, ci->i_max_files);
+	size_t ret;
+	char buf[(2*INT_STR_SIZE(unsigned long long)) + 10 + 11];
+
+	ret = snprintf(buf, size ? sizeof(buf) : 0,
+		       "max_bytes=%llu max_files=%llu",
+		       ci->i_max_bytes, ci->i_max_files);
+	if (ret <= size)
+		memcpy(val, buf, ret);
+	return ret;
 }
 
 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 format_llu_xattr(val, size, 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 format_llu_xattr(val, size, ci->i_max_files);
 }
 
 /* snapshots */
@@ -266,8 +317,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,
-			ci->i_snap_btime.tv_nsec);
+	return format_ts64_xattr(val, size, &ci->i_snap_btime);
 }
 
 #define CEPH_XATTR_NAME(_type, _name)	XATTR_CEPH_PREFIX #_type "." #_name
@@ -803,8 +853,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;
 	}