| 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: <66c4a136d9764_2ddc2429435@iweiny-mobl.notmuch>
Date: Tue, 20 Aug 2024 08:59:18 -0500
From: Ira Weiny <ira.weiny@...el.com>
To: Zijun Hu <zijun_hu@...oud.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>, 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: Zijun Hu <zijun_hu@...oud.com>, <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
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.
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.
> + 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?
> +{
> + 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.
> +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).
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)) {
+ 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;
+}
+
static int match_auto_decoder(struct device *dev, void *data)
{
struct cxl_region_params *p = data;
@@ -840,7 +858,6 @@ cxl_region_find_decoder(struct cxl_port *port,
struct cxl_region *cxlr)
{
struct device *dev;
- int id = 0;
if (port == cxled_to_port(cxled))
return &cxled->cxld;
@@ -849,7 +866,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 = find_free_decoder(&port->dev);
+
if (!dev)
return NULL;
/*
Powered by blists - more mailing lists