Re: [RFC PATCH 0/3] Fix errors on DT overlay removal with devlinks

[Saravana Kannan]

On Wed, Oct 21, 2020 at 2:02 PM Frank Rowand <frowand.list@xxxxxxxxx> wrote:
>
> Hi Saravana,
>
> Michael found an issue related to the removal of a devicetree node
> which involves devlinks:
>
> On 10/14/20 2:36 PM, Michael Auchter wrote:
> > After updating to v5.9, I've started seeing errors in the kernel log
> > when using device tree overlays. Specifically, the problem seems to
> > happen when removing a device tree overlay that contains two devices
> > with some dependency between them (e.g., a device that provides a clock
> > and a device that consumes that clock). Removing such an overlay results
> > in:
> >
> >   OF: ERROR: memory leak, expected refcount 1 instead of 2, of_node_get()/of_node_put() unbalanced - destroy
> >   OF: ERROR: memory leak, expected refcount 1 instead of 2, of_node_get()/of_node_put() unbalanced - destroy
> >
> > followed by hitting some REFCOUNT_WARNs in refcount.c
> >
> > In the first patch, I've included a unittest that can be used to
> > reproduce this when built with CONFIG_OF_UNITTEST [1].
> >
> > I believe the issue is caused by the cleanup performed when releasing
> > the devlink device that's created to represent the dependency between
> > devices. The devlink device has references to the consumer and supplier
> > devices, which it drops in device_link_free; the devlink device's
> > release callback calls device_link_free via call_srcu.
> >
> > When the overlay is being removed, all devices are removed, and
> > eventually the release callback for the devlink device run, and
> > schedules cleanup using call_srcu. Before device_link_free can and call
> > put_device on the consumer/supplier, the rest of the overlay removal
> > process runs, resulting in the error traces above.
>
> When a devicetree node in an overlay is removed, the remove code expects
> all previous users of the related device to have done the appropriate put
> of the device and to have no later references.
>
> As Michael described above, the devlink release callback defers the
> put_device().  The cleanup via srcu was implemented in commit
> 843e600b8a2b01463c4d873a90b2c2ea8033f1f6 "driver core: Fix sleeping
> in invalid context during device link deletion" to solve yet another
> issue.
>
>
> >
> > Patches 2 and 3 are an attempt at fixing this: call srcu_barrier to wait
> > for any pending device_link_free's to execute before continuing on with
> > the removal process.
> >
> > These patches resolve the issue, but probably not in the best way. In
> > particular, it seems strange to need to leak details of devlinks into
> > the device tree overlay code. So, I'd be curious to get some feedback or
> > hear any other ideas for how to resolve this issue.
>
> I agree with Michael that adding an indirect call of srcu_barrier(&device_links_srcu)
> into the devicetree overlay code is not an appropriate solution.

I kind of see your point too. I wonder if the srcu_barrier() should
happen inside like so:
device_del() -> device_links_purge()->srcu_barrier()

I don't know what contention the use of SRCUs in device links was
trying to avoid, but I think the srcu_barrier() call path I suggested
above shouldn't be a problem. If that fixes the issue, the best way to
know if it's an issue is to send out a patch and see if Rafael has any
problem with it :)

> Is there some other way to fix the problem that 843e600b8a2b solves without
> deferring the put_device() done by the devlink release callback?

Ok I finally got some time to look into this closely.

Even if you revert 843e600b8a2b, you'll see that device_link_free()
(which drops the reference to the consumer and supplier devices) was
scheduled to run when the SRCU clean up occurs. So I think this issue
was present even before 843e600b8a2b, but commit 843e600b8a2b just
made it more likely to hit this scenario because it introduces some
delay in dropping the ref count of the supplier and consumer by going
through the device link device's release path. So, I think this issue
isn't related to 843e600b8a2b.

As to why 843e600b8a2b had to be written to call call_srcu() from the
device link device's release path, it's a mess of dependencies/delays:
1. The device link device is part of the struct device_link. So we
can't free device_link before the device_link.link_dev refcount goes
to 0.
2. But I can't assume device_link.link_dev's refcount will go to 0 as
soon as I call put_device() on it because of
CONFIG_DEBUG_KOBJECT_RELEASE which frees up the kobject after a random
delay.
3. The use of SRCU also means I can't free device_link until the SRCU
is cleaned up.

Because of (1), (2) and (3), when the device_link_del() (or any of the
other device link deletion APIs are called) I first have to do a
put_device(device_link.link_dev) to make sure the device memory is no
longer referenced, then trigger an SRCU clean up and then in the
scheduled SRCU cleanup I can free struct device_link. And obviously,
until struct device_link is ready to be freed up, I can't drop the
reference to the supplier and consumer devices (as that old copy of
device_link could be used by some code to refer to the supplier and
consumer devices).

Hope that helps explain the SRCU and device link device release dependencies.

Also, even if this patch series is applied as is, I wonder if the
current overlay code has a bug related to CONFIG_DEBUG_KOBJECT_RELEASE
delaying the actual freeing of the device. Something to look into?

-Saravana