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: <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

Powered by Openwall GNU/*/Linux Powered by OpenVZ