From patchwork Tue Oct 8 11:29:34 2019 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Kevin Hao X-Patchwork-Id: 11179455 Return-Path: Received: from mail.kernel.org (pdx-korg-mail-1.web.codeaurora.org [172.30.200.123]) by pdx-korg-patchwork-2.web.codeaurora.org (Postfix) with ESMTP id 70EFF1668 for ; Tue, 8 Oct 2019 11:35:39 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id 3DE7320673 for ; Tue, 8 Oct 2019 11:35:39 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (2048-bit key) header.d=gmail.com header.i=@gmail.com header.b="oqUDOpqe" Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1730601AbfJHLfj (ORCPT ); Tue, 8 Oct 2019 07:35:39 -0400 Received: from mail-pf1-f195.google.com ([209.85.210.195]:38247 "EHLO mail-pf1-f195.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1730393AbfJHLfi (ORCPT ); Tue, 8 Oct 2019 07:35:38 -0400 Received: by mail-pf1-f195.google.com with SMTP id h195so10619655pfe.5 for ; Tue, 08 Oct 2019 04:35:38 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=from:to:cc:subject:date:message-id; bh=I3aUPldbci24u3cr9dD9qgMCyY04nc95U2j9Ccf7nAw=; b=oqUDOpqeMi4SGZpBz86iEqdRmyR6mfuqyVkq2bx2gQuRBwu4CaD1i7S2fmMk2p9HRz WOYfZ8rpzLB9XphK2PEq9apoZojRncYHqdmIroWtJsF3UKmTFBcoivXxhyi+YklBF7V8 egBcg5edLpO/TRITn4ltcb2yuzmWMm2x1PKOqLDI0HYm3KJB5pE7ir34pn4FtwhUWb/H M3xIHjBMhHbyv3UvIseq64tzBuzaMzyC0MYuuIa2BRmAuCX4u6ciIeWg4a8Zyyfpo1/Z GOJuJiJpsEHuRMBcKbIGktmJcbqSvgeoejhx696regeE/IvTX1Md73A7jCXmXle2DP07 SfpA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:from:to:cc:subject:date:message-id; bh=I3aUPldbci24u3cr9dD9qgMCyY04nc95U2j9Ccf7nAw=; b=hXH3/DLe0AcXWgoW32e3bycS+3vaAU/Ye+j9cpdPJ+JnmKE/XDvRljI3j4tzDt4hH+ kLvrOydOHQ784KTSoXQ+vwZr5pFR8xMiPnscpmJ+OJxJW3UMaqfBE523u9yRNybVtbZE CMb+Za7wNeYyuYcjifyLoZcfLyYE1/Wy8P8HkU3M9RuSS0ph+x7c/E340/m77LImMjA/ 7ChAyacgimv+GKhKiSKnBigoty7aQUqMwxrcFJpFlmczAK5+NNlsQovHjtlSiLtJfwv2 fn2I17Il3DLYvhznu/AZl1GRDRQu9IGZ0JPFmKoXF54EvFxbcMgQDpcyQxFKYgPQWAlc rujA== X-Gm-Message-State: APjAAAWnhyrpf7R8HU7P5CU7aYv/ymzKiM7wi+UEnk6I0ieyWdrz6fnS FCEJ+BxyvQachILbD2IYKbRIXpynqyA= X-Google-Smtp-Source: APXvYqzNYzt3T1k5S1MEdRCqgt9OsjLLKmNPCC7hoqyaFBMv4cudsBZS9KfUwYXrNA+uhrhrcGSBBw== X-Received: by 2002:a17:90a:1b28:: with SMTP id q37mr5331608pjq.91.1570534537300; Tue, 08 Oct 2019 04:35:37 -0700 (PDT) Received: from pek-lpggp6.wrs.com (unknown-105-123.windriver.com. [147.11.105.123]) by smtp.gmail.com with ESMTPSA id s36sm1241487pgk.84.2019.10.08.04.35.31 (version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256); Tue, 08 Oct 2019 04:35:36 -0700 (PDT) From: Kevin Hao To: linux-watchdog@vger.kernel.org Cc: Wim Van Sebroeck , Guenter Roeck Subject: [PATCH] watchdog: Fix the race between the release of watchdog_core_data and cdev Date: Tue, 8 Oct 2019 19:29:34 +0800 Message-Id: <20191008112934.29669-1-haokexin@gmail.com> X-Mailer: git-send-email 2.14.4 Sender: linux-watchdog-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-watchdog@vger.kernel.org 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: Kevin Hao Reviewed-by: Guenter Roeck --- drivers/watchdog/watchdog_dev.c | 70 +++++++++++++++++++---------------------- 1 file changed, 32 insertions(+), 38 deletions(-) diff --git a/drivers/watchdog/watchdog_dev.c b/drivers/watchdog/watchdog_dev.c index dbd2ad4c9294..55db4c95f0e8 100644 --- a/drivers/watchdog/watchdog_dev.c +++ b/drivers/watchdog/watchdog_dev.c @@ -34,7 +34,6 @@ #include /* For __init/__exit/... */ #include /* For hrtimers */ #include /* For printk/panic/... */ -#include /* For data references */ #include /* For kthread_work */ #include /* For handling misc devices */ #include /* For module stuff/... */ @@ -52,14 +51,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; @@ -839,7 +838,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); /* * open_timeout only applies for the first open from @@ -860,11 +859,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); } @@ -924,7 +923,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; } @@ -943,17 +942,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; @@ -961,7 +965,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; @@ -990,23 +993,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 = ktime_sub(ktime_get(), 1); watchdog_set_open_deadline(wd_data); @@ -1017,7 +1030,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); if (handle_boot_enabled) hrtimer_start(&wd_data->timer, 0, HRTIMER_MODE_REL); else @@ -1040,7 +1053,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; @@ -1059,15 +1072,9 @@ static void watchdog_cdev_unregister(struct watchdog_device *wdd) hrtimer_cancel(&wd_data->timer); kthread_cancel_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, -}; - static int watchdog_reboot_notifier(struct notifier_block *nb, unsigned long code, void *data) { @@ -1098,27 +1105,14 @@ static int watchdog_reboot_notifier(struct notifier_block *nb, 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); return ret; } @@ -1126,7 +1120,8 @@ int watchdog_dev_register(struct watchdog_device *wdd) if (test_bit(WDOG_STOP_ON_REBOOT, &wdd->status)) { wdd->reboot_nb.notifier_call = watchdog_reboot_notifier; - ret = devm_register_reboot_notifier(dev, &wdd->reboot_nb); + ret = devm_register_reboot_notifier(&wdd->wd_data->dev, + &wdd->reboot_nb); if (ret) { pr_err("watchdog%d: Cannot register reboot notifier (%d)\n", wdd->id, ret); @@ -1148,7 +1143,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); }