linux-kernel - Re: [GIT PULL] overlayfs update for 6.16

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAOQ4uxjXvcj8Vf3y81KJCbn6W5CSm9fFofV8P5ihtcZ=zYSREA@mail.gmail.com>
Date: Fri, 6 Jun 2025 08:17:33 +0200
From: Amir Goldstein <amir73il@...il.com>
To: Linus Torvalds <torvalds@...ux-foundation.org>
Cc: Miklos Szeredi <miklos@...redi.hu>, overlayfs <linux-unionfs@...r.kernel.org>, 
	linux-fsdevel@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [GIT PULL] overlayfs update for 6.16

On Thu, Jun 5, 2025 at 9:34 PM Linus Torvalds
<torvalds@...ux-foundation.org> wrote:
>
> On Thu, 5 Jun 2025 at 07:51, Miklos Szeredi <miklos@...redi.hu> 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.

View attachment "fs-constify-file-ptr-in-backing_file_user_path.patch" of type "text/x-patch" (3262 bytes)