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: <4525565.mVzskEzFVD@avalon>
Date:	Mon, 23 Nov 2015 17:42:57 +0200
From:	Laurent Pinchart <laurent.pinchart@...asonboard.com>
To:	Javier Martinez Canillas <javier@....samsung.com>
Cc:	linux-kernel@...r.kernel.org,
	Mauro Carvalho Chehab <mchehab@....samsung.com>,
	linux-media@...r.kernel.org, Shuah Khan <shuahkh@....samsung.com>
Subject: Re: [PATCH 1/2] [media] media: don't try to empty links list in media_entity_cleanup()

Hello Javier,

(Resending as I've replied by mistake to the version of the patch you had sent 
to the media workshop list only)

Thank you for the patch.

On Monday 12 October 2015 13:44:11 Mauro Carvalho Chehab wrote:
> From: Javier Martinez Canillas <javier@....samsung.com>
> 
> The media_entity_cleanup() function only cleans up the entity links list
> but this operation is already made in media_device_unregister_entity().
> 
> In most cases this should be harmless (besides having duplicated code)
> since the links list would be empty so the iteration would not happen
> but the links list is initialized in media_device_register_entity() so
> if a driver fails to register an entity with a media device and clean up
> the entity in the error path, a NULL deference pointer error will happen.
> 
> So don't try to empty the links list in media_entity_cleanup() since
> is either done already or haven't been initialized yet.

Does this mean that it's an invalid usage of the API to create links before 
registering entities ? If so it should be clearly documented somewhere, such 
as in the kerneldoc of the media_create_pad_link() function.

And yes, that means that all exported API functions need kerneldoc. Sorry for 
being a killjoy 

On a related note, we need to solve the userspace API race caused by 
registering the MC devnode before all entities and links are created an 
registered. It's not a new issue so I won't call for fixing it as part of this 
patch series, but we'll need to fix that with the dynamic graph update 
implementation at the latest. It will likely require reworking the 
initialization and registration sequences.

> Signed-off-by: Javier Martinez Canillas <javier@....samsung.com>
> ---
>  drivers/media/media-entity.c | 7 -------
>  1 file changed, 7 deletions(-)
> 
> diff --git a/drivers/media/media-entity.c b/drivers/media/media-entity.c
> index 2c984fb7d497..eaeda2589ce5 100644
> --- a/drivers/media/media-entity.c
> +++ b/drivers/media/media-entity.c
> @@ -268,13 +268,6 @@ EXPORT_SYMBOL_GPL(media_entity_init);
>  void
>  media_entity_cleanup(struct media_entity *entity)
>  {
> -     struct media_link *link, *tmp;
> -
> -     list_for_each_entry_safe(link, tmp, &entity->links, list) {
> -             media_gobj_remove(&link->graph_obj);
> -             list_del(&link->list);
> -             kfree(link);
> -     }
>  }
>  EXPORT_SYMBOL_GPL(media_entity_cleanup);

As media_entity_cleanup is now empty I'd turn it into a static inline. We need 
to keep the function in case cleanup ends up being needed later, but there's 
no reason not to optimize the call away for now.

-- 
Regards,

Laurent Pinchart

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ