From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from bombadil.infradead.org (bombadil.infradead.org [198.137.202.133]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id 40FCBC433F5 for ; Thu, 24 Feb 2022 07:48:48 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20210309; h=Sender: Content-Transfer-Encoding:Content-Type:List-Subscribe:List-Help:List-Post: List-Archive:List-Unsubscribe:List-Id:In-Reply-To:From:References:Cc:To: Subject:MIME-Version:Date:Message-ID:Reply-To:Content-ID:Content-Description: Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID: List-Owner; bh=AAxcZcfLwhsWlz9FWh+j8q1bXQZ3SnHQ4OpkvQ0VwhQ=; b=GZFo25KIMJQWUC wjGDLA86tlWF1Bh1icLKHI4K7BbapGzqtKHP+OWQujobYEHWceFUFK6x8D+huvBNDbeqKIGLr4v7O 2UsyvYcjM1Yv7syVbUfc9L/M6TkuO8361LwvGqp0PDep+OqNggVpjqvGWGiZ/uUiIALhd6TZcf9yn kBmaBD6j9U9moLGaWl+51410+o1S4KJ0yko7rLsa39goHambtMIDaqcue3e7kp5idD7MiUBgacRhZ B68N+6XF22agV9/BlRjCu0TYxQtMCrR4LS9YI10Hin1M7o119xhuSvRySqlONxoEbiXuNn3XAkxRG NLy1CwPCc1C7VzoY1F3Q==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.94.2 #2 (Red Hat Linux)) id 1nN8qG-00H9uK-36; Thu, 24 Feb 2022 07:47:12 +0000 Received: from smtp-relay-internal-1.canonical.com ([185.125.188.123]) by bombadil.infradead.org with esmtps (Exim 4.94.2 #2 (Red Hat Linux)) id 1nN8qC-00H9tJ-DN for linux-arm-kernel@lists.infradead.org; Thu, 24 Feb 2022 07:47:10 +0000 Received: from mail-ed1-f70.google.com (mail-ed1-f70.google.com [209.85.208.70]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by smtp-relay-internal-1.canonical.com (Postfix) with ESMTPS id 56E4B3FCA5 for ; Thu, 24 Feb 2022 07:47:02 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=canonical.com; s=20210705; t=1645688822; bh=FxOVZjnNiVO0i9jH0i6UiovfwrDBwVKl5GL2sYOaVX4=; h=Message-ID:Date:MIME-Version:Subject:To:Cc:References:From: In-Reply-To:Content-Type; b=YhVaA7UJN9z4b3F6vAxLvYQrYGK1yoJIwMtX41LJ3QgHsSXs2eEv2D88Qev+Ydlyu qijFV9K7FipVkQ9+uQtmdAqgLSe/LxXdrI7RiuwoRSbCU9n1GpgPyJXHoJx8grMMkj nYERX6Mkoc7dxGiIL32SGRtGA2Ja8HYLWq8hO+KIEA+S9cHbs218NIlkpbEA4hkT7n gG3OGy4tTpDeoxo37caay+Epi4DMIRqCOMXXv5u5RAvzHGj3+D4nLW2IuHVKne5NgY SP44lm2LDFnjLeuu+cElG0LKuJN7dCa0oHcTsAfX4IUINATVCd3h8L3fJBrZv4X8/J qLSi6Qn8hJ16Q== Received: by mail-ed1-f70.google.com with SMTP id m12-20020a056402510c00b00413298c3c42so348408edd.15 for ; Wed, 23 Feb 2022 23:47:02 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:message-id:date:mime-version:user-agent:subject :content-language:to:cc:references:from:in-reply-to :content-transfer-encoding; bh=FxOVZjnNiVO0i9jH0i6UiovfwrDBwVKl5GL2sYOaVX4=; b=h+cSVFIe9i4oJbXvk4wqGB4fZYRT2NL6ZbZp5X8ZmpM0hNOM1+r2K0nVPxXeBZkgRl n0ZbHMMZfjMcrXSKVnev6Hc11aX93G8PJSV1IuBVQIxIJ+9uyj4FglSfyuzS3PDlqb2f 7WBkgcbUggs1kJJf0+Jb/NBKvkVNr/Ag1WCNx6LXHupWXLTCoJGVwrIGiPgpT144f43H GQ1Irr3j43FlFl2cNTt1jstbLBQFus/7l0MxWLi3mp/9w8RnOYtjzF9nlpWunPtK8ST0 sCka6rmCH12sdsnCHrWazOwqjt/SeM804B9nCSnWE6fNjiml+jLxw/0aF3sA2FsdZslA Nxgg== X-Gm-Message-State: AOAM533xiitRcSUwb4ZN9epnQftfMBjJ12zdUSQuN87MDR3b8vVW3YtY nKRu7yteetLouWq3wT7y/cN4DbUW+L/YgO6wQ2MyPHuhN3nTxqCA6cXydvrLEOHopU89XYsMJs0 alhutcl5zGk85EZFfSX9bgcjv0TAVTwVkW+I5uX8CSMRLWTV3neUf X-Received: by 2002:a17:907:365:b0:6d1:bf9:9164 with SMTP id rs5-20020a170907036500b006d10bf99164mr1290191ejb.598.1645688811409; Wed, 23 Feb 2022 23:46:51 -0800 (PST) X-Google-Smtp-Source: ABdhPJyF2QxOV5sZ3l9SN1ANQlm/aYyNKUMNdm1sWIKjHtTsjqFzwsoCzjjUbc5J7jIvGVZuZRXOYQ== X-Received: by 2002:a17:907:365:b0:6d1:bf9:9164 with SMTP id rs5-20020a170907036500b006d10bf99164mr1290164ejb.598.1645688811188; Wed, 23 Feb 2022 23:46:51 -0800 (PST) Received: from [192.168.0.127] (xdsl-188-155-181-108.adslplus.ch. [188.155.181.108]) by smtp.gmail.com with ESMTPSA id t24sm914666ejx.187.2022.02.23.23.46.49 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Wed, 23 Feb 2022 23:46:50 -0800 (PST) Message-ID: <3e4f387b-53fb-b031-223c-88adac7d4dae@canonical.com> Date: Thu, 24 Feb 2022 08:46:49 +0100 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101 Thunderbird/91.5.0 Subject: Re: [PATCH v2 01/11] driver: platform: add and use helper for safer setting of driver_override Content-Language: en-US To: "Michael S. Tsirkin" Cc: Greg Kroah-Hartman , "Rafael J. Wysocki" , Stuart Yoder , Laurentiu Tudor , Abel Vesa , Shawn Guo , Sascha Hauer , Fabio Estevam , "K. Y. Srinivasan" , Haiyang Zhang , Stephen Hemminger , Wei Liu , Dexuan Cui , Bjorn Helgaas , Bjorn Andersson , Mathieu Poirier , Vineeth Vijayan , Peter Oberparleiter , Heiko Carstens , Vasily Gorbik , Alexander Gordeev , Christian Borntraeger , Sven Schnelle , Andy Gross , Srinivas Kandagatla , Mark Brown , Jason Wang , linux-kernel@vger.kernel.org, linux-clk@vger.kernel.org, NXP Linux Team , linux-arm-kernel@lists.infradead.org, linux-hyperv@vger.kernel.org, linux-pci@vger.kernel.org, linux-remoteproc@vger.kernel.org, linux-s390@vger.kernel.org, linux-arm-msm@vger.kernel.org, alsa-devel@alsa-project.org, linux-spi@vger.kernel.org, virtualization@lists.linux-foundation.org, Linus Torvalds , Rasmus Villemoes References: <20220223191310.347669-1-krzysztof.kozlowski@canonical.com> <20220223191310.347669-2-krzysztof.kozlowski@canonical.com> <20220223162538-mutt-send-email-mst@kernel.org> From: Krzysztof Kozlowski In-Reply-To: <20220223162538-mutt-send-email-mst@kernel.org> X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20220223_234708_599008_6690709B X-CRM114-Status: GOOD ( 34.41 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org On 23/02/2022 22:33, Michael S. Tsirkin wrote: > On Wed, Feb 23, 2022 at 08:13:00PM +0100, Krzysztof Kozlowski wrote: >> Several core drivers and buses expect that driver_override is a >> dynamically allocated memory thus later they can kfree() it. >> >> However such assumption is not documented, there were in the past and >> there are already users setting it to a string literal. This leads to >> kfree() of static memory during device release (e.g. in error paths or >> during unbind): >> >> kernel BUG at ../mm/slub.c:3960! >> Internal error: Oops - BUG: 0 [#1] PREEMPT SMP ARM >> ... >> (kfree) from [] (platform_device_release+0x88/0xb4) >> (platform_device_release) from [] (device_release+0x2c/0x90) >> (device_release) from [] (kobject_put+0xec/0x20c) >> (kobject_put) from [] (exynos5_clk_probe+0x154/0x18c) >> (exynos5_clk_probe) from [] (platform_drv_probe+0x6c/0xa4) >> (platform_drv_probe) from [] (really_probe+0x280/0x414) >> (really_probe) from [] (driver_probe_device+0x78/0x1c4) >> (driver_probe_device) from [] (bus_for_each_drv+0x74/0xb8) >> (bus_for_each_drv) from [] (__device_attach+0xd4/0x16c) >> (__device_attach) from [] (bus_probe_device+0x88/0x90) >> (bus_probe_device) from [] (device_add+0x3dc/0x62c) >> (device_add) from [] (of_platform_device_create_pdata+0x94/0xbc) >> (of_platform_device_create_pdata) from [] (of_platform_bus_create+0x1a8/0x4fc) >> (of_platform_bus_create) from [] (of_platform_bus_create+0x20c/0x4fc) >> (of_platform_bus_create) from [] (of_platform_populate+0x84/0x118) >> (of_platform_populate) from [] (of_platform_default_populate_init+0xa0/0xb8) >> (of_platform_default_populate_init) from [] (do_one_initcall+0x8c/0x404) >> (do_one_initcall) from [] (kernel_init_freeable+0x3d0/0x4d8) >> (kernel_init_freeable) from [] (kernel_init+0x8/0x114) >> (kernel_init) from [] (ret_from_fork+0x14/0x20) >> >> Provide a helper which clearly documents the usage of driver_override. >> This will allow later to reuse the helper and reduce amount of >> duplicated code. >> >> Signed-off-by: Krzysztof Kozlowski >> --- >> drivers/base/driver.c | 44 +++++++++++++++++++++++++++++++++ >> drivers/base/platform.c | 24 +++--------------- >> include/linux/device/driver.h | 1 + >> include/linux/platform_device.h | 6 ++++- >> 4 files changed, 54 insertions(+), 21 deletions(-) >> >> diff --git a/drivers/base/driver.c b/drivers/base/driver.c >> index 8c0d33e182fd..79efe51bb4c0 100644 >> --- a/drivers/base/driver.c >> +++ b/drivers/base/driver.c >> @@ -30,6 +30,50 @@ static struct device *next_device(struct klist_iter *i) >> return dev; >> } >> >> +/* >> + * set_driver_override() - Helper to set or clear driver override. >> + * @dev: Device to change >> + * @override: Address of string to change (e.g. &device->driver_override); >> + * The contents will be freed and hold newly allocated override. >> + * @s: NULL terminated string, new driver name to force a match, pass empty > > Don't you mean NUL terminated? Yeah, NUL. > Do all callers really validate that it's NUL terminated? Good point, the callers use it in device attributes (sysfs) only, so it might come non-NUL. Previously this was solved by kstrndup() which is always terminating the string. > >> + * string to clear it >> + * >> + * Helper to setr or clear driver override in a device, intended for the cases > > set? D'oh! > >> + * when the driver_override field is allocated by driver/bus code. >> + * >> + * Returns: 0 on success or a negative error code on failure. >> + */ >> +int driver_set_override(struct device *dev, char **override, const char *s) >> +{ >> + char *new, *old, *cp; >> + >> + if (!dev || !override || !s) >> + return -EINVAL; >> + >> + new = kstrndup(s, strlen(s), GFP_KERNEL); > > > what's the point of this kstrndup then? why not just kstrdup? Thanks, it's a copy-paste. Useless now, but I'll pass the count directly from the callers and then this will be NULL-terminating it. > >> + if (!new) >> + return -ENOMEM; >> + >> + cp = strchr(new, '\n'); >> + if (cp) >> + *cp = '\0'; >> + >> + device_lock(dev); >> + old = *override; >> + if (strlen(new)) { > > We are re-reading the string like 3 times here. Yep, the same in old code. I guess we could compare just pointers - whether 'cp' is not NULL and different than 's'. > >> + *override = new; >> + } else { >> + kfree(new); >> + *override = NULL; >> + } >> + device_unlock(dev); >> + >> + kfree(old); >> + >> + return 0; >> +} >> +EXPORT_SYMBOL_GPL(driver_set_override); >> + >> /** >> * driver_for_each_device - Iterator for devices bound to a driver. >> * @drv: Driver we're iterating. >> diff --git a/drivers/base/platform.c b/drivers/base/platform.c >> index 6cb04ac48bf0..d8853b32ea10 100644 >> --- a/drivers/base/platform.c >> +++ b/drivers/base/platform.c >> @@ -1275,31 +1275,15 @@ static ssize_t driver_override_store(struct device *dev, >> const char *buf, size_t count) >> { >> struct platform_device *pdev = to_platform_device(dev); >> - char *driver_override, *old, *cp; >> + int ret; >> >> /* We need to keep extra room for a newline */ >> if (count >= (PAGE_SIZE - 1)) >> return -EINVAL; > > Given everyone seems to repeat this check, how about passing > in count and doing the validation in the helper? Good idea. > We will then also avoid the need to do strlen and strchr. The strlen() could be removed, but the strchr() should stay. What solution do you have in mind to remove strchr()? Thanks for review. Best regards, Krzysztof _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel