On Thu, Jun 5, 2025 at 9:34 PM Linus Torvalds <torvalds@xxxxxxxxxxxxxxxxxxxx> wrote: > > On Thu, 5 Jun 2025 at 07:51, Miklos Szeredi <miklos@xxxxxxxxxx> wrote: > > > > - The above fix contains a cast to non-const, which is not actually > > needed. So add the necessary helpers postfixed with _c that allow the > > cast to be removed (touches vfs files but only in trivial ways) > I must have snoozed the review of this one :-/ > Grr. > > I despise those "trivial ways". > > In particular, I despise how this renames the backing_file_user_path() > helper to something actively *worse*. > > The "_c()" makes no sense as a name. Yes, I realize that the "c" > stands for "const", but it still makes absolutely zero sense, since > everybody wants the const version. > > The only user of the non-const version is the *ointernal* > implementation that is never exported to other modules, and that could > have the special name. > > Although I suspect it doesn't even need it, it could just use the > backing_file(f) macro directly and that should just be moved to > internal.h, and then the 'const'ness would come from the argument as > required. > > In fact, most of the _internal_ vfs users don't even want the > non-const version, ie as far as I can tell the user in > file_get_write_access() would be perfectly happy with the const > version too. > > So you made the *normal* case have an odd name, and then kept the old > sane name for the case nobody else really wants to see. > > If anything, the internal non-const version is the one that should be > renamed (and *not* using some crazy "_nc()" postfix nasty crud). Not > the one that gets exported and that everybody wants. > IMO, it would be nicer to use backing_file_set_user_path() (patch attached). > So I could fix up that last commit to not hate it, but honestly, I > don't want that broken state in the kernel in the first place. > Would you consider pulling ovl-update-6.16^ and applying the attached patch [*]? Thanks, Amir. [*] I did not include the removal of non-const casting to keep this patch independent of the ovl PR. Feel free to add it to my patch or I can send the patch post merge or cleanup of casting post merge.
From bc79491bf671b8a1d6603e0367963341339f943f Mon Sep 17 00:00:00 2001 From: Amir Goldstein <amir73il@xxxxxxxxx> Date: Fri, 6 Jun 2025 07:46:02 +0200 Subject: [PATCH] fs: constify file ptr in backing_file_user_path() Add internal helper backing_file_set_user_path() for the only two cases that need to modify user path. Signed-off-by: Amir Goldstein <amir73il@xxxxxxxxx> --- fs/backing-file.c | 4 ++-- fs/file_table.c | 13 ++++++++----- fs/internal.h | 1 + include/linux/fs.h | 2 +- 4 files changed, 12 insertions(+), 8 deletions(-) diff --git a/fs/backing-file.c b/fs/backing-file.c index 763fbe9b72b2..8c7396bff121 100644 --- a/fs/backing-file.c +++ b/fs/backing-file.c @@ -41,7 +41,7 @@ struct file *backing_file_open(const struct path *user_path, int flags, return f; path_get(user_path); - *backing_file_user_path(f) = *user_path; + backing_file_set_user_path(f, user_path); error = vfs_open(real_path, f); if (error) { fput(f); @@ -65,7 +65,7 @@ struct file *backing_tmpfile_open(const struct path *user_path, int flags, return f; path_get(user_path); - *backing_file_user_path(f) = *user_path; + backing_file_set_user_path(f, user_path); error = vfs_tmpfile(real_idmap, real_parentpath, f, mode); if (error) { fput(f); diff --git a/fs/file_table.c b/fs/file_table.c index c04ed94cdc4b..8ac2fbbd4f6d 100644 --- a/fs/file_table.c +++ b/fs/file_table.c @@ -52,17 +52,20 @@ struct backing_file { }; }; -static inline struct backing_file *backing_file(struct file *f) -{ - return container_of(f, struct backing_file, file); -} +#define backing_file(f) container_of(f, struct backing_file, file) -struct path *backing_file_user_path(struct file *f) +struct path *backing_file_user_path(const struct file *f) { return &backing_file(f)->user_path; } EXPORT_SYMBOL_GPL(backing_file_user_path); +void backing_file_set_user_path(struct file *f, const struct path *path) +{ + backing_file(f)->user_path = *path; +} +EXPORT_SYMBOL_GPL(backing_file_set_user_path); + static inline void file_free(struct file *f) { security_file_free(f); diff --git a/fs/internal.h b/fs/internal.h index 213bf3226213..3860b022e57c 100644 --- a/fs/internal.h +++ b/fs/internal.h @@ -101,6 +101,7 @@ extern void chroot_fs_refs(const struct path *, const struct path *); struct file *alloc_empty_file(int flags, const struct cred *cred); struct file *alloc_empty_file_noaccount(int flags, const struct cred *cred); struct file *alloc_empty_backing_file(int flags, const struct cred *cred); +void backing_file_set_user_path(struct file *f, const struct path *path); static inline void file_put_write_access(struct file *file) { diff --git a/include/linux/fs.h b/include/linux/fs.h index a4fd649e2c3f..c745aee9c88a 100644 --- a/include/linux/fs.h +++ b/include/linux/fs.h @@ -2813,7 +2813,7 @@ struct file *dentry_open_nonotify(const struct path *path, int flags, const struct cred *cred); struct file *dentry_create(const struct path *path, int flags, umode_t mode, const struct cred *cred); -struct path *backing_file_user_path(struct file *f); +struct path *backing_file_user_path(const struct file *f); /* * When mmapping a file on a stackable filesystem (e.g., overlayfs), the file -- 2.34.1