linux-kernel - Re: [PATCH v5 5/7] revocable: Add fops replacement

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: <07ebf7d9-0071-483c-a68e-645853e83831@kernel.org>
Date: Thu, 23 Oct 2025 17:32:51 +0200
From: Danilo Krummrich <dakr@...nel.org>
To: Jason Gunthorpe <jgg@...dia.com>
Cc: Tzung-Bi Shih <tzungbi@...nel.org>, Benson Leung <bleung@...omium.org>,
 Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
 "Rafael J . Wysocki" <rafael@...nel.org>, Jonathan Corbet <corbet@....net>,
 Shuah Khan <shuah@...nel.org>, linux-doc@...r.kernel.org,
 linux-kernel@...r.kernel.org, chrome-platform@...ts.linux.dev,
 linux-kselftest@...r.kernel.org,
 Laurent Pinchart <laurent.pinchart@...asonboard.com>,
 Bartosz Golaszewski <brgl@...ev.pl>,
 Wolfram Sang <wsa+renesas@...g-engineering.com>,
 Simona Vetter <simona.vetter@...ll.ch>,
 Dan Williams <dan.j.williams@...el.com>
Subject: Re: [PATCH v5 5/7] revocable: Add fops replacement

On Sat Oct 18, 2025 at 12:56 AM CEST, Jason Gunthorpe wrote:
> On Fri, Oct 17, 2025 at 11:41:56PM +0200, Danilo Krummrich wrote:
>> On Fri Oct 17, 2025 at 8:44 PM CEST, Jason Gunthorpe wrote:
>> > On Fri, Oct 17, 2025 at 08:19:06PM +0200, Danilo Krummrich wrote:
>> >> On 10/17/25 6:37 PM, Jason Gunthorpe wrote:
>> >> > On Fri, Oct 17, 2025 at 06:29:10PM +0200, Danilo Krummrich wrote:
>> >> > 
>> >> >> I'm not sure about MISC device though. Unless there's a good reason,
>> >> >> I think MISC device should be "fenced" instead.
>> >> > 
>> >> > misc is a very small wrapper around raw fops, and raw fops are
>> >> > optimized for performance. Adding locking that many important things
>> >> > like normal files don't need to all fops would not be agreed.
>> >> > 
>> >> > The sketch in this series where we have a core helper to provide a
>> >> > shim fops that adds on the lock is smart and I think could be an
>> >> > agreeable way to make a synchronous misc and cdev unregister for
>> >> > everyone to trivially use.
>> >> 
>> >> Sure, for MISC devices without a parent for instance there are no device
>> >> resources to access anyways.
>> >
>> > There are many situations with misc that can get people into trouble without
>> > parent:
>> >
>> >  misc_deregister(x);
>> >  timer_shutdown_sync(y);
>> >  kfree(z);
>> >
>> > For example. It is is buggy if the fops touch y or z.
>> >
>> > This is why a _sync version is such a nice clean idea because with 5
>> > letters the above can just be fixed.
>> >
>> > Wrapping everything in a revocable would be a huge PITA.
>> 
>> That's a bit of a different problem though. Revocable clearly isn't the
>> solution. _sync() works, but doesn't account for the actual problem, which is
>> that the file private has at least shared ownership of y and z.
>> 
>> So, it's more of an ownership / lifetime problem. The file private data should
>> either own y and z entirely or a corresponding reference count that is dropped
>> in fops release().
>
> I think both versions are popular in the kernel.. You can legimately
> treat y and z the same as device resources without creating a
> correctness problem and it is less code.
>
> You can also do refcounts.
>
> For instance at least in C you'd never argue that people should use
> refcount private data when they use a timer or irq subsystem. You'd
> use a simple sync cleanup and be done with it.

Don't get me wrong, I'm well aware that there are multiple ways to achieve
correctness, and I know what's common, not common, etc.

And I also don't mean to say "just reference count your private data", which in
a lot of (maybe even most) cases clearly is the wrong thing to do (compared to
specific objects contained within the private data).

What I'm saying is that it's better to think about the lifetime of an object and
what it is owned by.

Let's say we have a memory allocation of some object X, then the question is who
owns this object. Is it the module, the file private data, device private data,
interrupt handler, etc.

If it is only owned by a single thing (e.g. file private), then it's simple:
allocate and initialize in open(), release in release(), and only ever access
in other fops.

However, if we have shared ownership of an object, and it is contained in
multiple different private data objects, then reference count is much more
robust, compared to having your driver manually keeping track of how it can
synchronize against all the owners of that object.