mm: kmemleak: check boundary of objects allocated with physical address when scan

mm/kmemleak.c EXPORT_SYMBOL(kmemleak_no_scan); · line 1138

Quoted 7 lines · Patrick Wang

mm/kmemleak.c +1 -2

@@ -1132,8 +1135,13 @@ EXPORT_SYMBOL(kmemleak_no_scan);
  void __ref kmemleak_alloc_phys(phys_addr_t phys, size_t size, int min_count,
  			       gfp_t gfp)
  {
-	if (PHYS_PFN(phys) >= min_low_pfn && PHYS_PFN(phys) < max_low_pfn)
-		kmemleak_alloc(__va(phys), size, min_count, gfp);
+	pr_debug("%s(0x%p, %zu, %d)\n", __func__, __va(phys), size, min_count);

I'd print just phys here since that's the function argument.

mm/kmemleak.c EXPORT_SYMBOL(kmemleak_no_scan); · line 1144

Quoted 6 lines

mm/kmemleak.c +5 -0

+	if (kmemleak_enabled && (unsigned long)__va(phys) >= PAGE_OFFSET &&
+	    !IS_ERR(__va(phys)))
+		/* create object with OBJECT_PHYS flag */
+		create_object((unsigned long)__va(phys), size, min_count,
+			      gfp, true);

Do we still need to check for __va(phys) >= PAGE_OFFSET? Also I don't think IS_ERR(__va(phys)) makes sense, we can't store an error in a physical address. The kmemleak_alloc_phys() function is only called on successful allocation, so shouldn't bother with error codes.

mm/kmemleak.c static void kmemleak_scan(void) · line 1445

Quoted 7 lines

mm/kmemleak.c +2 -0

@@ -1436,6 +1441,13 @@ static void kmemleak_scan(void)
  			dump_object_info(object);
  		}
  #endif
+
+		/* outside lowmem, make it black */

Maybe a bit more verbose:

		/* ignore objects outside lowmem (paint them black) */

mm/kmemleak.c static void kmemleak_scan(void) · line 1449

Quoted 5 lines

mm/kmemleak.c +4 -0

+		if (object->flags & OBJECT_PHYS)
+			if (PHYS_PFN(__pa((void *)object->pointer)) < min_low_pfn ||
+			    PHYS_PFN(__pa((void *)object->pointer)) >= max_low_pfn)
+				__paint_it(object, KMEMLEAK_BLACK);

I'd skip the checks if the object is OBJECT_NO_SCAN (side-effect of __paint_it()) so that the next scan won't have to go through the __pa() checks again. It's also probably more correct to check the upper object boundary). Something like:

		if ((object->flags & OBJECT_PHYS) &&
		    !(object->flags & OBJECT_NO_SCAN)) {
			unsigned long phys = __pa((void *)object->pointer);
			if (PHYS_PFN(phys) < min_low_pfn ||
			    PHYS_PFN(phys + object->size) >= max_low_pfn)
				__paint_it(object, KMEMLEAK_BLACK);
		}

Quoted 11 lines · Catalin Marinas

mm/kmemleak.c EXPORT_SYMBOL(kmemleak_no_scan); · line 1144

Quoted 5 lines · Patrick Wang

mm/kmemleak.c +5 -0

+	if (kmemleak_enabled && (unsigned long)__va(phys) >= PAGE_OFFSET &&
+	    !IS_ERR(__va(phys)))
+		/* create object with OBJECT_PHYS flag */
+		create_object((unsigned long)__va(phys), size, min_count,
+			      gfp, true);

Do we still need to check for __va(phys) >= PAGE_OFFSET? Also I don't think IS_ERR(__va(phys)) makes sense, we can't store an error in a physical address. The kmemleak_alloc_phys() function is only called on successful allocation, so shouldn't bother with error codes.

In this commit:
972fa3a7c17c(mm: kmemleak: alloc gray object for reserved region with direct map)

The kmemleak_alloc_phys() function is called directly by passing physical address from devicetree. So I'm concerned that could __va() => __pa() convert always get the phys back? I thought check for __va(phys) might help, but it probably dosen't work and using IS_ERR is indeed inappropriate.

We might have to store phys in object and convert it via __va() for normal use like:

#define object_pointer(obj)	\
	(obj->flags & OBJECT_PHYS ? (unsigned long)__va((void *)obj->pointer)	\
				: obj->pointer)

Quoted 23 lines · Patrick Wang

Quoted 11 lines · Catalin Marinas

mm/kmemleak.c EXPORT_SYMBOL(kmemleak_no_scan); · line 1144

Quoted 5 lines · Patrick Wang

+ if (kmemleak_enabled && (unsigned long)__va(phys) >= PAGE_OFFSET &&

mm/kmemleak.c +4 -0

+     !IS_ERR(__va(phys)))
+         /* create object with OBJECT_PHYS flag */
+         create_object((unsigned long)__va(phys), size, min_count,
+                       gfp, true);

Do we still need to check for __va(phys) >= PAGE_OFFSET? Also I don't think IS_ERR(__va(phys)) makes sense, we can't store an error in a physical address. The kmemleak_alloc_phys() function is only called on successful allocation, so shouldn't bother with error codes.

In this commit:
972fa3a7c17c(mm: kmemleak: alloc gray object for reserved region with direct map)

The kmemleak_alloc_phys() function is called directly by passing physical address from devicetree. So I'm concerned that could __va() => __pa() convert always get the phys back? I thought check for __va(phys) might help, but it probably dosen't work and using IS_ERR is indeed inappropriate.

Quoted 6 lines

We might have to store phys in object and convert it via __va() for normal use like:

#define object_pointer(obj)   \
      (obj->flags & OBJECT_PHYS ? (unsigned long)__va((void *)obj->pointer)   \
                              : obj->pointer)

In the commit you mentioned, the kmemleak callback is skipped if the memory is marked no-map.

But you have a point with the va->pa conversion. On 32-bit architectures, the __va() is no longer valid if the pfn is above max_low_pfn. So whatever we add to the rbtree may be entirely bogus, and we can't guarantee that the va->pa conversion back is correct.

Storing the phys address in object->pointer only solves the conversion but it doesn't solve the rbtree problem (VA and PA values may overlap, we can't just store the physical address either). And we use the rbtree for searching objects on freeing as well.

Given that all the kmemleak_alloc_phys() calls always pass min_count=0 (we should probably get rid of the extra arguments), we don't expect them to leak, so there's no point in adding them to the rbtree. We can instead add a new object_phys_tree_root to store these objects by the physical address for when we need to search (kmemleak_free_part_phys()). This would probably look simpler than recording the callbacks and replaying them.

Wherever we use object_tree_root we should check for OBJECT_PHYS and use object_phys_tree_root instead. There aren't many places.