Re: [PATCH] driver core: Fix uevent_show() vs driver detach race

[Tetsuo Handa <penguin-kernel@xxxxxxxxxxxxxxxxxxx>]

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...)
  }