linux-kernel - Re: [PATCH] driver core: Fix uevent

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <0b34da9e-c13f-4fab-a67d-244b0ebba394@I-love.SAKURA.ne.jp>
Date: Sat, 13 Jul 2024 15:05:24 +0900
From: Tetsuo Handa <penguin-kernel@...ove.SAKURA.ne.jp>
To: Dan Williams <dan.j.williams@...el.com>, gregkh@...uxfoundation.org
Cc: syzbot+4762dd74e32532cda5ff@...kaller.appspotmail.com,
        stable@...r.kernel.org, Ashish Sangwan <a.sangwan@...sung.com>,
        Namjae Jeon <namjae.jeon@...sung.com>,
        Dirk Behme <dirk.behme@...bosch.com>,
        "Rafael J. Wysocki" <rafael@...nel.org>, linux-kernel@...r.kernel.org,
        linux-cxl@...r.kernel.org
Subject: Re: [PATCH] driver core: Fix uevent_show() vs driver detach race

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