linux-kernel - Re: [PATCH 2/4] drm: adp: Handle drm_crtc_vblank

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] [day] [month] [year] [list]

Message-ID: <20250416223931.GC8400@robin.jannau.net>
Date: Thu, 17 Apr 2025 00:39:31 +0200
From: Janne Grunau <j@...nau.net>
To: Alyssa Rosenzweig <alyssa@...enzweig.io>
Cc: Sasha Finkelstein <fnkl.kernel@...il.com>,
	Maarten Lankhorst <maarten.lankhorst@...ux.intel.com>,
	Maxime Ripard <mripard@...nel.org>,
	Thomas Zimmermann <tzimmermann@...e.de>,
	David Airlie <airlied@...il.com>, Simona Vetter <simona@...ll.ch>,
	Neal Gompa <neal@...pa.dev>, Dmitry Baryshkov <lumag@...nel.org>,
	dri-devel@...ts.freedesktop.org, asahi@...ts.linux.dev,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH 2/4] drm: adp: Handle drm_crtc_vblank_get() errors

On Wed, Apr 16, 2025 at 04:58:18PM -0400, Alyssa Rosenzweig wrote:
> > -	spin_lock_irqsave(&crtc->dev->event_lock, flags);
> >  	if (crtc->state->event) {
> > -		drm_crtc_vblank_get(crtc);
> > -		adp->event = crtc->state->event;
> > +		spin_lock_irqsave(&crtc->dev->event_lock, flags);
> > +
> > +		if (drm_crtc_vblank_get(crtc) != 0)
> > +			drm_crtc_send_vblank_event(crtc, crtc->state->event);
> > +		else
> > +			adp->event = crtc->state->event;
> > +
> > +		spin_unlock_irqrestore(&crtc->dev->event_lock, flags);
> >  	}
> >  	crtc->state->event = NULL;
> > -	spin_unlock_irqrestore(&crtc->dev->event_lock, flags);
> 
> Kind of confused about
> 
> >  	crtc->state->event = NULL;
> 
> now being out of the lock. Should we set to NULL in the if, since
> if we don't take the if, we know event is already NULL? Or should we
> hold the lock for the whole time, the way the code did before your
> change? I'm not sure between the two, but the in-between here smells
> wrong.

I struggled with this as well. To my understanding event_lock is
necessary for drm_crtc_send_vblank_event(), adp->event and
drm_crtc_vblank_get(). The first according to event_lock's
documentation, the second to avoid avoid races with the irq handler and
the third to ensure vblank interrupts are not disabled.
Based on examples in other drivers I assumed `crtc->state->event` is
protected by another lock held externally. Not sure about that now. To
my understanding sruct drm_crtc::mutex protects `crtc->state`.

I did not move "crtc->state->event = NULL;" to avoid churn. No point
setting it to NULL if it is already NULL.

I'll look tomorrow if the locking for crtc->state->event is sufficient.

Janne