Fix missing validation when binding memory to buffers with the shader_device_address usage (#2031)

* Add `MemoryAllocateFlags`

* Add validation to `UnsafeBuffer::bind_memory`

* Fix fn ptr in `BufferAccess::raw_device_address`

* Oopsie

* More validation, reject `device_address` on EXT

* Fix links

* Fix docs
This commit is contained in:
marc0246 2022-10-09 13:49:50 +02:00 committed by GitHub
parent 5ea2f3feba
commit 605bf429ac
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
8 changed files with 157 additions and 10 deletions

View File

@ -77,6 +77,8 @@ where
/// # Panics
///
/// - Panics if `T` has zero size.
/// - Panics if `usage.shader_device_address` is `true`.
// TODO: ^
pub fn from_data(
device: Arc<Device>,
usage: BufferUsage,
@ -130,6 +132,8 @@ where
///
/// - Panics if `T` has zero size.
/// - Panics if `data` is empty.
/// - Panics if `usage.shader_device_address` is `true`.
// TODO: ^
pub fn from_iter<I>(
device: Arc<Device>,
usage: BufferUsage,
@ -172,6 +176,8 @@ where
///
/// - Panics if `T` has zero size.
/// - Panics if `len` is zero.
/// - Panics if `usage.shader_device_address` is `true`.
// TODO: ^
pub unsafe fn uninitialized_array(
device: Arc<Device>,
len: DeviceSize,
@ -201,6 +207,8 @@ where
/// # Panics
///
/// - Panics if `size` is zero.
/// - Panics if `usage.shader_device_address` is `true`.
// TODO: ^
pub unsafe fn raw(
device: Arc<Device>,
size: DeviceSize,

View File

@ -197,9 +197,12 @@ where
/// # Panics
///
/// - Panics if `T` has zero size.
/// - Panics if `usage.shader_device_address` is `true`.
// TODO: ^
#[inline]
pub fn new(device: Arc<Device>, usage: BufferUsage) -> CpuBufferPool<T> {
assert!(size_of::<T>() > 0);
assert!(!usage.shader_device_address);
let pool = device.standard_memory_pool();
CpuBufferPool {

View File

@ -181,6 +181,11 @@ where
/// the initial upload operation. In order to be allowed to use the `DeviceLocalBuffer`, you
/// must either submit your operation after this future, or execute this future and wait for it
/// to be finished before submitting your own operation.
///
/// # Panics
///
/// - Panics if `usage.shader_device_address` is `true`.
// TODO: ^
pub fn from_buffer<B, L, A>(
source: Arc<B>,
usage: BufferUsage,
@ -235,6 +240,8 @@ where
/// # Panics
///
/// - Panics if `T` has zero size.
/// - Panics if `usage.shader_device_address` is `true`.
// TODO: ^
pub fn from_data<L, A>(
data: T,
usage: BufferUsage,
@ -264,6 +271,8 @@ where
///
/// - Panics if `T` has zero size.
/// - Panics if `data` is empty.
/// - Panics if `usage.shader_device_address` is `true`.
// TODO: ^
pub fn from_iter<D, L, A>(
data: D,
usage: BufferUsage,
@ -297,6 +306,8 @@ where
///
/// - Panics if `T` has zero size.
/// - Panics if `len` is zero.
/// - Panics if `usage.shader_device_address` is `true`.
// TODO: ^
pub fn array(
device: Arc<Device>,
len: DeviceSize,
@ -327,6 +338,8 @@ where
/// # Panics
///
/// - Panics if `size` is zero.
/// - Panics if `usage.shader_device_address` is `true`.
// TODO: ^
pub unsafe fn raw(
device: Arc<Device>,
size: DeviceSize,
@ -367,6 +380,8 @@ where
/// # Panics
///
/// - Panics if `size` is zero.
/// - Panics if `usage.shader_device_address` is `true`.
// TODO: ^
pub unsafe fn raw_with_exportable_fd(
device: Arc<Device>,
size: DeviceSize,

View File

@ -399,6 +399,15 @@ impl UnsafeBuffer {
}
/// Binds device memory to this buffer.
///
/// # Panics
///
/// - Panics if `self.usage.shader_device_address` is `true` and the `memory` was not allocated
/// with the [`device_address`] flag set and the [`ext_buffer_device_address`] extension is
/// not enabled on the device.
///
/// [`device_address`]: crate::memory::MemoryAllocateFlags::device_address
/// [`ext_buffer_device_address`]: crate::device::DeviceExtensions::ext_buffer_device_address
pub unsafe fn bind_memory(
&self,
memory: &DeviceMemory,
@ -435,6 +444,13 @@ impl UnsafeBuffer {
}
}
// VUID-vkBindBufferMemory-bufferDeviceAddress-03339
if self.usage.shader_device_address
&& !self.device.enabled_extensions().ext_buffer_device_address
{
assert!(memory.flags().device_address);
}
(fns.v1_0.bind_buffer_memory)(
self.device.internal_object(),
self.handle,

View File

@ -8,7 +8,7 @@
// according to those terms.
use super::{sys::UnsafeBuffer, BufferContents, BufferSlice, BufferUsage};
use crate::{device::DeviceOwned, DeviceSize, RequiresOneOf, SafeDeref, VulkanObject};
use crate::{device::DeviceOwned, DeviceSize, RequiresOneOf, SafeDeref, Version, VulkanObject};
use std::{
error::Error,
fmt::{Debug, Display, Error as FmtError, Formatter},
@ -100,13 +100,17 @@ pub unsafe trait BufferAccess: DeviceOwned + Send + Sync {
..Default::default()
};
let fns = device.fns();
let ptr = (fns.ext_buffer_device_address.get_buffer_device_address_ext)(
device.internal_object(),
&info,
);
let f = if device.api_version() >= Version::V1_2 {
fns.v1_2.get_buffer_device_address
} else if device.enabled_extensions().khr_buffer_device_address {
fns.khr_buffer_device_address.get_buffer_device_address_khr
} else {
fns.ext_buffer_device_address.get_buffer_device_address_ext
};
let ptr = f(device.internal_object(), &info);
if ptr == 0 {
panic!("got null ptr from a valid GetBufferDeviceAddressEXT call");
panic!("got null ptr from a valid GetBufferDeviceAddress call");
}
Ok(NonZeroU64::new_unchecked(ptr + inner.offset))

View File

@ -44,6 +44,13 @@ vulkan_bitflags! {
indirect_buffer = INDIRECT_BUFFER,
/// The buffer's device address can be retrieved.
///
/// A buffer created with this usage can only be bound to device memory allocated with the
/// [`device_address`] flag set unless the [`ext_buffer_device_address`] extension is enabled
/// on the device.
///
/// [`device_address`]: crate::memory::MemoryAllocateFlags::device_address
/// [`ext_buffer_device_address`]: crate::device::DeviceExtensions::ext_buffer_device_address
shader_device_address = SHADER_DEVICE_ADDRESS {
api_version: V1_2,
device_extensions: [khr_buffer_device_address, ext_buffer_device_address],

View File

@ -55,6 +55,7 @@ pub struct DeviceMemory {
allocation_size: DeviceSize,
memory_type_index: u32,
export_handle_types: ExternalMemoryHandleTypes,
flags: MemoryAllocateFlags,
}
impl DeviceMemory {
@ -80,16 +81,17 @@ impl DeviceMemory {
memory_type_index,
dedicated_allocation: _,
export_handle_types,
flags,
_ne: _,
} = allocate_info;
Ok(DeviceMemory {
handle,
device,
allocation_size,
memory_type_index,
export_handle_types,
flags,
})
}
@ -99,6 +101,7 @@ impl DeviceMemory {
///
/// - `handle` must be a valid Vulkan object handle created from `device`.
/// - `allocate_info` must match the info used to create the object.
#[inline]
pub unsafe fn from_handle(
device: Arc<Device>,
handle: ash::vk::DeviceMemory,
@ -109,16 +112,17 @@ impl DeviceMemory {
memory_type_index,
dedicated_allocation: _,
export_handle_types,
flags,
_ne: _,
} = allocate_info;
DeviceMemory {
handle,
device,
allocation_size,
memory_type_index,
export_handle_types,
flags,
}
}
@ -146,16 +150,17 @@ impl DeviceMemory {
memory_type_index,
dedicated_allocation: _,
export_handle_types,
flags,
_ne: _,
} = allocate_info;
Ok(DeviceMemory {
handle,
device,
allocation_size,
memory_type_index,
export_handle_types,
flags,
})
}
@ -169,6 +174,7 @@ impl DeviceMemory {
memory_type_index,
ref mut dedicated_allocation,
export_handle_types,
flags,
_ne: _,
} = allocate_info;
@ -379,6 +385,44 @@ impl DeviceMemory {
}
}
if !flags.is_empty()
&& device.physical_device().api_version() < Version::V1_1
&& !device.enabled_extensions().khr_device_group
{
return Err(DeviceMemoryError::RequirementNotMet {
required_for: "`allocate_info.flags` is not empty",
requires_one_of: RequiresOneOf {
api_version: Some(Version::V1_1),
device_extensions: &["khr_device_group"],
..Default::default()
},
});
}
if flags.device_address {
// VUID-VkMemoryAllocateInfo-flags-03331
if !device.enabled_features().buffer_device_address {
return Err(DeviceMemoryError::RequirementNotMet {
required_for: "`allocate_info.flags.device_address` is `true`",
requires_one_of: RequiresOneOf {
features: &["buffer_device_address"],
..Default::default()
},
});
}
if device.enabled_extensions().ext_buffer_device_address {
return Err(DeviceMemoryError::RequirementNotMet {
required_for: "`allocate_info.flags.device_address` is `true`",
requires_one_of: RequiresOneOf {
api_version: Some(Version::V1_2),
device_extensions: &["khr_buffer_device_address"],
..Default::default()
},
});
}
}
Ok(())
}
@ -392,6 +436,7 @@ impl DeviceMemory {
memory_type_index,
dedicated_allocation,
export_handle_types,
flags,
_ne: _,
} = allocate_info;
@ -466,6 +511,15 @@ impl DeviceMemory {
allocate_info = allocate_info.push_next(info);
}
let mut flags_info = ash::vk::MemoryAllocateFlagsInfo {
flags: flags.into(),
..Default::default()
};
if !flags.is_empty() {
allocate_info = allocate_info.push_next(&mut flags_info);
}
let mut allocation_count = device.allocation_count().lock();
// VUID-vkAllocateMemory-maxMemoryAllocationCount-04101
@ -516,6 +570,12 @@ impl DeviceMemory {
self.export_handle_types
}
/// Returns the flags the memory was allocated with.
#[inline]
pub fn flags(&self) -> MemoryAllocateFlags {
self.flags
}
/// Retrieves the amount of lazily-allocated memory that is currently commited to this
/// memory object.
///
@ -692,6 +752,15 @@ pub struct MemoryAllocateInfo<'d> {
/// The handle types that can be exported from the allocated memory.
pub export_handle_types: ExternalMemoryHandleTypes,
/// Additional flags for the memory allocation.
///
/// If not empty, the device API version must be at least 1.1, or the
/// [`khr_device_group`](crate::device::DeviceExtensions::khr_device_group) extension must be
/// enabled on the device.
///
/// The default value is [`MemoryAllocateFlags::empty()`].
pub flags: MemoryAllocateFlags,
pub _ne: crate::NonExhaustive,
}
@ -703,6 +772,7 @@ impl Default for MemoryAllocateInfo<'static> {
memory_type_index: u32::MAX,
dedicated_allocation: None,
export_handle_types: ExternalMemoryHandleTypes::empty(),
flags: MemoryAllocateFlags::empty(),
_ne: crate::NonExhaustive(()),
}
}
@ -717,6 +787,7 @@ impl<'d> MemoryAllocateInfo<'d> {
memory_type_index: u32::MAX,
dedicated_allocation: Some(dedicated_allocation),
export_handle_types: ExternalMemoryHandleTypes::empty(),
flags: MemoryAllocateFlags::empty(),
_ne: crate::NonExhaustive(()),
}
}
@ -936,6 +1007,28 @@ impl ExternalMemoryHandleTypes {
}
}
vulkan_bitflags! {
/// A mask specifying flags for device memory allocation.
#[non_exhaustive]
MemoryAllocateFlags = MemoryAllocateFlags(u32);
// TODO: implement
// device_mask = DEVICE_MASK,
/// Specifies that the allocated device memory can be bound to a buffer created with the
/// [`shader_device_address`] usage. This requires that the [`buffer_device_address`] feature
/// is enabled on the device and the [`ext_buffer_device_address`] extension is not enabled on
/// the device.
///
/// [`shader_device_address`]: crate::buffer::BufferUsage::shader_device_address
/// [`buffer_device_address`]: crate::device::Features::buffer_device_address
/// [`ext_buffer_device_address`]: crate::device::DeviceExtensions::ext_buffer_device_address
device_address = DEVICE_ADDRESS,
// TODO: implement
// device_address_capture_replay = DEVICE_ADDRESS_CAPTURE_REPLAY,
}
/// Error type returned by functions related to `DeviceMemory`.
#[derive(Clone, Debug, PartialEq, Eq)]
pub enum DeviceMemoryError {

View File

@ -95,7 +95,8 @@
pub use self::{
device_memory::{
DeviceMemory, DeviceMemoryError, ExternalMemoryHandleType, ExternalMemoryHandleTypes,
MappedDeviceMemory, MemoryAllocateInfo, MemoryImportInfo, MemoryMapError,
MappedDeviceMemory, MemoryAllocateFlags, MemoryAllocateInfo, MemoryImportInfo,
MemoryMapError,
},
pool::MemoryPool,
};