Background
==========
Linus suggested printing the full path of 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_unsafe() which is similar to d_path() except it doesn't take any
seqlock/spinlock.
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. kasprintf
TODO
====
I plan to do the followup work after '%pD' behavior is changed.
- s390/hmcdrv: remove the redundant directory path in printing string.
- fs/iomap: simplify the iomap_swapfile_fail() with '%pD'.
- simplify the string printing when file_path() is invoked(in some
cases, not all).
- change the previous '%pD[2,3,4]' to '%pD'
Changelog
=========
v7:
- rebase to 5.14-rc1 after Al Viro d_path cleanup series was merged
- add patch1/5 to fix the kernel doc validator issue
- refine the commit msg, add more comments for the smp_load_acquire in
prepend_name_with_len()
v6(new v2):https://lkml.org/lkml/2021/6/23/44
- refine the commit msg/comments (Andy)
- pass the validator check by "make C=1 W=1"
- add the R-b for patch 4/4 from Andy
v5(new v1): https://lkml.org/lkml/2021/6/22/680
- remove the RFC tag
- refine the commit msg/comments(by Petr, Andy)
- make using_scratch_space a new parameter of the test case
RFCv4:
- don't support spec.precision anymore for '%pD'
- add Rasmus's patch into this series
RFCv3:
- 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.
RFCv2:
- 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
RFCv1: https://lkml.org/lkml/2021/5/8/122
Jia He (4):
d_path: fix Kernel doc validator complaints
d_path: introduce helper d_path_unsafe()
lib/vsprintf.c: make '%pD' print the full path of file
lib/test_printf.c: add test cases for '%pD'
Rasmus Villemoes (1):
lib/test_printf.c: split write-beyond-buffer check in two
Documentation/core-api/printk-formats.rst | 7 +-
fs/d_path.c | 123 ++++++++++++++++++++--
include/linux/dcache.h | 1 +
lib/test_printf.c | 54 ++++++++--
lib/vsprintf.c | 40 ++++++-
5 files changed, 202 insertions(+), 23 deletions(-)
--
2.17.1
After the behaviour of specifier '%pD' is changed to print the full path
of struct file, the related test cases are also updated.
Given the full path string of '%pD' is prepended from the end of the scratch
buffer, the check of "wrote beyond the nul-terminator" should be skipped
for '%pD'.
Parameterize the new using_scratch_space in __test(), do_test() and wrapper
macros to skip the test case mentioned above,
Signed-off-by: Jia He <[email protected]>
Reviewed-by: Andy Shevchenko <[email protected]>
---
lib/test_printf.c | 49 +++++++++++++++++++++++++++++++++++++----------
1 file changed, 39 insertions(+), 10 deletions(-)
diff --git a/lib/test_printf.c b/lib/test_printf.c
index cabdf9f5fd15..381c9dcd3c2d 100644
--- a/lib/test_printf.c
+++ b/lib/test_printf.c
@@ -16,6 +16,7 @@
#include <linux/bitmap.h>
#include <linux/dcache.h>
+#include <linux/fs.h>
#include <linux/socket.h>
#include <linux/in.h>
@@ -37,8 +38,8 @@ static char *alloced_buffer __initdata;
extern bool no_hash_pointers;
-static int __printf(4, 0) __init
-do_test(int bufsize, const char *expect, int elen,
+static int __printf(5, 0) __init
+do_test(int bufsize, const char *expect, int elen, bool using_scratch_space,
const char *fmt, va_list ap)
{
va_list aq;
@@ -78,7 +79,7 @@ do_test(int bufsize, const char *expect, int elen,
return 1;
}
- if (memchr_inv(test_buffer + written + 1, FILL_CHAR, bufsize - (written + 1))) {
+ if (!using_scratch_space && 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;
@@ -97,8 +98,9 @@ do_test(int bufsize, const char *expect, int elen,
return 0;
}
-static void __printf(3, 4) __init
-__test(const char *expect, int elen, const char *fmt, ...)
+static void __printf(4, 5) __init
+__test(const char *expect, int elen, bool using_scratch_space,
+ const char *fmt, ...)
{
va_list ap;
int rand;
@@ -119,11 +121,11 @@ __test(const char *expect, int elen, const char *fmt, ...)
* enough and 0), and then we also test that kvasprintf would
* be able to print it as expected.
*/
- failed_tests += do_test(BUF_SIZE, expect, elen, fmt, ap);
+ failed_tests += do_test(BUF_SIZE, expect, elen, using_scratch_space, fmt, ap);
rand = 1 + prandom_u32_max(elen+1);
/* Since elen < BUF_SIZE, we have 1 <= rand <= BUF_SIZE. */
- failed_tests += do_test(rand, expect, elen, fmt, ap);
- failed_tests += do_test(0, expect, elen, fmt, ap);
+ failed_tests += do_test(rand, expect, elen, using_scratch_space, fmt, ap);
+ failed_tests += do_test(0, expect, elen, using_scratch_space, fmt, ap);
p = kvasprintf(GFP_KERNEL, fmt, ap);
if (p) {
@@ -138,8 +140,15 @@ __test(const char *expect, int elen, const char *fmt, ...)
va_end(ap);
}
+/*
+ * More relaxed test for non-standard formats that are using the provided buffer
+ * as a scratch space and write beyond the trailing '\0'.
+ */
+#define test_using_scratch_space(expect, fmt, ...) \
+ __test(expect, strlen(expect), true, fmt, ##__VA_ARGS__)
+
#define test(expect, fmt, ...) \
- __test(expect, strlen(expect), fmt, ##__VA_ARGS__)
+ __test(expect, strlen(expect), false, fmt, ##__VA_ARGS__)
static void __init
test_basic(void)
@@ -150,7 +159,7 @@ test_basic(void)
test("", &nul);
test("100%", "100%%");
test("xxx%yyy", "xxx%cyyy", '%');
- __test("xxx\0yyy", 7, "xxx%cyyy", '\0');
+ __test("xxx\0yyy", 7, false, "xxx%cyyy", '\0');
}
static void __init
@@ -501,6 +510,25 @@ dentry(void)
test(" bravo/alfa| bravo/alfa", "%12pd2|%*pd2", &test_dentry[2], 12, &test_dentry[2]);
}
+static struct vfsmount test_vfsmnt __initdata = {};
+
+static struct file test_file __initdata = {
+ .f_path = { .dentry = &test_dentry[2],
+ .mnt = &test_vfsmnt,
+ },
+};
+
+static void __init
+f_d_path(void)
+{
+ test("(null)", "%pD", NULL);
+ test("(efault)", "%pD", PTR_INVALID);
+
+ test_using_scratch_space("/bravo/alfa |/bravo/alfa ", "%-14pD|%*pD", &test_file, -14, &test_file);
+ test_using_scratch_space(" /bravo/alfa| /bravo/alfa", "%14pD|%*pD", &test_file, 14, &test_file);
+ test_using_scratch_space(" /bravo/alfa|/bravo/alfa ", "%14pD|%-14pD", &test_file, &test_file);
+}
+
static void __init
struct_va_format(void)
{
@@ -789,6 +817,7 @@ test_pointer(void)
ip();
uuid();
dentry();
+ f_d_path();
struct_va_format();
time_and_date();
struct_clk();
--
2.17.1
On Thu 2021-07-15 09:14:07, Jia He wrote:
> After the behaviour of specifier '%pD' is changed to print the full path
> of struct file, the related test cases are also updated.
>
> Given the full path string of '%pD' is prepended from the end of the scratch
> buffer, the check of "wrote beyond the nul-terminator" should be skipped
> for '%pD'.
>
> Parameterize the new using_scratch_space in __test(), do_test() and wrapper
> macros to skip the test case mentioned above,
>
> Signed-off-by: Jia He <[email protected]>
> Reviewed-by: Andy Shevchenko <[email protected]>
Reviewed-by: Petr Mladek <[email protected]>
Best Regards,
Petr
On Thu 2021-07-15 09:14:02, Jia He wrote:
> Background
> ==========
> Linus suggested printing the full path of 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_unsafe() which is similar to d_path() except it doesn't take any
> seqlock/spinlock.
>
> TODO
> ====
> I plan to do the followup work after '%pD' behavior is changed.
> - s390/hmcdrv: remove the redundant directory path in printing string.
> - fs/iomap: simplify the iomap_swapfile_fail() with '%pD'.
> - simplify the string printing when file_path() is invoked(in some
> cases, not all).
> - change the previous '%pD[2,3,4]' to '%pD'
>
> Jia He (4):
> d_path: fix Kernel doc validator complaints
> d_path: introduce helper d_path_unsafe()
> lib/vsprintf.c: make '%pD' print the full path of file
> lib/test_printf.c: add test cases for '%pD'
>
> Rasmus Villemoes (1):
> lib/test_printf.c: split write-beyond-buffer check in two
The patchset is ready for linux-next from my POV.
I could take it via printk tree if Al provides Acks for the
first two "d_path: *: patches.
Or Al could take it via his tree if he would prefer to do so.
Best Regards,
Petr
Hi Al
> -----Original Message-----
> From: Petr Mladek <[email protected]>
> Sent: Wednesday, July 21, 2021 10:12 PM
> To: Justin He <[email protected]>; Alexander Viro <[email protected]>
> Cc: Steven Rostedt <[email protected]>; Sergey Senozhatsky
> <[email protected]>; Andy Shevchenko
> <[email protected]>; Rasmus Villemoes
> <[email protected]>; Jonathan Corbet <[email protected]>; Linus
> Torvalds <[email protected]>; Peter Zijlstra (Intel)
> <[email protected]>; Eric Biggers <[email protected]>; Ahmed S.
> Darwish <[email protected]>; [email protected]; linux-
> [email protected]; [email protected]; Matthew Wilcox
> <[email protected]>; Christoph Hellwig <[email protected]>; nd
> <[email protected]>
> Subject: Re: [PATCH v7 0/5] make '%pD' print the full path of file
>
> On Thu 2021-07-15 09:14:02, Jia He wrote:
> > Background
> > ==========
> > Linus suggested printing the full path of 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_unsafe() which is similar to d_path() except it doesn't take any
> > seqlock/spinlock.
> >
> > TODO
> > ====
> > I plan to do the followup work after '%pD' behavior is changed.
> > - s390/hmcdrv: remove the redundant directory path in printing string.
> > - fs/iomap: simplify the iomap_swapfile_fail() with '%pD'.
> > - simplify the string printing when file_path() is invoked(in some
> > cases, not all).
> > - change the previous '%pD[2,3,4]' to '%pD'
> >
> > Jia He (4):
> > d_path: fix Kernel doc validator complaints
> > d_path: introduce helper d_path_unsafe()
> > lib/vsprintf.c: make '%pD' print the full path of file
> > lib/test_printf.c: add test cases for '%pD'
> >
> > Rasmus Villemoes (1):
> > lib/test_printf.c: split write-beyond-buffer check in two
>
> The patchset is ready for linux-next from my POV.
>
> I could take it via printk tree if Al provides Acks for the
> first two "d_path: *: patches.
>
> Or Al could take it via his tree if he would prefer to do so.
Kindly ping ????
--
Cheers,
Justin (Jia He)