kasan: avoid overflowing quarantine size on low memory systems

If the total amount of memory assigned to quarantine is less than the amount of memory assigned to per-cpu quarantines, |new_quarantine_size| may overflow. Instead, set it to zero.

mm/kasan/quarantine.c +2 -1

@@ -214,7 +214,15 @@ void quarantine_reduce(void)
 	 */
 	new_quarantine_size = (READ_ONCE(totalram_pages) << PAGE_SHIFT) /
 		QUARANTINE_FRACTION;
-	new_quarantine_size -= QUARANTINE_PERCPU_SIZE * num_online_cpus();
+	percpu_quarantines = QUARANTINE_PERCPU_SIZE * num_online_cpus();
+	if (new_quarantine_size < percpu_quarantines) {

If the total amount of memory assigned to quarantine is less than the amount of memory assigned to per-cpu quarantines, |new_quarantine_size| may overflow. Instead, set it to zero.

Reported-by Dmitry Vyukov <[email protected]>

Fixes 55834c59098d ("mm: kasan: initial memory quarantine

implementation")

Signed-off-by Alexander Potapenko <[email protected]>

---
 mm/kasan/quarantine.c | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

mm/kasan/quarantine.c +1 -1

index 65793f1..416d3b0 100644
@@ -196,7 +196,7 @@ void quarantine_put(struct kasan_free_meta *info, struct kmem_cache *cache)
 
 void quarantine_reduce(void)
 {
-	size_t new_quarantine_size;
+	size_t new_quarantine_size, percpu_quarantines;
 	unsigned long flags;
 	struct qlist_head to_free = QLIST_INIT;
 	size_t size_to_free = 0;

Quoted 4 lines · Alexander Potapenko

If the total amount of memory assigned to quarantine is less than the amount of memory assigned to per-cpu quarantines, |new_quarantine_size| may overflow. Instead, set it to zero.

Just curious, how did find this?
Overflow is possible if system has more than 32 cpus per GB of memory. AFIAK this quite unusual.

Quoted 25 lines

Reported-by Dmitry Vyukov <[email protected]>

Fixes 55834c59098d ("mm: kasan: initial memory quarantine

implementation")

Signed-off-by Alexander Potapenko <[email protected]>

---
 mm/kasan/quarantine.c | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

mm/kasan/quarantine.c +1 -1

index 65793f1..416d3b0 100644
@@ -196,7 +196,7 @@ void quarantine_put(struct kasan_free_meta *info, struct kmem_cache *cache)
 
 void quarantine_reduce(void)
 {
-     size_t new_quarantine_size;
+     size_t new_quarantine_size, percpu_quarantines;
      unsigned long flags;
      struct qlist_head to_free = QLIST_INIT;
      size_t size_to_free = 0;

@@ -214,7 +214,15 @@ void quarantine_reduce(void)
       */
      new_quarantine_size = (READ_ONCE(totalram_pages) << PAGE_SHIFT) /
              QUARANTINE_FRACTION;

mm/kasan/quarantine.c void quarantine_reduce(void) · line 221

Quoted 6 lines

mm/kasan/quarantine.c +5 -1

-     new_quarantine_size -= QUARANTINE_PERCPU_SIZE * num_online_cpus();
+     percpu_quarantines = QUARANTINE_PERCPU_SIZE * num_online_cpus();
+     if (new_quarantine_size < percpu_quarantines) {
+             WARN_ONCE(1,
+                     "Too little memory, disabling global KASAN quarantine.\n",
+             );

Why WARN? I'd suggest pr_warn_once();

Quoted 8 lines

mm/kasan/quarantine.c +4 -0

+             new_quarantine_size = 0;
+     } else {
+             new_quarantine_size -= percpu_quarantines;
+     }
      WRITE_ONCE(quarantine_size, new_quarantine_size);
 
      last = global_quarantine.head;

Quoted 4 lines · Alexander Potapenko

If the total amount of memory assigned to quarantine is less than the amount of memory assigned to per-cpu quarantines, |new_quarantine_size| may overflow. Instead, set it to zero.

Just curious, how did find this?
Overflow is possible if system has more than 32 cpus per GB of memory. AFIAK this quite unusual.

Quoted 25 lines

Reported-by Dmitry Vyukov <[email protected]>

Fixes 55834c59098d ("mm: kasan: initial memory quarantine

implementation")

Signed-off-by Alexander Potapenko <[email protected]>

---
 mm/kasan/quarantine.c | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

mm/kasan/quarantine.c +1 -1

index 65793f1..416d3b0 100644
@@ -196,7 +196,7 @@ void quarantine_put(struct kasan_free_meta *info, struct kmem_cache *cache)
 
 void quarantine_reduce(void)
 {
-     size_t new_quarantine_size;
+     size_t new_quarantine_size, percpu_quarantines;
      unsigned long flags;
      struct qlist_head to_free = QLIST_INIT;
      size_t size_to_free = 0;

@@ -214,7 +214,15 @@ void quarantine_reduce(void)
       */
      new_quarantine_size = (READ_ONCE(totalram_pages) << PAGE_SHIFT) /
              QUARANTINE_FRACTION;

mm/kasan/quarantine.c void quarantine_reduce(void) · line 221

Quoted 6 lines

mm/kasan/quarantine.c +5 -1

-     new_quarantine_size -= QUARANTINE_PERCPU_SIZE * num_online_cpus();
+     percpu_quarantines = QUARANTINE_PERCPU_SIZE * num_online_cpus();
+     if (new_quarantine_size < percpu_quarantines) {
+             WARN_ONCE(1,
+                     "Too little memory, disabling global KASAN quarantine.\n",
+             );

Why WARN? I'd suggest pr_warn_once();

mm/kasan/quarantine.c void quarantine_reduce(void) · line 228

Quoted 8 lines

mm/kasan/quarantine.c +4 -0

+             new_quarantine_size = 0;
+     } else {
+             new_quarantine_size -= percpu_quarantines;
+     }
      WRITE_ONCE(quarantine_size, new_quarantine_size);
 
      last = global_quarantine.head;

Quoted 10 lines · Andrey Ryabinin

Quoted 4 lines · Alexander Potapenko

If the total amount of memory assigned to quarantine is less than the amount of memory assigned to per-cpu quarantines, |new_quarantine_size| may overflow. Instead, set it to zero.

Just curious, how did find this?
Overflow is possible if system has more than 32 cpus per GB of memory. AFIAK this quite unusual.

I was reading code for unrelated reason.

Quoted 25 lines

Quoted 25 lines

Reported-by Dmitry Vyukov <[email protected]>

Fixes 55834c59098d ("mm: kasan: initial memory quarantine

implementation")

Signed-off-by Alexander Potapenko <[email protected]>

---
 mm/kasan/quarantine.c | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

mm/kasan/quarantine.c +1 -1

index 65793f1..416d3b0 100644
@@ -196,7 +196,7 @@ void quarantine_put(struct kasan_free_meta *info, struct kmem_cache *cache)
 
 void quarantine_reduce(void)
 {
-     size_t new_quarantine_size;
+     size_t new_quarantine_size, percpu_quarantines;
      unsigned long flags;
      struct qlist_head to_free = QLIST_INIT;
      size_t size_to_free = 0;

@@ -214,7 +214,15 @@ void quarantine_reduce(void)
       */
      new_quarantine_size = (READ_ONCE(totalram_pages) << PAGE_SHIFT) /
              QUARANTINE_FRACTION;

Quoted 8 lines

mm/kasan/quarantine.c void quarantine_reduce(void) · line 221

Quoted 6 lines

mm/kasan/quarantine.c +5 -1

-     new_quarantine_size -= QUARANTINE_PERCPU_SIZE * num_online_cpus();
+     percpu_quarantines = QUARANTINE_PERCPU_SIZE * num_online_cpus();
+     if (new_quarantine_size < percpu_quarantines) {
+             WARN_ONCE(1,
+                     "Too little memory, disabling global KASAN quarantine.\n",
+             );

Why WARN? I'd suggest pr_warn_once();

I would suggest to just do something useful. Setting quarantine new_quarantine_size to 0 looks fine.
What would user do with this warning? Number of CPUs and amount of memory are generally fixed. Why is it an issue for end user at all? We still have some quarantine per-cpu. A WARNING means a [non-critical] kernel bug. E.g. syzkaller will catch each and every boot of such system as a bug.

Quoted 9 lines

Quoted 9 lines

mm/kasan/quarantine.c +4 -0

+             new_quarantine_size = 0;
+     } else {
+             new_quarantine_size -= percpu_quarantines;
+     }
      WRITE_ONCE(quarantine_size, new_quarantine_size);
 
      last = global_quarantine.head;

Quoted 50 lines · Dmitry Vyukov

Quoted 10 lines · Andrey Ryabinin

Quoted 4 lines · Alexander Potapenko

If the total amount of memory assigned to quarantine is less than the amount of memory assigned to per-cpu quarantines, |new_quarantine_size| may overflow. Instead, set it to zero.

Just curious, how did find this?
Overflow is possible if system has more than 32 cpus per GB of memory. AFIAK this quite unusual.

I was reading code for unrelated reason.

Quoted 25 lines

Quoted 25 lines

Reported-by Dmitry Vyukov <[email protected]>

Fixes 55834c59098d ("mm: kasan: initial memory quarantine

implementation")

Signed-off-by Alexander Potapenko <[email protected]>

---
 mm/kasan/quarantine.c | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

mm/kasan/quarantine.c +1 -1

index 65793f1..416d3b0 100644
@@ -196,7 +196,7 @@ void quarantine_put(struct kasan_free_meta *info, struct kmem_cache *cache)
 
 void quarantine_reduce(void)
 {
-     size_t new_quarantine_size;
+     size_t new_quarantine_size, percpu_quarantines;
      unsigned long flags;
      struct qlist_head to_free = QLIST_INIT;
      size_t size_to_free = 0;

@@ -214,7 +214,15 @@ void quarantine_reduce(void)
       */
      new_quarantine_size = (READ_ONCE(totalram_pages) << PAGE_SHIFT) /
              QUARANTINE_FRACTION;

Quoted 8 lines

mm/kasan/quarantine.c void quarantine_reduce(void) · line 221

Quoted 6 lines

mm/kasan/quarantine.c +5 -1

-     new_quarantine_size -= QUARANTINE_PERCPU_SIZE * num_online_cpus();
+     percpu_quarantines = QUARANTINE_PERCPU_SIZE * num_online_cpus();
+     if (new_quarantine_size < percpu_quarantines) {
+             WARN_ONCE(1,
+                     "Too little memory, disabling global KASAN quarantine.\n",
+             );

Why WARN? I'd suggest pr_warn_once();

Quoted 7 lines

I would suggest to just do something useful. Setting quarantine new_quarantine_size to 0 looks fine.
What would user do with this warning? Number of CPUs and amount of memory are generally fixed. Why is it an issue for end user at all? We still have some quarantine per-cpu. A WARNING means a [non-critical] kernel bug. E.g. syzkaller will catch each and every boot of such system as a bug.

How about printk_once then?
Silently setting the quarantine size to zero may puzzle the user.

Quoted 8 lines

Quoted 8 lines

mm/kasan/quarantine.c void quarantine_reduce(void) · line 228

Quoted 8 lines

mm/kasan/quarantine.c +4 -0

+             new_quarantine_size = 0;
+     } else {
+             new_quarantine_size -= percpu_quarantines;
+     }
      WRITE_ONCE(quarantine_size, new_quarantine_size);
 
      last = global_quarantine.head;

--
Alexander Potapenko
Software Engineer

Google Germany GmbH
Erika-Mann-Straße, 33
80636 München

Geschäftsführer: Matthew Scott Sucherman, Paul Terence Manicle Registergericht und -nummer: Hamburg, HRB 86891
Sitz der Gesellschaft: Hamburg

Quoted 5 lines · Dmitry Vyukov

On Tue, Aug 2, 2016 at 12:00 PM, Andrey Ryabinin

Quoted 2 lines

Why WARN? I'd suggest pr_warn_once();

Quoted 7 lines

I would suggest to just do something useful. Setting quarantine new_quarantine_size to 0 looks fine.
What would user do with this warning? Number of CPUs and amount of memory are generally fixed. Why is it an issue for end user at all? We still have some quarantine per-cpu. A WARNING means a [non-critical] kernel bug. E.g. syzkaller will catch each and every boot of such system as a bug.

How about printk_once then?
Silently setting the quarantine size to zero may puzzle the user.

Quoted 13 lines · Alexander Potapenko

Quoted 12 lines · Dmitry Vyukov

On Tue, Aug 2, 2016 at 12:00 PM, Andrey Ryabinin

Quoted 2 lines

Why WARN? I'd suggest pr_warn_once();

I would suggest to just do something useful. Setting quarantine new_quarantine_size to 0 looks fine.
What would user do with this warning? Number of CPUs and amount of memory are generally fixed. Why is it an issue for end user at all? We still have some quarantine per-cpu. A WARNING means a [non-critical] kernel bug. E.g. syzkaller will catch each and every boot of such system as a bug.

Quoted 3 lines

How about printk_once then?
Silently setting the quarantine size to zero may puzzle the user.

Nope, user will not notice anything. So keeping it silent would be better. Plus it's very unlikely that this will ever happen in real life.

If the total amount of memory assigned to quarantine is less than the amount of memory assigned to per-cpu quarantines, |new_quarantine_size| may overflow. Instead, set it to zero.

Reported-by Dmitry Vyukov <[email protected]>

Fixes 55834c59098d ("mm: kasan: initial memory quarantine

implementation")

Signed-off-by Alexander Potapenko <[email protected]>

---
 mm/kasan/quarantine.c | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

mm/kasan/quarantine.c +10 -2

index 65793f1..416d3b0 100644
@@ -196,7 +196,7 @@ void quarantine_put(struct kasan_free_meta *info, struct kmem_cache *cache)
 
 void quarantine_reduce(void)
 {
-	size_t new_quarantine_size;
+	size_t new_quarantine_size, percpu_quarantines;
 	unsigned long flags;
 	struct qlist_head to_free = QLIST_INIT;
 	size_t size_to_free = 0;

@@ -214,7 +214,15 @@ void quarantine_reduce(void)
 	 */
 	new_quarantine_size = (READ_ONCE(totalram_pages) << PAGE_SHIFT) /
 		QUARANTINE_FRACTION;
-	new_quarantine_size -= QUARANTINE_PERCPU_SIZE * num_online_cpus();
+	percpu_quarantines = QUARANTINE_PERCPU_SIZE * num_online_cpus();
+	if (new_quarantine_size < percpu_quarantines) {
+		WARN_ONCE(1,
+			"Too little memory, disabling global KASAN quarantine.\n",
+		);
+		new_quarantine_size = 0;
+	} else {
+		new_quarantine_size -= percpu_quarantines;
+	}
 	WRITE_ONCE(quarantine_size, new_quarantine_size);
 
 	last = global_quarantine.head;

Signature · 1 line

2.8.0.rc3.226.g39d4020

If the total amount of memory assigned to quarantine is less than the amount of memory assigned to per-cpu quarantines, |new_quarantine_size| may overflow. Instead, set it to zero.

Reported-by Dmitry Vyukov <[email protected]>

Fixes 55834c59098d ("mm: kasan: initial memory quarantine

implementation")

Signed-off-by Alexander Potapenko <[email protected]>

---
 mm/kasan/quarantine.c | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

mm/kasan/quarantine.c +10 -2

index 65793f1..416d3b0 100644
@@ -196,7 +196,7 @@ void quarantine_put(struct kasan_free_meta *info, struct kmem_cache *cache)
 
 void quarantine_reduce(void)
 {
-	size_t new_quarantine_size;
+	size_t new_quarantine_size, percpu_quarantines;
 	unsigned long flags;
 	struct qlist_head to_free = QLIST_INIT;
 	size_t size_to_free = 0;

@@ -214,7 +214,15 @@ void quarantine_reduce(void)
 	 */
 	new_quarantine_size = (READ_ONCE(totalram_pages) << PAGE_SHIFT) /
 		QUARANTINE_FRACTION;
-	new_quarantine_size -= QUARANTINE_PERCPU_SIZE * num_online_cpus();
+	percpu_quarantines = QUARANTINE_PERCPU_SIZE * num_online_cpus();
+	if (new_quarantine_size < percpu_quarantines) {
+		WARN_ONCE(1,
+			"Too little memory, disabling global KASAN quarantine.\n",
+		);
+		new_quarantine_size = 0;
+	} else {
+		new_quarantine_size -= percpu_quarantines;
+	}
 	WRITE_ONCE(quarantine_size, new_quarantine_size);
 
 	last = global_quarantine.head;

Signature · 1 line

2.8.0.rc3.226.g39d4020