netdev - Re: [PATCH net-next v5 1/5] rust: core abstractions for network PHY drivers

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-Id: <20231021.220012.2089903288409349337.fujita.tomonori@gmail.com>
Date: Sat, 21 Oct 2023 22:00:12 +0900 (JST)
From: FUJITA Tomonori <fujita.tomonori@...il.com>
To: benno.lossin@...ton.me
Cc: fujita.tomonori@...il.com, netdev@...r.kernel.org,
 rust-for-linux@...r.kernel.org, andrew@...n.ch,
 miguel.ojeda.sandonis@...il.com, tmgross@...ch.edu, boqun.feng@...il.com,
 wedsonaf@...il.com, greg@...ah.com
Subject: Re: [PATCH net-next v5 1/5] rust: core abstractions for network
 PHY drivers

On Sat, 21 Oct 2023 12:50:10 +0000
Benno Lossin <benno.lossin@...ton.me> wrote:

> On 21.10.23 14:38, FUJITA Tomonori wrote:
>> On Sat, 21 Oct 2023 12:13:32 +0000
>> Benno Lossin <benno.lossin@...ton.me> wrote:
>> 
>>>>>>> Can you please share your setup and the error? For me it booted
>>>>>>> fine.
>>>>>>
>>>>>> You use ASIX PHY hardware?
>>>>>
>>>>> It seems I have configured something wrong. Can you share your testing
>>>>> setup? Do you use a virtual PHY device in qemu, or do you boot it from
>>>>> real hardware with a real ASIX PHY device?
>>>>
>>>> real hardware with real ASIX PHY device.
>>>
>>> I see.
>>>
>>>> Qemu supports a virtual PHY device?
>>>
>>> I have no idea.
>> 
>> When I had a look at Qemu several months ago, it didn't support such.
>> 
>>> [...]
>>>
>>>>> I think this is very weird, do you have any idea why this
>>>>> could happen?
>>>>
>>>> DriverVtable is created on kernel stack, I guess.
>>>
>>> But how does that invalidate the function pointers?
>> 
>> Not only funciton pointers. You can't store something on stack for
>> later use.
> 
> It is not stored on the stack, it is only created on the stack and
> moved to a global static later on. The `module!` macro creates a
> `static mut __MOD: Option<Module>` where the module data is stored in.

I know. The problem is that we call phy_drivers_register() with
DriverVTable on stack. Then it was moved.


> It seems that constructing the driver table not at that location
> is somehow interfering with something?
> 
> Wedson has a patch [1] to create in-place initialized modules, but
> it probably is not completely finished, as he has not yet begun to
> post it to the list. But I am sure that it is mature enough for
> you to test this hypothesis.
> 
> [1]: https://github.com/wedsonaf/linux/commit/484ec70025ff9887d9ca228ec631264039cee355
> 
> -- 
> Cheers,
> Benno
> 
>>>>> If you don't mind, could you try if the following changes
>>>>> anything?
>>>>
>>>> I don't think it works. If you use const for DriverTable, DriverTable
>>>> is placed on read-only pages. The C side modifies DriverVTable array
>>>> so it does't work.
>>>
>>> Did you try it? Note that I copy the `DriverVTable` into the Module
>>> struct, so it will not be placed on a read-only page.
>> 
>> Ah, I misunderstood code. It doesn't work. DriverVTable on stack.
>> 
>> 
>>>>>        (drivers: [$($driver:ident),+], device_table: [$($dev:expr),+], $($f:tt)*) => {
>>>>>            const N: usize = $crate::module_phy_driver!(@count_devices $($driver),+);
>>>>>            struct Module {
>>>>>                _drivers: [::kernel::net::phy::DriverVTable; N],
>>>>>            }
>>>>>
>>>>>            $crate::prelude::module! {
>>>>>                type: Module,
>>>>>                $($f)*
>>>>>            }
>>>>>
>>>>>            unsafe impl Sync for Module {}
>>>>>
>>>>>            impl ::kernel::Module for Module {
>>>>>                fn init(module: &'static ThisModule) -> Result<Self> {
>>>>> 		const DRIVERS: [::kernel::net::phy::DriverVTable; N] = [$(::kernel::net::phy::create_phy_driver::<$driver>()),+];
>>>>>                    let mut m = Module {
>>>>>                        _drivers: unsafe { core::ptr::read(&DRIVERS) },
>>>>>                    };
>>>>>                    let ptr = m._drivers.as_mut_ptr().cast::<::kernel::bindings::phy_driver>();
>>>>>                    ::kernel::error::to_result(unsafe {
>>>>>                        kernel::bindings::phy_drivers_register(ptr, m._drivers.len().try_into()?, module.as_ptr())
>>>>>                    })?;
>>>>>                    Ok(m)
>>>>>                }
>>>>>            }
>>>>>
>>>>> and also the variation where you replace `const DRIVERS` with
>>>>> `static DRIVERS`.
>>>>
>>>> Probably works. But looks like similar with the current code? This is
>>>> simpler?
>>>
>>> Just curious if it has to do with using `static` vs `const`.
>> 
>> static doesn't work too due to the same reason.
> 
> 
>