linux-kernel - Re: [PATCH v3 3/7] rust: property: Introduce PropertyGuard

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: <ee888c8f-4802-48a1-bd08-b454b782fff4@de.bosch.com>
Date: Mon, 28 Apr 2025 07:03:07 +0200
From: Dirk Behme <dirk.behme@...bosch.com>
To: Danilo Krummrich <dakr@...nel.org>, Dirk Behme <dirk.behme@...il.com>
CC: Remo Senekowitsch <remo@...nzli.dev>, Rob Herring <robh@...nel.org>,
	Saravana Kannan <saravanak@...gle.com>, 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>, Benno Lossin <benno.lossin@...ton.me>, "Andreas
 Hindborg" <a.hindborg@...nel.org>, Alice Ryhl <aliceryhl@...gle.com>, "Trevor
 Gross" <tmgross@...ch.edu>, Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	"Rafael J. Wysocki" <rafael@...nel.org>, <linux-kernel@...r.kernel.org>,
	<devicetree@...r.kernel.org>, <rust-for-linux@...r.kernel.org>
Subject: Re: [PATCH v3 3/7] rust: property: Introduce PropertyGuard

On 27/04/2025 14:23, Danilo Krummrich wrote:
> On Sun, Apr 27, 2025 at 08:11:58AM +0200, Dirk Behme wrote:
>> On 26.04.25 12:15, Danilo Krummrich wrote:
>>> On Sat, Apr 26, 2025 at 08:19:09AM +0200, Dirk Behme wrote:
>>>> On 25.04.25 17:35, Danilo Krummrich wrote:
>>>>> On Fri, Apr 25, 2025 at 05:01:26PM +0200, Remo Senekowitsch wrote:
>>>>>> +impl<T> PropertyGuard<'_, '_, T> {
>>>>>> +    /// Access the property, indicating it is required.
>>>>>> +    ///
>>>>>> +    /// If the property is not present, the error is automatically logged. If a
>>>>>> +    /// missing property is not an error, use [`Self::optional`] instead.
>>>>>> +    pub fn required(self) -> Result<T> {
>>>>>> +        if self.inner.is_err() {
>>>>>> +            pr_err!(
>>>>>> +                "{}: property '{}' is missing\n",
>>>>>> +                self.fwnode.display_path(),
>>>>>> +                self.name
>>>>>> +            );
>>>>>
>>>>> Hm, we can't use the device pointer of the fwnode_handle, since it is not
>>>>> guaranteed to be valid, hence the pr_*() print...
>>>>>
>>>>> Anyways, I'm not sure we need to print here at all. If a driver wants to print
>>>>> that it is unhappy about a missing required property it can do so by itself, I
>>>>> think.
>>>>
>>>> Hmm, the driver said by using 'required' that it *is* required. So a
>>>> missing property is definitely an error here. Else it would have used
>>>> 'optional'. Which doesn't print in case the property is missing.
>>>>
>>>> If I remember correctly having 'required' and 'optional' is the result
>>>> of some discussion on Zulip. And one conclusion of that discussion was
>>>> to move checking & printing the error out of the individual drivers
>>>> into a central place to avoid this error checking & printing in each
>>>> and every driver. I think the idea is that the drivers just have to do
>>>> ...required()?; and that's it, then.
>>>
>>> Yes, I get the idea.
>>>
>>> If it'd be possible to use dev_err!() instead I wouldn't object in this specific
>>> case. But this code is used by drivers from probe(), hence printing the error
>>> without saying for which device it did occur is a bit pointless.
>>
>> Thinking a little about this, yes, we don't know the device here. But:
>> Does the device matter here?
> 
> If the above fails it means that for a (specific) device a driver expects that
> a specific property of some firmware node is present. So, yes, I think it does
> matter.
> 
>> There is nothing wrong with the (unknown)
>> device, no? What is wrong here is the firmware (node). It misses
>> something.
> 
> How do we know the firmware node is wrong? Maybe the driver has wrong
> expectations for this device?
> 
>> And this is exactly what the message tells: "There is an
>> error due to the missing node 'name' in 'path', please fix it". That
>> should be sufficient to identify the firmware/device tree description
>> and fix it.
> 
> I think we can't always fix them, even if they're wrong. How do we fix ACPI
> firmware nodes for instance?

So the argument here is that the device (driver) is expecting something
to be "required" is wrong and might need to be fixed. Not the firmware.
Yes, ok, that is a valid argument. I have a device tree background and
there in 99% of the cases the device tree needs a fix ;)

But let me ask the other way around, then: What will it hurt or break if
we keep the pr_err() like Remo did? Even knowing that its not perfect?
But knowing that it will give at least a note that something is wrong
with at least a starting point for searching what needs to be fixed. I
mean even if we don't get the device, we will get the affected node we
can search for which device uses it as "required".

Could we somehow agree that in 90% of the cases this should be catched
at device (driver) development time, already? And therefore it should be
beneficial if we don't require each and every driver to be "bloated"
with checking this individually?

Dirk