| Message ID | 20190621141833.17551-2-jlayton@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | ceph: don't NULL terminate virtual xattr values | expand |
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
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.
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
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; }
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(-)