| Message ID | 20240811-const_dfc_prepare-v1-2-d67cc416b3d3@quicinc.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | driver core: Prevent device_find_child() from modifying caller's match data | expand |
On Sun, Aug 11, 2024 at 08:18:08AM +0800, Zijun Hu wrote: > From: Zijun Hu <quic_zijuhu@quicinc.com> > > Introduce constify_device_find_child_helper() to replace existing > device_find_child()'s usages whose match functions will modify > caller's match data. Ick, that's not a good name, it should be "noun_verb" with the subsystem being on the prefix always. But why is this even needed? Device pointers are NOT const for the obvious reason that they can be changed by loads of different things. Trying to force them to be const is going to be hard, if not impossible. thanks, greg k-h
On 8/13/2024 5:45 PM, Greg Kroah-Hartman wrote: > On Sun, Aug 11, 2024 at 08:18:08AM +0800, Zijun Hu wrote: >> From: Zijun Hu <quic_zijuhu@quicinc.com> >> >> Introduce constify_device_find_child_helper() to replace existing >> device_find_child()'s usages whose match functions will modify >> caller's match data. > > Ick, that's not a good name, it should be "noun_verb" with the subsystem being on the prefix always. > okay, got it. is it okay to use device_find_child_mut() suggested by Przemek Kitszel ? > But why is this even needed? Device pointers are NOT const for the > obvious reason that they can be changed by loads of different things. > Trying to force them to be const is going to be hard, if not impossible. > [PATCH 3/5] have more discussion about these questions with below link: https://lore.kernel.org/all/8b8ce122-f16b-4207-b03b-f74b15756ae7@icloud.com/ 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 the The ultimate goal is to protect caller's *match data* @*data NOT device @*dev. > thanks, > > greg k-h
On Tue, Aug 13, 2024 at 06:50:04PM +0800, quic_zijuhu wrote: > On 8/13/2024 5:45 PM, Greg Kroah-Hartman wrote: > > On Sun, Aug 11, 2024 at 08:18:08AM +0800, Zijun Hu wrote: > >> From: Zijun Hu <quic_zijuhu@quicinc.com> > >> > >> Introduce constify_device_find_child_helper() to replace existing > >> device_find_child()'s usages whose match functions will modify > >> caller's match data. > > > > Ick, that's not a good name, it should be "noun_verb" with the subsystem being on the prefix always. > > > okay, got it. > > is it okay to use device_find_child_mut() suggested by Przemek Kitszel ? No, just switch all callers over to be const and keep the same name. > > But why is this even needed? Device pointers are NOT const for the > > obvious reason that they can be changed by loads of different things. > > Trying to force them to be const is going to be hard, if not impossible. > > > > [PATCH 3/5] have more discussion about these questions with below link: > https://lore.kernel.org/all/8b8ce122-f16b-4207-b03b-f74b15756ae7@icloud.com/ > > > 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 the The ultimate goal is to protect caller's *match data* @*data NOT > device @*dev. Ok, sorry, I was confused. thanks, greg k-h
On 8/13/2024 6:57 PM, Greg Kroah-Hartman wrote: > On Tue, Aug 13, 2024 at 06:50:04PM +0800, quic_zijuhu wrote: >> On 8/13/2024 5:45 PM, Greg Kroah-Hartman wrote: >>> On Sun, Aug 11, 2024 at 08:18:08AM +0800, Zijun Hu wrote: >>>> From: Zijun Hu <quic_zijuhu@quicinc.com> >>>> >>>> Introduce constify_device_find_child_helper() to replace existing >>>> device_find_child()'s usages whose match functions will modify >>>> caller's match data. >>> >>> Ick, that's not a good name, it should be "noun_verb" with the subsystem being on the prefix always. >>> >> okay, got it. >> >> is it okay to use device_find_child_mut() suggested by Przemek Kitszel ? > > No, just switch all callers over to be const and keep the same name. > >>> But why is this even needed? Device pointers are NOT const for the >>> obvious reason that they can be changed by loads of different things. >>> Trying to force them to be const is going to be hard, if not impossible. >>> >> >> [PATCH 3/5] have more discussion about these questions with below link: >> https://lore.kernel.org/all/8b8ce122-f16b-4207-b03b-f74b15756ae7@icloud.com/ >> >> >> 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 the The ultimate goal is to protect caller's *match data* @*data NOT >> device @*dev. > > Ok, sorry, I was confused. > Current prototype of the API: struct device *device_find_child(struct device *dev, void *data, int (*match)(struct device *dev, void *data)); prototype we want: struct device *device_find_child(struct device *dev, const void *data, int (*match)(struct device *dev, const void *data)); The only differences are shown below: void *data -> const void *data // as argument of paramter @data of (*match)(). int (*match)(struct device *dev, void *data) -> int (*match)(struct device *dev, const void *data). We don't change type of @dev. we just make above two parameters have the same types as below existing finding APIs. struct device *bus_find_device(const struct bus_type *bus, struct device *start, const void *data, int (*match)(struct device *dev, const void *data)); struct device *driver_find_device(const struct device_driver *drv, struct device *start, const void *data, int (*match)(struct device *dev, const void *data)); struct device *class_find_device(const struct class *class, const struct device *start, const void *data, int (*match)(struct device *, const void *)); > thanks, > > greg k-h
diff --git a/drivers/base/core.c b/drivers/base/core.c index b1dd8c5590dc..3f3ebdb5aa0b 100644 --- a/drivers/base/core.c +++ b/drivers/base/core.c @@ -4128,6 +4128,41 @@ struct device *device_find_any_child(struct device *parent) } EXPORT_SYMBOL_GPL(device_find_any_child); +struct fn_data_struct { + int (*match)(struct device *dev, void *data); + void *data; + struct device *target_device; +}; + +static int constify_device_match_fn(struct device *dev, void *data) +{ + struct fn_data_struct *fn_data = data; + int res; + + res = fn_data->match(dev, fn_data->data); + if (res && get_device(dev)) { + fn_data->target_device = dev; + return res; + } + + return 0; +} + +/* + * My mission is to clean up existing match functions which will modify + * caller's match data for device_find_child(), so i do not introduce + * myself here to prevent that i am used for any other purpose. + */ +struct device *constify_device_find_child_helper(struct device *parent, void *data, + int (*match)(struct device *dev, void *data)) +{ + struct fn_data_struct fn_data = {match, data, NULL}; + + device_for_each_child(parent, &fn_data, constify_device_match_fn); + return fn_data.target_device; +} +EXPORT_SYMBOL_GPL(constify_device_find_child_helper); + int __init devices_init(void) { devices_kset = kset_create_and_add("devices", &device_uevent_ops, NULL); diff --git a/include/linux/device.h b/include/linux/device.h index 34eb20f5966f..b2423fca3d45 100644 --- a/include/linux/device.h +++ b/include/linux/device.h @@ -1078,6 +1078,13 @@ struct device *device_find_child(struct device *dev, void *data, struct device *device_find_child_by_name(struct device *parent, const char *name); struct device *device_find_any_child(struct device *parent); +/* + * My mission is to clean up existing match functions which will modify + * caller's match data for device_find_child(), so please DO NOT use me + * for any other purpose. + */ +struct device *constify_device_find_child_helper(struct device *parent, void *data, + int (*match)(struct device *dev, void *data)); int device_rename(struct device *dev, const char *new_name); int device_move(struct device *dev, struct device *new_parent,