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]

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.