Re: [PATCH] 9p: dynamically allocate directory entry names

Re: [PATCH] 9p: dynamically allocate directory entry names

[Dominique Martinet]

+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

Re: [PATCH] 9p: dynamically allocate directory entry names

[Jan Kara]

On Mon 14-09-26 00:12:25, Dominique Martinet wrote:
> +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?

So my take on this is that I'd refuse any entries larger than NAME_MAX
(255). As you say it is very likely something in userspace or other parts
of the kernel (fs drivers, ...) will break with such names so I don't think
there's a good reason to provoke these bugs. For example POSIX states
readdir(2) should be returning component of at most NAME_MAX length and
although we don't take POSIX very seriously in the kernel in this case I
don't think there's a good enough reason to deviate from it.

								Honza

> 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
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [PATCH] 9p: dynamically allocate directory entry names

[Dominique Martinet]

Jan Kara wrote on Wed, Sep 16, 2026 at 11:47:00AM +0200:
> On Mon 14-09-26 00:12:25, Dominique Martinet wrote:
> > +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?
> 
> So my take on this is that I'd refuse any entries larger than NAME_MAX
> (255). As you say it is very likely something in userspace or other parts
> of the kernel (fs drivers, ...) will break with such names so I don't think
> there's a good reason to provoke these bugs. For example POSIX states
> readdir(2) should be returning component of at most NAME_MAX length and
> although we don't take POSIX very seriously in the kernel in this case I
> don't think there's a good enough reason to deviate from it.

Thanks for taking the time to confirm this! Let's go with that direction
then.


@hoobnn: as said in my previous mail I can't take your patch unless you
give me a name, but the idea behind the patch is trivial so if I don't
hear back from you in say ~2 weeks I'll send a patch under my name that
reimplements what you did, plus check the strlen() result that's already
done in v9fs_dir_readdir_dotl() so entries > NAME_MAX are skipped over.

If you reply then there's no hurry on my end and you can take your time,
please send a v2 of this as two patches, one skipping the extra copy
into p9_dirent as done here and one for the limit

-- 
Dominique