Bug Hunting

It is interesting how you find bugs in code when unit tests unexpectedly not
break as changes are being done to them. One such incident was the discovery of
a bug related to a memory segment that got removed from the tree as all the
pages within it got unplugged.

Cherry (cherry@) was working on some stuff related to uvm_page_physunload_force()
when he noticed that the handle to the memory segment which had all of it’s
pages removed was still valid. Basically the uvm_physseg_valid() would
still recognize the handle has valid handle when clearly it was not.

Here is a small excerpt of the code from uvm_physseg_unplug()

1
2
3
    rb_tree_remove_node(&(vm_physmem.rb_tree), upm);
    uvm_physseg_free(seg, sizeof(struct vm_physseg));
    vm_physmem.nentries--;

A small part of test that was checking out this specific part of the code

1
2
3
4
5
6
7
8
9
10
11
    ATF_REQUIRE_EQ(VALID_AVAIL_START_PFN_1,
        uvm_physseg_get_avail_start(upm));

    for(paddr_t i = VALID_AVAIL_START_PFN_1;
        i < VALID_AVAIL_END_PFN_1; i++) {
            ATF_CHECK_EQ(true,
                uvm_page_physunload_force(upm, VM_FREELIST_DEFAULT, &p));
            ATF_CHECK_EQ(i, atop(p));
    }

    ATF_CHECK_EQ(1, uvm_physseg_get_entries());

The issue in the above test snippet is the fact that I assumed that
uvm_physseg_free() would make the handle invalid, apparently this was a wrong
assumption. uvm_physseg_free() internally uses free() to free the handle
and this does not necessarly mean that the data within the handle becomes
invalid.

So couple of changes were made

In uvm_physseg_unplug()

1
2
3
4
     rb_tree_remove_node(&(vm_physmem.rb_tree), upm);
+    memset(seg, 0, sizeof(struct vm_physseg));
     uvm_physseg_free(seg, sizeof(struct vm_physseg));
     vm_physmem.nentries--;

the content of the struct is cleared by setting it to zeroes before passing it
for getting free()d, there by the memory segment becomes invalid and
This change made the test fail (as expected) because as soon as the pages within
the segment collapsed it would be zeroed out. So when uvm_physseg_get_avail_start()
would be called on the handle it would return UVM_PHYSSEG_TYPE_INVALID

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
             ATF_CHECK_EQ(i, atop(p));
-            ATF_CHECK_EQ(i + 1, uvm_physseg_get_avail_start(upm));
+
+            if(i + 1 < VALID_AVAIL_END_PFN_1)
+                    ATF_CHECK_EQ(i + 1, uvm_physseg_get_avail_start(upm));
     }
+
+    /*
+     * Now we try to retrieve the segment, which has been removed
+     * from the system through force unloading all the pages inside it.
+     */
+    upm = vm_physseg_find(VALID_AVAIL_END_PFN_1 - 1, NULL);
+
+    /* It should no longer exist */
+    ATF_CHECK_EQ(NULL, upm);

     ATF_CHECK_EQ(1, uvm_physseg_get_entries());

There was a slight change made in the test to explicity check if the segment
gets removed from the tree and it no longer valid.