Donate to e Foundation | Murena handsets with /e/OS | Own a part of Murena! Learn more

Commit f76905ce authored by Kevin Hao's avatar Kevin Hao Committed by Greg Kroah-Hartman
Browse files

watchdog: Fix the race between the release of watchdog_core_data and cdev 



commit 72139dfa2464e43957d330266994740bb7be2535 upstream.

The struct cdev is embedded in the struct watchdog_core_data. In the
current code, we manage the watchdog_core_data with a kref, but the
cdev is manged by a kobject. There is no any relationship between
this kref and kobject. So it is possible that the watchdog_core_data is
freed before the cdev is entirely released. We can easily get the
following call trace with CONFIG_DEBUG_KOBJECT_RELEASE and
CONFIG_DEBUG_OBJECTS_TIMERS enabled.
  ODEBUG: free active (active state 0) object type: timer_list hint: delayed_work_timer_fn+0x0/0x38
  WARNING: CPU: 23 PID: 1028 at lib/debugobjects.c:481 debug_print_object+0xb0/0xf0
  Modules linked in: softdog(-) deflate ctr twofish_generic twofish_common camellia_generic serpent_generic blowfish_generic blowfish_common cast5_generic cast_common cmac xcbc af_key sch_fq_codel openvswitch nsh nf_conncount nf_nat nf_conntrack nf_defrag_ipv6 nf_defrag_ipv4
  CPU: 23 PID: 1028 Comm: modprobe Not tainted 5.3.0-next-20190924-yoctodev-standard+ #180
  Hardware name: Marvell OcteonTX CN96XX board (DT)
  pstate: 00400009 (nzcv daif +PAN -UAO)
  pc : debug_print_object+0xb0/0xf0
  lr : debug_print_object+0xb0/0xf0
  sp : ffff80001cbcfc70
  x29: ffff80001cbcfc70 x28: ffff800010ea2128
  x27: ffff800010bad000 x26: 0000000000000000
  x25: ffff80001103c640 x24: ffff80001107b268
  x23: ffff800010bad9e8 x22: ffff800010ea2128
  x21: ffff000bc2c62af8 x20: ffff80001103c600
  x19: ffff800010e867d8 x18: 0000000000000060
  x17: 0000000000000000 x16: 0000000000000000
  x15: ffff000bd7240470 x14: 6e6968207473696c
  x13: 5f72656d6974203a x12: 6570797420746365
  x11: 6a626f2029302065 x10: 7461747320657669
  x9 : 7463612820657669 x8 : 3378302f3078302b
  x7 : 0000000000001d7a x6 : ffff800010fd5889
  x5 : 0000000000000000 x4 : 0000000000000000
  x3 : 0000000000000000 x2 : ffff000bff948548
  x1 : 276a1c9e1edc2300 x0 : 0000000000000000
  Call trace:
   debug_print_object+0xb0/0xf0
   debug_check_no_obj_freed+0x1e8/0x210
   kfree+0x1b8/0x368
   watchdog_cdev_unregister+0x88/0xc8
   watchdog_dev_unregister+0x38/0x48
   watchdog_unregister_device+0xa8/0x100
   softdog_exit+0x18/0xfec4 [softdog]
   __arm64_sys_delete_module+0x174/0x200
   el0_svc_handler+0xd0/0x1c8
   el0_svc+0x8/0xc

This is a common issue when using cdev embedded in a struct.
Fortunately, we already have a mechanism to solve this kind of issue.
Please see commit 233ed09d7fda ("chardev: add helper function to
register char devs with a struct device") for more detail.

In this patch, we choose to embed the struct device into the
watchdog_core_data, and use the API provided by the commit 233ed09d7fda
to make sure that the release of watchdog_core_data and cdev are
in sequence.

Signed-off-by: default avatarKevin Hao <haokexin@gmail.com>
Reviewed-by: default avatarGuenter Roeck <linux@roeck-us.net>
Link: https://lore.kernel.org/r/20191008112934.29669-1-haokexin@gmail.com


Signed-off-by: default avatarGuenter Roeck <linux@roeck-us.net>
Signed-off-by: default avatarWim Van Sebroeck <wim@linux-watchdog.org>
[bwh: Backported to 4.9:
 - There's no reboot notifier here
 - Adjust context]
Signed-off-by: default avatarBen Hutchings <ben.hutchings@codethink.co.uk>
Signed-off-by: default avatarGreg Kroah-Hartman <gregkh@linuxfoundation.org>
parent a5233a3f

drivers/watchdog/watchdog_dev.c

+30 −37
Original line number Diff line number Diff line
@@ -38,7 +38,6 @@ 
#include <linux/init.h>		/* For __init/__exit/... */

#include <linux/jiffies.h>	/* For timeout functions */

#include <linux/kernel.h>	/* For printk/panic/... */

#include <linux/kref.h>		/* For data references */

#include <linux/miscdevice.h>	/* For handling misc devices */

#include <linux/module.h>	/* For module stuff/... */

#include <linux/mutex.h>	/* For mutexes */
@@ -53,14 +52,14 @@ 


/*

 * struct watchdog_core_data - watchdog core internal data

 * @kref:	Reference count.

 * @dev:	The watchdog's internal device

 * @cdev:	The watchdog's Character device.

 * @wdd:	Pointer to watchdog device.

 * @lock:	Lock for watchdog core.

 * @status:	Watchdog core internal status bits.

 */

struct watchdog_core_data {

	struct kref kref;

	struct device dev;

	struct cdev cdev;

	struct watchdog_device *wdd;

	struct mutex lock;
@@ -794,7 +793,7 @@ static int watchdog_open(struct inode *inode, struct file *file) 
	file->private_data = wd_data;



	if (!hw_running)

		kref_get(&wd_data->kref);

		get_device(&wd_data->dev);



	/* dev/watchdog is a virtual (and thus non-seekable) filesystem */

	return nonseekable_open(inode, file);
@@ -806,11 +805,11 @@ static int watchdog_open(struct inode *inode, struct file *file) 
	return err;

}



static void watchdog_core_data_release(struct kref *kref)

static void watchdog_core_data_release(struct device *dev)

{

	struct watchdog_core_data *wd_data;



	wd_data = container_of(kref, struct watchdog_core_data, kref);

	wd_data = container_of(dev, struct watchdog_core_data, dev);



	kfree(wd_data);

}
@@ -870,7 +869,7 @@ static int watchdog_release(struct inode *inode, struct file *file) 
	 */

	if (!running) {

		module_put(wd_data->cdev.owner);

		kref_put(&wd_data->kref, watchdog_core_data_release);

		put_device(&wd_data->dev);

	}

	return 0;

}
@@ -889,17 +888,22 @@ static struct miscdevice watchdog_miscdev = { 
	.fops		= &watchdog_fops,

};



static struct class watchdog_class = {

	.name =		"watchdog",

	.owner =	THIS_MODULE,

	.dev_groups =	wdt_groups,

};



/*

 *	watchdog_cdev_register: register watchdog character device

 *	@wdd: watchdog device

 *	@devno: character device number

 *

 *	Register a watchdog character device including handling the legacy

 *	/dev/watchdog node. /dev/watchdog is actually a miscdevice and

 *	thus we set it up like that.

 */



static int watchdog_cdev_register(struct watchdog_device *wdd, dev_t devno)

static int watchdog_cdev_register(struct watchdog_device *wdd)

{

	struct watchdog_core_data *wd_data;

	int err;
@@ -907,7 +911,6 @@ static int watchdog_cdev_register(struct watchdog_device *wdd, dev_t devno) 
	wd_data = kzalloc(sizeof(struct watchdog_core_data), GFP_KERNEL);

	if (!wd_data)

		return -ENOMEM;

	kref_init(&wd_data->kref);

	mutex_init(&wd_data->lock);



	wd_data->wdd = wdd;
@@ -934,23 +937,33 @@ static int watchdog_cdev_register(struct watchdog_device *wdd, dev_t devno) 
		}

	}



	device_initialize(&wd_data->dev);

	wd_data->dev.devt = MKDEV(MAJOR(watchdog_devt), wdd->id);

	wd_data->dev.class = &watchdog_class;

	wd_data->dev.parent = wdd->parent;

	wd_data->dev.groups = wdd->groups;

	wd_data->dev.release = watchdog_core_data_release;

	dev_set_drvdata(&wd_data->dev, wdd);

	dev_set_name(&wd_data->dev, "watchdog%d", wdd->id);



	/* Fill in the data structures */

	cdev_init(&wd_data->cdev, &watchdog_fops);

	wd_data->cdev.owner = wdd->ops->owner;



	/* Add the device */

	err = cdev_add(&wd_data->cdev, devno, 1);

	err = cdev_device_add(&wd_data->cdev, &wd_data->dev);

	if (err) {

		pr_err("watchdog%d unable to add device %d:%d\n",

			wdd->id,  MAJOR(watchdog_devt), wdd->id);

		if (wdd->id == 0) {

			misc_deregister(&watchdog_miscdev);

			old_wd_data = NULL;

			kref_put(&wd_data->kref, watchdog_core_data_release);

			put_device(&wd_data->dev);

		}

		return err;

	}



	wd_data->cdev.owner = wdd->ops->owner;



	/* Record time of most recent heartbeat as 'just before now'. */

	wd_data->last_hw_keepalive = jiffies - 1;
@@ -960,7 +973,7 @@ static int watchdog_cdev_register(struct watchdog_device *wdd, dev_t devno) 
	 */

	if (watchdog_hw_running(wdd)) {

		__module_get(wdd->ops->owner);

		kref_get(&wd_data->kref);

		get_device(&wd_data->dev);

		queue_delayed_work(watchdog_wq, &wd_data->work, 0);

	}
@@ -979,7 +992,7 @@ static void watchdog_cdev_unregister(struct watchdog_device *wdd) 
{

	struct watchdog_core_data *wd_data = wdd->wd_data;



	cdev_del(&wd_data->cdev);

	cdev_device_del(&wd_data->cdev, &wd_data->dev);

	if (wdd->id == 0) {

		misc_deregister(&watchdog_miscdev);

		old_wd_data = NULL;
@@ -992,15 +1005,9 @@ static void watchdog_cdev_unregister(struct watchdog_device *wdd) 


	cancel_delayed_work_sync(&wd_data->work);



	kref_put(&wd_data->kref, watchdog_core_data_release);

	put_device(&wd_data->dev);

}



static struct class watchdog_class = {

	.name =		"watchdog",

	.owner =	THIS_MODULE,

	.dev_groups =	wdt_groups,

};



/*

 *	watchdog_dev_register: register a watchdog device

 *	@wdd: watchdog device
@@ -1012,27 +1019,14 @@ static struct class watchdog_class = { 


int watchdog_dev_register(struct watchdog_device *wdd)

{

	struct device *dev;

	dev_t devno;

	int ret;



	devno = MKDEV(MAJOR(watchdog_devt), wdd->id);



	ret = watchdog_cdev_register(wdd, devno);

	ret = watchdog_cdev_register(wdd);

	if (ret)

		return ret;



	dev = device_create_with_groups(&watchdog_class, wdd->parent,

					devno, wdd, wdd->groups,

					"watchdog%d", wdd->id);

	if (IS_ERR(dev)) {

		watchdog_cdev_unregister(wdd);

		return PTR_ERR(dev);

	}



	ret = watchdog_register_pretimeout(wdd);

	if (ret) {

		device_destroy(&watchdog_class, devno);

		watchdog_cdev_unregister(wdd);

	}
@@ -1050,7 +1044,6 @@ int watchdog_dev_register(struct watchdog_device *wdd) 
void watchdog_dev_unregister(struct watchdog_device *wdd)

{

	watchdog_unregister_pretimeout(wdd);

	device_destroy(&watchdog_class, wdd->wd_data->cdev.dev);

	watchdog_cdev_unregister(wdd);

}