Re: [PATCH v2] Input: sparse-keymap - use a managed allocation for keymap copy

[Dmitry Torokhov]

On Thu, Mar 02, 2017 at 01:02:42PM +0100, MichaÅ KÄpieÅ wrote:
> Some platform drivers use devm_input_allocate_device() together with
> sparse_keymap_setup() in their .probe callbacks.  While using the former
> simplifies error handling, using the latter necessitates calling
> sparse_keymap_free() in the error path and upon module unloading to
> avoid leaking the copy of the keymap allocated by sparse_keymap_setup().
> 
> To help prevent such leaks and enable simpler error handling, make
> sparse_keymap_setup() use devm_kcalloc() to allocate memory for the
> keymap copy so that it gets automatically freed.
> 
> This works for both managed and non-managed input devices, although both
> are handled a bit differently.  Specifically, the managed keymap copy is
> attached to a different struct device:
> 
>   - for managed input devices, as all other managed resources are
>     attached to the device owning the input device, we do the same to
>     ensure freeing the keymap copy is properly slotted in the devres
>     stack,
> 
>   - for non-managed input devices, the managed keymap copy is attached
>     to the struct device embedded inside the input device itself, so
>     that the keymap is freed after the last reference to the input
>     device is dropped, i.e. after input_unregister_device() is called.
> 
> Note that actions previously taken by sparse_keymap_free(), i.e. taking
> the input device's mutex and zeroing its keycode and keycodemax fields,
> are now redundant because the managed keymap will always be freed after
> the input device is unregistered.
> 
> Signed-off-by: MichaÅ KÄpieÅ <kernel@xxxxxxxxxx>
> ---
> Changes from v1:
> 
>   - Do not add a new function.  Instead, make sparse_keymap_setup()
>     always use a managed allocation, which allows making
>     sparse_keymap_free() a noop and simplifies error handling.
> 
>   - Update subject, commit message and comments accordingly.
> 
>  drivers/input/sparse-keymap.c | 37 +++++++++----------------------------
>  1 file changed, 9 insertions(+), 28 deletions(-)
> 
> diff --git a/drivers/input/sparse-keymap.c b/drivers/input/sparse-keymap.c
> index e7409c45bdd0..fd391c339b4e 100644
> --- a/drivers/input/sparse-keymap.c
> +++ b/drivers/input/sparse-keymap.c
> @@ -160,12 +160,12 @@ static int sparse_keymap_setkeycode(struct input_dev *dev,
>   * @keymap: Keymap in form of array of &key_entry structures ending
>   *	with %KE_END type entry
>   * @setup: Function that can be used to adjust keymap entries
> - *	depending on device's deeds, may be %NULL
> + *	depending on device's needs, may be %NULL
>   *
>   * The function calculates size and allocates copy of the original
>   * keymap after which sets up input device event bits appropriately.
> - * Before destroying input device allocated keymap should be freed
> - * with a call to sparse_keymap_free().
> + * The allocated copy of the keymap is automatically freed when it
> + * is no longer needed.
>   */
>  int sparse_keymap_setup(struct input_dev *dev,
>  			const struct key_entry *keymap,
> @@ -180,7 +180,8 @@ int sparse_keymap_setup(struct input_dev *dev,
>  	for (e = keymap; e->type != KE_END; e++)
>  		map_size++;
>  
> -	map = kcalloc(map_size, sizeof(struct key_entry), GFP_KERNEL);
> +	map = devm_kcalloc(dev->devres_managed ? dev->dev.parent : &dev->dev,

Please always use input device as the owner of the keymap; there is no
reason to make distinction between managed and non-managed case.

Also maybe do:

	map = devm_kmemdup(&dev->dev, map_size * sizeof(*map), GFP_KERNEL);

There is not a chance of owerflow as we are copying existing valid
object, not using arbitrary parameters passed in.

Thanks.

-- 
Dmitry