Re: [dm-devel] [PATCH v2 5/5] Input: evdev - Switch to bitmap_zalloc()
On Wed, Jun 20, 2018 at 11:13:21AM +0300, Yury Norov wrote:
> On Tue, Jun 19, 2018 at 11:33:16AM -0700, Dmitry Torokhov wrote:
> > External Email
> >
> > On Sat, Jun 16, 2018 at 12:42:31AM +0300, Yury Norov wrote:
> > > Hi Andy,
> > >
> > > On Fri, Jun 15, 2018 at 04:20:17PM +0300, Andy Shevchenko wrote:
> > > > Switch to bitmap_zalloc() to show clearly what we are allocating.
> > > > Besides that it returns pointer of bitmap type instead of opaque void *.
> > > >
> > > > Acked-by: Dmitry Torokhov
> > > > Signed-off-by: Andy Shevchenko
> > > > ---
> > > > drivers/input/evdev.c | 16 +++-
> > > > 1 file changed, 7 insertions(+), 9 deletions(-)
> > > >
> > > > diff --git a/drivers/input/evdev.c b/drivers/input/evdev.c
> > > > index c81c79d01d93..370206f987f9 100644
> > > > --- a/drivers/input/evdev.c
> > > > +++ b/drivers/input/evdev.c
> > > > @@ -481,7 +481,7 @@ static int evdev_release(struct inode *inode,
> > > > struct file *file)
> > > > evdev_detach_client(evdev, client);
> > > >
> > > > for (i = 0; i < EV_CNT; ++i)
> > > > - kfree(client->evmasks[i]);
> > > > + bitmap_free(client->evmasks[i]);
> > > >
> > > > kvfree(client);
> > > >
> > > > @@ -925,17 +925,15 @@ static int evdev_handle_get_val(struct
> > > > evdev_client *client,
> > > > {
> > > > int ret;
> > > > unsigned long *mem;
> > > > - size_t len;
> > > >
> > > > - len = BITS_TO_LONGS(maxbit) * sizeof(unsigned long);
> > > > - mem = kmalloc(len, GFP_KERNEL);
> > > > + mem = bitmap_alloc(maxbit, GFP_KERNEL);
> > > > if (!mem)
> > > > return -ENOMEM;
> > >
> > > But in commit message you say you switch to bitmap_zalloc(). IIUC
> > > bitmap_alloc() is OK here. But could you please update comment to
> > > avoid confusing.
> > >
> > > >
> > > > spin_lock_irq(&dev->event_lock);
> > > > spin_lock(&client->buffer_lock);
> > > >
> > > > - memcpy(mem, bits, len);
> > > > + bitmap_copy(mem, bits, maxbit);
> > > >
> > > > spin_unlock(&dev->event_lock);
> > > >
> > > > @@ -947,7 +945,7 @@ static int evdev_handle_get_val(struct evdev_client
> > > > *client,
> > > > if (ret < 0)
> > > > evdev_queue_syn_dropped(client);
> > > >
> > > > - kfree(mem);
> > > > + bitmap_free(mem);
> > > >
> > > > return ret;
> > > > }
> > > > @@ -1003,13 +1001,13 @@ static int evdev_set_mask(struct evdev_client
> > > > *client,
> > > > if (!cnt)
> > > > return 0;
> > > >
> > > > - mask = kcalloc(sizeof(unsigned long), BITS_TO_LONGS(cnt),
> > > > GFP_KERNEL);
> > > > + mask = bitmap_zalloc(cnt, GFP_KERNEL);
> > > > if (!mask)
> > > > return -ENOMEM;
> > > >
> > > > error = bits_from_user(mask, cnt - 1, codes_size, codes,
> > > > compat);
> > >
> > > If my understanding of bits_from_user() correct, here you can also use
> > > bitmap_alloc(), true?
> >
> > bits_from_user() copies as much as user supplied, we want to zero out
> > the tail to make sure there is no garbage, so we want to use
> > kcalloc/kzalloc/bitmap_zalloc here.
>
> I don't understand that. Tail bits of bitmap (i.e. after last used bit
> till the end of last word) are always ignored by kernel code and there's
> no matter what was stored in that bits.
Users can supply as little as one long word worth of data (codes_size =
maxlen = 4). You really do not want the rest of the mask you will be
applying to contain random heap garbage.
>
> (With the exception of copying bitmap from kernel to userspace. For this
> case we have bitmap_copy_clear_tail() to avoid unintended exposing kernel
> data to user.)
>
> If you know any bitmap function that don't ignore tail bits, this is a
> bug and should be fixed.
>
> By the way, bits_from_user() is bad-designed because it takes 2 size
> arguments - maxbit and maxlen, and should be reworked. There's a
> single user of this function, and I suspect, it can be switched to
> existing core API, like bitmap_from_arr32().
I'm afraid you suspect wrong, as (unfortunately, but it is ABI now) we
are not dealing with masks consisting of u32 or u64 elements, but
"unsigned long" elements, which change size depending on 32/64 bit
architecture and whether we are dealing with compat or native userspace.
It also needs both maxbit and maxlen, because one is kernel's limit
while the other is limit from userspace POV and you need to reconcile
both to make sure you do not overrun buffers on either side.
Thanks.
--
Dmitry
--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel
Re: [dm-devel] [PATCH v2 5/5] Input: evdev - Switch to bitmap_zalloc()
On Sat, Jun 16, 2018 at 12:42:31AM +0300, Yury Norov wrote:
> Hi Andy,
>
> On Fri, Jun 15, 2018 at 04:20:17PM +0300, Andy Shevchenko wrote:
> > Switch to bitmap_zalloc() to show clearly what we are allocating.
> > Besides that it returns pointer of bitmap type instead of opaque void *.
> >
> > Acked-by: Dmitry Torokhov
> > Signed-off-by: Andy Shevchenko
> > ---
> > drivers/input/evdev.c | 16 +++-
> > 1 file changed, 7 insertions(+), 9 deletions(-)
> >
> > diff --git a/drivers/input/evdev.c b/drivers/input/evdev.c
> > index c81c79d01d93..370206f987f9 100644
> > --- a/drivers/input/evdev.c
> > +++ b/drivers/input/evdev.c
> > @@ -481,7 +481,7 @@ static int evdev_release(struct inode *inode, struct
> > file *file)
> > evdev_detach_client(evdev, client);
> >
> > for (i = 0; i < EV_CNT; ++i)
> > - kfree(client->evmasks[i]);
> > + bitmap_free(client->evmasks[i]);
> >
> > kvfree(client);
> >
> > @@ -925,17 +925,15 @@ static int evdev_handle_get_val(struct evdev_client
> > *client,
> > {
> > int ret;
> > unsigned long *mem;
> > - size_t len;
> >
> > - len = BITS_TO_LONGS(maxbit) * sizeof(unsigned long);
> > - mem = kmalloc(len, GFP_KERNEL);
> > + mem = bitmap_alloc(maxbit, GFP_KERNEL);
> > if (!mem)
> > return -ENOMEM;
>
> But in commit message you say you switch to bitmap_zalloc(). IIUC
> bitmap_alloc() is OK here. But could you please update comment to
> avoid confusing.
>
> >
> > spin_lock_irq(&dev->event_lock);
> > spin_lock(&client->buffer_lock);
> >
> > - memcpy(mem, bits, len);
> > + bitmap_copy(mem, bits, maxbit);
> >
> > spin_unlock(&dev->event_lock);
> >
> > @@ -947,7 +945,7 @@ static int evdev_handle_get_val(struct evdev_client
> > *client,
> > if (ret < 0)
> > evdev_queue_syn_dropped(client);
> >
> > - kfree(mem);
> > + bitmap_free(mem);
> >
> > return ret;
> > }
> > @@ -1003,13 +1001,13 @@ static int evdev_set_mask(struct evdev_client
> > *client,
> > if (!cnt)
> > return 0;
> >
> > - mask = kcalloc(sizeof(unsigned long), BITS_TO_LONGS(cnt),
> > GFP_KERNEL);
> > + mask = bitmap_zalloc(cnt, GFP_KERNEL);
> > if (!mask)
> > return -ENOMEM;
> >
> > error = bits_from_user(mask, cnt - 1, codes_size, codes, compat);
>
> If my understanding of bits_from_user() correct, here you can also use
> bitmap_alloc(), true?
bits_from_user() copies as much as user supplied, we want to zero out
the tail to make sure there is no garbage, so we want to use
kcalloc/kzalloc/bitmap_zalloc here.
Thanks.
--
Dmitry
--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel
Re: [dm-devel] [PATCH v2 5/5] Input: evdev - Switch to bitmap_zalloc()
On Tue, Jun 19, 2018 at 11:33:16AM -0700, Dmitry Torokhov wrote:
> External Email
>
> On Sat, Jun 16, 2018 at 12:42:31AM +0300, Yury Norov wrote:
> > Hi Andy,
> >
> > On Fri, Jun 15, 2018 at 04:20:17PM +0300, Andy Shevchenko wrote:
> > > Switch to bitmap_zalloc() to show clearly what we are allocating.
> > > Besides that it returns pointer of bitmap type instead of opaque void *.
> > >
> > > Acked-by: Dmitry Torokhov
> > > Signed-off-by: Andy Shevchenko
> > > ---
> > > drivers/input/evdev.c | 16 +++-
> > > 1 file changed, 7 insertions(+), 9 deletions(-)
> > >
> > > diff --git a/drivers/input/evdev.c b/drivers/input/evdev.c
> > > index c81c79d01d93..370206f987f9 100644
> > > --- a/drivers/input/evdev.c
> > > +++ b/drivers/input/evdev.c
> > > @@ -481,7 +481,7 @@ static int evdev_release(struct inode *inode, struct
> > > file *file)
> > > evdev_detach_client(evdev, client);
> > >
> > > for (i = 0; i < EV_CNT; ++i)
> > > - kfree(client->evmasks[i]);
> > > + bitmap_free(client->evmasks[i]);
> > >
> > > kvfree(client);
> > >
> > > @@ -925,17 +925,15 @@ static int evdev_handle_get_val(struct evdev_client
> > > *client,
> > > {
> > > int ret;
> > > unsigned long *mem;
> > > - size_t len;
> > >
> > > - len = BITS_TO_LONGS(maxbit) * sizeof(unsigned long);
> > > - mem = kmalloc(len, GFP_KERNEL);
> > > + mem = bitmap_alloc(maxbit, GFP_KERNEL);
> > > if (!mem)
> > > return -ENOMEM;
> >
> > But in commit message you say you switch to bitmap_zalloc(). IIUC
> > bitmap_alloc() is OK here. But could you please update comment to
> > avoid confusing.
> >
> > >
> > > spin_lock_irq(&dev->event_lock);
> > > spin_lock(&client->buffer_lock);
> > >
> > > - memcpy(mem, bits, len);
> > > + bitmap_copy(mem, bits, maxbit);
> > >
> > > spin_unlock(&dev->event_lock);
> > >
> > > @@ -947,7 +945,7 @@ static int evdev_handle_get_val(struct evdev_client
> > > *client,
> > > if (ret < 0)
> > > evdev_queue_syn_dropped(client);
> > >
> > > - kfree(mem);
> > > + bitmap_free(mem);
> > >
> > > return ret;
> > > }
> > > @@ -1003,13 +1001,13 @@ static int evdev_set_mask(struct evdev_client
> > > *client,
> > > if (!cnt)
> > > return 0;
> > >
> > > - mask = kcalloc(sizeof(unsigned long), BITS_TO_LONGS(cnt),
> > > GFP_KERNEL);
> > > + mask = bitmap_zalloc(cnt, GFP_KERNEL);
> > > if (!mask)
> > > return -ENOMEM;
> > >
> > > error = bits_from_user(mask, cnt - 1, codes_size, codes, compat);
> >
> > If my understanding of bits_from_user() correct, here you can also use
> > bitmap_alloc(), true?
>
> bits_from_user() copies as much as user supplied, we want to zero out
> the tail to make sure there is no garbage, so we want to use
> kcalloc/kzalloc/bitmap_zalloc here.
I don't understand that. Tail bits of bitmap (i.e. after last used bit
till the end of last word) are always ignored by kernel code and there's
no matter what was stored in that bits.
(With the exception of copying bitmap from kernel to userspace. For this
case we have bitmap_copy_clear_tail() to avoid unintended exposing kernel
data to user.)
If you know any bitmap function that don't ignore tail bits, this is a
bug and should be fixed.
By the way, bits_from_user() is bad-designed because it takes 2 size
arguments - maxbit and maxlen, and should be reworked. There's a
single user of this function, and I suspect, it can be switched to
existing core API, like bitmap_from_arr32().
Yury
--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel
Re: [dm-devel] [PATCH v2 5/5] Input: evdev - Switch to bitmap_zalloc()
On Sat, 2018-06-16 at 12:16 -0700, Joe Perches wrote: > On Sat, 2018-06-16 at 21:45 +0300, Andy Shevchenko wrote: > > On Sat, Jun 16, 2018 at 12:46 AM Yury Norov > om> wrote: > > > On Fri, Jun 15, 2018 at 04:20:17PM +0300, Andy Shevchenko wrote: > > > > Switch to bitmap_zalloc() to show clearly what we are > > > > allocating. > > > > Besides that it returns pointer of bitmap type instead of opaque > > > > void *. > > > > > > > > + mem = bitmap_alloc(maxbit, GFP_KERNEL); > > > > if (!mem) > > > > return -ENOMEM; > > > > > > But in commit message you say you switch to bitmap_zalloc(). IIUC > > > bitmap_alloc() is OK here. But could you please update comment to > > > avoid confusing. > > > > There are two places, one with alloc, another with zalloc. > > I will clarify this in commit message of next version. > > > > > > + mask = bitmap_zalloc(cnt, GFP_KERNEL); > > > > if (!mask) > > > > return -ENOMEM; > > > > > > > > error = bits_from_user(mask, cnt - 1, codes_size, codes, > > > > compat); > > > > > > If my understanding of bits_from_user() correct, here you can also > > > use > > > bitmap_alloc(), true? > > Also it might be useful to have a separate bitmap_from_user > to alloc and copy. Maybe. I didn't check if there are such users except this driver. Anyway, it's out of scope of the series. -- Andy Shevchenko Intel Finland Oy -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel
Re: [dm-devel] [PATCH v2 5/5] Input: evdev - Switch to bitmap_zalloc()
On Mon, Jun 18, 2018 at 6:49 PM, Joe Perches wrote: > On Mon, 2018-06-18 at 15:02 +0300, Andy Shevchenko wrote: >> On Sat, 2018-06-16 at 12:16 -0700, Joe Perches wrote: >> > On Sat, 2018-06-16 at 21:45 +0300, Andy Shevchenko wrote: >> > > On Sat, Jun 16, 2018 at 12:46 AM Yury Norov > > > om> wrote: >> > > > On Fri, Jun 15, 2018 at 04:20:17PM +0300, Andy Shevchenko wrote: >> > > > > Switch to bitmap_zalloc() to show clearly what we are >> > > > > allocating. >> > > > > Besides that it returns pointer of bitmap type instead of opaque >> > > > > void *. >> > > >> > > >> > > > > + mem = bitmap_alloc(maxbit, GFP_KERNEL); >> > > > > if (!mem) >> > > > > return -ENOMEM; >> > > > >> > > > But in commit message you say you switch to bitmap_zalloc(). IIUC >> > > > bitmap_alloc() is OK here. But could you please update comment to >> > > > avoid confusing. >> > > >> > > There are two places, one with alloc, another with zalloc. >> > > I will clarify this in commit message of next version. >> > > >> > > > > + mask = bitmap_zalloc(cnt, GFP_KERNEL); >> > > > > if (!mask) >> > > > > return -ENOMEM; >> > > > > >> > > > > error = bits_from_user(mask, cnt - 1, codes_size, codes, >> > > > > compat); >> > > > >> > > > If my understanding of bits_from_user() correct, here you can also >> > > > use >> > > > bitmap_alloc(), true? >> > >> > Also it might be useful to have a separate bitmap_from_user >> > to alloc and copy. >> >> Maybe. I didn't check if there are such users except this driver. >> >> Anyway, it's out of scope of the series. > > That seems incorrect as you are introducing alloc/free helpers. > > Perhaps bitmap_dup_user [or some better name] could or should > be one of the helpers. Can you help with estimation how many existing users need this kind of functionality? One of them evdev, which has an open coded variant. Also, pay attention to that fact we have already bitmap_parse_user() and bitmap_parlelist_user() which are called by several users. If the estimation will show something like 3+, it would definitely make sense, otherwise, I wouldn't like spend time on it. -- With Best Regards, Andy Shevchenko -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel
Re: [dm-devel] [PATCH v2 5/5] Input: evdev - Switch to bitmap_zalloc()
On Mon, 2018-06-18 at 22:56 +0300, Andy Shevchenko wrote: > On Mon, Jun 18, 2018 at 6:49 PM, Joe Perches wrote: > > Perhaps bitmap_dup_user [or some better name] could or should > > be one of the helpers. > > Can you help with estimation how many existing users need this kind of > functionality? One of them evdev, which has an open coded variant. My estimation via cocci script below is 1 existing user, so it's almost certainly not worthwhile. $ cat copy_from_user.cocci @@ unsigned long *p; expression e1, e2; @@ * copy_from_user(p, e1, e2) -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel
Re: [dm-devel] [PATCH v2 5/5] Input: evdev - Switch to bitmap_zalloc()
On Mon, 2018-06-18 at 15:02 +0300, Andy Shevchenko wrote: > On Sat, 2018-06-16 at 12:16 -0700, Joe Perches wrote: > > On Sat, 2018-06-16 at 21:45 +0300, Andy Shevchenko wrote: > > > On Sat, Jun 16, 2018 at 12:46 AM Yury Norov > > om> wrote: > > > > On Fri, Jun 15, 2018 at 04:20:17PM +0300, Andy Shevchenko wrote: > > > > > Switch to bitmap_zalloc() to show clearly what we are > > > > > allocating. > > > > > Besides that it returns pointer of bitmap type instead of opaque > > > > > void *. > > > > > > > > > > > + mem = bitmap_alloc(maxbit, GFP_KERNEL); > > > > > if (!mem) > > > > > return -ENOMEM; > > > > > > > > But in commit message you say you switch to bitmap_zalloc(). IIUC > > > > bitmap_alloc() is OK here. But could you please update comment to > > > > avoid confusing. > > > > > > There are two places, one with alloc, another with zalloc. > > > I will clarify this in commit message of next version. > > > > > > > > + mask = bitmap_zalloc(cnt, GFP_KERNEL); > > > > > if (!mask) > > > > > return -ENOMEM; > > > > > > > > > > error = bits_from_user(mask, cnt - 1, codes_size, codes, > > > > > compat); > > > > > > > > If my understanding of bits_from_user() correct, here you can also > > > > use > > > > bitmap_alloc(), true? > > > > Also it might be useful to have a separate bitmap_from_user > > to alloc and copy. > > Maybe. I didn't check if there are such users except this driver. > > Anyway, it's out of scope of the series. That seems incorrect as you are introducing alloc/free helpers. Perhaps bitmap_dup_user [or some better name] could or should be one of the helpers. -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel
Re: [dm-devel] [PATCH v2 5/5] Input: evdev - Switch to bitmap_zalloc()
On Sat, Jun 16, 2018 at 12:46 AM Yury Norov wrote: > On Fri, Jun 15, 2018 at 04:20:17PM +0300, Andy Shevchenko wrote: > > Switch to bitmap_zalloc() to show clearly what we are allocating. > > Besides that it returns pointer of bitmap type instead of opaque void *. > > + mem = bitmap_alloc(maxbit, GFP_KERNEL); > > if (!mem) > > return -ENOMEM; > > But in commit message you say you switch to bitmap_zalloc(). IIUC > bitmap_alloc() is OK here. But could you please update comment to > avoid confusing. There are two places, one with alloc, another with zalloc. I will clarify this in commit message of next version. > > + mask = bitmap_zalloc(cnt, GFP_KERNEL); > > if (!mask) > > return -ENOMEM; > > > > error = bits_from_user(mask, cnt - 1, codes_size, codes, compat); > > If my understanding of bits_from_user() correct, here you can also use > bitmap_alloc(), true? While it might be true, it's a material for separate change. Original code uses zalloc version. -- With Best Regards, Andy Shevchenko -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel
Re: [dm-devel] [PATCH v2 5/5] Input: evdev - Switch to bitmap_zalloc()
Hi Andy,
On Fri, Jun 15, 2018 at 04:20:17PM +0300, Andy Shevchenko wrote:
> Switch to bitmap_zalloc() to show clearly what we are allocating.
> Besides that it returns pointer of bitmap type instead of opaque void *.
>
> Acked-by: Dmitry Torokhov
> Signed-off-by: Andy Shevchenko
> ---
> drivers/input/evdev.c | 16 +++-
> 1 file changed, 7 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/input/evdev.c b/drivers/input/evdev.c
> index c81c79d01d93..370206f987f9 100644
> --- a/drivers/input/evdev.c
> +++ b/drivers/input/evdev.c
> @@ -481,7 +481,7 @@ static int evdev_release(struct inode *inode, struct file
> *file)
> evdev_detach_client(evdev, client);
>
> for (i = 0; i < EV_CNT; ++i)
> - kfree(client->evmasks[i]);
> + bitmap_free(client->evmasks[i]);
>
> kvfree(client);
>
> @@ -925,17 +925,15 @@ static int evdev_handle_get_val(struct evdev_client
> *client,
> {
> int ret;
> unsigned long *mem;
> - size_t len;
>
> - len = BITS_TO_LONGS(maxbit) * sizeof(unsigned long);
> - mem = kmalloc(len, GFP_KERNEL);
> + mem = bitmap_alloc(maxbit, GFP_KERNEL);
> if (!mem)
> return -ENOMEM;
But in commit message you say you switch to bitmap_zalloc(). IIUC
bitmap_alloc() is OK here. But could you please update comment to
avoid confusing.
>
> spin_lock_irq(&dev->event_lock);
> spin_lock(&client->buffer_lock);
>
> - memcpy(mem, bits, len);
> + bitmap_copy(mem, bits, maxbit);
>
> spin_unlock(&dev->event_lock);
>
> @@ -947,7 +945,7 @@ static int evdev_handle_get_val(struct evdev_client
> *client,
> if (ret < 0)
> evdev_queue_syn_dropped(client);
>
> - kfree(mem);
> + bitmap_free(mem);
>
> return ret;
> }
> @@ -1003,13 +1001,13 @@ static int evdev_set_mask(struct evdev_client *client,
> if (!cnt)
> return 0;
>
> - mask = kcalloc(sizeof(unsigned long), BITS_TO_LONGS(cnt), GFP_KERNEL);
> + mask = bitmap_zalloc(cnt, GFP_KERNEL);
> if (!mask)
> return -ENOMEM;
>
> error = bits_from_user(mask, cnt - 1, codes_size, codes, compat);
If my understanding of bits_from_user() correct, here you can also use
bitmap_alloc(), true?
> if (error < 0) {
> - kfree(mask);
> + bitmap_free(mask);
> return error;
> }
>
> @@ -1018,7 +1016,7 @@ static int evdev_set_mask(struct evdev_client *client,
> client->evmasks[type] = mask;
> spin_unlock_irqrestore(&client->buffer_lock, flags);
>
> - kfree(oldmask);
> + bitmap_free(oldmask);
>
> return 0;
> }
> --
> 2.17.1
--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel
[dm-devel] [PATCH v2 5/5] Input: evdev - Switch to bitmap_zalloc()
Switch to bitmap_zalloc() to show clearly what we are allocating.
Besides that it returns pointer of bitmap type instead of opaque void *.
Acked-by: Dmitry Torokhov
Signed-off-by: Andy Shevchenko
---
drivers/input/evdev.c | 16 +++-
1 file changed, 7 insertions(+), 9 deletions(-)
diff --git a/drivers/input/evdev.c b/drivers/input/evdev.c
index c81c79d01d93..370206f987f9 100644
--- a/drivers/input/evdev.c
+++ b/drivers/input/evdev.c
@@ -481,7 +481,7 @@ static int evdev_release(struct inode *inode, struct file
*file)
evdev_detach_client(evdev, client);
for (i = 0; i < EV_CNT; ++i)
- kfree(client->evmasks[i]);
+ bitmap_free(client->evmasks[i]);
kvfree(client);
@@ -925,17 +925,15 @@ static int evdev_handle_get_val(struct evdev_client
*client,
{
int ret;
unsigned long *mem;
- size_t len;
- len = BITS_TO_LONGS(maxbit) * sizeof(unsigned long);
- mem = kmalloc(len, GFP_KERNEL);
+ mem = bitmap_alloc(maxbit, GFP_KERNEL);
if (!mem)
return -ENOMEM;
spin_lock_irq(&dev->event_lock);
spin_lock(&client->buffer_lock);
- memcpy(mem, bits, len);
+ bitmap_copy(mem, bits, maxbit);
spin_unlock(&dev->event_lock);
@@ -947,7 +945,7 @@ static int evdev_handle_get_val(struct evdev_client *client,
if (ret < 0)
evdev_queue_syn_dropped(client);
- kfree(mem);
+ bitmap_free(mem);
return ret;
}
@@ -1003,13 +1001,13 @@ static int evdev_set_mask(struct evdev_client *client,
if (!cnt)
return 0;
- mask = kcalloc(sizeof(unsigned long), BITS_TO_LONGS(cnt), GFP_KERNEL);
+ mask = bitmap_zalloc(cnt, GFP_KERNEL);
if (!mask)
return -ENOMEM;
error = bits_from_user(mask, cnt - 1, codes_size, codes, compat);
if (error < 0) {
- kfree(mask);
+ bitmap_free(mask);
return error;
}
@@ -1018,7 +1016,7 @@ static int evdev_set_mask(struct evdev_client *client,
client->evmasks[type] = mask;
spin_unlock_irqrestore(&client->buffer_lock, flags);
- kfree(oldmask);
+ bitmap_free(oldmask);
return 0;
}
--
2.17.1
--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel
Re: [dm-devel] [PATCH v2 5/5] Input: evdev - Switch to bitmap_zalloc()
On Sat, 2018-06-16 at 21:45 +0300, Andy Shevchenko wrote: > On Sat, Jun 16, 2018 at 12:46 AM Yury Norov wrote: > > On Fri, Jun 15, 2018 at 04:20:17PM +0300, Andy Shevchenko wrote: > > > Switch to bitmap_zalloc() to show clearly what we are allocating. > > > Besides that it returns pointer of bitmap type instead of opaque void *. > > > > > + mem = bitmap_alloc(maxbit, GFP_KERNEL); > > > if (!mem) > > > return -ENOMEM; > > > > But in commit message you say you switch to bitmap_zalloc(). IIUC > > bitmap_alloc() is OK here. But could you please update comment to > > avoid confusing. > > There are two places, one with alloc, another with zalloc. > I will clarify this in commit message of next version. > > > > + mask = bitmap_zalloc(cnt, GFP_KERNEL); > > > if (!mask) > > > return -ENOMEM; > > > > > > error = bits_from_user(mask, cnt - 1, codes_size, codes, compat); > > > > If my understanding of bits_from_user() correct, here you can also use > > bitmap_alloc(), true? Also it might be useful to have a separate bitmap_from_user to alloc and copy. -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel
