| 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: <9030e7e9-7255-497e-be4c-5bba3a373a54@icloud.com>
Date: Sat, 24 Aug 2024 06:18:56 +0800
From: Zijun Hu <zijun_hu@...oud.com>
To: Ira Weiny <ira.weiny@...el.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>,
Dan Williams <dan.j.williams@...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>
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>
Subject: Re: [PATCH v2 2/4] cxl/region: Prevent device_find_child() from
modifying caller's match data
On 2024/8/24 02:10, Ira Weiny wrote:
> Zijun Hu wrote:
>> On 2024/8/20 21:59, Ira Weiny wrote:
>>> Zijun Hu wrote:
>>>> From: Zijun Hu <quic_zijuhu@...cinc.com>
>>>>
>>>> To prepare for constifying the following old driver core API:
>>>>
>>>> struct device *device_find_child(struct device *dev, void *data,
>>>> int (*match)(struct device *dev, void *data));
>>>> to new:
>>>> struct device *device_find_child(struct device *dev, const void *data,
>>>> int (*match)(struct device *dev, const void *data));
>>>>
>>>> The new API does not allow its match function (*match)() to modify
>>>> caller's match data @*data, but match_free_decoder() as the old API's
>>>> match function indeed modifies relevant match data, so it is not
>>>> suitable for the new API any more, fixed by implementing a equivalent
>>>> cxl_device_find_child() instead of the old API usage.
>>>
>>> Generally it seems ok but I think some name changes will make this more
>>> clear. See below.
>>>
>>
>> okay.
>>
>>> Also for those working on CXL I'm questioning the use of ID here and the
>>> dependence on the id's being added to the parent in order. Is that a
>>> guarantee?
>>>
>>>>
>>>> Signed-off-by: Zijun Hu <quic_zijuhu@...cinc.com>
>>>> ---
>>>> drivers/cxl/core/region.c | 36 +++++++++++++++++++++++++++++++++++-
>>>> 1 file changed, 35 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
>>>> index 21ad5f242875..8d8f0637f7ac 100644
>>>> --- a/drivers/cxl/core/region.c
>>>> +++ b/drivers/cxl/core/region.c
>>>> @@ -134,6 +134,39 @@ static const struct attribute_group *get_cxl_region_access1_group(void)
>>>> return &cxl_region_access1_coordinate_group;
>>>> }
>>>>
>>>> +struct cxl_dfc_data {
>>>
>>> struct cxld_match_data
>>>
>>> 'cxld' == cxl decoder in our world.
>>>
>>
>> make sense.
>>
>>>> + int (*match)(struct device *dev, void *data);
>>>> + void *data;
>>>> + struct device *target_device;
>>>> +};
>>>> +
>>>> +static int cxl_dfc_match_modify(struct device *dev, void *data)
>>>
>>> Why not just put this logic into match_free_decoder?
>>>
>>
>> Actually, i ever considered solution B as you suggested in the end.
>>
>> For this change, namely, solution A:
>> 1) this change is clearer and easier to understand.
>> 2) this change does not touch any existing cxld logic
>>
>> For solution B:
>> it is more reasonable
>>
>> i finally select A since it can express my concern and relevant solution
>> clearly.
>
> Understood.
>
>>
>>>> +{
>>>> + struct cxl_dfc_data *dfc_data = data;
>>>> + int res;
>>>> +
>>>> + res = dfc_data->match(dev, dfc_data->data);
>>>> + if (res && get_device(dev)) {
>>>> + dfc_data->target_device = dev;
>>>> + return res;
>>>> + }
>>>> +
>>>> + return 0;
>>>> +}
>>>> +
>>>> +/*
>>>> + * I have the same function as device_find_child() but allow to modify
>>>> + * caller's match data @*data.
>>>> + */
>>>
>>> No need for this comment after the new API is established.
>>>
>>
>> i have given up the idea within v1 to introduce a new API which *should
>> ONLY* be used by this patch series, so it is not worthy of a new API
>> even if it can bring convenient for this patch series.
>
> I'm not clear on this. Are you still proposing to change the parameter to
> const?
>
yes.
>>
>>>> +static struct device *cxl_device_find_child(struct device *parent, void *data,
>>>> + int (*match)(struct device *dev, void *data))
>>>> +{
>>>> + struct cxl_dfc_data dfc_data = {match, data, NULL};
>>>> +
>>>> + device_for_each_child(parent, &dfc_data, cxl_dfc_match_modify);
>>>> + return dfc_data.target_device;
>>>> +}
>>>> +
>>>> static ssize_t uuid_show(struct device *dev, struct device_attribute *attr,
>>>> char *buf)
>>>> {
>>>> @@ -849,7 +882,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 = cxl_device_find_child(&port->dev, &id,
>>>> + match_free_decoder);
>>>
>>> This is too literal. How about the following (passes basic cxl-tests).
>>>
>>
>> it is reasonable.
>>
>> do you need me to submit that you suggest in the end and add you as
>> co-developer ?
>
> You can submit it with Suggested-by:
>
okay.
>>
>> OR
>>
>> you submit it by yourself ?
>>
>> either is okay for me.
>>
>>> Ira
>>>
>>> diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
>>> index 21ad5f242875..c1e46254efb8 100644
>>> --- a/drivers/cxl/core/region.c
>>> +++ b/drivers/cxl/core/region.c
>>> @@ -794,10 +794,15 @@ static size_t show_targetN(struct cxl_region *cxlr, char *buf, int pos)
>>> return rc;
>>> }
>>>
>>> +struct cxld_match_data {
>>> + int id;
>>> + struct device *target_device;
>>> +};
>>> +
>>> static int match_free_decoder(struct device *dev, void *data)
>>> {
>>> + struct cxld_match_data *match_data = data;
>>> struct cxl_decoder *cxld;
>>> - int *id = data;
>>>
>>> if (!is_switch_decoder(dev))
>>> return 0;
>>> @@ -805,17 +810,30 @@ static int match_free_decoder(struct device *dev, void *data)
>>> cxld = to_cxl_decoder(dev);
>>>
>>> /* enforce ordered allocation */
>>> - if (cxld->id != *id)
>>> + if (cxld->id != match_data->id)
>>> return 0;
>>>
>>> - if (!cxld->region)
>>> + if (!cxld->region && get_device(dev)) {
>>
>> get_device(dev) failure may cause different logic against existing
>> but i think it should be impossible to happen normally.
>
> Indeed this is slightly different. :-/
>
> Move the get_device() to find_free_decoder()?
>
i think we can keep your change. so ignore this slight difference.
i also notice that you have done some verification for this change.
> Ira
>
>>
>>> + match_data->target_device = dev;
>>> return 1;
>>> + }
>>>
>>> - (*id)++;
>>> + match_data->id++;
>>>
>>> return 0;
>>> }
>>>
>>> +static struct device *find_free_decoder(struct device *parent)
>>> +{
>>> + struct cxld_match_data match_data = {
>>> + .id = 0,
>>> + .target_device = NULL,
>>> + };
>>> +
>>> + device_for_each_child(parent, &match_data, match_free_decoder);
>>> + return match_data.target_device;
>>> +}
>>> +
>
> [snip]
Powered by blists - more mailing lists