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

Message ID 20190619164528.31958-1-jlayton@kernel.org
Headers show
Series
  • ceph: don't NULL terminate virtual xattr values
Related show

Message

Jeff Layton June 19, 2019, 4:45 p.m. UTC
v2: drop bogus EXPORT_SYMBOL of static function

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

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

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

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

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

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

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

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

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

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

Comments

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

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

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

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

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

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

Fine.

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

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

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

> Finally, we fix ceph to
> return -ERANGE properly when the string didn't fit in the buffer.
> 
> Jeff Layton (3):
>   lib/vsprintf: add snprintf_noterm
>   ceph: don't NULL terminate virtual xattr strings
>   ceph: return -ERANGE if virtual xattr value didn't fit in buffer
> 
>  fs/ceph/xattr.c        |  49 +++++++-------
>  include/linux/kernel.h |   2 +
>  lib/vsprintf.c         | 144 ++++++++++++++++++++++++++++-------------
>  3 files changed, 129 insertions(+), 66 deletions(-)
> 
> -- 
> 2.21.0
>
Jeff Layton June 20, 2019, 11:41 a.m. UTC | #2
On Thu, 2019-06-20 at 13:24 +0300, Andy Shevchenko wrote:
> On Wed, Jun 19, 2019 at 12:45:25PM -0400, Jeff Layton wrote:
> > v2: drop bogus EXPORT_SYMBOL of static function
> > 
> > The only real difference between this set and the one I sent originally
> > is the removal of a spurious EXPORT_SYMBOL in the snprintf patch.
> > 
> > I'm mostly sending this with a wider cc list in an effort to get a
> > review from the maintainers of the printf code. Basically ceph needs a
> > snprintf variant that does not NULL terminate in order to handle its
> > virtual xattrs.
> > 
> > Joe Perches had expressed some concerns about stack usage in vsnprintf
> > with this, but I'm not sure I really understand the basis of that
> > concern. If it is problematic, then I could use suggestions as to how
> > best to fix that up.
> 
> It might be problematic, since vsnprintf() can be called recursively.
> 

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

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

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

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

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

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

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

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

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

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

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

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

Gr{oetje,eeting}s,

                        Geert
Andy Shevchenko June 20, 2019, 12:34 p.m. UTC | #4
On Thu, Jun 20, 2019 at 07:41:06AM -0400, Jeff Layton wrote:
> On Thu, 2019-06-20 at 13:24 +0300, Andy Shevchenko wrote:
> > On Wed, Jun 19, 2019 at 12:45:25PM -0400, Jeff Layton wrote:

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

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

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

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

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

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

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

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

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

Hmm. I'll have to think about that.

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

Sure, will do.

Thanks for the suggestions so far (and from Andy too),
Steven Rostedt June 25, 2019, 10:50 p.m. UTC | #6
On Thu, 20 Jun 2019 09:54:13 -0400
Jeff Layton <jlayton@kernel.org> wrote:

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

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

-- Steve