Avoid sign extension of value passed to kva_alloc from uma_zone_reserve_kva

[zbb]

Summary

Fixes "panic: vm_radix_reserve_kva: unable to reserve KVA" on 64-bit architecture
caused by sign extension of "pages * UMA_SLAB_SIZE" value passed to kva_alloc()
which takes unsigned long argument.

In the erroneus case that triggered this bug, the number of pages
to allocate in uma_zone_reserve_kva() was 0x8ebe6, that gave the
total number of bytes to allocate equal to 0x8ebe6000 (int).
This was then sign extended in kva_alloc() to 0xffffffff8ebe6000
(unsigned long).

Reviewed by:
Submitted by:
Obtained from: Semihalf
Sponsored by: The FreeBSD Foundation
Differential Revision:

[kib]

vm_pindex_t usually considered more appropriate type for the page counts (it was discussed heatly at least once). IMO you could use u_int for pages, the only drawback is that you would need to cast it to vm_offset_t when multiplying in the kva_alloc() argument calculation.

[zbb]

In D3346#68013, @kib wrote:

vm_pindex_t usually considered more appropriate type for the page counts (it was discussed heatly at least once). IMO you could use u_int for pages, the only drawback is that you would need to cast it to vm_offset_t when multiplying in the kva_alloc() argument calculation.

You mean cast it to vm_size_t (not vm_offset_t)? You are right, this may be a better idea to use u_int and cast it explicitly to vm_size_t . If this is what you think is right, I will update the diff.

[alc]

Who exactly argues for the use of vm_pindex_t to represent *all* page counts? It's not me. :-) And, from your comment, I don't think that it's you either. Am I wrong? There are cases where vm_pindex_t is needed to represent a page count, but this isn't one of them. In this case, the use of vm_pindex_t would force 32-bit machines that will never have address spaces larger than 2^32 bytes to do 64-bit arithmetic unnecessarily.

[kib]

In D3346#68014, @zbb wrote:

You mean cast it to vm_size_t (not vm_offset_t)? You are right, this may be a better idea to use u_int and cast it explicitly to vm_size_t . If this is what you think is right, I will update the diff.

This is fine with me.

[alc]

In D3346#68014, @zbb wrote:

In D3346#68013, @kib wrote:

vm_pindex_t usually considered more appropriate type for the page counts (it was discussed heatly at least once). IMO you could use u_int for pages, the only drawback is that you would need to cast it to vm_offset_t when multiplying in the kva_alloc() argument calculation.

You mean cast it to vm_size_t (not vm_offset_t)? You are right, this may be a better idea to use u_int and cast it explicitly to vm_size_t . If this is what you think is right, I will update the diff.

What's being calculated is a size rather than an address, so I would use vm_size_t rather than vm_offset_t.

[zbb]

In D3346#68018, @alc wrote:

In D3346#68014, @zbb wrote:

In D3346#68013, @kib wrote:

vm_pindex_t usually considered more appropriate type for the page counts (it was discussed heatly at least once). IMO you could use u_int for pages, the only drawback is that you would need to cast it to vm_offset_t when multiplying in the kva_alloc() argument calculation.

You mean cast it to vm_size_t (not vm_offset_t)? You are right, this may be a better idea to use u_int and cast it explicitly to vm_size_t . If this is what you think is right, I will update the diff.

What's being calculated is a size rather than an address, so I would use vm_size_t rather than vm_offset_t.

Yes, I thought so. That is why I assumed this was a typo. I'm updating the diff now.

[kib]

It may be reasonable to change KPI of uma_zone_reserve_kva() to take unsigned count, in the separate change.

[zbb]

Thank you all for your review. Just sent the change.