unmap with size 0x1

[tsatke]

Hi, I have implemented a mapper, and my kernel gave me an error originating from the unmap function. I've logged the allocations and deallocations that the mapper wants to perform, and I get

allocated interval at 0x0000111111110000 size 0x1000
allocated interval at 0x0000111111111000 size 0x1000
allocated interval at 0x0000111111112000 size 0x1000
allocated interval at 0x0000111111113000 size 0x1000
allocated interval at 0x0000111111114000 size 0x1000
allocated interval at 0x0000111111115000 size 0x1000
allocated interval at 0x0000111111116000 size 0x1000
allocated interval at 0x0000111111117000 size 0x1000
allocated interval at 0x0000111111118000 size 0x1000
allocated interval at 0x0000111111119000 size 0x1000
allocated interval at 0x000011111111a000 size 0x1000
allocated interval at 0x000011111111b000 size 0x1000
allocated interval at 0x000011111111c000 size 0x1000
allocated interval at 0x000011111111d000 size 0x1000
allocated interval at 0x000011111111e000 size 0x1000
allocated interval at 0x000011111111f000 size 0x1000
allocated interval at 0x0000111111120000 size 0x1000
allocated interval at 0x0000111111121000 size 0x1000
allocated interval at 0x0000111111122000 size 0x1000
allocated interval at 0x0000111111123000 size 0x1000
unmap: 0x0000111111110000 size 0x1

My kernel does all of these allocations, but then the mapper wants to deallocate a valid pointer with an invalid size.

My mapper is the following:

impl Mapper for XhciMapper {
    unsafe fn map(&mut self, phys_start: usize, bytes: usize) -> NonZeroUsize {
        let frames = {
            let start = PhysFrame::<Size4KiB>::containing_address(PhysAddr::new(phys_start as u64));
            let end = PhysFrame::<Size4KiB>::containing_address(PhysAddr::new(phys_start as u64 + bytes as u64 - 1));
            PhysFrameRangeInclusive { start, end }
        };

        let interval = vmm().reserve(bytes).unwrap().leak();
        serial_println!("allocated interval at {:#p} size {:#x}", interval.start(), interval.size());

        for (i, frame) in frames.enumerate() {
            let vaddr = interval.start() + (i as u64 * frame.size());
            map_page!(
                Page::containing_address(vaddr),
                frame,
                Size4KiB,
                PageTableFlags::PRESENT
            );
        }

        NonZeroUsize::new(interval.start().as_u64() as usize).unwrap()
    }

    fn unmap(&mut self, virt_start: usize, bytes: usize) {
        serial_println!("unmap: {:#p} size {:#x}", VirtAddr::new(virt_start as u64), bytes);
        assert!(vmm().release(Interval::new(VirtAddr::new(virt_start as u64), bytes)));
    }
}

To me, it seems like the mapper is called incorrectly from inside the xhci crate, but I may be wrong. I'd be thankful for any help.

[toku-sa-n]

Thanks for the report. Do you have a public repository for this code? I'd like to check what's going on there.

[tsatke]

https://github.com/tsatke/devos/blob/7098ccd5a70c35ec455a36362f6b590b7ace4bb0/kernel/src/main.rs#L58

You should be able to run/debug with lldb if you want to (obviously let me know if you need any more info, I don't expect you to debug my stuff).

The BAR of the device has a phys addr of 0x8_0000_0000 (qemu monitor also says that) and a size of 0x4000, so it must map at least 4 pages (that's my understanding at least).

Also, 0x0000111111110000 is just a memory area that the vmm uses, kind of a second heap for exactly such stuff.

[tsatke]

Adding more output:

expand...

allocated interval at 0x0000111111110000 size 0x1000
mapping phys PhysFrame(0x800000000) to virt 0x0000111111110000
done mapping, returning 0x0000111111110000
allocated interval at 0x0000111111111000 size 0x1000

mapping phys PhysFrame(0x800000000) to virt 0x0000111111111000

done mapping, returning 0x0000111111111000
allocated interval at 0x0000111111112000 size 0x1000

mapping phys PhysFrame(0x800000000) to virt 0x0000111111112000

done mapping, returning 0x0000111111112000
allocated interval at 0x0000111111113000 size 0x1000

mapping phys PhysFrame(0x800000000) to virt 0x0000111111113000

done mapping, returning 0x0000111111113000
allocated interval at 0x0000111111114000 size 0x1000

mapping phys PhysFrame(0x800000000) to virt 0x0000111111114000

done mapping, returning 0x0000111111114000
allocated interval at 0x0000111111115000 size 0x1000

mapping phys PhysFrame(0x800000000) to virt 0x0000111111115000

done mapping, returning 0x0000111111115000
allocated interval at 0x0000111111116000 size 0x1000

mapping phys PhysFrame(0x800000000) to virt 0x0000111111116000

done mapping, returning 0x0000111111116000
allocated interval at 0x0000111111117000 size 0x1000

mapping phys PhysFrame(0x800000000) to virt 0x0000111111117000

done mapping, returning 0x0000111111117000
allocated interval at 0x0000111111118000 size 0x1000

mapping phys PhysFrame(0x800000000) to virt 0x0000111111118000

done mapping, returning 0x0000111111118000
allocated interval at 0x0000111111119000 size 0x1000

mapping phys PhysFrame(0x800000000) to virt 0x0000111111119000

done mapping, returning 0x0000111111119000
allocated interval at 0x000011111111a000 size 0x1000

mapping phys PhysFrame(0x801000000) to virt 0x000011111111a000

done mapping, returning 0x000011111111a000
allocated interval at 0x000011111111b000 size 0x1000

mapping phys PhysFrame(0x800000000) to virt 0x000011111111b000

done mapping, returning 0x000011111111b000
allocated interval at 0x000011111111c000 size 0x1000

mapping phys PhysFrame(0x800000000) to virt 0x000011111111c000

done mapping, returning 0x000011111111c000
allocated interval at 0x000011111111d000 size 0x1000

mapping phys PhysFrame(0x800000000) to virt 0x000011111111d000

done mapping, returning 0x000011111111d000
allocated interval at 0x000011111111e000 size 0x1000

mapping phys PhysFrame(0x800000000) to virt 0x000011111111e000

done mapping, returning 0x000011111111e000
allocated interval at 0x000011111111f000 size 0x1000

mapping phys PhysFrame(0x800000000) to virt 0x000011111111f000

done mapping, returning 0x000011111111f000
allocated interval at 0x0000111111120000 size 0x1000

mapping phys PhysFrame(0x800000000) to virt 0x0000111111120000

done mapping, returning 0x0000111111120000
allocated interval at 0x0000111111121000 size 0x1000

mapping phys PhysFrame(0x800000000) to virt 0x0000111111121000

done mapping, returning 0x0000111111121000
allocated interval at 0x0000111111122000 size 0x1000

mapping phys PhysFrame(0x800000000) to virt 0x0000111111122000

done mapping, returning 0x0000111111122000
allocated interval at 0x0000111111123000 size 0x1000

mapping phys PhysFrame(0x801000000) to virt 0x0000111111123000

done mapping, returning 0x0000111111123000

It looks like the mapper is being told to map the same frame repeatedly to different addresses (without unmapping before re-mapping).

Btw, I'm attaching qemu-kbd via qemu-xhci as the only USB device. If you were to do that, I guess you should get the same addresses and sizes.

---

As far as I can see, ReadOnly immediately maps, and we create multiple ReadOnlys when creating the Registers object. The mapper that I implemented could theoretically remember the mappings, however I think this would only be valid for ReadOnly (right?), and the mapper has no idea when a mapping is read only.

unmap backtrace (it's being called because the Registers is not used and immediately dropped):

* thread #1, stop reason = breakpoint 3.1
  * frame #0: 0xffff800000038379 kernel-a753aa1064c5b39d`_$LT$kernel..XhciMapper$u20$as$u20$accessor..mapper..Mapper$GT$::unmap::h93db4a0cd8310dcb(self=0xffff808000020c20, virt_start=18764998443008, bytes=1) at main.rs:96:52
    frame #1: 0xffff80000003a8bb kernel-a753aa1064c5b39d`_$LT$accessor..single..Generic$LT$T$C$M$C$A$GT$$u20$as$u20$core..ops..drop..Drop$GT$::drop::ha4045d82ad227524(self=0xffff808000020c10) at single.rs:258:9
    frame #2: 0xffff80000003a3da kernel-a753aa1064c5b39d`core::ptr::drop_in_place$LT$accessor..single..Generic$LT$xhci..registers..capability..CapabilityRegistersLength$C$kernel..XhciMapper$C$accessor..marker..ReadWrite$GT$$GT$::h1c5a1fb7d39185a7((null)=0xffff808000020c10) at mod.rs:574:1
    frame #3: 0xffff80000003a503 kernel-a753aa1064c5b39d`core::ptr::drop_in_place$LT$xhci..registers..capability..Capability$LT$kernel..XhciMapper$GT$$GT$::hbe6dacd0dcceeb8c((null)=0xffff808000020c10) at mod.rs:574:1
    frame #4: 0xffff80000003a493 kernel-a753aa1064c5b39d`core::ptr::drop_in_place$LT$xhci..registers..Registers$LT$kernel..XhciMapper$GT$$GT$::hb5ab1abf43e05083((null)=0xffff808000020c10) at mod.rs:574:1
    frame #5: 0xffff80000003704b kernel-a753aa1064c5b39d`kernel::kernel_main::_$u7b$$u7b$closure$u7d$$u7d$::h7760b391a5774fb0((null)=0xffff808000020e2f, device=0xffff8000010e0e30) at main.rs:61:9
    frame #6: 0xffff80000003ac12 kernel-a753aa1064c5b39d`core::iter::adapters::filter_map::filter_map_fold::_$u7b$$u7b$closure$u7d$$u7d$::h542d3f29044f81d8(acc=<unavailable>, item=0xffff8000010e0e30) at filter_map.rs:40:28
    frame #7: 0xffff800000036b1f kernel-a753aa1064c5b39d`core::iter::traits::iterator::Iterator::fold::h3dd1eb8a12d703ac(self=DevicesIter @ 0xffff808000020e18, init=<unavailable>, f={closure_env#0}<&pci::device::PciDevice, pci::header::BaseAddressRegister, (), kernel::kernel_main::{closure_env#2}, core::iter::traits::iterator::Iterator::for_each::call::{closure_env#0}<pci::header::BaseAddressRegister, kernel::kernel_main::{closure_env#3}>> @ 0xffff808000020e2f) at iterator.rs:2583:21
    frame #8: 0xffff80000003abc2 kernel-a753aa1064c5b39d`_$LT$core..iter..adapters..filter_map..FilterMap$LT$I$C$F$GT$$u20$as$u20$core..iter..traits..iterator..Iterator$GT$::fold::hea2590e2be162f00(self=FilterMap<pci::DevicesIter, kernel::kernel_main::{closure_env#2}> @ 0xffff808000020e50, init=<unavailable>, fold=(f = kernel::kernel_main::{closure_env#3} @ 0xffff808000020e66)) at filter_map.rs:149:9
    frame #9: 0xffff80000003abe2 kernel-a753aa1064c5b39d`core::iter::traits::iterator::Iterator::for_each::h28eaaf8767a547ac(self=FilterMap<pci::DevicesIter, kernel::kernel_main::{closure_env#2}> @ 0xffff808000020e70, f={closure_env#3} @ 0xffff808000020e87) at iterator.rs:813:9
    frame #10: 0xffff8000000379f9 kernel-a753aa1064c5b39d`kernel::kernel_main::hbddf92c0abcc47e2(boot_info=0xffff818000000000) at main.rs:46:5
    frame #11: 0xffff800000038d17 kernel-a753aa1064c5b39d`_start(boot_info=0xffff818000000000) at lib.rs:131:17

[tsatke]

Looking into it even further, this behavior doesn't surprise me anymore.

1. During creation, a Capabilities register is created as ReadOnly<CapabilityRegistersLength, _> - that's effectively a Generic<u8, _, _>
2. In drop, the Capabilities register is dropped, calling drop on the ReadOnly<u8, _>
3. The essence of that impl is

   impl<T, _, _> Drop for Generic<T, _, _>
   {
       fn drop(&mut self) {
           let bytes = mem::size_of::<T>(); // this is 1, since T is u8
           self.mapper.unmap(self.virt, bytes);
       }
   }

The debugger confirms this.

This doesn't seem correct to me, but then I also don't know the design behind the crate, or I am missing something.

[toku-sa-n]

Thanks for the investigation, and sorry for the late reply and for bothering you. I believe your investigation is correct.
The current design, one accessor per register, is intentional. If we pack some registers into one accessor, writing through the packed one may result in an undesired behavior (see #142 for example.), and thus the mapper implementation needs to handle this kind of per-byte memory allocation/deallocation.

[tsatke]

I think this is reasonable given how easy it makes the underlying implementation of the xhci crate, however I think this should be well documented, and could probably also be implemented (maybe in the accessor crate?) so that the crate consumer really just has to implement mappings as the Mapper trait seems to describe it right now.

[toku-sa-n]

For the documentation, I agree that a little more docs are needed for the xhci crate, like "the mapper and unmapper must be able to deal with per byte mappings."

and could probably also be implemented (maybe in the accessor crate?) so that the crate consumer really just has to implement mappings as the Mapper trait seems to describe it right now.

I'm sorry, but I couldn't understand it. Could you tell me a bit more?

[tsatke]

The accessor crate could already implement a structure such that a user doesn't have to handle single byte deallocations. Everyone would probably implement it in the same way.

This would also make mapping and unmapping consistent. Right now, the mapper may need to map 1 full page, but for that page he gets 4096 single byte unmap requests. I think it would make sense if it worked like a memory allocator, where the unmaps are of the same size as the maps.

This would also be consistent with the Mapper struct name. With paging enabled, I can only map and unmap pages. I can't unmap single bytes in a mapped page.