Hello, We have addressed the concurrency issue in the match driver interface at a higher level, as detailed in the [https://lore.kernel.org/all/20241112163041.40083-1-chenqiuji666@xxxxxxxxx/]. Due to the widespread nature of the issue, it is more appropriate to resolve it by adding a lock at the higher level. If this patch is merged with the lock added at the lower level, it could potentially lead to a deadlock issue. Therefore, I kindly ask that you do not merge this patch. I sincerely apologize for the inconvenience caused and thank you for your understanding. Regards, Qiu-ji Chen On Fri, Oct 18, 2024 at 5:39 PM Suzuki K Poulose <suzuki.poulose@xxxxxxx> wrote: > > On 18/10/2024 09:15, Qiu-ji Chen wrote: > > Atomicity violation occurs during consecutive reads of > > pcdev->driver_override. Consider a scenario: after pvdev->driver_override > > passes the if statement, due to possible concurrency, > > pvdev->driver_override may change. This leads to pvdev->driver_override > > passing the condition with an old value, but entering the > > return !strcmp(pcdev->driver_override, drv->name); statement with a new > > value. This causes the function to return an unexpected result. > > Since pvdev->driver_override is a string that is modified byte by byte, > > without considering atomicity, data races may cause a partially modified > > pvdev->driver_override to enter both the condition and return statements, > > resulting in an error. > > > > To fix this, we suggest protecting all reads of pvdev->driver_override > > with a lock, and storing the result of the strcmp() function in a new > > variable retval. This ensures that pvdev->driver_override does not change > > during the entire operation, allowing the function to return the expected > > result. > > > > This possible bug is found by an experimental static analysis tool > > developed by our team. This tool analyzes the locking APIs > > to extract function pairs that can be concurrently executed, and then > > analyzes the instructions in the paired functions to identify possible > > concurrency bugs including data races and atomicity violations. > > > > Fixes: 5150a8f07f6c ("amba: reorder functions") > > Cc: stable@xxxxxxxxxxxxxxx > > Signed-off-by: Qiu-ji Chen <chenqiuji666@xxxxxxxxx> > > --- > > drivers/amba/bus.c | 11 +++++++++-- > > 1 file changed, 9 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/amba/bus.c b/drivers/amba/bus.c > > index 34bc880ca20b..e310f4f83b27 100644 > > --- a/drivers/amba/bus.c > > +++ b/drivers/amba/bus.c > > @@ -209,6 +209,7 @@ static int amba_match(struct device *dev, const struct device_driver *drv) > > { > > struct amba_device *pcdev = to_amba_device(dev); > > const struct amba_driver *pcdrv = to_amba_driver(drv); > > + int retval; > > > > mutex_lock(&pcdev->periphid_lock); > > if (!pcdev->periphid) { > > @@ -230,8 +231,14 @@ static int amba_match(struct device *dev, const struct device_driver *drv) > > mutex_unlock(&pcdev->periphid_lock); > > > > /* When driver_override is set, only bind to the matching driver */ > > - if (pcdev->driver_override) > > - return !strcmp(pcdev->driver_override, drv->name); > > + > > + device_lock(dev); > > + if (pcdev->driver_override) { > > + retval = !strcmp(pcdev->driver_override, drv->name); > > + device_unlock(dev); > > + return retval; > > + } > > + device_unlock(dev); > > > > return amba_lookup(pcdrv->id_table, pcdev) != NULL; > > } > > > Looks correct to me > > Reviewed-by: Suzuki K Poulose <suzuki.poulose@xxxxxxx> > >
