| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <87a5k4omg5.fsf@metaspace.dk>
Date: Sat, 01 Jun 2024 14:51:38 +0200
From: Andreas Hindborg <nmi@...aspace.dk>
To: Benno Lossin <benno.lossin@...ton.me>
Cc: Jens Axboe <axboe@...nel.dk>, Christoph Hellwig <hch@....de>, Keith
Busch <kbusch@...nel.org>, Damien Le Moal <dlemoal@...nel.org>, Bart Van
Assche <bvanassche@....org>, Hannes Reinecke <hare@...e.de>, Ming Lei
<ming.lei@...hat.com>, "linux-block@...r.kernel.org"
<linux-block@...r.kernel.org>, Andreas Hindborg <a.hindborg@...sung.com>,
Wedson Almeida Filho <wedsonaf@...il.com>, Greg KH
<gregkh@...uxfoundation.org>, Matthew Wilcox <willy@...radead.org>,
Miguel Ojeda <ojeda@...nel.org>, Alex Gaynor <alex.gaynor@...il.com>,
Boqun Feng <boqun.feng@...il.com>, Gary Guo <gary@...yguo.net>,
Björn
Roy Baron <bjorn3_gh@...tonmail.com>, Alice Ryhl <aliceryhl@...gle.com>,
Chaitanya Kulkarni <chaitanyak@...dia.com>, Luis Chamberlain
<mcgrof@...nel.org>, Yexuan Yang <1182282462@...t.edu.cn>, Sergio
González Collado <sergio.collado@...il.com>, Joel
Granados
<j.granados@...sung.com>, "Pankaj Raghav (Samsung)"
<kernel@...kajraghav.com>, Daniel Gomez <da.gomez@...sung.com>, Niklas
Cassel <Niklas.Cassel@....com>, Philipp Stanner <pstanner@...hat.com>,
Conor Dooley <conor@...nel.org>, Johannes Thumshirn
<Johannes.Thumshirn@....com>, Matias Bjørling
<m@...rling.me>, open list
<linux-kernel@...r.kernel.org>, "rust-for-linux@...r.kernel.org"
<rust-for-linux@...r.kernel.org>, "lsf-pc@...ts.linux-foundation.org"
<lsf-pc@...ts.linux-foundation.org>, "gost.dev@...sung.com"
<gost.dev@...sung.com>
Subject: Re: [PATCH v3 1/3] rust: block: introduce `kernel::block::mq` module
Benno Lossin <benno.lossin@...ton.me> writes:
> On 01.06.24 13:11, Andreas Hindborg wrote:
>> Benno Lossin <benno.lossin@...ton.me> writes:
>>> On 01.06.24 10:18, Andreas Hindborg wrote:
>>>> + /// This function is called by the C kernel. A pointer to this function is
>>>> + /// installed in the `blk_mq_ops` vtable for the driver.
>>>> + ///
>>>> + /// # Safety
>>>> + ///
>>>> + /// This function may only be called by blk-mq C infrastructure.
>>>> + unsafe extern "C" fn commit_rqs_callback(_hctx: *mut bindings::blk_mq_hw_ctx) {
>>>> + T::commit_rqs()
>>>> + }
>>>> +
>>>> + /// This function is called by the C kernel. It is not currently
>>>> + /// implemented, and there is no way to exercise this code path.
>>>
>>> Is it also possible to completely remove it? ie use `None` in the
>>> VTABLE, or will the C side error?
>>
>> No, this pointer is not allowed to be null. It must be a callable
>> function, hence the stub. It will be populated soon enough when I send
>> patches for the remote completion logic.
>
> Makes sense.
>
> [...]
>
>>>> +impl<T: Operations> TagSet<T> {
>>>> + /// Try to create a new tag set
>>>> + pub fn try_new(
>>>> + nr_hw_queues: u32,
>>>> + num_tags: u32,
>>>> + num_maps: u32,
>>>> + ) -> impl PinInit<Self, error::Error> {
>>>> + try_pin_init!( TagSet {
>>>> + // INVARIANT: We initialize `inner` here and it is valid after the
>>>> + // initializer has run.
>>>> + inner <- unsafe {kernel::init::pin_init_from_closure(move |place: *mut Opaque<bindings::blk_mq_tag_set>| -> Result<()> {
>>>> + let place = place.cast::<bindings::blk_mq_tag_set>();
>>>> +
>>>> + // SAFETY: pin_init_from_closure promises that `place` is writable, and
>>>> + // zeroes is a valid bit pattern for this structure.
>>>> + core::ptr::write_bytes(place, 0, 1);
>>>> +
>>>> + /// For a raw pointer to a struct, write a struct field without
>>>> + /// creating a reference to the field
>>>> + macro_rules! write_ptr_field {
>>>> + ($target:ident, $field:ident, $value:expr) => {
>>>> + ::core::ptr::write(::core::ptr::addr_of_mut!((*$target).$field), $value)
>>>> + };
>>>> + }
>>>> +
>>>> + // SAFETY: pin_init_from_closure promises that `place` is writable
>>>> + write_ptr_field!(place, ops, OperationsVTable::<T>::build());
>>>> + write_ptr_field!(place, nr_hw_queues , nr_hw_queues);
>>>> + write_ptr_field!(place, timeout , 0); // 0 means default which is 30 * HZ in C
>>>> + write_ptr_field!(place, numa_node , bindings::NUMA_NO_NODE);
>>>> + write_ptr_field!(place, queue_depth , num_tags);
>>>> + write_ptr_field!(place, cmd_size , core::mem::size_of::<RequestDataWrapper>().try_into()?);
>>>> + write_ptr_field!(place, flags , bindings::BLK_MQ_F_SHOULD_MERGE);
>>>> + write_ptr_field!(place, driver_data , core::ptr::null_mut::<core::ffi::c_void>());
>>>> + write_ptr_field!(place, nr_maps , num_maps);
>>>
>>> Did something not work with my suggestion?
>>
>> I did not want to change it if we are rewriting it to `Opaque::init`
>> in a cycle or two, which I think is a better solution.
>
> Ah I was suggesting to do it now, but emulate the `Opaque::init`
> function (I should have been clearer about that).
> I tried to do exactly that, but failed to easily implement it, since the
> fields of `blk_mq_tag_set` include some structs, which of course do not
> implement `Zeroable`.
>
> Instead I came up with the following solution, which I find a lot nicer:
>
> pub fn try_new(
> nr_hw_queues: u32,
> num_tags: u32,
> num_maps: u32,
> ) -> impl PinInit<Self, error::Error> {
> // SAFETY: `blk_mq_tag_set` only contains integers and pointers, which all are allowed to be 0.
> let tag_set: bindings::blk_mq_tag_set = unsafe { core::mem::zeroed() };
> let tag_set = bindings::blk_mq_tag_set {
> ops: OperationsVTable::<T>::build(),
> nr_hw_queues,
> timeout: 0, // 0 means default which is 30Hz in C
> numa_node: bindings::NUMA_NO_NODE,
> queue_depth: num_tags,
> cmd_size: core::mem::size_of::<RequestDataWrapper>().try_into()?,
> flags: bindings::BLK_MQ_F_SHOULD_MERGE,
> driver_data: core::ptr::null_mut::<core::ffi::c_void>(),
> nr_maps: num_maps,
> ..tag_set
> };
> try_pin_init!(TagSet {
> inner <- PinInit::<_, error::Error>::pin_chain(Opaque::new(tag_set), |tag_set| {
> // SAFETY: we do not move out of `tag_set`.
> let tag_set = unsafe { Pin::get_unchecked_mut(tag_set) };
> // SAFETY: `tag_set` is a reference to an initialized `blk_mq_tag_set`.
> error::to_result(unsafe { bindings::blk_mq_alloc_tag_set(tag_set) })
> }),
> _p: PhantomData,
> })
> }
>
> I haven't tested it though, let me know if you have any problems.
I'll give it a try!
BR Andreas
Powered by blists - more mailing lists