Re: [PATCH 1/4] drm/plane: Make init functions panic consitently and explicitly

[Jani Nikula <jani.nikula@linux.intel.com>]

On Wed, 06 Dec 2023, Maxime Ripard <mripard@kernel.org> wrote:
> All of the current plane init / allocation functions behave slightly
> differently when it comes to argument sanitizing:
>
>  - drm_universal_plane_init() implicitly panics if the drm_device
>    pointer or the drm_plane_funcs pointer are NULL, and calls WARN_ON if
>    there's no destroy implementation but goes on with the initialization.
>
>  - drm_universal_plane_alloc() implicitly panics if the drm_device
>    pointer is NULL, and will call WARN_ON and return an error if the
>    drm_plane_funcs pointer is NULL.
>
>  - drmm_universal_plane_alloc() implicitly panics if the drm_device
>    pointer is NULL, and will call WARN_ON and return an error if the
>    drm_plane_funcs pointer is NULL or if there is a destroy
>    implementation.

NULL deref oopses but doesn't necessarily panic, right? Adding BUG() or
BUG_ON() to unconditionally panic is not the way to go, either.

BR,
Jani.


>
> The current consensus is that the drm_device pointer, the
> drm_plane_funcs pointer, and the drm_plane pointer if relevant, should
> be considered pre-requisite and the function should panic if we
> encounter such a situation, and that returning an error in such a
> situation is not welcome.
>
> Let's make all functions consider those three pointers to be always set
> and explicitly panic if they aren't. And let's document that behaviour
> too.
>
> Link: https://lore.kernel.org/dri-devel/20231128-kms-hdmi-connector-state-v4-5-c7602158306e@kernel.org/
> Signed-off-by: Maxime Ripard <mripard@kernel.org>
> ---
>  drivers/gpu/drm/drm_plane.c | 15 +++++++++++----
>  include/drm/drm_plane.h     |  6 ++++++
>  2 files changed, 17 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c
> index 9e8e4c60983d..ce0fa98a0e3f 100644
> --- a/drivers/gpu/drm/drm_plane.c
> +++ b/drivers/gpu/drm/drm_plane.c
> @@ -482,6 +482,9 @@ static int __drm_universal_plane_init(struct drm_device *dev,
>   *
>   * Returns:
>   * Zero on success, error code on failure.
> + *
> + * Panics:
> + * If @dev, @plane or @funcs are NULL.
>   */
>  int drm_universal_plane_init(struct drm_device *dev, struct drm_plane *plane,
>  			     uint32_t possible_crtcs,
> @@ -494,6 +497,9 @@ int drm_universal_plane_init(struct drm_device *dev, struct drm_plane *plane,
>  	va_list ap;
>  	int ret;
>  
> +	BUG_ON(!dev);
> +	BUG_ON(!plane);
> +	BUG_ON(!funcs);
>  	WARN_ON(!funcs->destroy);
>  
>  	va_start(ap, name);
> @@ -528,8 +534,9 @@ void *__drmm_universal_plane_alloc(struct drm_device *dev, size_t size,
>  	va_list ap;
>  	int ret;
>  
> -	if (WARN_ON(!funcs || funcs->destroy))
> -		return ERR_PTR(-EINVAL);
> +	BUG_ON(!dev);
> +	BUG_ON(!funcs);
> +	WARN_ON(funcs->destroy);
>  
>  	container = drmm_kzalloc(dev, size, GFP_KERNEL);
>  	if (!container)
> @@ -567,8 +574,8 @@ void *__drm_universal_plane_alloc(struct drm_device *dev, size_t size,
>  	va_list ap;
>  	int ret;
>  
> -	if (drm_WARN_ON(dev, !funcs))
> -		return ERR_PTR(-EINVAL);
> +	BUG_ON(!dev);
> +	BUG_ON(!funcs);
>  
>  	container = kzalloc(size, GFP_KERNEL);
>  	if (!container)
> diff --git a/include/drm/drm_plane.h b/include/drm/drm_plane.h
> index c6565a6f9324..2dab1b360fa2 100644
> --- a/include/drm/drm_plane.h
> +++ b/include/drm/drm_plane.h
> @@ -824,6 +824,9 @@ void *__drmm_universal_plane_alloc(struct drm_device *dev,
>   *
>   * Returns:
>   * Pointer to new plane, or ERR_PTR on failure.
> + *
> + * Panics:
> + * If @dev or @funcs are NULL.
>   */
>  #define drmm_universal_plane_alloc(dev, type, member, possible_crtcs, funcs, formats, \
>  				   format_count, format_modifiers, plane_type, name, ...) \
> @@ -868,6 +871,9 @@ void *__drm_universal_plane_alloc(struct drm_device *dev,
>   *
>   * Returns:
>   * Pointer to new plane, or ERR_PTR on failure.
> + *
> + * Panics:
> + * If @dev or @funcs are NULL.
>   */
>  #define drm_universal_plane_alloc(dev, type, member, possible_crtcs, funcs, formats, \
>  				   format_count, format_modifiers, plane_type, name, ...) \

-- 
Jani Nikula, Intel