Re: [PATCH v6 9/9] drm: writeback: rename drm_writeback_connector_init_with_encoder()

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

New issues:
- [Low] The kernel-doc comment for `drm_writeback_connector_init` has mismatched parameter ordering compared to the newly adapted function signature.

Pre-existing issues:
- [Critical] Failure to initialize a subsequent writeback connector destroys global properties attached to already initialized writeback connectors, causing a Use-After-Free.
--

commit 04d131d999200c125f2670f5258a856880015f21
Author: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com>

drm: writeback: rename drm_writeback_connector_init_with_encoder()
    
Rename drm_writeback_connector_init_with_encoder() to
drm_writeback_connector_init() and adapt its interface to follow
drmm_writeback_connector_init().

> diff --git a/drivers/gpu/drm/drm_writeback.c b/drivers/gpu/drm/drm_writeback.c
> index 9fc15168c34f6..68fdac745f42f 100644
> --- a/drivers/gpu/drm/drm_writeback.c
> +++ b/drivers/gpu/drm/drm_writeback.c
> @@ -235,7 +235,7 @@ static int __drm_writeback_connector_init(struct drm_device *dev,
>  }

[Severity: Critical]
This isn't a bug introduced by this patch, but does the error path in
__drm_writeback_connector_init() improperly destroy global properties?

When __drm_writeback_connector_init() fails, it jumps to the
failed_properties label:

failed_properties:
	delete_writeback_properties(dev);
	return ret;

Then delete_writeback_properties() destroys the properties attached to the
device:

	if (dev->mode_config.writeback_pixel_formats_property) {
		drm_property_destroy(dev, dev->mode_config.writeback_pixel_formats_property);
		dev->mode_config.writeback_pixel_formats_property = NULL;
	}

If a driver attempts to initialize a second writeback connector and it fails,
won't this unconditionally free the global property objects that are still
attached to the first successfully initialized connector, causing a
use-after-free when those dangling pointers are later accessed?

>  
>  /**
> - * drm_writeback_connector_init_with_encoder - Initialize a writeback connector with
> + * drm_writeback_connector_init - Initialize a writeback connector with
>   * a custom encoder

[ ... ]

> @@ -263,11 +263,11 @@ static int __drm_writeback_connector_init(struct drm_device *dev,
>   *
>   * Returns: 0 on success, or a negative error code
>   */
> -int drm_writeback_connector_init_with_encoder(struct drm_device *dev,
> -					      struct drm_writeback_connector *wb_connector,
> -					      struct drm_encoder *enc,
> -					      const struct drm_connector_funcs *con_funcs,
> -					      const u32 *formats, int n_formats)
> +int drm_writeback_connector_init(struct drm_device *dev,
> +				 struct drm_writeback_connector *wb_connector,
> +				 const struct drm_connector_funcs *con_funcs,
> +				 struct drm_encoder *enc,
> +				 const u32 *formats, int n_formats)

[Severity: Low]
Since the parameter order of con_funcs and enc has been swapped in the
signature, does the kernel-doc comment above this function need to be
updated?

The kernel-doc still lists @enc before @con_funcs, which creates a mismatch
between the documentation and the new function signature.

>  {
>  	struct drm_connector *connector = &wb_connector->base;
>  	int ret;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260612-wb-drop-encoder-v6-0-9f3a54f81310@oss.qualcomm.com?part=9