linux-kernel - Re: [PATCH] drm/irq: Add irq as false detection

Open Source and information security mailing list archives

Message-ID: <CAKMK7uHBjV8nEw9xW_MkGMeDrRSo9wuqCawdhw5D5t=JhVD5DA@mail.gmail.com>
Date:   Mon, 2 Nov 2020 13:51:52 +0100
From:   Daniel Vetter <daniel@...ll.ch>
To:     Thomas Zimmermann <tzimmermann@...e.de>
Cc:     Tian Tao <tiantao6@...ilicon.com>,
        Maarten Lankhorst <maarten.lankhorst@...ux.intel.com>,
        Maxime Ripard <mripard@...nel.org>,
        Dave Airlie <airlied@...ux.ie>,
        dri-devel <dri-devel@...ts.freedesktop.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] drm/irq: Add irq as false detection

On Mon, Nov 2, 2020 at 1:40 PM Thomas Zimmermann <tzimmermann@...e.de> wrote:
>
> Hi
>
> Am 02.11.20 um 13:09 schrieb Thomas Zimmermann:
> > Hi
> >
> > Am 02.11.20 um 12:19 schrieb Tian Tao:
> >> Add the detection of false for irq, so that the EINVAL is not
> >> returned when dev->irq_enabled is false.
> >>
> >> Signed-off-by: Tian Tao <tiantao6@...ilicon.com>
> >> ---
> >>  drivers/gpu/drm/drm_irq.c | 3 +++
> >>  1 file changed, 3 insertions(+)
> >>
> >> diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c
> >> index 09d6e9e..7537a3d 100644
> >> --- a/drivers/gpu/drm/drm_irq.c
> >> +++ b/drivers/gpu/drm/drm_irq.c
> >> @@ -172,6 +172,9 @@ int drm_irq_uninstall(struct drm_device *dev)
> >>      bool irq_enabled;
> >>      int i;
> >>
> >> +    if (!dev->irq_enabled || !dev)
> >> +            return 0;
> >> +
> >
> > Dereferencing a pointer before NULL-checking it, is Yoda programming. :)
> > I'd just drop the test for !dev and assume that dev is always valid.
> >
> > I suggest to change the int return value to void and return nothing.
> >
> > Re-reading the actual implementation of this function, this location
> > might be too early. Further below there already is a test for
> > irq_enabled. Put a drm_WARN_ON around it and it should be fine. This way
> > the vblank handlers will be disabled and erroneous callers will see a
> > warning in the kernel's log.
>
> In addition to these changes, you could also add a managed
> implementation of drm_irq_install(). The canonical name should be
> devm_drm_irq_install(). The function would call drm_irq_install() and
> register a cleanup handler via devm_add_action(). Example code is at [1].
>
> KMS drivers, such as hibmc, would then not have to bother about
> uninstalling the DRM irq.

Yup, devm_ is the right fix here imo, not trying to make the uninstall
hook resilient against drivers which can't keep track of stuff. So if
that's all there is, imo this patch is a bad idea, since we have
proper tools to make driver unloading easier on driver author's
nowadays.
-Daniel

> Best regards
> Thomas
>
> [1]
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/gpu/drm/drm_drv.c#n664
>
>
> >
> > Best regards
> > Thomas
> >
> >>      irq_enabled = dev->irq_enabled;
> >>      dev->irq_enabled = false;
> >>
> >>
> >
> >
> > _______________________________________________
> > dri-devel mailing list
> > dri-devel@...ts.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/dri-devel
> >
>
> --
> Thomas Zimmermann
> Graphics Driver Developer
> SUSE Software Solutions Germany GmbH
> Maxfeldstr. 5, 90409 Nürnberg, Germany
> (HRB 36809, AG Nürnberg)
> Geschäftsführer: Felix Imendörffer


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

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