lkml.org 
[lkml]   [2017]   [Mar]   [7]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH v2] Input: sparse-keymap - use a managed allocation for keymap copy
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@kempniu.pl>
> ---
> 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

\
 
 \ /
  Last update: 2017-03-07 19:16    [W:0.075 / U:0.216 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site