linux-kernel - Re: [PATCH 3/5] cxl/region: Prevent device_find_child() from modifying caller's match data

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: <8b8ce122-f16b-4207-b03b-f74b15756ae7@icloud.com>
Date: Mon, 12 Aug 2024 23:32:44 +0800
From: Zijun Hu <zijun_hu@...oud.com>
To: Przemek Kitszel <przemyslaw.kitszel@...el.com>,
 Dan Williams <dan.j.williams@...el.com>
Cc: linux-kernel@...r.kernel.org, linux-cxl@...r.kernel.org,
 linux1394-devel@...ts.sourceforge.net, netdev@...r.kernel.org,
 Zijun Hu <quic_zijuhu@...cinc.com>,
 Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
 "Rafael J. Wysocki" <rafael@...nel.org>, Davidlohr Bueso
 <dave@...olabs.net>, Jonathan Cameron <jonathan.cameron@...wei.com>,
 Dave Jiang <dave.jiang@...el.com>,
 Alison Schofield <alison.schofield@...el.com>,
 Vishal Verma <vishal.l.verma@...el.com>, Ira Weiny <ira.weiny@...el.com>,
 Takashi Sakamoto <o-takashi@...amocchi.jp>, Timur Tabi <timur@...nel.org>,
 "David S. Miller" <davem@...emloft.net>, Eric Dumazet <edumazet@...gle.com>,
 Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>
Subject: Re: [PATCH 3/5] cxl/region: Prevent device_find_child() from
 modifying caller's match data

On 2024/8/12 20:54, Przemek Kitszel wrote:
> On 8/11/24 02:18, Zijun Hu wrote:
>> From: Zijun Hu <quic_zijuhu@...cinc.com>
>>
>> It does not make sense for match_free_decoder() as device_find_child()'s
>> match function to modify caller's match data, 
> 
> match_free_decoder() is just doing that, treating caller's match data as
> a piece of memory to store their int.
> (So it is hard to tell it does not make sense "for [it] ... to").
> 

Thanks for reply (^^)

The ultimate goal is to make device_find_child() have below prototype:

struct device *device_find_child(struct device *dev, const void *data,
		int (*match)(struct device *dev, const void *data));

Why ?

(1) It does not make sense, also does not need to, for such device
finding operation to modify caller's match data which is mainly
used for comparison.

(2) It will make the API's match function parameter have the same
signature as all other APIs (bus|class|driver)_find_device().

My idea is that:
use device_find_child() for READ only accessing caller's match data.

use below API if need to Modify caller's data as
constify_device_find_child_helper() does.
int device_for_each_child(struct device *dev, void *data,
                    int (*fn)(struct device *dev, void *data));

So match_free_decoder() is not proper as device_find_child()'s
match function.

>> fixed by using
>> constify_device_find_child_helper() instead of device_find_child().
> 
> I don't like the constify... name, I would go with something like
> device_find_child_mut() or similar.
> 

What about below alternative option i ever thought about ?

Don't introduce API constify_device_find_child_helper() at all, and
change in involved driver such as cxl/core/region.c directly.

>>
>> Signed-off-by: Zijun Hu <quic_zijuhu@...cinc.com>
>> ---
>>   drivers/cxl/core/region.c | 3 ++-
>>   1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
>> index 21ad5f242875..266231d69dff 100644
>> --- a/drivers/cxl/core/region.c
>> +++ b/drivers/cxl/core/region.c
>> @@ -849,7 +849,8 @@ cxl_region_find_decoder(struct cxl_port *port,
>>           dev = device_find_child(&port->dev, &cxlr->params,
>>                       match_auto_decoder);
>>       else
>> -        dev = device_find_child(&port->dev, &id, match_free_decoder);
>> +        dev = constify_device_find_child_helper(&port->dev, &id,
>> +                            match_free_decoder);
>>       if (!dev)
>>           return NULL;
>>       /*
>>
>