| 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: <20231021.203622.624978584179221727.fujita.tomonori@gmail.com>
Date: Sat, 21 Oct 2023 20:36:22 +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 11:21:12 +0000
Benno Lossin <benno.lossin@...ton.me> wrote:
> On 21.10.23 12:27, FUJITA Tomonori wrote:
>> On Sat, 21 Oct 2023 08:37:08 +0000
>> Benno Lossin <benno.lossin@...ton.me> wrote:
>>
>>> On 21.10.23 09:30, FUJITA Tomonori wrote:
>>>> On Sat, 21 Oct 2023 07:25:17 +0000
>>>> Benno Lossin <benno.lossin@...ton.me> wrote:
>>>>
>>>>> On 20.10.23 14:54, FUJITA Tomonori wrote:
>>>>>> On Fri, 20 Oct 2023 09:34:46 +0900 (JST)
>>>>>> FUJITA Tomonori <fujita.tomonori@...il.com> wrote:
>>>>>>
>>>>>>> On Thu, 19 Oct 2023 15:20:51 +0000
>>>>>>> Benno Lossin <benno.lossin@...ton.me> wrote:
>>>>>>>
>>>>>>>> I would like to remove the mutable static variable and simplify
>>>>>>>> the macro.
>>>>>>>
>>>>>>> How about adding DriverVTable array to Registration?
>>>>>>>
>>>>>>> /// Registration structure for a PHY driver.
>>>>>>> ///
>>>>>>> /// # Invariants
>>>>>>> ///
>>>>>>> /// The `drivers` slice are currently registered to the kernel via `phy_drivers_register`.
>>>>>>> pub struct Registration<const N: usize> {
>>>>>>> drivers: [DriverVTable; N],
>>>>>>> }
>>>>>>>
>>>>>>> impl<const N: usize> Registration<{ N }> {
>>>>>>> /// Registers a PHY driver.
>>>>>>> pub fn register(
>>>>>>> module: &'static crate::ThisModule,
>>>>>>> drivers: [DriverVTable; N],
>>>>>>> ) -> Result<Self> {
>>>>>>> let mut reg = Registration { drivers };
>>>>>>> let ptr = reg.drivers.as_mut_ptr().cast::<bindings::phy_driver>();
>>>>>>> // SAFETY: The type invariants of [`DriverVTable`] ensure that all elements of the `drivers` slice
>>>>>>> // are initialized properly. So an FFI call with a valid pointer.
>>>>>>> to_result(unsafe {
>>>>>>> bindings::phy_drivers_register(ptr, reg.drivers.len().try_into()?, module.0)
>>>>>>> })?;
>>>>>>> // INVARIANT: The `drivers` slice is successfully registered to the kernel via `phy_drivers_register`.
>>>>>>> Ok(reg)
>>>>>>> }
>>>>>>> }
>>>>>>
>>>>>> Scratch this.
>>>>>>
>>>>>> This doesn't work. Also simply putting slice of DriverVTable into
>>>>>> Module strcut doesn't work.
>>>>>
>>>>> Why does it not work? I tried it and it compiled fine for me.
>>>>
>>>> You can compile but the kernel crashes. The addresses of the callback
>>>> functions are invalid.
>>>
>>> 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.
Qemu supports a virtual PHY device?
>> I use the following macro:
>>
>> (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> {
>> let mut m = Module {
>> _drivers:[$(::kernel::net::phy::create_phy_driver::<$driver>()),+],
>> };
>> 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)
>> }
>> }
>>
(snip)
>> [ 615.365054] RIP: 0010:phy_check_link_status+0x28/0xd0
>> [ 615.365065] Code: 1f 00 0f 1f 44 00 00 55 48 89 e5 41 56 53 f6 87 dd 03 00 00 01 0f 85 ac 00 00 00 49 89 fe 48 8b 87 40 03 00 00 48 85 c0 74 13 <48> 8b 80 10 01 00 00 4c 89 f7 48 85 c0 74 0e ff d0 eb 0f bb fb ff
>> [ 615.365104] RSP: 0018:ffffc90000823de8 EFLAGS: 00010286
>> [ 615.365116] RAX: ffffc90000752d88 RBX: ffff8881023524e0 RCX: ffff888102e39980
>> [ 615.365130] RDX: ffff88846fbb18e8 RSI: 0000000000000000 RDI: ffff888102352000
>> [ 615.365144] RBP: ffffc90000823df8 R08: 8080808080808080 R09: fefefefefefefeff
>> [ 615.365157] R10: 0000000000000007 R11: 6666655f7265776f R12: ffff88846fbb18c0
>> [ 615.365171] R13: ffff888102b75000 R14: ffff888102352000 R15: ffff888102352000
>> [ 615.365185] FS: 0000000000000000(0000) GS:ffff88846fb80000(0000) knlGS:0000000000000000
>> [ 615.365210] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
>> [ 615.365222] CR2: ffffc90000752e98 CR3: 0000000111635000 CR4: 0000000000750ee0
>> [ 615.365237] PKRU: 55555554
>> [ 615.365247] note: kworker/14:1[147] exited with irqs disabled
>
> I think this is very weird, do you have any idea why this
> could happen?
DriverVtable is created on kernel stack, I guess.
> 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.
> (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?
Powered by blists - more mailing lists