linux-kernel - Re: [PATCH 2/2] tools/nolibc: add support for directory access

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <20250201104659.GA8168@1wt.eu>
Date: Sat, 1 Feb 2025 11:46:59 +0100
From: Willy Tarreau <w@....eu>
To: Thomas Weißschuh <linux@...ssschuh.net>
Cc: Shuah Khan <shuah@...nel.org>, linux-kernel@...r.kernel.org,
        linux-kselftest@...r.kernel.org,
        Thomas Weißschuh <thomas.weissschuh@...utronix.de>
Subject: Re: [PATCH 2/2] tools/nolibc: add support for directory access

On Sat, Feb 01, 2025 at 11:41:58AM +0100, Thomas Weißschuh wrote:
> On 2025-02-01 11:34:38+0100, Willy Tarreau wrote:
> > On Thu, Jan 30, 2025 at 08:54:03PM +0100, Thomas Weißschuh wrote:
> > > From: Thomas Weißschuh <thomas.weissschuh@...utronix.de>
> > > 
> > > Add an allocation-free implementation of readdir() and related
> > > functions. The implementation is modelled after the one for FILE.
> > 
> > I think you'd need to mention/remind the two important points that
> > come out of that choice, one being that DIR is a fake pointer that
> > instead stores ~fd so that it can be turned back to a valid FD, and
> > that subsequent readdir() calls will only work from the same file
> > unit since it relies on a local static storage.
> 
> Point one is true.
> Point two not so much. It is fine to have multiple directories open at
> the same time. All state is kept inside the kernel.
> Only the *current* return value is in the static buffer.

Excellent point! It also needs to be mentioned.

> So multithreading wouldn't work, but nolibc doesn't support that
> anyways.

Not only that, but it also means recursive directory scanning is not
possible either, this definitely deserve being mentioned!

That's something we could improve in the future if there is some demand,
by also keeping a static "level" and use a hand-built stack. But let's
not overengineer something that nobody asked for yet ;-)

> > > I'm not entirely sure where to put it. It doesn't really belong into
> > > stdio.h, but that's where the FILE stuff is.
> > > sys.h wants alphabetical ordering, but IMO these functions should stick
> > > together.
> > 
> > My man pages suggest that userland code will include <dirent.h>, thus
> > I think it could be the moment to create it with that new code.
> 
> Ack. Now that you suggest it, it seems obvious.

Obvious is always the hardest thing to find as it's assumed to be known
and is rarely documented ;-)

> > I'm uncertain where NAME_MAX is defined, I haven't found it in the
> > nolibc sources, just double-checking that it's not just in your build
> > environment by accident.
> 
> It comes from linux/limits.h. I don't think it can ever go away.

Ah fine then!

> > Just out of curiosity, could this copy fail, and if so, should we handle
> > it (e.g. NAME_MAX != 255) ? My guess here is that if it could almost never
> > fail and checking it would needlessly complicate the function, let's just
> > handle it with a comment for now. And if it cannot at all, let's mention
> > why on top of it as well.
> 
> If NAME_MAX changes it would be a userspace regression IMO.
> I'll add a comment.

Perfect! Consider an acked-by from me on the next one as I'm generally
fine with the principles.

Willy