| Message ID | 20200802083458.24323-2-brgl@bgdev.pl (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | devres: provide and use devm_krealloc() | expand |
On Sun, Aug 2, 2020 at 11:37 AM Bartosz Golaszewski <brgl@bgdev.pl> wrote: > > From: Bartosz Golaszewski <bgolaszewski@baylibre.com> > > Implement the managed variant of krealloc(). This function works with > all memory allocated by devm_kmalloc() (or devres functions using it > implicitly like devm_kmemdup(), devm_kstrdup() etc.). > > Managed realloc'ed chunks can be manually released with devm_kfree(). Some thoughts below. You may ignore nit-picks, of course :-) ... > + * Managed krealloc(). Resizes the memory chunk allocated with devm_kmalloc(). > + * Behaves similarly to regular krealloc(): if @ptr is NULL or ZERO_SIZE_PTR, > + * it's the equivalent of devm_kmalloc(). If new_size is zero, it frees the equivalent for > + * previously allocated memory and returns ZERO_SIZE_PTR. This function doesn't > + * change the order in which the release callback for the re-alloc'ed devres > + * will be called (except when falling back to devm_kmalloc() or when freeing > + * resources when new_size is zero). The contents of the memory are preserved > + * up to the lesser of new and old sizes. Might deserve to say about pointers to RO, but see below. ... > + if (WARN_ON(is_kernel_rodata((unsigned long)ptr))) > + /* > + * We cannot reliably realloc a const string returned by > + * devm_kstrdup_const(). > + */ > + return NULL; I was thinking about this bit... Shouldn't we rather issue a simple dev_warn() and return the existing pointer? For example in some cases we might want to have resources coming either from heap or from constant. Then, if at some circumstances we would like to extend that memory (only for non-constant cases) we would need to manage this ourselves. Otherwise we may simply call krealloc(). It seems that devm_kstrdup_const returns an initial pointer. Getting NULL is kinda inconvenient (and actually dev_warn() might also be quite a noise, however I would give a message to the user, because it's something worth checking). ... > + spin_lock_irqsave(&dev->devres_lock, flags); > + old_dr = find_dr(dev, devm_kmalloc_release, devm_kmalloc_match, ptr); > + spin_unlock_irqrestore(&dev->devres_lock, flags); > + if (!old_dr) { I would have this under spin lock b/c of below. > + WARN(1, "Memory chunk not managed or managed by a different device."); > + return NULL; > + } > + old_head = old_dr->node.entry; This would be still better to be under spin lock. > + new_dr = krealloc(old_dr, total_size, gfp); > + if (!new_dr) > + return NULL; And perhaps spin lock taken already here. > + if (new_dr != old_dr) { > + spin_lock_irqsave(&dev->devres_lock, flags); > + list_replace(&old_head, &new_dr->node.entry); > + spin_unlock_irqrestore(&dev->devres_lock, flags); > + } Yes, I understand that covering more code under spin lock does not fix any potential race, but at least it minimizes scope of the code that is not under it to see exactly what is problematic. I probably will think more about a better approach to avoid potential races.
On Sun, Aug 2, 2020 at 12:42 PM Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > [snip] > > I was thinking about this bit... Shouldn't we rather issue a simple > dev_warn() and return the existing pointer? > For example in some cases we might want to have resources coming > either from heap or from constant. Then, if at some circumstances we > would like to extend that memory (only for non-constant cases) we > would need to manage this ourselves. Otherwise we may simply call > krealloc(). > It seems that devm_kstrdup_const returns an initial pointer. Getting > NULL is kinda inconvenient (and actually dev_warn() might also be > quite a noise, however I would give a message to the user, because > it's something worth checking). > But this is inconsistent behavior: if you pass a pointer to ro memory to devm_krealloc() it will not resize it but by returning a valid pointer it will make you think it did -> you end up writing to ro memory in good faith. > ... > > > + spin_lock_irqsave(&dev->devres_lock, flags); > > + old_dr = find_dr(dev, devm_kmalloc_release, devm_kmalloc_match, ptr); > > + spin_unlock_irqrestore(&dev->devres_lock, flags); > > > + if (!old_dr) { > > I would have this under spin lock b/c of below. > > > + WARN(1, "Memory chunk not managed or managed by a different device."); > > + return NULL; > > + } > > > + old_head = old_dr->node.entry; > > This would be still better to be under spin lock. > > > + new_dr = krealloc(old_dr, total_size, gfp); > > + if (!new_dr) > > + return NULL; > > And perhaps spin lock taken already here. > > > + if (new_dr != old_dr) { > > + spin_lock_irqsave(&dev->devres_lock, flags); > > + list_replace(&old_head, &new_dr->node.entry); > > + spin_unlock_irqrestore(&dev->devres_lock, flags); > > + } > > Yes, I understand that covering more code under spin lock does not fix > any potential race, but at least it minimizes scope of the code that > is not under it to see exactly what is problematic. > > I probably will think more about a better approach to avoid potential races. My thinking behind this was this: we already have users who call devres_find() and do something with the retrieved resources without holding the devres_lock - so it's assumed that users are sane enough to not be getting in each other's way. Now I see that the difference is that here we're accessing the list node and it can change if another thread is adding a different devres to the same device. So this should definitely be protected somehow. I think that we may have to give up using real krealloc() and instead just reimplement its behavior in the following way: Still outside of spinlock check if the new total size is smaller or equal to the previous one. If so: return the same pointer. If not: allocate a new devres as if it were for devm_kmalloc() but don't add it to the list yet. Take the spinlock - check if we can find the devres - if not: kfree() the new and old chunk and return NULL. If yes: copy the contents of the devres node into the new chunk as well as the memory contents. Replace the old one on the list and free it. Release spinlock and return. Does that work? Bart
diff --git a/Documentation/driver-api/driver-model/devres.rst b/Documentation/driver-api/driver-model/devres.rst index eaaaafc21134..f318a5c0033c 100644 --- a/Documentation/driver-api/driver-model/devres.rst +++ b/Documentation/driver-api/driver-model/devres.rst @@ -354,6 +354,7 @@ MEM devm_kmalloc() devm_kmalloc_array() devm_kmemdup() + devm_krealloc() devm_kstrdup() devm_kvasprintf() devm_kzalloc() diff --git a/drivers/base/devres.c b/drivers/base/devres.c index ed615d3b9cf1..68a5a7201065 100644 --- a/drivers/base/devres.c +++ b/drivers/base/devres.c @@ -837,6 +837,71 @@ void *devm_kmalloc(struct device *dev, size_t size, gfp_t gfp) } EXPORT_SYMBOL_GPL(devm_kmalloc); +/** + * devm_krealloc - Resource-managed krealloc() + * @dev: Device to re-allocate memory for + * @ptr: Pointer to the memory chunk to re-allocate + * @new_size: New allocation size + * @gfp: Allocation gfp flags + * + * Managed krealloc(). Resizes the memory chunk allocated with devm_kmalloc(). + * Behaves similarly to regular krealloc(): if @ptr is NULL or ZERO_SIZE_PTR, + * it's the equivalent of devm_kmalloc(). If new_size is zero, it frees the + * previously allocated memory and returns ZERO_SIZE_PTR. This function doesn't + * change the order in which the release callback for the re-alloc'ed devres + * will be called (except when falling back to devm_kmalloc() or when freeing + * resources when new_size is zero). The contents of the memory are preserved + * up to the lesser of new and old sizes. + */ +void *devm_krealloc(struct device *dev, void *ptr, size_t new_size, gfp_t gfp) +{ + struct devres *old_dr, *new_dr; + struct list_head old_head; + unsigned long flags; + size_t total_size; + + if (unlikely(!new_size)) { + devm_kfree(dev, ptr); + return ZERO_SIZE_PTR; + } + + if (unlikely(ZERO_OR_NULL_PTR(ptr))) + return devm_kmalloc(dev, new_size, gfp); + + if (WARN_ON(is_kernel_rodata((unsigned long)ptr))) + /* + * We cannot reliably realloc a const string returned by + * devm_kstrdup_const(). + */ + return NULL; + + if (!check_dr_size(new_size, &total_size)) + return NULL; + + spin_lock_irqsave(&dev->devres_lock, flags); + old_dr = find_dr(dev, devm_kmalloc_release, devm_kmalloc_match, ptr); + spin_unlock_irqrestore(&dev->devres_lock, flags); + if (!old_dr) { + WARN(1, "Memory chunk not managed or managed by a different device."); + return NULL; + } + + old_head = old_dr->node.entry; + + new_dr = krealloc(old_dr, total_size, gfp); + if (!new_dr) + return NULL; + + if (new_dr != old_dr) { + spin_lock_irqsave(&dev->devres_lock, flags); + list_replace(&old_head, &new_dr->node.entry); + spin_unlock_irqrestore(&dev->devres_lock, flags); + } + + return new_dr->data; +} +EXPORT_SYMBOL_GPL(devm_krealloc); + /** * devm_kstrdup - Allocate resource managed space and * copy an existing string into that. diff --git a/include/linux/device.h b/include/linux/device.h index 79ce404619e6..51fb33400d7a 100644 --- a/include/linux/device.h +++ b/include/linux/device.h @@ -206,6 +206,8 @@ int devres_release_group(struct device *dev, void *id); /* managed devm_k.alloc/kfree for device drivers */ void *devm_kmalloc(struct device *dev, size_t size, gfp_t gfp) __malloc; +void *devm_krealloc(struct device *dev, void *ptr, size_t size, + gfp_t gfp) __must_check; __printf(3, 0) char *devm_kvasprintf(struct device *dev, gfp_t gfp, const char *fmt, va_list ap) __malloc; __printf(3, 4) char *devm_kasprintf(struct device *dev, gfp_t gfp,