netdev - Re: [PATCH net-next v3 1/3] 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: <c807ef34-3926-4284-ab18-b516c8af57c7@proton.me>
Date: Sat, 14 Oct 2023 22:18:58 +0000
From: Benno Lossin <benno.lossin@...ton.me>
To: Andrew Lunn <andrew@...n.ch>
Cc: FUJITA Tomonori <fujita.tomonori@...il.com>, boqun.feng@...il.com, tmgross@...ch.edu, netdev@...r.kernel.org, rust-for-linux@...r.kernel.org, miguel.ojeda.sandonis@...il.com, greg@...ah.com
Subject: Re: [PATCH net-next v3 1/3] rust: core abstractions for network PHY drivers

On 14.10.23 23:55, Andrew Lunn wrote:
>>> The PHY driver asks the state from the abstractions then the
>>> abstractions ask the state from PHYLIB. So when the abstractions get a
>>> bad value from PHYLIB, the abstractions must return something to the
>>> PHY driver. As I wrote, the abstractions return a random value or an
>>> error. In either way, probably the system cannot continue.
>>
>> Sure then let the system BUG if it cannot continue. I think that
>> allowing UB is worse than BUGing.
> 
> There is nothing a PHY driver can do which justifies calling BUG().

I was mostly replying to Tomonori's comment "In either way, probably
the system cannot continue.". I think that defaulting the value to
`PHY_ERROR` when it is out of range to be a lot better way of handling
this.

> BUG() indicates the system is totally messed up, and running further
> is going to result in more file system corruption, causing more data
> loss, so we need to stop the machine immediately.
> 
> Anyway, we are talking about this bit of code in the C driver:
> 
>          /* Reset PHY, otherwise MII_LPA will provide outdated information.
>           * This issue is reproducible only with some link partner PHYs
>           */
>          if (phydev->state == PHY_NOLINK) {
>                  phy_init_hw(phydev);
>                  _phy_start_aneg(phydev);
>          }
> 

I think that we are talking about `Device::state` i.e. the
Rust wrapper function of `phydev->state`.

> and what we should do if phydev->state is not one of the values
> defined in enum phy_state, but is actually 42. The system will
> continue, but it could be that the hardware reports the wrong value
> for LPA, the Link Partner Advertisement. That is not critical
> information, the link is likely to work, but the debug tool ethtool(1)
> will report the wrong value.
> 
> Can we turn this UB into DB? I guess you can make the abstraction
> accept any value, validate it against the values in enum phy_state, do
> a WARN_ON_ONCE() if its not valid so we get a stack trace, and pass
> the value on. Life will likely continue, hopefully somebody will
> report the stack trace, and we can try to figure out what went wrong.

Then we are on the same page, as that would be my preferred solution.

-- 
Cheers,
Benno