| Message ID | 20250118025717.GU1977892@ZenIV (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [RFC] fix io_uring_show_fdinfo() misuse of ->d_iname | expand |
On 1/17/25 7:57 PM, Al Viro wrote: > The output of io_uring_show_fdinfo() is crazy - for > each slot of io_uring file_table it produces either > INDEX: <none> > or > INDEX: NAME > where INDEX runs through all numbers from 0 to ctx->file_table.data.nr-1 > and NAME is usually the last component of pathname of file in slot > #INDEX. Usually == if it's no longer than 39 bytes. If it's longer, > you get junk. Oh, and if it contains newlines, you get several lines and > no way to tell that it has happened, them's the breaks. If it's happens > to be /home/luser/<none>, well, what you see is indistinguishable from what > you'd get if it hadn't been there... > > According to Jens, it's *not* cast in stone, so we should be able to > change that to something saner. I see two options: > > 1) replace NAME with actual pathname of the damn file, quoted to reasonable > extent. > > 2) same, and skip the INDEX: <none> lines. It's not as if they contained > any useful information - the size of table is printed right before that, > so you'd get > > ... > UserFiles: 16 > 0: foo > 11: bar > UserBufs: .... > > instead of > > ... > UserFiles: 16 > 0: foo > 1: <none> > 2: <none> > 3: <none> > 4: <none> > 5: <none> > 6: <none> > 7: <none> > 8: <none> > 9: <none> > 10: <none> > 11: bar > 12: <none> > 13: <none> > 14: <none> > 15: <none> > UserBufs: .... > > IMO the former is more useful for any debugging purposes. > > The patch is trivial either way - (1) is > ------------------------ > diff --git a/io_uring/fdinfo.c b/io_uring/fdinfo.c > index b214e5a407b5..1017249ae610 100644 > --- a/io_uring/fdinfo.c > +++ b/io_uring/fdinfo.c > @@ -211,10 +211,12 @@ __cold void io_uring_show_fdinfo(struct seq_file *m, struct file *file) > > if (ctx->file_table.data.nodes[i]) > f = io_slot_file(ctx->file_table.data.nodes[i]); > + seq_printf(m, "%5u: ", i); > if (f) > - seq_printf(m, "%5u: %s\n", i, file_dentry(f)->d_iname); > + seq_file_path(m, f, " \t\n\\<"); > else > - seq_printf(m, "%5u: <none>\n", i); > + seq_puts(m, "<none>"); > + seq_puts(m, "\n"); > } > seq_printf(m, "UserBufs:\t%u\n", ctx->buf_table.nr); > for (i = 0; has_lock && i < ctx->buf_table.nr; i++) { > ------------------------ > and (2) - > ------------------------ > diff --git a/io_uring/fdinfo.c b/io_uring/fdinfo.c > index b214e5a407b5..f60d0a9d505e 100644 > --- a/io_uring/fdinfo.c > +++ b/io_uring/fdinfo.c > @@ -211,10 +211,11 @@ __cold void io_uring_show_fdinfo(struct seq_file *m, struct file *file) > > if (ctx->file_table.data.nodes[i]) > f = io_slot_file(ctx->file_table.data.nodes[i]); > - if (f) > - seq_printf(m, "%5u: %s\n", i, file_dentry(f)->d_iname); > - else > - seq_printf(m, "%5u: <none>\n", i); > + if (f) { > + seq_printf(m, "%5u: ", i); > + seq_file_path(m, f, " \t\n\\"); > + seq_puts(m, "\n"); > + } > } > seq_printf(m, "UserBufs:\t%u\n", ctx->buf_table.nr); > for (i = 0; has_lock && i < ctx->buf_table.nr; i++) { > ------------------------ > > Preferences? The difference in seq_printf() argument is due to the need > to quote < in filenames if we need to distinguish them from <none>; > whitespace and \ needs to be quoted in either case. I like #2, there's no reason to dump the empty nodes. Thanks Al!
Output of io_uring_show_fdinfo() has several problems:
* racy use of ->d_iname
* junk if the name is long - in that case it's not stored in ->d_iname
at all
* lack of quoting (names can contain newlines, etc. - or be equal to "<none>",
for that matter).
* lines for empty slots are pointless noise - we already have the total
amount, so having just the non-empty ones would carry the same information.
Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
---
diff --git a/io_uring/fdinfo.c b/io_uring/fdinfo.c
index b214e5a407b5..f60d0a9d505e 100644
--- a/io_uring/fdinfo.c
+++ b/io_uring/fdinfo.c
@@ -211,10 +211,11 @@ __cold void io_uring_show_fdinfo(struct seq_file *m, struct file *file)
if (ctx->file_table.data.nodes[i])
f = io_slot_file(ctx->file_table.data.nodes[i]);
- if (f)
- seq_printf(m, "%5u: %s\n", i, file_dentry(f)->d_iname);
- else
- seq_printf(m, "%5u: <none>\n", i);
+ if (f) {
+ seq_printf(m, "%5u: ", i);
+ seq_file_path(m, f, " \t\n\\");
+ seq_puts(m, "\n");
+ }
}
seq_printf(m, "UserBufs:\t%u\n", ctx->buf_table.nr);
for (i = 0; has_lock && i < ctx->buf_table.nr; i++) {
On 1/18/25 8:26 PM, Al Viro wrote: > Output of io_uring_show_fdinfo() has several problems: > * racy use of ->d_iname > * junk if the name is long - in that case it's not stored in ->d_iname > at all > * lack of quoting (names can contain newlines, etc. - or be equal to "<none>", > for that matter). > * lines for empty slots are pointless noise - we already have the total > amount, so having just the non-empty ones would carry the same information. Thanks Al, I'll queue this up.
diff --git a/io_uring/fdinfo.c b/io_uring/fdinfo.c index b214e5a407b5..1017249ae610 100644 --- a/io_uring/fdinfo.c +++ b/io_uring/fdinfo.c @@ -211,10 +211,12 @@ __cold void io_uring_show_fdinfo(struct seq_file *m, struct file *file) if (ctx->file_table.data.nodes[i]) f = io_slot_file(ctx->file_table.data.nodes[i]); + seq_printf(m, "%5u: ", i); if (f) - seq_printf(m, "%5u: %s\n", i, file_dentry(f)->d_iname); + seq_file_path(m, f, " \t\n\\<"); else - seq_printf(m, "%5u: <none>\n", i); + seq_puts(m, "<none>"); + seq_puts(m, "\n"); } seq_printf(m, "UserBufs:\t%u\n", ctx->buf_table.nr); for (i = 0; has_lock && i < ctx->buf_table.nr; i++) { ------------------------ and (2) - ------------------------ diff --git a/io_uring/fdinfo.c b/io_uring/fdinfo.c index b214e5a407b5..f60d0a9d505e 100644 --- a/io_uring/fdinfo.c +++ b/io_uring/fdinfo.c @@ -211,10 +211,11 @@ __cold void io_uring_show_fdinfo(struct seq_file *m, struct file *file) if (ctx->file_table.data.nodes[i]) f = io_slot_file(ctx->file_table.data.nodes[i]); - if (f) - seq_printf(m, "%5u: %s\n", i, file_dentry(f)->d_iname); - else - seq_printf(m, "%5u: <none>\n", i); + if (f) { + seq_printf(m, "%5u: ", i); + seq_file_path(m, f, " \t\n\\"); + seq_puts(m, "\n"); + } } seq_printf(m, "UserBufs:\t%u\n", ctx->buf_table.nr); for (i = 0; has_lock && i < ctx->buf_table.nr; i++) {
The output of io_uring_show_fdinfo() is crazy - for each slot of io_uring file_table it produces either INDEX: <none> or INDEX: NAME where INDEX runs through all numbers from 0 to ctx->file_table.data.nr-1 and NAME is usually the last component of pathname of file in slot #INDEX. Usually == if it's no longer than 39 bytes. If it's longer, you get junk. Oh, and if it contains newlines, you get several lines and no way to tell that it has happened, them's the breaks. If it's happens to be /home/luser/<none>, well, what you see is indistinguishable from what you'd get if it hadn't been there... According to Jens, it's *not* cast in stone, so we should be able to change that to something saner. I see two options: 1) replace NAME with actual pathname of the damn file, quoted to reasonable extent. 2) same, and skip the INDEX: <none> lines. It's not as if they contained any useful information - the size of table is printed right before that, so you'd get ... UserFiles: 16 0: foo 11: bar UserBufs: .... instead of ... UserFiles: 16 0: foo 1: <none> 2: <none> 3: <none> 4: <none> 5: <none> 6: <none> 7: <none> 8: <none> 9: <none> 10: <none> 11: bar 12: <none> 13: <none> 14: <none> 15: <none> UserBufs: .... IMO the former is more useful for any debugging purposes. The patch is trivial either way - (1) is ------------------------ ------------------------ Preferences? The difference in seq_printf() argument is due to the need to quote < in filenames if we need to distinguish them from <none>; whitespace and \ needs to be quoted in either case.