| Message ID | 20200629065008.27620-5-brgl@bgdev.pl (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
| Series | devres: provide and use devm_krealloc() | expand |
Hi Bartosz, On 29/06/2020 07:50, Bartosz Golaszewski wrote: > From: Bartosz Golaszewski <bgolaszewski@baylibre.com> > > Make devm_kmalloc() behave similarly to non-managed kmalloc(): return > ZERO_SIZE_PTR when requested size is 0. Update devm_kfree() to handle > this case. > > Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com> > --- > drivers/base/devres.c | 9 ++++++--- > 1 file changed, 6 insertions(+), 3 deletions(-) > > diff --git a/drivers/base/devres.c b/drivers/base/devres.c > index 1df1fb10b2d9..ed615d3b9cf1 100644 > --- a/drivers/base/devres.c > +++ b/drivers/base/devres.c > @@ -819,6 +819,9 @@ void *devm_kmalloc(struct device *dev, size_t size, gfp_t gfp) > { > struct devres *dr; > > + if (unlikely(!size)) > + return ZERO_SIZE_PTR; > + > /* use raw alloc_dr for kmalloc caller tracing */ > dr = alloc_dr(devm_kmalloc_release, size, gfp, dev_to_node(dev)); > if (unlikely(!dr)) > @@ -950,10 +953,10 @@ void devm_kfree(struct device *dev, const void *p) > int rc; > > /* > - * Special case: pointer to a string in .rodata returned by > - * devm_kstrdup_const(). > + * Special cases: pointer to a string in .rodata returned by > + * devm_kstrdup_const() or NULL/ZERO ptr. > */ > - if (unlikely(is_kernel_rodata((unsigned long)p))) > + if (unlikely(is_kernel_rodata((unsigned long)p) || ZERO_OR_NULL_PTR(p))) > return; > > rc = devres_destroy(dev, devm_kmalloc_release, This change caught a bug in one of our Tegra drivers, which I am in the process of fixing. Once I bisected to this commit it was easy to track down, but I am wondering if there is any reason why we don't add a WARN_ON() if size is 0 in devm_kmalloc? It was essentially what I ended up doing to find the bug. Jon
On Fri, Jul 10, 2020 at 3:46 PM Jon Hunter <jonathanh@nvidia.com> wrote: > > Hi Bartosz, > > On 29/06/2020 07:50, Bartosz Golaszewski wrote: > > From: Bartosz Golaszewski <bgolaszewski@baylibre.com> > > > > Make devm_kmalloc() behave similarly to non-managed kmalloc(): return > > ZERO_SIZE_PTR when requested size is 0. Update devm_kfree() to handle > > this case. > > > > Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com> > > --- > > drivers/base/devres.c | 9 ++++++--- > > 1 file changed, 6 insertions(+), 3 deletions(-) > > > > diff --git a/drivers/base/devres.c b/drivers/base/devres.c > > index 1df1fb10b2d9..ed615d3b9cf1 100644 > > --- a/drivers/base/devres.c > > +++ b/drivers/base/devres.c > > @@ -819,6 +819,9 @@ void *devm_kmalloc(struct device *dev, size_t size, gfp_t gfp) > > { > > struct devres *dr; > > > > + if (unlikely(!size)) > > + return ZERO_SIZE_PTR; > > + > > /* use raw alloc_dr for kmalloc caller tracing */ > > dr = alloc_dr(devm_kmalloc_release, size, gfp, dev_to_node(dev)); > > if (unlikely(!dr)) > > @@ -950,10 +953,10 @@ void devm_kfree(struct device *dev, const void *p) > > int rc; > > > > /* > > - * Special case: pointer to a string in .rodata returned by > > - * devm_kstrdup_const(). > > + * Special cases: pointer to a string in .rodata returned by > > + * devm_kstrdup_const() or NULL/ZERO ptr. > > */ > > - if (unlikely(is_kernel_rodata((unsigned long)p))) > > + if (unlikely(is_kernel_rodata((unsigned long)p) || ZERO_OR_NULL_PTR(p))) > > return; > > > > rc = devres_destroy(dev, devm_kmalloc_release, > > > This change caught a bug in one of our Tegra drivers, which I am in the > process of fixing. Once I bisected to this commit it was easy to track > down, but I am wondering if there is any reason why we don't add a > WARN_ON() if size is 0 in devm_kmalloc? It was essentially what I ended > up doing to find the bug. > > Jon > > -- > nvpublic Hi Jon, this is in line with what the regular kmalloc() does. If size is zero, it returns ZERO_SIZE_PTR. It's not an error condition. Actually in user-space malloc() does a similar thing: for size == 0 it allocates one-byte and returns a pointer to it (at least in glibc). Bartosz
On 10/07/2020 17:03, Bartosz Golaszewski wrote: > On Fri, Jul 10, 2020 at 3:46 PM Jon Hunter <jonathanh@nvidia.com> wrote: >> >> Hi Bartosz, >> >> On 29/06/2020 07:50, Bartosz Golaszewski wrote: >>> From: Bartosz Golaszewski <bgolaszewski@baylibre.com> >>> >>> Make devm_kmalloc() behave similarly to non-managed kmalloc(): return >>> ZERO_SIZE_PTR when requested size is 0. Update devm_kfree() to handle >>> this case. >>> >>> Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com> >>> --- >>> drivers/base/devres.c | 9 ++++++--- >>> 1 file changed, 6 insertions(+), 3 deletions(-) >>> >>> diff --git a/drivers/base/devres.c b/drivers/base/devres.c >>> index 1df1fb10b2d9..ed615d3b9cf1 100644 >>> --- a/drivers/base/devres.c >>> +++ b/drivers/base/devres.c >>> @@ -819,6 +819,9 @@ void *devm_kmalloc(struct device *dev, size_t size, gfp_t gfp) >>> { >>> struct devres *dr; >>> >>> + if (unlikely(!size)) >>> + return ZERO_SIZE_PTR; >>> + >>> /* use raw alloc_dr for kmalloc caller tracing */ >>> dr = alloc_dr(devm_kmalloc_release, size, gfp, dev_to_node(dev)); >>> if (unlikely(!dr)) >>> @@ -950,10 +953,10 @@ void devm_kfree(struct device *dev, const void *p) >>> int rc; >>> >>> /* >>> - * Special case: pointer to a string in .rodata returned by >>> - * devm_kstrdup_const(). >>> + * Special cases: pointer to a string in .rodata returned by >>> + * devm_kstrdup_const() or NULL/ZERO ptr. >>> */ >>> - if (unlikely(is_kernel_rodata((unsigned long)p))) >>> + if (unlikely(is_kernel_rodata((unsigned long)p) || ZERO_OR_NULL_PTR(p))) >>> return; >>> >>> rc = devres_destroy(dev, devm_kmalloc_release, >> >> >> This change caught a bug in one of our Tegra drivers, which I am in the >> process of fixing. Once I bisected to this commit it was easy to track >> down, but I am wondering if there is any reason why we don't add a >> WARN_ON() if size is 0 in devm_kmalloc? It was essentially what I ended >> up doing to find the bug. >> >> Jon >> >> -- >> nvpublic > > Hi Jon, > > this is in line with what the regular kmalloc() does. If size is zero, > it returns ZERO_SIZE_PTR. It's not an error condition. Actually in > user-space malloc() does a similar thing: for size == 0 it allocates > one-byte and returns a pointer to it (at least in glibc). Yes that's fine, I was just wondering if there is any reason not to WARN as well? Cheers Jon
On Fri, Jul 10, 2020 at 6:11 PM Jon Hunter <jonathanh@nvidia.com> wrote: > > > On 10/07/2020 17:03, Bartosz Golaszewski wrote: > > On Fri, Jul 10, 2020 at 3:46 PM Jon Hunter <jonathanh@nvidia.com> wrote: > >> > >> Hi Bartosz, > >> > >> On 29/06/2020 07:50, Bartosz Golaszewski wrote: > >>> From: Bartosz Golaszewski <bgolaszewski@baylibre.com> > >>> > >>> Make devm_kmalloc() behave similarly to non-managed kmalloc(): return > >>> ZERO_SIZE_PTR when requested size is 0. Update devm_kfree() to handle > >>> this case. > >>> > >>> Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com> > >>> --- > >>> drivers/base/devres.c | 9 ++++++--- > >>> 1 file changed, 6 insertions(+), 3 deletions(-) > >>> > >>> diff --git a/drivers/base/devres.c b/drivers/base/devres.c > >>> index 1df1fb10b2d9..ed615d3b9cf1 100644 > >>> --- a/drivers/base/devres.c > >>> +++ b/drivers/base/devres.c > >>> @@ -819,6 +819,9 @@ void *devm_kmalloc(struct device *dev, size_t size, gfp_t gfp) > >>> { > >>> struct devres *dr; > >>> > >>> + if (unlikely(!size)) > >>> + return ZERO_SIZE_PTR; > >>> + > >>> /* use raw alloc_dr for kmalloc caller tracing */ > >>> dr = alloc_dr(devm_kmalloc_release, size, gfp, dev_to_node(dev)); > >>> if (unlikely(!dr)) > >>> @@ -950,10 +953,10 @@ void devm_kfree(struct device *dev, const void *p) > >>> int rc; > >>> > >>> /* > >>> - * Special case: pointer to a string in .rodata returned by > >>> - * devm_kstrdup_const(). > >>> + * Special cases: pointer to a string in .rodata returned by > >>> + * devm_kstrdup_const() or NULL/ZERO ptr. > >>> */ > >>> - if (unlikely(is_kernel_rodata((unsigned long)p))) > >>> + if (unlikely(is_kernel_rodata((unsigned long)p) || ZERO_OR_NULL_PTR(p))) > >>> return; > >>> > >>> rc = devres_destroy(dev, devm_kmalloc_release, > >> > >> > >> This change caught a bug in one of our Tegra drivers, which I am in the > >> process of fixing. Once I bisected to this commit it was easy to track > >> down, but I am wondering if there is any reason why we don't add a > >> WARN_ON() if size is 0 in devm_kmalloc? It was essentially what I ended > >> up doing to find the bug. > >> > >> Jon > >> > >> -- > >> nvpublic > > > > Hi Jon, > > > > this is in line with what the regular kmalloc() does. If size is zero, > > it returns ZERO_SIZE_PTR. It's not an error condition. Actually in > > user-space malloc() does a similar thing: for size == 0 it allocates > > one-byte and returns a pointer to it (at least in glibc). > > > Yes that's fine, I was just wondering if there is any reason not to WARN > as well? > > Cheers > Jon > Why? Nothing bad happens. Regular kmalloc() doesn't warn, why should devm_kmalloc() do? Bartosz
On 10/07/2020 17:24, Bartosz Golaszewski wrote: > On Fri, Jul 10, 2020 at 6:11 PM Jon Hunter <jonathanh@nvidia.com> wrote: >> >> >> On 10/07/2020 17:03, Bartosz Golaszewski wrote: >>> On Fri, Jul 10, 2020 at 3:46 PM Jon Hunter <jonathanh@nvidia.com> wrote: >>>> >>>> Hi Bartosz, >>>> >>>> On 29/06/2020 07:50, Bartosz Golaszewski wrote: >>>>> From: Bartosz Golaszewski <bgolaszewski@baylibre.com> >>>>> >>>>> Make devm_kmalloc() behave similarly to non-managed kmalloc(): return >>>>> ZERO_SIZE_PTR when requested size is 0. Update devm_kfree() to handle >>>>> this case. >>>>> >>>>> Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com> >>>>> --- >>>>> drivers/base/devres.c | 9 ++++++--- >>>>> 1 file changed, 6 insertions(+), 3 deletions(-) >>>>> >>>>> diff --git a/drivers/base/devres.c b/drivers/base/devres.c >>>>> index 1df1fb10b2d9..ed615d3b9cf1 100644 >>>>> --- a/drivers/base/devres.c >>>>> +++ b/drivers/base/devres.c >>>>> @@ -819,6 +819,9 @@ void *devm_kmalloc(struct device *dev, size_t size, gfp_t gfp) >>>>> { >>>>> struct devres *dr; >>>>> >>>>> + if (unlikely(!size)) >>>>> + return ZERO_SIZE_PTR; >>>>> + >>>>> /* use raw alloc_dr for kmalloc caller tracing */ >>>>> dr = alloc_dr(devm_kmalloc_release, size, gfp, dev_to_node(dev)); >>>>> if (unlikely(!dr)) >>>>> @@ -950,10 +953,10 @@ void devm_kfree(struct device *dev, const void *p) >>>>> int rc; >>>>> >>>>> /* >>>>> - * Special case: pointer to a string in .rodata returned by >>>>> - * devm_kstrdup_const(). >>>>> + * Special cases: pointer to a string in .rodata returned by >>>>> + * devm_kstrdup_const() or NULL/ZERO ptr. >>>>> */ >>>>> - if (unlikely(is_kernel_rodata((unsigned long)p))) >>>>> + if (unlikely(is_kernel_rodata((unsigned long)p) || ZERO_OR_NULL_PTR(p))) >>>>> return; >>>>> >>>>> rc = devres_destroy(dev, devm_kmalloc_release, >>>> >>>> >>>> This change caught a bug in one of our Tegra drivers, which I am in the >>>> process of fixing. Once I bisected to this commit it was easy to track >>>> down, but I am wondering if there is any reason why we don't add a >>>> WARN_ON() if size is 0 in devm_kmalloc? It was essentially what I ended >>>> up doing to find the bug. >>>> >>>> Jon >>>> >>>> -- >>>> nvpublic >>> >>> Hi Jon, >>> >>> this is in line with what the regular kmalloc() does. If size is zero, >>> it returns ZERO_SIZE_PTR. It's not an error condition. Actually in >>> user-space malloc() does a similar thing: for size == 0 it allocates >>> one-byte and returns a pointer to it (at least in glibc). >> >> >> Yes that's fine, I was just wondering if there is any reason not to WARN >> as well? >> >> Cheers >> Jon >> > > Why? Nothing bad happens. Regular kmalloc() doesn't warn, why should > devm_kmalloc() do? Simply because it is easier to track down a bug. In my case the NULL pointer crash did not occur until entering suspend when the memory, that was allocated at probe time, was first actually accessed. So it was not immediately obvious which call to devm_kmalloc caused the problem. Anyway, if kmalloc does not warn either, then fine, it was purely a question. Jon
Hi Bartosz, On Mon, Jun 29, 2020 at 1:56 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote: > > From: Bartosz Golaszewski <bgolaszewski@baylibre.com> > > Make devm_kmalloc() behave similarly to non-managed kmalloc(): return > ZERO_SIZE_PTR when requested size is 0. Update devm_kfree() to handle > this case. This is wrong if you consider devm_krealloc API that you added. The premise of devm_krealloc() is that it does not disturb devres "stack", however in this case there is no entry in the stack. Consider: ptr = devm_kzalloc(dev, 0, GFP_KERNEL); ... more devm API calls ... /* This allocation will be on top of devm stack, not bottom ! */ ptr = devm_krealloc(dev, ptr, 16, GFP_KERNEL); And also: ptr = devm_kzalloc(dev, 16, GFP_KERNEL); ... more devm API calls ... /* Here we lose out position */ ptr = devm_krealloc(dev, ptr, 0, GFP_KERNEL); ... /* and now our memory allocation will be released first */ ptr = devm_krealloc(dev, ptr, 16, GFP_KERNEL); IMO special-casing 0-size allocations for managed memory allocations should not be done. Thanks.
On Sun, Apr 11, 2021 at 5:21 AM Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote: > > Hi Bartosz, > > On Mon, Jun 29, 2020 at 1:56 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote: > > > > From: Bartosz Golaszewski <bgolaszewski@baylibre.com> > > > > Make devm_kmalloc() behave similarly to non-managed kmalloc(): return > > ZERO_SIZE_PTR when requested size is 0. Update devm_kfree() to handle > > this case. > > This is wrong if you consider devm_krealloc API that you added. The > premise of devm_krealloc() is that it does not disturb devres "stack", > however in this case there is no entry in the stack. Consider: > > ptr = devm_kzalloc(dev, 0, GFP_KERNEL); > ... > more devm API calls > ... > > /* This allocation will be on top of devm stack, not bottom ! */ > ptr = devm_krealloc(dev, ptr, 16, GFP_KERNEL); > > And also: > > ptr = devm_kzalloc(dev, 16, GFP_KERNEL); > ... > more devm API calls > ... > /* Here we lose out position */ > ptr = devm_krealloc(dev, ptr, 0, GFP_KERNEL); > ... > /* and now our memory allocation will be released first */ > ptr = devm_krealloc(dev, ptr, 16, GFP_KERNEL); > > > IMO special-casing 0-size allocations for managed memory allocations > should not be done. > > Thanks. > > -- > Dmitry You're right about the ordering being lost. At the same time allocating 0 bytes is quite a special case and should result in returning ZERO_SIZE_PTR as the fault dump resulting from its dereference will indicate what the bug is. I need to give it a thought because I'm not yet sure what the right solution would be. Let me get back to you. Bartosz
diff --git a/drivers/base/devres.c b/drivers/base/devres.c index 1df1fb10b2d9..ed615d3b9cf1 100644 --- a/drivers/base/devres.c +++ b/drivers/base/devres.c @@ -819,6 +819,9 @@ void *devm_kmalloc(struct device *dev, size_t size, gfp_t gfp) { struct devres *dr; + if (unlikely(!size)) + return ZERO_SIZE_PTR; + /* use raw alloc_dr for kmalloc caller tracing */ dr = alloc_dr(devm_kmalloc_release, size, gfp, dev_to_node(dev)); if (unlikely(!dr)) @@ -950,10 +953,10 @@ void devm_kfree(struct device *dev, const void *p) int rc; /* - * Special case: pointer to a string in .rodata returned by - * devm_kstrdup_const(). + * Special cases: pointer to a string in .rodata returned by + * devm_kstrdup_const() or NULL/ZERO ptr. */ - if (unlikely(is_kernel_rodata((unsigned long)p))) + if (unlikely(is_kernel_rodata((unsigned long)p) || ZERO_OR_NULL_PTR(p))) return; rc = devres_destroy(dev, devm_kmalloc_release,