diff mbox series

drm/atomic_helper: Add missing NULL check for drm_plane_helper_funcs.atomic_update

Message ID 20240927204616.697467-1-lyude@redhat.com (mailing list archive)
State New, archived
Headers show
Series drm/atomic_helper: Add missing NULL check for drm_plane_helper_funcs.atomic_update | expand

Commit Message

Lyude Paul Sept. 27, 2024, 8:46 p.m. UTC 
Something I discovered while writing rvkms since some versions of the
driver didn't have a filled out atomic_update function - we mention that
this callback is "optional", but we don't actually check whether it's NULL
or not before calling it. As a result, we'll segfault if it's not filled
in.

  rvkms rvkms.0: [drm:drm_atomic_helper_commit_modeset_disables] modeset on [ENCODER:36:Virtual-36]
  BUG: kernel NULL pointer dereference, address: 0000000000000000
  PGD 0 P4D 0
  Oops: Oops: 0010 [#1] PREEMPT SMP NOPTI
  Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS edk2-20240813-1.fc40 08/13/2024
  RIP: 0010:0x0

So, let's fix that.

Signed-off-by: Lyude Paul <lyude@redhat.com>
Fixes: c2fcd274bce5 ("drm: Add atomic/plane helpers")
Cc: dri-devel@lists.freedesktop.org
Cc: <stable@vger.kernel.org> # v3.19+
---
 drivers/gpu/drm/drm_atomic_helper.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)


base-commit: 22512c3ee0f47faab5def71c4453638923c62522

Comments

Thomas Zimmermann Sept. 30, 2024, 6:26 a.m. UTC | #1 
Am 27.09.24 um 22:46 schrieb Lyude Paul:
> Something I discovered while writing rvkms since some versions of the
> driver didn't have a filled out atomic_update function - we mention that
> this callback is "optional", but we don't actually check whether it's NULL
> or not before calling it. As a result, we'll segfault if it's not filled
> in.
>
>    rvkms rvkms.0: [drm:drm_atomic_helper_commit_modeset_disables] modeset on [ENCODER:36:Virtual-36]
>    BUG: kernel NULL pointer dereference, address: 0000000000000000
>    PGD 0 P4D 0
>    Oops: Oops: 0010 [#1] PREEMPT SMP NOPTI
>    Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS edk2-20240813-1.fc40 08/13/2024
>    RIP: 0010:0x0
>
> So, let's fix that.
>
> Signed-off-by: Lyude Paul <lyude@redhat.com>
> Fixes: c2fcd274bce5 ("drm: Add atomic/plane helpers")
> Cc: dri-devel@lists.freedesktop.org
> Cc: <stable@vger.kernel.org> # v3.19+

Reviewed-by: Thomas Zimmermann <tzimmermann@suse.de>

> ---
>   drivers/gpu/drm/drm_atomic_helper.c | 6 ++++--
>   1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> index 43cdf39019a44..b3c507040c6d6 100644
> --- a/drivers/gpu/drm/drm_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> @@ -2797,7 +2797,8 @@ void drm_atomic_helper_commit_planes(struct drm_device *dev,
>   
>   			funcs->atomic_disable(plane, old_state);
>   		} else if (new_plane_state->crtc || disabling) {
> -			funcs->atomic_update(plane, old_state);
> +			if (funcs->atomic_update)
> +				funcs->atomic_update(plane, old_state);
>   
>   			if (!disabling && funcs->atomic_enable) {
>   				if (drm_atomic_plane_enabling(old_plane_state, new_plane_state))
> @@ -2889,7 +2890,8 @@ drm_atomic_helper_commit_planes_on_crtc(struct drm_crtc_state *old_crtc_state)
>   		if (disabling && plane_funcs->atomic_disable) {
>   			plane_funcs->atomic_disable(plane, old_state);
>   		} else if (new_plane_state->crtc || disabling) {
> -			plane_funcs->atomic_update(plane, old_state);
> +			if (plane_funcs->atomic_update)
> +				plane_funcs->atomic_update(plane, old_state);
>   
>   			if (!disabling && plane_funcs->atomic_enable) {
>   				if (drm_atomic_plane_enabling(old_plane_state, new_plane_state))
>
> base-commit: 22512c3ee0f47faab5def71c4453638923c62522
Maxime Ripard Sept. 30, 2024, 7:01 a.m. UTC | #2 
Hi,

On Fri, Sep 27, 2024 at 04:46:16PM GMT, Lyude Paul wrote:
> Something I discovered while writing rvkms since some versions of the
> driver didn't have a filled out atomic_update function - we mention that
> this callback is "optional", but we don't actually check whether it's NULL
> or not before calling it. As a result, we'll segfault if it's not filled
> in.
> 
>   rvkms rvkms.0: [drm:drm_atomic_helper_commit_modeset_disables] modeset on [ENCODER:36:Virtual-36]
>   BUG: kernel NULL pointer dereference, address: 0000000000000000
>   PGD 0 P4D 0
>   Oops: Oops: 0010 [#1] PREEMPT SMP NOPTI
>   Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS edk2-20240813-1.fc40 08/13/2024
>   RIP: 0010:0x0
> 
> So, let's fix that.
> 
> Signed-off-by: Lyude Paul <lyude@redhat.com>
> Fixes: c2fcd274bce5 ("drm: Add atomic/plane helpers")
> Cc: dri-devel@lists.freedesktop.org
> Cc: <stable@vger.kernel.org> # v3.19+

So we had kind of a similar argument with drm_connector_init early this
year, but I do agree we shouldn't fault if we're missing a callback.

I do wonder how we can implement a plane without atomic_update though?
Do we have drivers in such a case?

If not, a better solution would be to make it mandatory and check it
when registering.

Maxime
Thomas Zimmermann Sept. 30, 2024, 7:06 a.m. UTC | #3 
Hi

Am 30.09.24 um 09:01 schrieb Maxime Ripard:
> Hi,
>
> On Fri, Sep 27, 2024 at 04:46:16PM GMT, Lyude Paul wrote:
>> Something I discovered while writing rvkms since some versions of the
>> driver didn't have a filled out atomic_update function - we mention that
>> this callback is "optional", but we don't actually check whether it's NULL
>> or not before calling it. As a result, we'll segfault if it's not filled
>> in.
>>
>>    rvkms rvkms.0: [drm:drm_atomic_helper_commit_modeset_disables] modeset on [ENCODER:36:Virtual-36]
>>    BUG: kernel NULL pointer dereference, address: 0000000000000000
>>    PGD 0 P4D 0
>>    Oops: Oops: 0010 [#1] PREEMPT SMP NOPTI
>>    Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS edk2-20240813-1.fc40 08/13/2024
>>    RIP: 0010:0x0
>>
>> So, let's fix that.
>>
>> Signed-off-by: Lyude Paul <lyude@redhat.com>
>> Fixes: c2fcd274bce5 ("drm: Add atomic/plane helpers")
>> Cc: dri-devel@lists.freedesktop.org
>> Cc: <stable@vger.kernel.org> # v3.19+
> So we had kind of a similar argument with drm_connector_init early this
> year, but I do agree we shouldn't fault if we're missing a callback.
>
> I do wonder how we can implement a plane without atomic_update though?
> Do we have drivers in such a case?

That would likely be an output with an entirely static display. Hard to 
imaging, I think.

>
> If not, a better solution would be to make it mandatory and check it
> when registering.

Although I r-b'ed the patch already, I'd also prefer this solution.


>
> Maxime
Lyude Paul Sept. 30, 2024, 7:45 p.m. UTC | #4 
On Mon, 2024-09-30 at 09:06 +0200, Thomas Zimmermann wrote:
> Hi
> 
> Am 30.09.24 um 09:01 schrieb Maxime Ripard:
> > Hi,
> > 
> > On Fri, Sep 27, 2024 at 04:46:16PM GMT, Lyude Paul wrote:
> > > Something I discovered while writing rvkms since some versions of the
> > > driver didn't have a filled out atomic_update function - we mention that
> > > this callback is "optional", but we don't actually check whether it's NULL
> > > or not before calling it. As a result, we'll segfault if it's not filled
> > > in.
> > > 
> > >    rvkms rvkms.0: [drm:drm_atomic_helper_commit_modeset_disables] modeset on [ENCODER:36:Virtual-36]
> > >    BUG: kernel NULL pointer dereference, address: 0000000000000000
> > >    PGD 0 P4D 0
> > >    Oops: Oops: 0010 [#1] PREEMPT SMP NOPTI
> > >    Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS edk2-20240813-1.fc40 08/13/2024
> > >    RIP: 0010:0x0
> > > 
> > > So, let's fix that.
> > > 
> > > Signed-off-by: Lyude Paul <lyude@redhat.com>
> > > Fixes: c2fcd274bce5 ("drm: Add atomic/plane helpers")
> > > Cc: dri-devel@lists.freedesktop.org
> > > Cc: <stable@vger.kernel.org> # v3.19+
> > So we had kind of a similar argument with drm_connector_init early this
> > year, but I do agree we shouldn't fault if we're missing a callback.
> > 
> > I do wonder how we can implement a plane without atomic_update though?
> > Do we have drivers in such a case?
> 
> That would likely be an output with an entirely static display. Hard to 
> imaging, I think.
> 
> > 
> > If not, a better solution would be to make it mandatory and check it
> > when registering.
> 
> Although I r-b'ed the patch already, I'd also prefer this solution.

Gotcha, FWIW the reason I went with this patch:
 * atomic_update is actually documented as being optional in the kernel docs,
   so we'd want to remove that if we make it mandatory
 * rvkms currently doesn't have an atomic_update. We will likely have one
   whenever I get a chance to actually add CRC and/or writeback connector
   supports - but for the time being all we do is register a KMS device with
   vblank support.

I am fine with either solution though

> 
> 
> > 
> > Maxime
>
Maxime Ripard Oct. 21, 2024, 1:30 p.m. UTC | #5 
Hi,

On Mon, Sep 30, 2024 at 03:45:13PM -0400, Lyude Paul wrote:
> On Mon, 2024-09-30 at 09:06 +0200, Thomas Zimmermann wrote:
> > Hi
> > 
> > Am 30.09.24 um 09:01 schrieb Maxime Ripard:
> > > Hi,
> > > 
> > > On Fri, Sep 27, 2024 at 04:46:16PM GMT, Lyude Paul wrote:
> > > > Something I discovered while writing rvkms since some versions of the
> > > > driver didn't have a filled out atomic_update function - we mention that
> > > > this callback is "optional", but we don't actually check whether it's NULL
> > > > or not before calling it. As a result, we'll segfault if it's not filled
> > > > in.
> > > > 
> > > >    rvkms rvkms.0: [drm:drm_atomic_helper_commit_modeset_disables] modeset on [ENCODER:36:Virtual-36]
> > > >    BUG: kernel NULL pointer dereference, address: 0000000000000000
> > > >    PGD 0 P4D 0
> > > >    Oops: Oops: 0010 [#1] PREEMPT SMP NOPTI
> > > >    Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS edk2-20240813-1.fc40 08/13/2024
> > > >    RIP: 0010:0x0
> > > > 
> > > > So, let's fix that.
> > > > 
> > > > Signed-off-by: Lyude Paul <lyude@redhat.com>
> > > > Fixes: c2fcd274bce5 ("drm: Add atomic/plane helpers")
> > > > Cc: dri-devel@lists.freedesktop.org
> > > > Cc: <stable@vger.kernel.org> # v3.19+
> > > So we had kind of a similar argument with drm_connector_init early this
> > > year, but I do agree we shouldn't fault if we're missing a callback.
> > > 
> > > I do wonder how we can implement a plane without atomic_update though?
> > > Do we have drivers in such a case?
> > 
> > That would likely be an output with an entirely static display. Hard to 
> > imaging, I think.
> > 
> > > 
> > > If not, a better solution would be to make it mandatory and check it
> > > when registering.
> > 
> > Although I r-b'ed the patch already, I'd also prefer this solution.
> 
> Gotcha, FWIW the reason I went with this patch:
>  * atomic_update is actually documented as being optional in the kernel docs,
>    so we'd want to remove that if we make it mandatory

Sure, that makes total sense :)

>  * rvkms currently doesn't have an atomic_update. We will likely have one
>    whenever I get a chance to actually add CRC and/or writeback connector
>    supports - but for the time being all we do is register a KMS device with
>    vblank support.

WIP drivers can provide an empty implementation. And even if actually
didn't need it for $REASONS, I'd argue that an empty implementation (and
a comment) makes that explicit instead of making the reader guess why
it's not needed.

Maxime
diff mbox series

Patch

diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
index 43cdf39019a44..b3c507040c6d6 100644
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -2797,7 +2797,8 @@  void drm_atomic_helper_commit_planes(struct drm_device *dev,
 
 			funcs->atomic_disable(plane, old_state);
 		} else if (new_plane_state->crtc || disabling) {
-			funcs->atomic_update(plane, old_state);
+			if (funcs->atomic_update)
+				funcs->atomic_update(plane, old_state);
 
 			if (!disabling && funcs->atomic_enable) {
 				if (drm_atomic_plane_enabling(old_plane_state, new_plane_state))
@@ -2889,7 +2890,8 @@  drm_atomic_helper_commit_planes_on_crtc(struct drm_crtc_state *old_crtc_state)
 		if (disabling && plane_funcs->atomic_disable) {
 			plane_funcs->atomic_disable(plane, old_state);
 		} else if (new_plane_state->crtc || disabling) {
-			plane_funcs->atomic_update(plane, old_state);
+			if (plane_funcs->atomic_update)
+				plane_funcs->atomic_update(plane, old_state);
 
 			if (!disabling && plane_funcs->atomic_enable) {
 				if (drm_atomic_plane_enabling(old_plane_state, new_plane_state))