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  PHC 
Open Source and information security mailing list archives
 
Hash Suite for Android: free password hash cracker in your pocket
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Date:   Tue, 16 Jun 2020 14:10:54 +0200
From:   Daniel Vetter <daniel@...ll.ch>
To:     Laurent Pinchart <laurent.pinchart@...asonboard.com>,
        Greg KH <gregkh@...uxfoundation.org>
Cc:     Aditya Pakki <pakki001@....edu>, kjlu@....edu, wu000273@....edu,
        Andrzej Hajda <a.hajda@...sung.com>,
        Neil Armstrong <narmstrong@...libre.com>,
        Jonas Karlman <jonas@...boo.se>,
        Jernej Skrabec <jernej.skrabec@...l.net>,
        David Airlie <airlied@...ux.ie>,
        Daniel Vetter <daniel@...ll.ch>,
        dri-devel@...ts.freedesktop.org, linux-kernel@...r.kernel.org,
        "Rafael J. Wysocki" <rafael@...nel.org>
Subject: Re: [PATCH] drm/bridge: fix reference count leaks due to
 pm_runtime_get_sync()

On Sun, Jun 14, 2020 at 04:46:55PM +0300, Laurent Pinchart wrote:
> Hi Aditya,
> 
> (CC'ing Rafael)
> 
> Thank you for the patch.
> 
> On Sat, Jun 13, 2020 at 09:40:05PM -0500, Aditya Pakki wrote:
> > On calling pm_runtime_get_sync() the reference count of the device
> > is incremented. In case of failure, decrement the
> > reference count before returning the error.
> > 
> > Signed-off-by: Aditya Pakki <pakki001@....edu>
> 
> I've seen lots of similar patches recently. Instead of mass-patching the
> drivers this way, shouldn't pm_runtime_get_sync() (and similar
> functions) decrease the refcount on their failure path ? That would
> require patching drivers that already handle this issue, but I believe
> that would cause less churn, and more importantly, avoid the issue once
> and for good for new code.

Yeah the current interface looks rather dodgy, generally drivers aren't
expected to clean up if the first function failed.

Rafael and Greg, what's your take on this here? See patch below and
Laurent's comment above.
-Daniel

> 
> > ---
> >  drivers/gpu/drm/bridge/cdns-dsi.c | 8 ++++++--
> >  1 file changed, 6 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/bridge/cdns-dsi.c b/drivers/gpu/drm/bridge/cdns-dsi.c
> > index 69c3892caee5..583cb8547106 100644
> > --- a/drivers/gpu/drm/bridge/cdns-dsi.c
> > +++ b/drivers/gpu/drm/bridge/cdns-dsi.c
> > @@ -788,8 +788,10 @@ static void cdns_dsi_bridge_enable(struct drm_bridge *bridge)
> >  	u32 tmp, reg_wakeup, div;
> >  	int nlanes;
> >  
> > -	if (WARN_ON(pm_runtime_get_sync(dsi->base.dev) < 0))
> > +	if (WARN_ON(pm_runtime_get_sync(dsi->base.dev) < 0)) {
> > +		pm_runtime_put(dsi->base.dev);
> >  		return;
> > +	}
> >  
> >  	mode = &bridge->encoder->crtc->state->adjusted_mode;
> >  	nlanes = output->dev->lanes;
> > @@ -1028,8 +1030,10 @@ static ssize_t cdns_dsi_transfer(struct mipi_dsi_host *host,
> >  	int ret, i, tx_len, rx_len;
> >  
> >  	ret = pm_runtime_get_sync(host->dev);
> > -	if (ret < 0)
> > +	if (ret < 0) {
> > +		pm_runtime_put(host->dev);
> >  		return ret;
> > +	}
> >  
> >  	cdns_dsi_init_link(dsi);
> >  
> 
> -- 
> Regards,
> 
> Laurent Pinchart

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

Powered by blists - more mailing lists