linux-kernel - Re: [PATCH v5 2/6] drm/bridge: Add a devm

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: <30057693.2anM9zYF0F@avalon>
Date:   Mon, 07 Aug 2017 13:22:23 +0300
From:   Laurent Pinchart <laurent.pinchart@...asonboard.com>
To:     Daniel Vetter <daniel@...ll.ch>
Cc:     Noralf Trønnes <noralf@...nnes.org>,
        Ilia Mirkin <imirkin@...m.mit.edu>,
        Eric Anholt <eric@...olt.net>,
        Daniel Vetter <daniel.vetter@...el.com>,
        Thierry Reding <thierry.reding@...il.com>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "dri-devel@...ts.freedesktop.org" <dri-devel@...ts.freedesktop.org>
Subject: Re: [PATCH v5 2/6] drm/bridge: Add a devm_ allocator for panel bridge.

Hi Daniel,

On Monday 07 Aug 2017 11:25:07 Daniel Vetter wrote:
> On Sat, Aug 05, 2017 at 12:59:07PM +0200, Noralf Trønnes wrote:
> > Den 05.08.2017 00.19, skrev Ilia Mirkin:
> >> On Fri, Aug 4, 2017 at 4:43 PM, Eric Anholt <eric@...olt.net> wrote:
> >>> Laurent Pinchart <laurent.pinchart@...asonboard.com> writes:
> >>>> On Tuesday 18 Jul 2017 14:05:06 Eric Anholt wrote:
> >>>>> This will let drivers reduce the error cleanup they need, in
> >>>>> particular the "is_panel_bridge" flag.
> >>>>> 
> >>>>> v2: Slight cleanup of remove function by Andrzej
> >>>> 
> >>>> I just want to point out that, in the context of Daniel's work on
> >>>> hot-unplug, 90% of the devm_* allocations are wrong and will get in
> >>>> the way. All DRM core objects that are accessible one way or
> >>>> another from userspace will need to be properly reference-counted
> >>>> and freed only when the last reference disappears, which could be
> >>>> well after the corresponding device is removed. I believe this
> >>>> could be one such objects :-/
> >>> 
> >>> Sure, if you're hotplugging, your life is pain.  For non-hotpluggable
> >>> devices, like our SOC platform devices (current panel-bridge
> >>> consumers), this still seems like an excellent simplification of
> >>> memory management.
> >> 
> >> At that point you may as well make your module non-unloadable, and
> >> return failure when trying to remove a device from management by the
> >> driver (whatever the opposite of "probe" is, I forget). Hotplugging
> >> doesn't only happen when physically removing, it can happen for all
> >> kinds of reasons... and userspace may still hold references in some of
> >> those cases.
> > 
> > If drm_open() gets a ref on dev->dev and puts it in drm_release(),
> > won't that delay devm_* cleanup until userspace is done?
> 
> No. drm_device is the thing that is refcounted for userspace references
> like open FD (we're not perfect about it, e.g. sysfs and dma-buf/fence
> don't).
> 
> devm_ otoh is tied to the lifetime of the underlying device, and that one
> can get outlived by drm_device. Or at least afaiui, devm_ stuff is nuked
> on unplug, and not when the final sw reference of the struct device
> disappears.
> 
> Not sure tough, it's complicated.

It's complicated when you have to understand the behaviour by reading the 
code, but the behaviour isn't that complex. devm resources are released both

1. right after the driver's .remove() operation returns
2. when the device is deleted (last reference released)

It's the first one that makes devm_* allocation unsuitable for any structure 
that is accessible from userspace (such as in file operation handlers).

-- 
Regards,

Laurent Pinchart