On 2024/07/13 8:49, Dan Williams wrote:
- /* Synchronize with dev_uevent() */
- synchronize_rcu();
this synchronize_rcu(), in order to make sure that READ_ONCE(dev->driver) in dev_uevent() observes NULL?
No, this synchronize_rcu() is to make sure that if dev_uevent() wins the race and observes that dev->driver is not NULL that it is still safe to dereference that result because the 'struct device_driver' object is still live.
I can't catch what the pair of rcu_read_lock()/rcu_read_unlock() in dev_uevent() and synchronize_rcu() in module_remove_driver() is for.
I think that the below race is possible. Please explain how "/* Synchronize with module_remove_driver() */" works.
Thread1: Thread2:
static int dev_uevent(const struct kobject *kobj, struct kobj_uevent_env *env) { const struct device *dev = kobj_to_dev(kobj); struct device_driver *driver; int retval = 0;
(...snipped...)
if (dev->type && dev->type->name) add_uevent_var(env, "DEVTYPE=%s", dev->type->name);
void module_remove_driver(struct device_driver *drv) { struct module_kobject *mk = NULL; char *driver_name;
if (!drv) return;
/* Synchronize with dev_uevent() */ synchronize_rcu(); // <= This can be no-op because rcu_read_lock() in dev_uevent() is not yet called.
// <= At this moment Thread1 can sleep for arbitrary duration due to preemption, can't it?
/* Synchronize with module_remove_driver() */ rcu_read_lock(); // <= What does this RCU want to synchronize with?
sysfs_remove_link(&drv->p->kobj, "module");
if (drv->owner) mk = &drv->owner->mkobj; else if (drv->p->mkobj) mk = drv->p->mkobj; if (mk && mk->drivers_dir) { driver_name = make_driver_name(drv); if (driver_name) { sysfs_remove_link(mk->drivers_dir, driver_name); kfree(driver_name); } } }
driver = READ_ONCE(dev->driver); // <= module_remove_driver() can be already completed even with RCU protection, can't it? if (driver) add_uevent_var(env, "DRIVER=%s", driver->name); rcu_read_unlock();
/* Add common DT information about the device */ of_device_uevent(dev, env);
(..snipped...) }