mm: util: update the kerneldoc for kstrdup_const()

PATCH Superseded Bartosz Golaszewski 3 participants 10 messages Jun 28, 2020 – Jun 30, 2020

Subsystems (3)

Memory management MM core Slab allocator

lore

Share & tools

lore

Bartosz Golaszewski

author

2020-06-28 15:25 UTC #0 lore raw

PATCH 1/1 mm: util: update the kerneldoc for kstrdup_const()

From: Bartosz Golaszewski <[email protected]>

Memory allocated with kstrdup_const() must not be passed to regular krealloc() as it is not aware of the possibility of the chunk residing in .rodata. Since there are no potential users of krealloc_const() at the moment, let's just update the doc to make it explicit.

Signed-off-by Bartosz Golaszewski <[email protected]>

---
 mm/util.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

mm/util.c +2 -1

index c63c8e47be57..27d6155edb8f 100644
@@ -69,7 +69,8 @@ EXPORT_SYMBOL(kstrdup);
  * @s: the string to duplicate
  * @gfp: the GFP mask used in the kmalloc() call when allocating memory
  *
- * Note: Strings allocated by kstrdup_const should be freed by kfree_const.
+ * Note: Strings allocated by kstrdup_const should be freed by kfree_const and
+ * must not be passed to krealloc().
  *
  * Return: source string if it is in .rodata section otherwise
  * fallback to kstrdup.

Signature · 1 line

2.26.1

Joe Perches

2020-06-28 17:37 UTC #1 lore raw

Quoted 6 lines · Bartosz Golaszewski

From: Bartosz Golaszewski <[email protected]>
Memory allocated with kstrdup_const() must not be passed to regular krealloc() as it is not aware of the possibility of the chunk residing in .rodata. Since there are no potential users of krealloc_const() at the moment, let's just update the doc to make it explicit.

Another option would be to return NULL if it's
used from krealloc with a pointer into rodata

---
 mm/slab_common.c | 6 ++++++
 1 file changed, 6 insertions(+)

mm/slab_common.c +6 -0

index 37d48a56431d..f8b49656171b 100644
@@ -1683,6 +1683,9 @@ static __always_inline void *__do_krealloc(const void *p, size_t new_size,
  * @new_size: how many bytes of memory are required.
  * @flags: the type of memory to allocate.
  *
+ * If the object pointed to is in rodata (likely from kstrdup_const)
+ * %NULL is returned.
+ *
  * The contents of the object pointed to are preserved up to the
  * lesser of the new and old sizes.  If @p is %NULL, krealloc()
  * behaves exactly like kmalloc().  If @new_size is 0 and @p is not a

@@ -1694,6 +1697,9 @@ void *krealloc(const void *p, size_t new_size, gfp_t flags)
 {
 	void *ret;
 
+	if (unlikely(is_kernel_rodata((unsigned long)p)))
+		return NULL;
+
 	if (unlikely(!new_size)) {
 		kfree(p);
 		return ZERO_SIZE_PTR;

Bartosz Golaszewski

author

2020-06-28 18:06 UTC #2 lore raw

Quoted 33 lines · Joe Perches

Quoted 6 lines · Bartosz Golaszewski
From: Bartosz Golaszewski <[email protected]>
Memory allocated with kstrdup_const() must not be passed to regular krealloc() as it is not aware of the possibility of the chunk residing in .rodata. Since there are no potential users of krealloc_const() at the moment, let's just update the doc to make it explicit.
Another option would be to return NULL if it's
used from krealloc with a pointer into rodata
---
 mm/slab_common.c | 6 ++++++
 1 file changed, 6 insertions(+)
mm/slab_common.c +3 -0
index 37d48a56431d..f8b49656171b 100644
@@ -1683,6 +1683,9 @@ static __always_inline void *__do_krealloc(const void *p, size_t new_size,
  * @new_size: how many bytes of memory are required.
  * @flags: the type of memory to allocate.
  *
+ * If the object pointed to is in rodata (likely from kstrdup_const)
+ * %NULL is returned.
+ *
  * The contents of the object pointed to are preserved up to the
  * lesser of the new and old sizes.  If @p is %NULL, krealloc()
  * behaves exactly like kmalloc().  If @new_size is 0 and @p is not a

mm/slab_common.c void *krealloc(const void *p, size_t new_size, gfp_t flags) · line 1705

Quoted 9 lines

mm/slab_common.c +3 -0

@@ -1694,6 +1697,9 @@ void *krealloc(const void *p, size_t new_size, gfp_t flags)
 {
        void *ret;
 
+       if (unlikely(is_kernel_rodata((unsigned long)p)))
+               return NULL;
+
        if (unlikely(!new_size)) {
                kfree(p);
                return ZERO_SIZE_PTR;

In that case we should probably add a WARN_ON() - otherwise the user will be baffled by krealloc() failing.

Bartosz

Joe Perches

2020-06-28 18:20 UTC #3 lore raw

Quoted 33 lines · Bartosz Golaszewski

Quoted 32 lines · Joe Perches
Quoted 6 lines · Bartosz Golaszewski
From: Bartosz Golaszewski <[email protected]>
Memory allocated with kstrdup_const() must not be passed to regular krealloc() as it is not aware of the possibility of the chunk residing in .rodata. Since there are no potential users of krealloc_const() at the moment, let's just update the doc to make it explicit.
Another option would be to return NULL if it's
used from krealloc with a pointer into rodata
---
 mm/slab_common.c | 6 ++++++
 1 file changed, 6 insertions(+)
mm/slab_common.c +3 -0
index 37d48a56431d..f8b49656171b 100644
@@ -1683,6 +1683,9 @@ static __always_inline void *__do_krealloc(const void *p, size_t new_size,
  * @new_size: how many bytes of memory are required.
  * @flags: the type of memory to allocate.
  *
+ * If the object pointed to is in rodata (likely from kstrdup_const)
+ * %NULL is returned.
+ *
  * The contents of the object pointed to are preserved up to the
  * lesser of the new and old sizes.  If @p is %NULL, krealloc()
  * behaves exactly like kmalloc().  If @new_size is 0 and @p is not a
@@ -1694,6 +1697,9 @@ void *krealloc(const void *p, size_t new_size, gfp_t flags)
 {
        void *ret;
 

Quoted 12 lines

mm/slab_common.c void *krealloc(const void *p, size_t new_size, gfp_t flags) · line 1705
Quoted 9 lines
mm/slab_common.c +3 -0
+       if (unlikely(is_kernel_rodata((unsigned long)p)))
+               return NULL;
+
        if (unlikely(!new_size)) {
                kfree(p);
                return ZERO_SIZE_PTR;
In that case we should probably add a WARN_ON() - otherwise the user will be baffled by krealloc() failing.

Maybe:

---
 mm/slab_common.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

mm/slab_common.c +11 -0

index a143a8c8f874..06c2714ab8c9 100644
@@ -1073,6 +1073,9 @@ static __always_inline void *__do_krealloc(const void *p, size_t new_size,
  * @new_size: how many bytes of memory are required.
  * @flags: the type of memory to allocate.
  *
+ * If the object pointed to is in rodata (likely from kstrdup_const)
+ * %NULL is returned.
+ *
  * The contents of the object pointed to are preserved up to the
  * lesser of the new and old sizes.  If @p is %NULL, krealloc()
  * behaves exactly like kmalloc().  If @new_size is 0 and @p is not a

@@ -1084,6 +1087,14 @@ void *krealloc(const void *p, size_t new_size, gfp_t flags)
 {
 	void *ret;
 
+	if (unlikely(is_kernel_rodata((unsigned long)p))) {
+		if (!(flags & __GFP_NOWARN)) {
+			printk(KERN_DEFAULT "invalid use of krealloc with const rodata\n");
+			dump_stack();
+		}
+		return NULL;
+	}
+
 	if (unlikely(!new_size)) {
 		kfree(p);
 		return ZERO_SIZE_PTR;

David Hildenbrand

2020-06-29 10:55 UTC #4 lore raw

In reply to Joe Perches

Quoted 30 lines · Joe Perches

Quoted 6 lines · Bartosz Golaszewski
From: Bartosz Golaszewski <[email protected]>
Memory allocated with kstrdup_const() must not be passed to regular krealloc() as it is not aware of the possibility of the chunk residing in .rodata. Since there are no potential users of krealloc_const() at the moment, let's just update the doc to make it explicit.
Another option would be to return NULL if it's
used from krealloc with a pointer into rodata
---
 mm/slab_common.c | 6 ++++++
 1 file changed, 6 insertions(+)
mm/slab_common.c +3 -0
index 37d48a56431d..f8b49656171b 100644
@@ -1683,6 +1683,9 @@ static __always_inline void *__do_krealloc(const void *p, size_t new_size,
  * @new_size: how many bytes of memory are required.
  * @flags: the type of memory to allocate.
  *
+ * If the object pointed to is in rodata (likely from kstrdup_const)
+ * %NULL is returned.
+ *
  * The contents of the object pointed to are preserved up to the
  * lesser of the new and old sizes.  If @p is %NULL, krealloc()
  * behaves exactly like kmalloc().  If @new_size is 0 and @p is not a

mm/slab_common.c void *krealloc(const void *p, size_t new_size, gfp_t flags) · line 1705

Quoted 9 lines

mm/slab_common.c +3 -0

@@ -1694,6 +1697,9 @@ void *krealloc(const void *p, size_t new_size, gfp_t flags)
 {
 	void *ret;
 
+	if (unlikely(is_kernel_rodata((unsigned long)p)))
+		return NULL;
+
 	if (unlikely(!new_size)) {
 		kfree(p);
 		return ZERO_SIZE_PTR;

Won't we have similar issues if somebody would do a kfree() instead of a kfree_const()? So I think the original patch makes sense.

Signature · 3 lines

Thanks,

David / dhildenb

Joe Perches

2020-06-29 19:21 UTC #5 lore raw

Quoted 11 lines · David Hildenbrand

Quoted 10 lines · Joe Perches
Quoted 6 lines · Bartosz Golaszewski
From: Bartosz Golaszewski <[email protected]>
Memory allocated with kstrdup_const() must not be passed to regular krealloc() as it is not aware of the possibility of the chunk residing in .rodata. Since there are no potential users of krealloc_const() at the moment, let's just update the doc to make it explicit.
Another option would be to return NULL if it's
used from krealloc with a pointer into rodata

[]

Quoted 1 line

mm/slab_common.c
Quoted 1 line
mm/slab_common.c +0 -0

[]

Quoted 7 lines

mm/slab_common.c static __always_inline void *__do_krealloc(const void *p, size_t new_size, · line 1688

Quoted 7 lines

mm/slab_common.c +3 -0

@@ -1683,6 +1683,9 @@ static __always_inline void *__do_krealloc(const void *p, size_t new_size,
  * @new_size: how many bytes of memory are required.
  * @flags: the type of memory to allocate.
  *
+ * If the object pointed to is in rodata (likely from kstrdup_const)
+ * %NULL is returned.
+ *

[]

Quoted 2 lines

Won't we have similar issues if somebody would do a kfree() instead of a kfree_const()? So I think the original patch makes sense.

Which is why I also suggested making kfree work for more types of memory freeing earlier this month.

https://lore.kernel.org/lkml/[email protected]/

David Hildenbrand

2020-06-30 08:57 UTC #6 lore raw

Quoted 23 lines · Joe Perches

Quoted 11 lines · David Hildenbrand
Quoted 10 lines · Joe Perches
Quoted 6 lines · Bartosz Golaszewski
From: Bartosz Golaszewski <[email protected]>
Memory allocated with kstrdup_const() must not be passed to regular krealloc() as it is not aware of the possibility of the chunk residing in .rodata. Since there are no potential users of krealloc_const() at the moment, let's just update the doc to make it explicit.
Another option would be to return NULL if it's
used from krealloc with a pointer into rodata
[]
Quoted 1 line
mm/slab_common.c
Quoted 1 line
mm/slab_common.c +0 -0
[]
Quoted 7 lines
mm/slab_common.c static __always_inline void *__do_krealloc(const void *p, size_t new_size, · line 1688
Quoted 7 lines
mm/slab_common.c +3 -0
@@ -1683,6 +1683,9 @@ static __always_inline void *__do_krealloc(const void *p, size_t new_size,
  * @new_size: how many bytes of memory are required.
  * @flags: the type of memory to allocate.
  *
+ * If the object pointed to is in rodata (likely from kstrdup_const)
+ * %NULL is returned.
+ *
[]

Quoted 7 lines

Quoted 2 lines
Won't we have similar issues if somebody would do a kfree() instead of a kfree_const()? So I think the original patch makes sense.
Which is why I also suggested making kfree work for more types of memory freeing earlier this month.
https://lore.kernel.org/lkml/[email protected]/

I'm curious, do we see a lot of BUGs that resulted from wrong usage such that we need runtime checks for such things that the code can just statically specify (If I use kstrdup_const() for allocation I can just use kfree_const() for freeing - no runtime checks needed).

IOW, what's the real benefit that is worth spending extra runtime cycles?

Signature · 3 lines

Thanks,

David / dhildenb

Joe Perches

2020-06-30 14:14 UTC #7 lore raw

Quoted 24 lines · David Hildenbrand

Quoted 23 lines · Joe Perches
Quoted 11 lines · David Hildenbrand
Quoted 10 lines · Joe Perches
Quoted 6 lines · Bartosz Golaszewski
From: Bartosz Golaszewski <[email protected]>
Memory allocated with kstrdup_const() must not be passed to regular krealloc() as it is not aware of the possibility of the chunk residing in .rodata. Since there are no potential users of krealloc_const() at the moment, let's just update the doc to make it explicit.
Another option would be to return NULL if it's
used from krealloc with a pointer into rodata
[]
Quoted 1 line
mm/slab_common.c
Quoted 1 line
mm/slab_common.c +0 -0
[]
Quoted 7 lines
mm/slab_common.c static __always_inline void *__do_krealloc(const void *p, size_t new_size, · line 1688
Quoted 7 lines
mm/slab_common.c +3 -0
@@ -1683,6 +1683,9 @@ static __always_inline void *__do_krealloc(const void *p, size_t new_size,
  * @new_size: how many bytes of memory are required.
  * @flags: the type of memory to allocate.
  *
+ * If the object pointed to is in rodata (likely from kstrdup_const)
+ * %NULL is returned.
+ *
[]

Quoted 7 lines

Quoted 7 lines
Quoted 2 lines
Won't we have similar issues if somebody would do a kfree() instead of a kfree_const()? So I think the original patch makes sense.
Which is why I also suggested making kfree work for more types of memory freeing earlier this month.
https://lore.kernel.org/lkml/[email protected]/

[]

Quoted 1 line

what's the real benefit that is worth spending extra runtime cycles?

I very much doubt there is an actual instance where the runtime cycles matter. Where could there be a fast-path instance of free?

ANd kvfree consolidation and coding simplicity make it somewhat useful.

David Hildenbrand

2020-06-30 14:36 UTC #8 lore raw

Quoted 31 lines · Joe Perches

Quoted 30 lines · David Hildenbrand
Quoted 29 lines · Joe Perches
Quoted 11 lines · David Hildenbrand
Quoted 10 lines · Joe Perches
Quoted 6 lines · Bartosz Golaszewski
From: Bartosz Golaszewski <[email protected]>
Memory allocated with kstrdup_const() must not be passed to regular krealloc() as it is not aware of the possibility of the chunk residing in .rodata. Since there are no potential users of krealloc_const() at the moment, let's just update the doc to make it explicit.
Another option would be to return NULL if it's
used from krealloc with a pointer into rodata
[]
Quoted 1 line
mm/slab_common.c
Quoted 1 line
mm/slab_common.c +0 -0
[]
Quoted 7 lines
mm/slab_common.c static __always_inline void *__do_krealloc(const void *p, size_t new_size, · line 1688
Quoted 7 lines
mm/slab_common.c +3 -0
@@ -1683,6 +1683,9 @@ static __always_inline void *__do_krealloc(const void *p, size_t new_size,
  * @new_size: how many bytes of memory are required.
  * @flags: the type of memory to allocate.
  *
+ * If the object pointed to is in rodata (likely from kstrdup_const)
+ * %NULL is returned.
+ *
[]
Quoted 2 lines
Won't we have similar issues if somebody would do a kfree() instead of a kfree_const()? So I think the original patch makes sense.
Which is why I also suggested making kfree work for more types of memory freeing earlier this month.

Quoted 7 lines

Quoted 1 line
Quoted 1 line
https://lore.kernel.org/lkml/[email protected]/
[]
Quoted 1 line
what's the real benefit that is worth spending extra runtime cycles?
I very much doubt there is an actual instance
where the runtime cycles matter. Where could
there be a fast-path instance of free?

Well, looking at kfree() I can directly spot "unlikely()", which sounds like somebody cares about branch prediction in the slab.

Once you have cases that can happen equally likely it most certainly degrades performance. The question is if we care.

Coming back to my question, so the major benefit you see is coding simplicity, correct?

Signature · 3 lines

Thanks,

David / dhildenb

Joe Perches

2020-06-30 14:51 UTC #9 lore raw

Quoted 34 lines · David Hildenbrand

Quoted 33 lines · Joe Perches
Quoted 24 lines · David Hildenbrand
Quoted 23 lines · Joe Perches
Quoted 11 lines · David Hildenbrand
Quoted 10 lines · Joe Perches
Quoted 6 lines · Bartosz Golaszewski
From: Bartosz Golaszewski <[email protected]>
Memory allocated with kstrdup_const() must not be passed to regular krealloc() as it is not aware of the possibility of the chunk residing in .rodata. Since there are no potential users of krealloc_const() at the moment, let's just update the doc to make it explicit.
Another option would be to return NULL if it's
used from krealloc with a pointer into rodata
[]
Quoted 1 line
mm/slab_common.c
Quoted 1 line
mm/slab_common.c +0 -0
[]
Quoted 7 lines
mm/slab_common.c static __always_inline void *__do_krealloc(const void *p, size_t new_size, · line 1688
Quoted 7 lines
mm/slab_common.c +3 -0
@@ -1683,6 +1683,9 @@ static __always_inline void *__do_krealloc(const void *p, size_t new_size,
  * @new_size: how many bytes of memory are required.
  * @flags: the type of memory to allocate.
  *
+ * If the object pointed to is in rodata (likely from kstrdup_const)
+ * %NULL is returned.
+ *
[]
Quoted 7 lines
Quoted 7 lines
Quoted 2 lines
Won't we have similar issues if somebody would do a kfree() instead of a kfree_const()? So I think the original patch makes sense.
Which is why I also suggested making kfree work for more types of memory freeing earlier this month.
https://lore.kernel.org/lkml/[email protected]/
[]

Quoted 8 lines

Quoted 5 lines
Quoted 1 line
what's the real benefit that is worth spending extra runtime cycles?
I very much doubt there is an actual instance
where the runtime cycles matter. Where could
there be a fast-path instance of free?
Well, looking at kfree() I can directly spot "unlikely()", which sounds like somebody cares about branch prediction in the slab.

Or is telling the compiler of a 95%+ likely case.

Quoted 2 lines

Once you have cases that can happen equally likely it most certainly degrades performance. The question is if we care.

Right.

Does 4 additional tests in what appears to be almost exclusively non-fast paths matter?

Quoted 2 lines

Coming back to my question, so the major benefit you see is coding simplicity, correct?

Yes.