linux-kernel - Re: [PATCH 1/2] kernfs: add kernfs_ops.free operation to free resources tied to the file

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: <ZJstlHU4Y3ZtiWJe@slm.duckdns.org>
Date:   Tue, 27 Jun 2023 08:42:28 -1000
From:   Tejun Heo <tj@...nel.org>
To:     Christian Brauner <brauner@...nel.org>
Cc:     Suren Baghdasaryan <surenb@...gle.com>, gregkh@...uxfoundation.org,
        peterz@...radead.org, lujialin4@...wei.com,
        lizefan.x@...edance.com, hannes@...xchg.org, mingo@...hat.com,
        ebiggers@...nel.org, oleg@...hat.com, akpm@...ux-foundation.org,
        viro@...iv.linux.org.uk, juri.lelli@...hat.com,
        vincent.guittot@...aro.org, dietmar.eggemann@....com,
        rostedt@...dmis.org, bsegall@...gle.com, mgorman@...e.de,
        bristot@...hat.com, vschneid@...hat.com,
        linux-kernel@...r.kernel.org, cgroups@...r.kernel.org,
        linux-fsdevel@...r.kernel.org, kernel-team@...roid.com
Subject: Re: [PATCH 1/2] kernfs: add kernfs_ops.free operation to free
 resources tied to the file

Hello, Christian.

On Tue, Jun 27, 2023 at 07:30:26PM +0200, Christian Brauner wrote:
...
> ->release() was added in
> 
>     commit 0e67db2f9fe91937e798e3d7d22c50a8438187e1
>     kernfs: add kernfs_ops->open/release() callbacks
> 
>     Add ->open/release() methods to kernfs_ops.  ->open() is called when
>     the file is opened and ->release() when the file is either released or
>     severed.  These callbacks can be used, for example, to manage
>     persistent caching objects over multiple seq_file iterations.
> 
>     Signed-off-by: Tejun Heo <tj@...nel.org>
>     Acked-by: Greg Kroah-Hartman <gregkh@...uxfoundation.org>
>     Acked-by: Acked-by: Zefan Li <lizefan@...wei.com>
> 
> which mentions "either releases or severed" which imho already points to
> separate methods.

This is because kernfs has revoking operation which doesn't exist for other
filesystems. Other filesystem implemenations can't just say "I'm done. Bye!"
and go away. Even if the underlying filesystem has completely failed, the
code still has to remain attached and keep aborting operations.

However, kernfs serves as the midlayer to a lot of device drivers and other
internal subsystems and it'd be really inconvenient for each of them to have
to implement "I want to go away but I gotta wait out this user who's holding
onto my tuning knob file". So, kernfs exposes a revoke or severing semantics
something that's exposing interface through kernfs wants to stop doing so.

If you look at it from file operation implementation POV, this seems exactly
like ->release. All open files are shutdown and there won't be any future
operations. After all, revoke is forced closing of all fd's. So, for most
users, treating severing just like ->release is the right thing to do.

The PSI file which caused this is a special case because it attaches
something to its kernfs file which outlives the severing operation bypassing
kernfs infra. A more complete way to fix this would be supporting the
required behavior from kernfs side, so that the PSI file operates on kernfs
interface which knows the severing event and detaches properly. That said,
currently, this is very much an one-off.

Suren, if you're interested, it might make sense to pipe poll through kernfs
properly so that it has its kernfs operation and kernfs can sever it. That
said, as this is a fix for something which is currently causing crashes,
it'd be better to merge this simpler fix first no matter what.

Thanks.

-- 
tejun