* [PATCH] 9p: dynamically allocate directory entry names
@ 2026-08-26 6:48 hoobnn
2026-09-13 15:12 ` Dominique Martinet
0 siblings, 1 reply; 2+ messages in thread
From: hoobnn @ 2026-08-26 6:48 UTC (permalink / raw)
To: ericvh, lucho, asmadeus, v9fs
Cc: linux_oss, davem, edumazet, kuba, pabeni, horms, sripathik,
aneesh.kumar, netdev, linux-kernel, hoobnn
p9dirent_read() copies names into a fixed 256-byte buffer. A name that is
valid on the host filesystem but longer than that buffer makes strscpy()
fail and aborts getdents64(), hiding the remaining directory entries.
Keep ownership of the protocol-allocated string in p9_dirent and free it
after dir_emit() consumes the name.
Fixes: 7751bdb3a095 ("9p: readdir implementation for 9p2000.L")
Closes: https://github.com/microsoft/WSL/issues/41192
Assisted-by: Codex:gpt-5
Signed-off-by: hoobnn <853555@gmail.com>
---
fs/9p/vfs_dir.c | 6 +++++-
include/net/9p/client.h | 2 +-
net/9p/protocol.c | 12 ++----------
3 files changed, 8 insertions(+), 12 deletions(-)
diff --git a/fs/9p/vfs_dir.c b/fs/9p/vfs_dir.c
index e0d34e4e9076..af00b79d801e 100644
--- a/fs/9p/vfs_dir.c
+++ b/fs/9p/vfs_dir.c
@@ -185,8 +185,12 @@ static int v9fs_dir_readdir_dotl(struct file *file, struct dir_context *ctx)
if (!dir_emit(ctx, curdirent.d_name,
strlen(curdirent.d_name),
QID2INO(&curdirent.qid),
- curdirent.d_type))
+ curdirent.d_type)) {
+ kfree(curdirent.d_name);
return 0;
+ }
+
+ kfree(curdirent.d_name);
ctx->pos = curdirent.d_off;
rdir->head += err;
diff --git a/include/net/9p/client.h b/include/net/9p/client.h
index 838a94218b59..9f3079c6f386 100644
--- a/include/net/9p/client.h
+++ b/include/net/9p/client.h
@@ -268,7 +268,7 @@ struct p9_dirent {
struct p9_qid qid;
u64 d_off;
unsigned char d_type;
- char d_name[256];
+ char *d_name;
};
struct iov_iter;
diff --git a/net/9p/protocol.c b/net/9p/protocol.c
index 67b0586d807f..d4335884e0c0 100644
--- a/net/9p/protocol.c
+++ b/net/9p/protocol.c
@@ -770,7 +770,7 @@ int p9dirent_read(struct p9_client *clnt, char *buf, int len,
{
struct p9_fcall fake_pdu;
int ret;
- char *nameptr;
+ char *nameptr = NULL;
fake_pdu.size = len;
fake_pdu.capacity = len;
@@ -785,15 +785,7 @@ int p9dirent_read(struct p9_client *clnt, char *buf, int len,
return ret;
}
- ret = strscpy(dirent->d_name, nameptr, sizeof(dirent->d_name));
- if (ret < 0) {
- p9_debug(P9_DEBUG_ERROR,
- "On the wire dirent name too long: %s\n",
- nameptr);
- kfree(nameptr);
- return ret;
- }
- kfree(nameptr);
+ dirent->d_name = nameptr;
return fake_pdu.offset;
}
base-commit: 028ef9c96e96197026887c0f092424679298aae8
--
2.54.0 (Apple Git-157)
^ permalink raw reply [flat|nested] 2+ messages in thread
* Re: [PATCH] 9p: dynamically allocate directory entry names
2026-08-26 6:48 [PATCH] 9p: dynamically allocate directory entry names hoobnn
@ 2026-09-13 15:12 ` Dominique Martinet
0 siblings, 0 replies; 2+ messages in thread
From: Dominique Martinet @ 2026-09-13 15:12 UTC (permalink / raw)
To: hoobnn
Cc: ericvh, lucho, v9fs, linux_oss, davem, edumazet, kuba, pabeni,
horms, sripathik, aneesh.kumar, netdev, linux-kernel,
linux-fsdevel, Matthew Wilcox, David Howells, Alexander Viro,
Christian Brauner, Jan Kara
+fsdevel@ & vfs maintainers to Cc TL;DR:
Should the 9p .iterate_shared() dir_emit files with path
components > NAME_LEN, as allowed in fs/readdir.c verify_dirent_name()
only enforcing length < PATH_MAX, or should we skip any such entry given
nothing can interact with them later anyway?
hoobnn wrote on Wed, Aug 26, 2026 at 02:48:19PM +0800:
> p9dirent_read() copies names into a fixed 256-byte buffer. A name that is
> valid on the host filesystem but longer than that buffer makes strscpy()
> fail and aborts getdents64(), hiding the remaining directory entries.
>
> Keep ownership of the protocol-allocated string in p9_dirent and free it
> after dir_emit() consumes the name.
>
> Fixes: 7751bdb3a095 ("9p: readdir implementation for 9p2000.L")
> Closes: https://github.com/microsoft/WSL/issues/41192
> Assisted-by: Codex:gpt-5
> Signed-off-by: hoobnn <853555@gmail.com>
Sorry, but pseudonyms are not allowed to contribute to the kernel:
please use a real name here.
Okay, I've just tried with a dummy server that adds a few bytes to every
filenames, and it's weirder than I expected... The vfs only checks for
PATH_MAX length, not NAME_MAX, so e.g. getdents64 will list the long
file name but then any operation on that file name will fail with
ENAMETOOLONG...
I think it'd make sense to be more clear about this in the commit
message.
(The patch subject is also pretty bad, by the way, in that "dynamically
allocate ..." sounds like it adds an extra allocation whereas it just
reuses the one that exists, so it should be more like "9p: skip
intermediate directory entry name copy in p9dirent_read()" and explain a
bit more how the current readdir processing works in the commit message
e.g. first alloc+copy in p9pdu_vreadf etc)
Anyway, code-wise:
- p9_dirent are very temporary and are meant to get freed immediately,
I think it's perfectly fine to skip the strscpy here and pass the
pointer directly.
- I'd like a second opinion on whether it's acceptable that we return a
file whose lenght is > NAME_MAX to userspace here; we're already calling
strlen() again in v9fs_dir_readdir_dotl() so we could just skip the
dir_emit() call here if required.
Thanks,
--
Dominique Martinet | Asmadeus
^ permalink raw reply [flat|nested] 2+ messages in thread
end of thread, other threads:[~2026-09-13 15:12 UTC | newest]
Thread overview: 2+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2026-08-26 6:48 [PATCH] 9p: dynamically allocate directory entry names hoobnn
2026-09-13 15:12 ` Dominique Martinet
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox
all inboxes | Powered by JetHome®