On Sat, 8 Nov 2025 at 22:38, Al Viro <[email protected]> wrote:
>
> These days we have very few places that import filename more than once
> (9 functions total) and it's easy to massage them so we get rid of all
> re-imports. With that done, we don't need audit_reusename() anymore.
> There's no need to memorize userland pointer either.
Lovely. Ack on the whole series.
I do wonder if we could go one step further, and try to make the
"struct filename" allocation rather much smaller, so that we could fit
it on the stack,m and avoid the whole __getname() call *entirely* for
shorter pathnames.
That __getname() allocation is fairly costly, and 99% of the time we
really don't need it because audit doesn't even get a ref to it so
it's all entirely thread-local.
Right now the allocation is a full page, which is almost entirely for
historical reasons ("__getname()" long long ago used to be
"__get_free_page()"m and then when it was made a kmemc_cache_alloc()
it just stayed page-sized, and we did replaced the size to PATH_MAX
and limited it to 4k regardless of page size - and then with the
embedded 'struct filename' we now have that odd
#define EMBEDDED_NAME_MAX (PATH_MAX - offsetof(struct
filename, iname))
and that PATH_MAX thing really is a random value these days, because
the size of the __getname() allocation has *NOTHING* to do with the
maximum pathname, and we actually have to do a *separate* allocation
if we have a long path that needs the whole PATH_MAX.
Now, that separate allocation we do oddly - in that we actually
continue to use that '__getname() allocation for the pathname, and the
new allocation is just for the initial part of 'struct filename'. But
it's *odd* and purely due to those historical oddities. We could make
the new allocation be the actual PATH_MAX size, and continue to use
the smaller original allocation for 'struct filename', and the code in
getname_flags() would be a lot more logical.
Now, for all the same historical reasons there are a few users that
mis-use "__getname()" and "__putname()" to *not* allocate an actual
'struct filename', but really just do a "kmalloc(PATH_MAX)". The fix
for that is to just leave "__getname()/__putname()" as that odd legacy
"allocate a pathname", and just make the actual real "struct filename"
use proper allocators with proper type checking.
The attached patch is ENTIRELY UNTESTED, so please see it as a
"something like this". But wouldn't it be really nice to not play
those odd games with "struct filename" that getname_flags() currently
plays? And with this, 'struct filename' is small enough that we
*could* just allocate it on the stack if we then also add code to deal
with the audit case (which this does *not* do, just to clarify: this
is literally just the "prep for a smaller structure" part).
Also note that this assumes that short pathname (smaller than that new
#define EMBEDDED_NAME_MAX 64
size) are actually the common case. With longer paths, and without the
"allocate on stack", this patch will cause two allocations, because it
then does that
kname = kmalloc(PATH_MAX, GFP_KERNEL);
to allocate the separate name when it didn't fit in the smaller
embedded path buffer. So in this form, this is actually a
pessimization, and again, none of this makes sense *unless* we then go
on to allocate the smaller filename struct on the stack.
Hmm? Comments?
Linus
fs/dcache.c | 8 ++++---
fs/namei.c | 61 +++++++++++++++++++++---------------------------------
include/linux/fs.h | 18 +++++++++++++---
3 files changed, 44 insertions(+), 43 deletions(-)
diff --git a/fs/dcache.c b/fs/dcache.c
index 035cccbc9276..9deaa26d0f46 100644
--- a/fs/dcache.c
+++ b/fs/dcache.c
@@ -3246,7 +3246,7 @@ static void __init dcache_init(void)
runtime_const_init(ptr, dentry_hashtable);
}
-/* SLAB cache for __getname() consumers */
+/* SLAB cache for alloc_filename() consumers */
struct kmem_cache *names_cachep __ro_after_init;
EXPORT_SYMBOL(names_cachep);
@@ -3263,8 +3263,10 @@ void __init vfs_caches_init_early(void)
void __init vfs_caches_init(void)
{
- names_cachep = kmem_cache_create_usercopy("names_cache", PATH_MAX, 0,
- SLAB_HWCACHE_ALIGN|SLAB_PANIC, 0, PATH_MAX, NULL);
+ names_cachep = kmem_cache_create_usercopy("names_cache",
+ sizeof(struct filename), 0, SLAB_PANIC,
+ offsetof(struct filename, iname), EMBEDDED_NAME_MAX,
+ NULL);
dcache_init();
inode_init();
diff --git a/fs/namei.c b/fs/namei.c
index 7377020a2cba..739f70372d92 100644
--- a/fs/namei.c
+++ b/fs/namei.c
@@ -123,8 +123,6 @@
* PATH_MAX includes the nul terminator --RR.
*/
-#define EMBEDDED_NAME_MAX (PATH_MAX - offsetof(struct filename, iname))
-
static inline void initname(struct filename *name, const char __user *uptr)
{
name->uptr = uptr;
@@ -143,7 +141,7 @@ getname_flags(const char __user *filename, int flags)
if (result)
return result;
- result = __getname();
+ result = alloc_filename();
if (unlikely(!result))
return ERR_PTR(-ENOMEM);
@@ -160,13 +158,13 @@ getname_flags(const char __user *filename, int flags)
*/
if (unlikely(len <= 0)) {
if (unlikely(len < 0)) {
- __putname(result);
+ free_filename(result);
return ERR_PTR(len);
}
/* The empty path is special. */
if (!(flags & LOOKUP_EMPTY)) {
- __putname(result);
+ free_filename(result);
return ERR_PTR(-ENOENT);
}
}
@@ -178,35 +176,26 @@ getname_flags(const char __user *filename, int flags)
* userland.
*/
if (unlikely(len == EMBEDDED_NAME_MAX)) {
- const size_t size = offsetof(struct filename, iname[1]);
- kname = (char *)result;
-
- /*
- * size is chosen that way we to guarantee that
- * result->iname[0] is within the same object and that
- * kname can't be equal to result->iname, no matter what.
- */
- result = kzalloc(size, GFP_KERNEL);
- if (unlikely(!result)) {
- __putname(kname);
+ kname = kmalloc(PATH_MAX, GFP_KERNEL);
+ if (unlikely(!kname)) {
+ free_filename(result);
return ERR_PTR(-ENOMEM);
}
- result->name = kname;
- len = strncpy_from_user(kname, filename, PATH_MAX);
+ memcpy(kname, result->iname, EMBEDDED_NAME_MAX);
+
+ // Copy remaining part of the name
+ len = strncpy_from_user(kname + EMBEDDED_NAME_MAX,
+ filename + EMBEDDED_NAME_MAX,
+ PATH_MAX-EMBEDDED_NAME_MAX);
if (unlikely(len < 0)) {
- __putname(kname);
- kfree(result);
+ free_filename(result);
+ kfree(kname);
return ERR_PTR(len);
}
- /* The empty path is special. */
- if (unlikely(!len) && !(flags & LOOKUP_EMPTY)) {
- __putname(kname);
- kfree(result);
- return ERR_PTR(-ENOENT);
- }
- if (unlikely(len == PATH_MAX)) {
- __putname(kname);
- kfree(result);
+ result->name = kname;
+ if (unlikely(len == PATH_MAX-EMBEDDED_NAME_MAX)) {
+ free_filename(result);
+ kfree(kname);
return ERR_PTR(-ENAMETOOLONG);
}
}
@@ -246,7 +235,7 @@ struct filename *getname_kernel(const char * filename)
struct filename *result;
int len = strlen(filename) + 1;
- result = __getname();
+ result = alloc_filename();
if (unlikely(!result))
return ERR_PTR(-ENOMEM);
@@ -258,13 +247,13 @@ struct filename *getname_kernel(const char * filename)
tmp = kmalloc(size, GFP_KERNEL);
if (unlikely(!tmp)) {
- __putname(result);
+ free_filename(result);
return ERR_PTR(-ENOMEM);
}
tmp->name = (char *)result;
result = tmp;
} else {
- __putname(result);
+ free_filename(result);
return ERR_PTR(-ENAMETOOLONG);
}
memcpy((char *)result->name, filename, len);
@@ -290,11 +279,9 @@ void putname(struct filename *name)
return;
}
- if (name->name != name->iname) {
- __putname(name->name);
- kfree(name);
- } else
- __putname(name);
+ if (name->name != name->iname)
+ kfree(name->name);
+ free_filename(name);
}
EXPORT_SYMBOL(putname);
diff --git a/include/linux/fs.h b/include/linux/fs.h
index c895146c1444..197a21897af2 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -2833,12 +2833,13 @@ extern struct kobject *fs_kobj;
/* fs/open.c */
struct audit_names;
+#define EMBEDDED_NAME_MAX 64
struct filename {
const char *name; /* pointer to actual string */
const __user char *uptr; /* original userland pointer */
atomic_t refcnt;
struct audit_names *aname;
- const char iname[];
+ const char iname[EMBEDDED_NAME_MAX];
};
static_assert(offsetof(struct filename, iname) % sizeof(long) == 0);
@@ -2960,8 +2961,19 @@ extern void __init vfs_caches_init(void);
extern struct kmem_cache *names_cachep;
-#define __getname() kmem_cache_alloc(names_cachep, GFP_KERNEL)
-#define __putname(name) kmem_cache_free(names_cachep, (void *)(name))
+static inline struct filename *alloc_filename(void)
+{
+ return kmem_cache_alloc(names_cachep, GFP_KERNEL);
+}
+
+static inline void free_filename(struct filename *name)
+{
+ kmem_cache_free(names_cachep, name);
+}
+
+// Crazy old legacy uses for pathname allocations
+#define __getname() kmalloc(PATH_MAX, GFP_KERNEL)
+#define __putname(name) kfree((void *)(name))
extern struct super_block *blockdev_superblock;
static inline bool sb_is_blkdev_sb(struct super_block *sb)
