| Message ID | 20180615132017.23889-6-andriy.shevchenko@linux.intel.com (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | Mike Snitzer |
| Headers | show |
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 <ynorov@caviumnetworks.c > > 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.
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 <ynorov@caviumnetworks.c > > > 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
On Mon, Jun 18, 2018 at 6:49 PM, Joe Perches <joe@perches.com> 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 <ynorov@caviumnetworks.c >> > > 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.
On Mon, 2018-06-18 at 22:56 +0300, Andy Shevchenko wrote: > On Mon, Jun 18, 2018 at 6:49 PM, Joe Perches <joe@perches.com> 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
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 <dmitry.torokhov@gmail.com> > > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > > --- > > 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.
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 <dmitry.torokhov@gmail.com> > > > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > > > --- > > > 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
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 <dmitry.torokhov@gmail.com> > > > > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > > > > --- > > > > 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.
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; }