[RFC][PATCH] fix io_uring_show_fdinfo() misuse of ->d_iname

[RFC][PATCH] fix io_uring_show_fdinfo() misuse of ->d_iname

[Al Viro]

	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.

Re: [RFC][PATCH] fix io_uring_show_fdinfo() misuse of ->d_iname

[Jens Axboe]

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!

-- 
Jens Axboe

Re: [RFC][PATCH] fix io_uring_show_fdinfo() misuse of ->d_iname

[Al Viro]

	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++) {

Re: [RFC][PATCH] fix io_uring_show_fdinfo() misuse of ->d_iname

[Jens Axboe]

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.

-- 
Jens Axboe