Re: cdev/devm_* issues (was Re: [TECH TOPIC] Rust for Linux)

[Daniel Vetter <daniel@ffwll.ch>]

On Wed, Jul 21, 2021 at 11:09 AM Dan Carpenter <dan.carpenter@oracle.com> wrote:
> On Thu, Jul 15, 2021 at 11:54:22AM +0200, Daniel Vetter wrote:
> > Since we're dropping notes, a few of my thoughts:
> >
> > - Personally I think an uapi resource cleanup system needs a different
> > namespace, so that you don't mix these up. Hence drmm_ vs devres_ for
> > drm, and maybe the generic version could be called uapires or ures or
> > whatever. It needs to stick out.
> >
> > - I'm wondering whether we could enlist checkers (maybe a runtime one)
> > to scream anytime we do an unprotected dereference from ures memory to
> > devres memory. This would help in subsystem where this problem is
> > solved by trying to decouple the uapi side from the device side (like
> > gpio and other subsystem with simpler interfaces). We have undefined
> > behaviour and data race checkers already, this should be doable. But I
> > have no idea how :-)
>
> So you want a warning with code like:
>
>         p->foo = bar;
>
> Where "p" is devres memory and "bar" is ures?  There are a couple
> approaches you could take and I would advise to implement both.

Pointing from devres to ures is totally normal, otherwise how can you
unregister your interface (drm_device for gpus), which is ures, if you
don't have a pointer that you can access from your hotunplug code,
which leaves in the devres world. Or when you're called in any other
hook from the device side of things (callbacks, interrupts, whatever
really). The usual approach is to clear these out on hotunplug, which
doesn't need locks because you're guaranteed that nothing from the
devres world can use them anymore since the device is gone. But ofc
you can screw this up easily.

Also, you need to have pointers from ures to devres world too, because
without those it's pretty hard to access your actual hw and make it do
things, which is the entire point of exposing a uapi interface (which
is in the ures world). The trouble is this access isn't protected
against the devres side disappearing in a hotunplug with a lock or
barrier or similar.

E.g. in drm we have this drm_dev_enter/exit stuff, and within that it
should be save to deref from ures to devres, but outside it's not.

> The first approach is to just check directly for if p is devres and bar
> is ures.  The problem is that sometimes you won't know if p is devres so
> some bugs will be missed.
>
> The second approach is to say something like if we find one type "p"
> that is devres, then let's assume they all are.  Same for ures.  Then
> based on our assumptions about types, print a warning if they don't
> match.  Then go through the warnings and make a list of types which lead
> to false positives and add it to smatch_data/kernel.ignore_devres_types.
> This is a bit hand wavey but that's basically the approach.
>
> A third approach would be to do something with manual annotations.  You
> could probably make Sparse work for something like that.

Yeah I think a classifier like that makes sense, but I'm not sure we
can get at the information about how it's protected statically.
-Daniel

>
> regards,
> dan carpenter
>


-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch