| 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: <66c8d0a7eddc5_a87cd294e1@iweiny-mobl.notmuch>
Date: Fri, 23 Aug 2024 13:10:47 -0500
From: Ira Weiny <ira.weiny@...el.com>
To: Zijun Hu <zijun_hu@...oud.com>, 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
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?
>
> >> +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:
>
> 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()?
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