Re: [PATCH v2 1/2] rust: add abstraction for struct device

[Greg KH <gregkh@linuxfoundation.org>]

On Wed, Jun 12, 2024 at 07:13:31PM +0200, Greg KH wrote:
> On Wed, Jun 12, 2024 at 06:18:38PM +0200, Danilo Krummrich wrote:
> > On Wed, Jun 12, 2024 at 05:50:42PM +0200, Greg KH wrote:
> > > On Wed, Jun 12, 2024 at 05:35:21PM +0200, Danilo Krummrich wrote:
> > > > On Wed, Jun 12, 2024 at 05:02:52PM +0200, Greg KH wrote:
> > > > > On Wed, Jun 12, 2024 at 04:51:42PM +0200, Danilo Krummrich wrote:
> > > > > > On 6/11/24 18:13, Boqun Feng wrote:
> > > > > > > On Tue, Jun 11, 2024 at 03:29:22PM +0200, Greg KH wrote:
> > > > > > > > On Tue, Jun 11, 2024 at 03:21:31PM +0200, Danilo Krummrich wrote:
> > > > > > > > > ...hence, I agree we should indeed add to the #Invariants and #Safety section
> > > > > > > > > that `->release` must be callable  from any thread.
> > > > > > > > > 
> > > > > > > > > However, this is just theory, do we actually have cases where `device::release`
> > > > > > > 
> > > > > > > @Danilo, right, it's only theorical, but it's good to call it out since
> > > > > > > it's the requirement for a safe Rust abstraction.
> > > > > > 
> > > > > > Similar to my previous reply, if we want to call this out as safety requirement
> > > > > > in `Device::from_raw`, we probably want to add it to the documentation of the C
> > > > > > `struct device`, such that we can argue that this is an invariant of C's
> > > > > > `struct device`.
> > > > > > 
> > > > > > Otherwise we'd have to write something like:
> > > > > > 
> > > > > > "It must also be ensured that the `->release` function of a `struct device` can
> > > > > > be called from any non-atomic context. While not being officially documented this
> > > > > > is guaranteed by the invariant of `struct device`."
> > > > > 
> > > > > In the 20+ years of the driver model being part of the kernel, I don't
> > > > > think this has come up yet, so maybe you can call the release function
> > > > > in irq context.  I don't know, I was just guessing :)
> > > > 
> > > > Ah, I see. I thought you know and it's defined, but just not documented.
> > > > 
> > > > This means it's simply undefined what we expect to happen when the last
> > > > reference of a device is dropped from atomic context.
> > > > 
> > > > Now, I understand (and would even expect) that practically this has never been
> > > > an issue. You'd need two circumstances, release() actually does something that
> > > > is not allowed in atomic context plus the last device reference is dropped from
> > > > atomic context - rather unlikely.
> > > > 
> > > > > 
> > > > > So let's not go adding constraints that we just do not have please.
> > > > > Same goes for the C code, so the rust code is no different here.
> > > > 
> > > > I agree we shouldn't add random constraints, but for writing safe code we also
> > > > have to rely on defined behavior.
> > > 
> > > As the rust code is relying on C code that could change at any point in
> > > time, how can that ever be "safe"?  :)
> > 
> > That's the same as with any other API. If the logic of an API is changed the
> > users (e.g a Rust abstraction) of the API have to be adjusted.
> 
> Agreed, just like any other in-kernel code, so there shouldn't be
> anything special here.
> 
> > > Sorry, this type of definition annoys me.
> > > 
> > > > I see two options:
> > > > 
> > > > (1) We globally (for struct device) define from which context release() is
> > > >     allowed to be called.
> > > 
> > > If you want, feel free to do that work please.  And then find out how to
> > > enforce it in the driver core.
> > 
> > If we *would* define non-atomic context only, we could enforce it with
> > might_sleep() for instance.
> 
> might_sleep() isn't always correct from what I remember.
> 
> > If we *would* define any context, there is nothing to enforce, but we'd need to
> > validate that no implementer of release() voids that.
> 
> Trying to validate that might be hard, again, I don't think it's worth
> it.
> 
> > The former is a constaint you don't want to add, the latter a lot of work. What
> > if we at least define that implementers of release() must *minimally* make sure
> > that it can be call from any non-atomic context.
> > 
> > That'd be something we can rely on in Rust.
> 
> Determining if you are, or are not, in atomic context is almost
> impossible in C, I don't know how you are going to do that at build time
> in Rust.  Good luck!

Note, the real problem with calling release() on a device, is when you
do so from within the device's sysfs file.  That causes all sorts of
"fun" times that people have had to work around over the years.  It's
not something you probably need to worry about yet, as you are not
writing a bus subsystem in Rust yet, but it is something that can get
tricky very quickly.  Look at the hoops that scsi has to deal with if
you want to see the gory details...

But this is all just "logic" bugs, not thread or irq issues at all.
Nothing is different, nor should be different, for code doing this in C
or in rust.

What I'm trying to get at is that you can't document all of the
"constraints" you are dealing with when handling C apis like this,
sorry.  It's going to be hard, if not impossible, to worry about that at
this point in the api.  It's better left to be documented where the real
call happens (i.e. in the C code), than attempting to keep it up to date
in the c<->rust binding point.

thanks,

greg k-h