linux-kernel - Re: [PATCH v2 2/2] drm/fb_helper: implement ioctl FBIO

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]

Date:   Thu, 9 Feb 2017 20:06:08 +0100
From:   Daniel Vetter <daniel@...ll.ch>
To:     Daniel Stone <daniel@...ishbar.org>
Cc:     Maxime Ripard <maxime.ripard@...e-electrons.com>,
        Daniel Vetter <daniel.vetter@...el.com>,
        David Airlie <airlied@...ux.ie>,
        Jani Nikula <jani.nikula@...ux.intel.com>,
        Sean Paul <seanpaul@...omium.org>,
        Stefan Christ <s.christ@...tec.de>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        dri-devel <dri-devel@...ts.freedesktop.org>
Subject: Re: [PATCH v2 2/2] drm/fb_helper: implement ioctl FBIO_WAITFORVSYNC

On Thu, Feb 09, 2017 at 05:38:26PM +0000, Daniel Stone wrote:
> Hi,
> 
> On 9 February 2017 at 17:01, Daniel Vetter <daniel@...ll.ch> wrote:
> > On Thu, Feb 02, 2017 at 11:31:57AM +0100, Maxime Ripard wrote:
> >> +int drm_fb_helper_ioctl(struct fb_info *info, unsigned int cmd, unsigned long arg)
> >> +{
> >> +     struct drm_fb_helper *fb_helper = info->par;
> >> +     struct drm_device *dev = fb_helper->dev;
> >> +     unsigned int i;
> >> +     int ret = 0;
> >> +
> >> +     drm_modeset_lock_all(dev);
> >> +     if (!drm_fb_helper_is_bound(fb_helper)) {
> >> +             drm_modeset_unlock_all(dev);
> >> +             return -EBUSY;
> >> +     }
> >> +
> >> +     switch (cmd) {
> >> +     case FBIO_WAITFORVSYNC:
> >> +             for (i = 0; i < fb_helper->crtc_count; i++) {
> >> +                     struct drm_mode_set *mode_set;
> >> +                     struct drm_crtc *crtc;
> >> +
> >> +                     mode_set = &fb_helper->crtc_info[i].mode_set;
> >> +                     crtc = mode_set->crtc;
> >> +
> >> +                     /*
> >> +                      * Only call drm_crtc_wait_one_vblank for crtcs that
> >> +                      * are currently enabled.
> >> +                      */
> >> +                     if (!crtc->enabled)
> >> +                             continue;
> >
> > This needs locking
> 
> More locking than the drm_modeset_lock_all() above the switch? Ouch. :)

Oh, I didn't spot that one. Well, that one should be removed imo, we dont
want any vblank wait path to hold these locks, it looks bad. Otoh we still
need to grab the dev->mode_config.mutex, because that's also the fbdev
lock. There's a plan somewhere to give the fbdev helpers their own lock,
so switching to just mode_config.mutex + the changes I suggestd would be
good.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch