| Message ID | 20190619164528.31958-1-jlayton@kernel.org (mailing list archive) |
|---|---|
| Headers | show |
| Series | ceph: don't NULL terminate virtual xattr values | expand |
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 >
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 > >
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
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.
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),
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