Background
==========
Linus suggested printing full path for file instead of printing
the components as '%pd'.
Typically, there is no need for printk specifiers to take any real locks
(ie mount_lock or rename_lock). So I introduce a new helper d_path_fast
which is similar to d_path except it doesn't take any seqlock/spinlock.
This series is based on Al Viro's d_path cleanup patches [1] which
lifted the inner lockless loop into a new helper.
[1] https://lkml.org/lkml/2021/5/18/1260
Test
====
The cases I tested:
1. print '%pD' with full path of ext4 file
2. mount a ext4 filesystem upon a ext4 filesystem, and print the file
with '%pD'
3. all test_print selftests, including the new '%14pD' '%-14pD'
4. kasnprintf
Changelog
=========
v4:
- don't support spec.precision anymore for '%pD'
- add Rasmus's patch into this series
v3:
- implement new d_path_unsafe to use [buf, end] instead of stack space for
filling bytes (by Matthew)
- add new test cases for '%pD'
- drop patch "hmcdrv: remove the redundant directory path" before removing rfc.
v2:
- implement new d_path_fast based on Al Viro's patches
- add check_pointer check (by Petr)
- change the max full path size to 256 in stack space
v1: https://lkml.org/lkml/2021/5/8/122
Jia He (4):
fs: introduce helper d_path_unsafe()
lib/vsprintf.c: make '%pD' print full path for file
lib/test_printf.c: split write-beyond-buffer check in two
lib/test_printf.c: add test cases for '%pD'
Documentation/core-api/printk-formats.rst | 5 +-
fs/d_path.c | 83 ++++++++++++++++++++++-
include/linux/dcache.h | 1 +
lib/test_printf.c | 31 ++++++++-
lib/vsprintf.c | 37 ++++++++--
5 files changed, 148 insertions(+), 9 deletions(-)
--
2.17.1
From: Rasmus Villemoes <[email protected]>
Before each invocation of vsnprintf(), do_test() memsets the entire
allocated buffer to a sentinel value. That buffer includes leading and
trailing padding which is never included in the buffer area handed to
vsnprintf (spaces merely for clarity):
pad test_buffer pad
**** **************** ****
Then vsnprintf() is invoked with a bufsize argument <=
BUF_SIZE. Suppose bufsize=10, then we'd have e.g.
|pad | test_buffer |pad |
**** pizza0 **** ****** ****
A B C D E
where vsnprintf() was given the area from B to D.
It is obviously a bug for vsnprintf to touch anything between A and B
or between D and E. The former is checked for as one would expect. But
for the latter, we are actually a little stricter in that we check the
area between C and E.
Split that check in two, providing a clearer error message in case it
was a genuine buffer overrun and not merely a write within the
provided buffer, but after the end of the generated string.
So far, no part of the vsnprintf() implementation has had any use for
using the whole buffer as scratch space, but it's not unreasonable to
allow that, as long as the result is properly nul-terminated and the
return value is the right one. However, it is somewhat unusual, and
most %<something> won't need this, so keep the [C,D] check, but make
it easy for a later patch to make that part opt-out for certain tests.
Signed-off-by: Rasmus Villemoes <[email protected]>
Tested-by: Jia He <[email protected]>
Signed-off-by: Jia He <[email protected]>
---
lib/test_printf.c | 7 ++++++-
1 file changed, 6 insertions(+), 1 deletion(-)
diff --git a/lib/test_printf.c b/lib/test_printf.c
index ec0d5976bb69..d1d2f898ebae 100644
--- a/lib/test_printf.c
+++ b/lib/test_printf.c
@@ -78,12 +78,17 @@ do_test(int bufsize, const char *expect, int elen,
return 1;
}
- if (memchr_inv(test_buffer + written + 1, FILL_CHAR, BUF_SIZE + PAD_SIZE - (written + 1))) {
+ if (memchr_inv(test_buffer + written + 1, FILL_CHAR, bufsize - (written + 1))) {
pr_warn("vsnprintf(buf, %d, \"%s\", ...) wrote beyond the nul-terminator\n",
bufsize, fmt);
return 1;
}
+ if (memchr_inv(test_buffer + bufsize, FILL_CHAR, BUF_SIZE + PAD_SIZE - bufsize)) {
+ pr_warn("vsnprintf(buf, %d, \"%s\", ...) wrote beyond buffer\n", bufsize, fmt);
+ return 1;
+ }
+
if (memcmp(test_buffer, expect, written)) {
pr_warn("vsnprintf(buf, %d, \"%s\", ...) wrote '%s', expected '%.*s'\n",
bufsize, fmt, test_buffer, written, expect);
--
2.17.1
On Tue, Jun 15, 2021 at 6:54 PM Jia He <[email protected]> wrote:
>
> Background
> ==========
> Linus suggested printing full path for file instead of printing
the full
the file
> the components as '%pd'.
>
> Typically, there is no need for printk specifiers to take any real locks
> (ie mount_lock or rename_lock). So I introduce a new helper d_path_fast
> which is similar to d_path except it doesn't take any seqlock/spinlock.
>
> This series is based on Al Viro's d_path cleanup patches [1] which
> lifted the inner lockless loop into a new helper.
>
> [1] https://lkml.org/lkml/2021/5/18/1260
>
> Test
> ====
> The cases I tested:
> 1. print '%pD' with full path of ext4 file
> 2. mount a ext4 filesystem upon a ext4 filesystem, and print the file
> with '%pD'
> 3. all test_print selftests, including the new '%14pD' '%-14pD'
> 4. kasnprintf
kasprintf()
After commenting on patch 1, I suggest you to always build with `make W=1 ...`
> Changelog
> =========
> v4:
> - don't support spec.precision anymore for '%pD'
> - add Rasmus's patch into this series
>
> v3:
> - implement new d_path_unsafe to use [buf, end] instead of stack space for
> filling bytes (by Matthew)
> - add new test cases for '%pD'
> - drop patch "hmcdrv: remove the redundant directory path" before removing rfc.
>
> v2:
> - implement new d_path_fast based on Al Viro's patches
> - add check_pointer check (by Petr)
> - change the max full path size to 256 in stack space
> v1: https://lkml.org/lkml/2021/5/8/122
>
> Jia He (4):
> fs: introduce helper d_path_unsafe()
> lib/vsprintf.c: make '%pD' print full path for file
> lib/test_printf.c: split write-beyond-buffer check in two
> lib/test_printf.c: add test cases for '%pD'
>
> Documentation/core-api/printk-formats.rst | 5 +-
> fs/d_path.c | 83 ++++++++++++++++++++++-
> include/linux/dcache.h | 1 +
> lib/test_printf.c | 31 ++++++++-
> lib/vsprintf.c | 37 ++++++++--
> 5 files changed, 148 insertions(+), 9 deletions(-)
>
> --
> 2.17.1
>
--
With Best Regards,
Andy Shevchenko
Btw, please throw in a patch to convert iomap_swapfile_fail over to
the new %pD as that started the whole flamewar^H^H^H^H^H^H^Hdiscussion.
Hi Christoph
> -----Original Message-----
> From: Christoph Hellwig <[email protected]>
> Sent: Wednesday, June 16, 2021 1:09 PM
> To: Justin He <[email protected]>
> Cc: Petr Mladek <[email protected]>; Steven Rostedt <[email protected]>;
> Sergey Senozhatsky <[email protected]>; Andy Shevchenko
> <[email protected]>; Rasmus Villemoes
> <[email protected]>; Jonathan Corbet <[email protected]>; Alexander
> Viro <[email protected]>; Linus Torvalds <torvalds@linux-
> foundation.org>; Peter Zijlstra (Intel) <[email protected]>; Eric
> Biggers <[email protected]>; Ahmed S. Darwish <[email protected]>;
> [email protected]; [email protected]; linux-
> [email protected]; Matthew Wilcox <[email protected]>
> Subject: Re: [PATCH RFCv4 0/4] make '%pD' print full path for file
>
> Btw, please throw in a patch to convert iomap_swapfile_fail over to
> the new %pD as that started the whole flamewar^H^H^H^H^H^H^Hdiscussion.
Ok, thanks for the reminder.
After all of the solution/helper/interface is stable (maybe after removing RFC),
I tend to add more patches:
1. s390/hmcdrv: remove the redundant directory after using '%pD'
2. remove all the '%pD[2,3,4]' usage in fs/
3. your mentioned iomap_swapfile_fail
--
Cheers,
Justin (Jia He)
On Tue 2021-06-15 23:49:51, Jia He wrote:
> From: Rasmus Villemoes <[email protected]>
>
> Before each invocation of vsnprintf(), do_test() memsets the entire
> allocated buffer to a sentinel value. That buffer includes leading and
> trailing padding which is never included in the buffer area handed to
> vsnprintf (spaces merely for clarity):
>
> pad test_buffer pad
> **** **************** ****
>
> Then vsnprintf() is invoked with a bufsize argument <=
> BUF_SIZE. Suppose bufsize=10, then we'd have e.g.
>
> |pad | test_buffer |pad |
> **** pizza0 **** ****** ****
> A B C D E
>
> where vsnprintf() was given the area from B to D.
>
> It is obviously a bug for vsnprintf to touch anything between A and B
> or between D and E. The former is checked for as one would expect. But
> for the latter, we are actually a little stricter in that we check the
> area between C and E.
>
> Split that check in two, providing a clearer error message in case it
> was a genuine buffer overrun and not merely a write within the
> provided buffer, but after the end of the generated string.
>
> So far, no part of the vsnprintf() implementation has had any use for
> using the whole buffer as scratch space, but it's not unreasonable to
> allow that, as long as the result is properly nul-terminated and the
> return value is the right one. However, it is somewhat unusual, and
> most %<something> won't need this, so keep the [C,D] check, but make
> it easy for a later patch to make that part opt-out for certain tests.
Excellent commit message.
> Signed-off-by: Rasmus Villemoes <[email protected]>
> Tested-by: Jia He <[email protected]>
> Signed-off-by: Jia He <[email protected]>
Reviewed-by: Petr Mladek <[email protected]>
Best Regards,
Petr
On 17/06/2021 16.17, Petr Mladek wrote:
> On Tue 2021-06-15 23:49:51, Jia He wrote:
>> From: Rasmus Villemoes <[email protected]>
>>
>> Before each invocation of vsnprintf(), do_test() memsets the entire
>> allocated buffer to a sentinel value. That buffer includes leading and
>> trailing padding which is never included in the buffer area handed to
>> vsnprintf (spaces merely for clarity):
>>
>> pad test_buffer pad
>> **** **************** ****
>>
>> Then vsnprintf() is invoked with a bufsize argument <=
>> BUF_SIZE. Suppose bufsize=10, then we'd have e.g.
>>
>> |pad | test_buffer |pad |
>> **** pizza0 **** ****** ****
>> A B C D E
>>
>> where vsnprintf() was given the area from B to D.
>>
>> It is obviously a bug for vsnprintf to touch anything between A and B
>> or between D and E. The former is checked for as one would expect. But
>> for the latter, we are actually a little stricter in that we check the
>> area between C and E.
>>
>> Split that check in two, providing a clearer error message in case it
>> was a genuine buffer overrun and not merely a write within the
>> provided buffer, but after the end of the generated string.
>>
>> So far, no part of the vsnprintf() implementation has had any use for
>> using the whole buffer as scratch space, but it's not unreasonable to
>> allow that, as long as the result is properly nul-terminated and the
>> return value is the right one. However, it is somewhat unusual, and
>> most %<something> won't need this, so keep the [C,D] check, but make
>> it easy for a later patch to make that part opt-out for certain tests.
>
> Excellent commit message.
>
>> Signed-off-by: Rasmus Villemoes <[email protected]>
>> Tested-by: Jia He <[email protected]>
>> Signed-off-by: Jia He <[email protected]>
>
> Reviewed-by: Petr Mladek <[email protected]>
Hi Petr
It seems Justin's series got stalled, but I still think this patch makes
sense on its own (especially since another series in flight mucks about
in this area), so can you please pick it up directly?
The lore link for the above is
https://lore.kernel.org/lkml/[email protected]/ ,
while my original submission is at
https://lore.kernel.org/lkml/[email protected]/
.
Rasmus
On 6/17/22 03:15, Rasmus Villemoes wrote:
> On 17/06/2021 16.17, Petr Mladek wrote:
>> On Tue 2021-06-15 23:49:51, Jia He wrote:
>>> From: Rasmus Villemoes <[email protected]>
>>>
>>> Before each invocation of vsnprintf(), do_test() memsets the entire
>>> allocated buffer to a sentinel value. That buffer includes leading and
>>> trailing padding which is never included in the buffer area handed to
>>> vsnprintf (spaces merely for clarity):
>>>
>>> pad test_buffer pad
>>> **** **************** ****
>>>
>>> Then vsnprintf() is invoked with a bufsize argument <=
>>> BUF_SIZE. Suppose bufsize=10, then we'd have e.g.
>>>
>>> |pad | test_buffer |pad |
>>> **** pizza0 **** ****** ****
>>> A B C D E
>>>
>>> where vsnprintf() was given the area from B to D.
>>>
>>> It is obviously a bug for vsnprintf to touch anything between A and B
>>> or between D and E. The former is checked for as one would expect. But
>>> for the latter, we are actually a little stricter in that we check the
>>> area between C and E.
>>>
>>> Split that check in two, providing a clearer error message in case it
>>> was a genuine buffer overrun and not merely a write within the
>>> provided buffer, but after the end of the generated string.
>>>
>>> So far, no part of the vsnprintf() implementation has had any use for
>>> using the whole buffer as scratch space, but it's not unreasonable to
>>> allow that, as long as the result is properly nul-terminated and the
>>> return value is the right one. However, it is somewhat unusual, and
>>> most %<something> won't need this, so keep the [C,D] check, but make
>>> it easy for a later patch to make that part opt-out for certain tests.
>>
>> Excellent commit message.
>>
>>> Signed-off-by: Rasmus Villemoes <[email protected]>
>>> Tested-by: Jia He <[email protected]>
>>> Signed-off-by: Jia He <[email protected]>
>>
>> Reviewed-by: Petr Mladek <[email protected]>
>
> Hi Petr
>
> It seems Justin's series got stalled, but I still think this patch makes
> sense on its own (especially since another series in flight mucks about
> in this area), so can you please pick it up directly?
>
> The lore link for the above is
> https://lore.kernel.org/lkml/[email protected]/ ,
> while my original submission is at
> https://lore.kernel.org/lkml/[email protected]/
That patch definitely clarifies things, feel free to add my reviewed-by tag
On Fri 2022-06-17 09:15:53, Rasmus Villemoes wrote:
> On 17/06/2021 16.17, Petr Mladek wrote:
> > On Tue 2021-06-15 23:49:51, Jia He wrote:
> >> From: Rasmus Villemoes <[email protected]>
> >>
> >> Before each invocation of vsnprintf(), do_test() memsets the entire
> >> allocated buffer to a sentinel value. That buffer includes leading and
> >> trailing padding which is never included in the buffer area handed to
> >> vsnprintf (spaces merely for clarity):
> >>
> >> pad test_buffer pad
> >> **** **************** ****
> >>
> >> Then vsnprintf() is invoked with a bufsize argument <=
> >> BUF_SIZE. Suppose bufsize=10, then we'd have e.g.
> >>
> >> |pad | test_buffer |pad |
> >> **** pizza0 **** ****** ****
> >> A B C D E
> >>
> >> where vsnprintf() was given the area from B to D.
> >>
> >> It is obviously a bug for vsnprintf to touch anything between A and B
> >> or between D and E. The former is checked for as one would expect. But
> >> for the latter, we are actually a little stricter in that we check the
> >> area between C and E.
> >>
> >> Split that check in two, providing a clearer error message in case it
> >> was a genuine buffer overrun and not merely a write within the
> >> provided buffer, but after the end of the generated string.
> >>
> >> So far, no part of the vsnprintf() implementation has had any use for
> >> using the whole buffer as scratch space, but it's not unreasonable to
> >> allow that, as long as the result is properly nul-terminated and the
> >> return value is the right one. However, it is somewhat unusual, and
> >> most %<something> won't need this, so keep the [C,D] check, but make
> >> it easy for a later patch to make that part opt-out for certain tests.
> >
> > Excellent commit message.
> >
> >> Signed-off-by: Rasmus Villemoes <[email protected]>
> >> Tested-by: Jia He <[email protected]>
> >> Signed-off-by: Jia He <[email protected]>
> >
> > Reviewed-by: Petr Mladek <[email protected]>
>
> Hi Petr
>
> It seems Justin's series got stalled, but I still think this patch makes
> sense on its own (especially since another series in flight mucks about
> in this area), so can you please pick it up directly?
I have just committed this patch into printk/linux.git, branch for-5.20.
I am sorry that it took so long. Too many things have happened during
last weeks.
Best Regards,
Petr