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 alsa0.perex.cz (alsa0.perex.cz [77.48.224.243]) (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 33F23C433EF for ; Fri, 25 Feb 2022 13:14:31 +0000 (UTC) Received: from alsa1.perex.cz (alsa1.perex.cz [207.180.221.201]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by alsa0.perex.cz (Postfix) with ESMTPS id 6CC191B17; Fri, 25 Feb 2022 14:13:39 +0100 (CET) DKIM-Filter: OpenDKIM Filter v2.11.0 alsa0.perex.cz 6CC191B17 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=alsa-project.org; s=default; t=1645794869; bh=mL+3A2Vh8pNv0p3qQeywELwmAOfks6TKf0crKSZtVMo=; h=Date:From:To:Subject:References:In-Reply-To:Cc:List-Id: List-Unsubscribe:List-Archive:List-Post:List-Help:List-Subscribe: From; b=Uf6igBF8RVpjFtcR3Y0KgCi3mNDgvi5CB8AJ7XBzq0fyhnbCk0MqHAxynNDrd7/Hl bnOBRoa5gUNmx8NqL0mMVcqjI75iUmXbJKcezVsC/yTjkmLm6cuT7q9Msqvd4CcB2Y gyQxiDpKOmFl87McIf1iD0QieVmT1bamESIDjge8= Received: from alsa1.perex.cz (localhost.localdomain [127.0.0.1]) by alsa1.perex.cz (Postfix) with ESMTP id AE592F8053A; Fri, 25 Feb 2022 14:11:24 +0100 (CET) Received: by alsa1.perex.cz (Postfix, from userid 50401) id 26F7CF80237; Wed, 23 Feb 2022 22:33:32 +0100 (CET) Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.129.124]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by alsa1.perex.cz (Postfix) with ESMTPS id 2841AF800F5 for ; Wed, 23 Feb 2022 22:33:24 +0100 (CET) DKIM-Filter: OpenDKIM Filter v2.11.0 alsa1.perex.cz 2841AF800F5 Authentication-Results: alsa1.perex.cz; dkim=pass (1024-bit key) header.d=redhat.com header.i=@redhat.com header.b="Zr7wTZFx" DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1645652002; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: in-reply-to:in-reply-to:references:references; bh=JVrDSsHDee470/2N5kdB0CUYnm9UIRcjrM+Vqrir8YU=; b=Zr7wTZFxWBcr56DUsJkJ1ZKYZmLGX8efarU8b697hShCmmKmsR3zhdl2L7KdwPtgkHvVb0 L/ItOHUQzRKHXfhnq+yt4dBXt3te1FdBJZ/FQde6nZmn4T148HZd0Tk5VC6ETM6FuYtxVu 5cvS8yV7HGRlDpBl7f5SBeqV2+aRThs= Received: from mail-wr1-f72.google.com (mail-wr1-f72.google.com [209.85.221.72]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id us-mta-527-6D98f-zKOvmeWVtU2ZyV0Q-1; Wed, 23 Feb 2022 16:33:19 -0500 X-MC-Unique: 6D98f-zKOvmeWVtU2ZyV0Q-1 Received: by mail-wr1-f72.google.com with SMTP id e1-20020adfa741000000b001e2e74c3d4eso10703879wrd.12 for ; Wed, 23 Feb 2022 13:33:19 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to; bh=JVrDSsHDee470/2N5kdB0CUYnm9UIRcjrM+Vqrir8YU=; b=Bx9B/BPf79LFig72LgfsK9cr2d9T+/Y3ULWROgIo4ezrqmZ3aJkIsWpPFZii8a0j0k zsx/jLdZ5wPEYVvmKFCAhEJ+8VH29c9cOzWI692PFs0xr9fkE46MdgLh/Kgvr6BlGJuF XwQAgy3jPxZhH3+AQ8eaRTZoarqhXN1S4Z/svDWHS+Oc8lTLJSRgusV5xiWmVdX5bXYb S+z93dCpfciRMDntinZZpirBV8atDCUeDR/kzi9d5YV9UcAG7fTLd4cGvN7wyVxjSRU2 y8+y8+3uVAUV2AQIjxBE6O06wyPiV2OZ5+sWlPjwQ0GuB49O9chxuOmOgBkQQkyYbTUr Wjpg== X-Gm-Message-State: AOAM530OtsB181LquXPflNpIRbR4V6bblnZ15t8O6FcO0cMMM4OaMA7E xC2piYWM++GrYu1iMrfoLFLtq/vys1NCSxBoKs61sOH/v9JJtKsFpdaX+UvgZMvMBTM1D928TSc g11/bOl4Q0NUYW34V/zB61eg= X-Received: by 2002:a5d:6f0a:0:b0:1e4:a354:a7e with SMTP id ay10-20020a5d6f0a000000b001e4a3540a7emr1081126wrb.423.1645651998033; Wed, 23 Feb 2022 13:33:18 -0800 (PST) X-Google-Smtp-Source: ABdhPJzVh3zKelOjcgzztRyyTOBFC5U69RZ7Dwi6phYx+UQ7pZLlXHi6mmPrXmx8tSFwquULC08JMw== X-Received: by 2002:a5d:6f0a:0:b0:1e4:a354:a7e with SMTP id ay10-20020a5d6f0a000000b001e4a3540a7emr1081114wrb.423.1645651997698; Wed, 23 Feb 2022 13:33:17 -0800 (PST) Received: from redhat.com ([2.55.166.187]) by smtp.gmail.com with ESMTPSA id l5sm677279wmq.7.2022.02.23.13.33.12 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Wed, 23 Feb 2022 13:33:17 -0800 (PST) Date: Wed, 23 Feb 2022 16:33:10 -0500 From: "Michael S. Tsirkin" To: Krzysztof Kozlowski Subject: Re: [PATCH v2 01/11] driver: platform: add and use helper for safer setting of driver_override Message-ID: <20220223162538-mutt-send-email-mst@kernel.org> References: <20220223191310.347669-1-krzysztof.kozlowski@canonical.com> <20220223191310.347669-2-krzysztof.kozlowski@canonical.com> MIME-Version: 1.0 In-Reply-To: <20220223191310.347669-2-krzysztof.kozlowski@canonical.com> Authentication-Results: relay.mimecast.com; auth=pass smtp.auth=CUSA124A263 smtp.mailfrom=mst@redhat.com X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=us-ascii Content-Disposition: inline X-Mailman-Approved-At: Fri, 25 Feb 2022 14:11:18 +0100 Cc: linux-hyperv@vger.kernel.org, Stuart Yoder , "Rafael J. Wysocki" , linux-pci@vger.kernel.org, Jason Wang , linux-remoteproc@vger.kernel.org, alsa-devel@alsa-project.org, Bjorn Andersson , Srinivas Kandagatla , Vineeth Vijayan , Alexander Gordeev , "K. Y. Srinivasan" , Fabio Estevam , linux-clk@vger.kernel.org, linux-s390@vger.kernel.org, Wei Liu , Stephen Hemminger , Abel Vesa , Dexuan Cui , Linus Torvalds , Andy Gross , NXP Linux Team , Christian Borntraeger , Heiko Carstens , Vasily Gorbik , linux-arm-msm@vger.kernel.org, Sascha Hauer , linux-spi@vger.kernel.org, Mark Brown , Rasmus Villemoes , Bjorn Helgaas , virtualization@lists.linux-foundation.org, linux-arm-kernel@lists.infradead.org, Laurentiu Tudor , Mathieu Poirier , Greg Kroah-Hartman , Haiyang Zhang , Peter Oberparleiter , linux-kernel@vger.kernel.org, Sven Schnelle , Shawn Guo X-BeenThere: alsa-devel@alsa-project.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: "Alsa-devel mailing list for ALSA developers - http://www.alsa-project.org" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: alsa-devel-bounces@alsa-project.org Sender: "Alsa-devel" 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? Do all callers really validate that it's NUL terminated? > + * string to clear it > + * > + * Helper to setr or clear driver override in a device, intended for the cases set? > + * 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? > + 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. > + *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? We will then also avoid the need to do strlen and strchr. > - driver_override = kstrndup(buf, count, GFP_KERNEL); > - if (!driver_override) > - return -ENOMEM; > - > - cp = strchr(driver_override, '\n'); > - if (cp) > - *cp = '\0'; > - > - device_lock(dev); > - old = pdev->driver_override; > - if (strlen(driver_override)) { > - pdev->driver_override = driver_override; > - } else { > - kfree(driver_override); > - pdev->driver_override = NULL; > - } > - device_unlock(dev); > - > - kfree(old); > + ret = driver_set_override(dev, &pdev->driver_override, buf); > + if (ret) > + return ret; > > return count; > } > diff --git a/include/linux/device/driver.h b/include/linux/device/driver.h > index 15e7c5e15d62..81c0d9f65a40 100644 > --- a/include/linux/device/driver.h > +++ b/include/linux/device/driver.h > @@ -151,6 +151,7 @@ extern int __must_check driver_create_file(struct device_driver *driver, > extern void driver_remove_file(struct device_driver *driver, > const struct driver_attribute *attr); > > +int driver_set_override(struct device *dev, char **override, const char *s); > extern int __must_check driver_for_each_device(struct device_driver *drv, > struct device *start, > void *data, > diff --git a/include/linux/platform_device.h b/include/linux/platform_device.h > index 7c96f169d274..37ac14459499 100644 > --- a/include/linux/platform_device.h > +++ b/include/linux/platform_device.h > @@ -31,7 +31,11 @@ struct platform_device { > struct resource *resource; > > const struct platform_device_id *id_entry; > - char *driver_override; /* Driver name to force a match */ > + /* > + * Driver name to force a match, use > + * driver_set_override() to set or clear it. > + */ > + char *driver_override; > > /* MFD cell pointer */ > struct mfd_cell *mfd_cell; > -- > 2.32.0 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 vger.kernel.org (vger.kernel.org [23.128.96.18]) by smtp.lore.kernel.org (Postfix) with ESMTP id B1E92C4332F for ; Wed, 23 Feb 2022 21:33:24 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S242772AbiBWVdv (ORCPT ); Wed, 23 Feb 2022 16:33:51 -0500 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:32938 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S242736AbiBWVdu (ORCPT ); Wed, 23 Feb 2022 16:33:50 -0500 Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.133.124]) by lindbergh.monkeyblade.net (Postfix) with ESMTP id 465DB4ECE8 for ; Wed, 23 Feb 2022 13:33:21 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1645652000; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: in-reply-to:in-reply-to:references:references; bh=JVrDSsHDee470/2N5kdB0CUYnm9UIRcjrM+Vqrir8YU=; b=RAzwl9Sez1TeT+nmEa1yYBZAZQ4KRu+18SqJ5YvCiLsN5ZoOFX4CZPFTlGdfYriTr4/+WA XXnR0ENpAnH0WVYQ6IDWFU8hF6VB+UXAFAgohlr+Zsldn+5r5XNapYpdZ3IxnHR1NhHHKK OVHzWqSkiwNvtpVRgw7agSTzlkdlpqo= Received: from mail-wr1-f70.google.com (mail-wr1-f70.google.com [209.85.221.70]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id us-mta-612-RQ9fkL3jPLOTpDxwOf_EMw-1; Wed, 23 Feb 2022 16:33:19 -0500 X-MC-Unique: RQ9fkL3jPLOTpDxwOf_EMw-1 Received: by mail-wr1-f70.google.com with SMTP id j8-20020adfc688000000b001e3322ced69so10767088wrg.13 for ; Wed, 23 Feb 2022 13:33:19 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to; bh=JVrDSsHDee470/2N5kdB0CUYnm9UIRcjrM+Vqrir8YU=; b=6z1VHLFkQWkBkmzOrO8Q2SRr9xtQu/s6RFDta/6Fh7a8t2xMBagPmv4BWlyOya941T 4muWdzthqbX1n0PSuNxaoDQvuPzQXcIwNwNoxu5l6TH66vCpFDabYdSMbmhvY0XCziYW wPezdU2B9DFBdGO9HBwI5tUoI5O4QomwjZPHe4xn1ePqEeR9vnh3T3/TxQ/GmhlDi6Qe FKU2K+/4yxouqpZQ3fLXoVy2rx2JHawMguCr7mS0egKJ05p9PlZoa0lfYYOg7DDSxEA+ VrmKIxYOgvIkgRLCjwcU+Q4lEk/ZGTgb0IGBUz30AKnUDRAua7alwX+ZwrKm5rxxjAz/ BaSA== X-Gm-Message-State: AOAM531GLJt9thVgYMar6FkkBYnG0nXrkOEnlfMiDjxED5/hRXOZPLSP 0+GoVqnu9rnE46JfMUulm03Rr8LuSdIGnH1o+s1mHG9zvuUIGy2ppcSLCiL8356TG54aa4m4vqz Wulr/rcZMhW3P69znIqsGHtbREA== X-Received: by 2002:a5d:6f0a:0:b0:1e4:a354:a7e with SMTP id ay10-20020a5d6f0a000000b001e4a3540a7emr1081139wrb.423.1645651998035; Wed, 23 Feb 2022 13:33:18 -0800 (PST) X-Google-Smtp-Source: ABdhPJzVh3zKelOjcgzztRyyTOBFC5U69RZ7Dwi6phYx+UQ7pZLlXHi6mmPrXmx8tSFwquULC08JMw== X-Received: by 2002:a5d:6f0a:0:b0:1e4:a354:a7e with SMTP id ay10-20020a5d6f0a000000b001e4a3540a7emr1081114wrb.423.1645651997698; Wed, 23 Feb 2022 13:33:17 -0800 (PST) Received: from redhat.com ([2.55.166.187]) by smtp.gmail.com with ESMTPSA id l5sm677279wmq.7.2022.02.23.13.33.12 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Wed, 23 Feb 2022 13:33:17 -0800 (PST) Date: Wed, 23 Feb 2022 16:33:10 -0500 From: "Michael S. Tsirkin" To: Krzysztof Kozlowski 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 Subject: Re: [PATCH v2 01/11] driver: platform: add and use helper for safer setting of driver_override Message-ID: <20220223162538-mutt-send-email-mst@kernel.org> References: <20220223191310.347669-1-krzysztof.kozlowski@canonical.com> <20220223191310.347669-2-krzysztof.kozlowski@canonical.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20220223191310.347669-2-krzysztof.kozlowski@canonical.com> Precedence: bulk List-ID: X-Mailing-List: linux-arm-msm@vger.kernel.org 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? Do all callers really validate that it's NUL terminated? > + * string to clear it > + * > + * Helper to setr or clear driver override in a device, intended for the cases set? > + * 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? > + 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. > + *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? We will then also avoid the need to do strlen and strchr. > - driver_override = kstrndup(buf, count, GFP_KERNEL); > - if (!driver_override) > - return -ENOMEM; > - > - cp = strchr(driver_override, '\n'); > - if (cp) > - *cp = '\0'; > - > - device_lock(dev); > - old = pdev->driver_override; > - if (strlen(driver_override)) { > - pdev->driver_override = driver_override; > - } else { > - kfree(driver_override); > - pdev->driver_override = NULL; > - } > - device_unlock(dev); > - > - kfree(old); > + ret = driver_set_override(dev, &pdev->driver_override, buf); > + if (ret) > + return ret; > > return count; > } > diff --git a/include/linux/device/driver.h b/include/linux/device/driver.h > index 15e7c5e15d62..81c0d9f65a40 100644 > --- a/include/linux/device/driver.h > +++ b/include/linux/device/driver.h > @@ -151,6 +151,7 @@ extern int __must_check driver_create_file(struct device_driver *driver, > extern void driver_remove_file(struct device_driver *driver, > const struct driver_attribute *attr); > > +int driver_set_override(struct device *dev, char **override, const char *s); > extern int __must_check driver_for_each_device(struct device_driver *drv, > struct device *start, > void *data, > diff --git a/include/linux/platform_device.h b/include/linux/platform_device.h > index 7c96f169d274..37ac14459499 100644 > --- a/include/linux/platform_device.h > +++ b/include/linux/platform_device.h > @@ -31,7 +31,11 @@ struct platform_device { > struct resource *resource; > > const struct platform_device_id *id_entry; > - char *driver_override; /* Driver name to force a match */ > + /* > + * Driver name to force a match, use > + * driver_set_override() to set or clear it. > + */ > + char *driver_override; > > /* MFD cell pointer */ > struct mfd_cell *mfd_cell; > -- > 2.32.0 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 smtp1.osuosl.org (smtp1.osuosl.org [140.211.166.138]) (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 B022BC433EF for ; Wed, 23 Feb 2022 21:33:27 +0000 (UTC) Received: from localhost (localhost [127.0.0.1]) by smtp1.osuosl.org (Postfix) with ESMTP id 3565682C21; Wed, 23 Feb 2022 21:33:27 +0000 (UTC) X-Virus-Scanned: amavisd-new at osuosl.org Received: from smtp1.osuosl.org ([127.0.0.1]) by localhost (smtp1.osuosl.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id 7sVyGyYGDWic; Wed, 23 Feb 2022 21:33:26 +0000 (UTC) Received: from lists.linuxfoundation.org (lf-lists.osuosl.org [IPv6:2605:bc80:3010:104::8cd3:938]) by smtp1.osuosl.org (Postfix) with ESMTPS id 9F2EE82969; Wed, 23 Feb 2022 21:33:25 +0000 (UTC) Received: from lf-lists.osuosl.org (localhost [127.0.0.1]) by lists.linuxfoundation.org (Postfix) with ESMTP id 5DE19C001A; Wed, 23 Feb 2022 21:33:25 +0000 (UTC) Received: from smtp1.osuosl.org (smtp1.osuosl.org [140.211.166.138]) by lists.linuxfoundation.org (Postfix) with ESMTP id 9D4CAC0011 for ; Wed, 23 Feb 2022 21:33:23 +0000 (UTC) Received: from localhost (localhost [127.0.0.1]) by smtp1.osuosl.org (Postfix) with ESMTP id 7374D82BC4 for ; Wed, 23 Feb 2022 21:33:23 +0000 (UTC) X-Virus-Scanned: amavisd-new at osuosl.org Received: from smtp1.osuosl.org ([127.0.0.1]) by localhost (smtp1.osuosl.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id 1CHSnoOW5Hei for ; Wed, 23 Feb 2022 21:33:22 +0000 (UTC) X-Greylist: domain auto-whitelisted by SQLgrey-1.8.0 Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.133.124]) by smtp1.osuosl.org (Postfix) with ESMTPS id 1230D82969 for ; Wed, 23 Feb 2022 21:33:21 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1645652000; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: in-reply-to:in-reply-to:references:references; bh=JVrDSsHDee470/2N5kdB0CUYnm9UIRcjrM+Vqrir8YU=; b=RAzwl9Sez1TeT+nmEa1yYBZAZQ4KRu+18SqJ5YvCiLsN5ZoOFX4CZPFTlGdfYriTr4/+WA XXnR0ENpAnH0WVYQ6IDWFU8hF6VB+UXAFAgohlr+Zsldn+5r5XNapYpdZ3IxnHR1NhHHKK OVHzWqSkiwNvtpVRgw7agSTzlkdlpqo= Received: from mail-wr1-f71.google.com (mail-wr1-f71.google.com [209.85.221.71]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id us-mta-612-mQODqp_eOBWVNH3ljayvYw-1; Wed, 23 Feb 2022 16:33:19 -0500 X-MC-Unique: mQODqp_eOBWVNH3ljayvYw-1 Received: by mail-wr1-f71.google.com with SMTP id f14-20020adfc98e000000b001e8593b40b0so10061100wrh.14 for ; Wed, 23 Feb 2022 13:33:19 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to; bh=JVrDSsHDee470/2N5kdB0CUYnm9UIRcjrM+Vqrir8YU=; b=dqVaD1rlRTgg4kHQe3Ax1VUoR+q17pMrOdyUjYSufndLkUO4xXAQZWfC6cd8nDdswU 0l03gyVRLVWKMiYq8KPkWRMwyF2f4TOkRcbQO3TdkiU47MdAZtRCM0xq/Y2nxbEUHqqH 2mWjylVV0lcDlOA/V2jvb6DiS1Ml+aUyrpbl6ETusG3kl9fF6qY8sJMmwRVlyQlHssUk fCWTJIQfGnMzrMtN9cPifJ/II0yvNra20dQyi/NPmgU/3ugUxcjXpnDO7ztYWLotkAWH jgIhtFJ0rjepDfZFjEoUo878KB3BpsMon1bpS912W8QWHqims1Qhg2TkkYyFCFQzGai6 O3ow== X-Gm-Message-State: AOAM5335awJjwjoGmdTBBcGr8juaMz1detA7HDAv+D7OngP240j7b6cZ BPqOZPwzbEN3ZJzckwgzeVUYa0cBD6VXdb7cQ13mhawNFE9WoSDK2cs4AfMcRpoM9TKAYh/mLOT 7f5T4ygErzKyQCLyPwYdUeR/C7nbD7XB5XxoG6iyN5g== X-Received: by 2002:a5d:6f0a:0:b0:1e4:a354:a7e with SMTP id ay10-20020a5d6f0a000000b001e4a3540a7emr1081132wrb.423.1645651998034; Wed, 23 Feb 2022 13:33:18 -0800 (PST) X-Google-Smtp-Source: ABdhPJzVh3zKelOjcgzztRyyTOBFC5U69RZ7Dwi6phYx+UQ7pZLlXHi6mmPrXmx8tSFwquULC08JMw== X-Received: by 2002:a5d:6f0a:0:b0:1e4:a354:a7e with SMTP id ay10-20020a5d6f0a000000b001e4a3540a7emr1081114wrb.423.1645651997698; Wed, 23 Feb 2022 13:33:17 -0800 (PST) Received: from redhat.com ([2.55.166.187]) by smtp.gmail.com with ESMTPSA id l5sm677279wmq.7.2022.02.23.13.33.12 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Wed, 23 Feb 2022 13:33:17 -0800 (PST) Date: Wed, 23 Feb 2022 16:33:10 -0500 From: "Michael S. Tsirkin" To: Krzysztof Kozlowski Subject: Re: [PATCH v2 01/11] driver: platform: add and use helper for safer setting of driver_override Message-ID: <20220223162538-mutt-send-email-mst@kernel.org> References: <20220223191310.347669-1-krzysztof.kozlowski@canonical.com> <20220223191310.347669-2-krzysztof.kozlowski@canonical.com> MIME-Version: 1.0 In-Reply-To: <20220223191310.347669-2-krzysztof.kozlowski@canonical.com> Authentication-Results: relay.mimecast.com; auth=pass smtp.auth=CUSA124A263 smtp.mailfrom=mst@redhat.com X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Disposition: inline Cc: linux-hyperv@vger.kernel.org, Stuart Yoder , "Rafael J. Wysocki" , linux-pci@vger.kernel.org, linux-remoteproc@vger.kernel.org, alsa-devel@alsa-project.org, Bjorn Andersson , Srinivas Kandagatla , Vineeth Vijayan , Alexander Gordeev , Fabio Estevam , linux-clk@vger.kernel.org, linux-s390@vger.kernel.org, Wei Liu , Stephen Hemminger , Abel Vesa , Dexuan Cui , Linus Torvalds , Andy Gross , NXP Linux Team , Heiko Carstens , Vasily Gorbik , linux-arm-msm@vger.kernel.org, Sascha Hauer , linux-spi@vger.kernel.org, Mark Brown , Rasmus Villemoes , Bjorn Helgaas , virtualization@lists.linux-foundation.org, linux-arm-kernel@lists.infradead.org, Laurentiu Tudor , Mathieu Poirier , Greg Kroah-Hartman , Haiyang Zhang , Peter Oberparleiter , linux-kernel@vger.kernel.org, Sven Schnelle , Shawn Guo X-BeenThere: virtualization@lists.linux-foundation.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: Linux virtualization List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Errors-To: virtualization-bounces@lists.linux-foundation.org Sender: "Virtualization" 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? Do all callers really validate that it's NUL terminated? > + * string to clear it > + * > + * Helper to setr or clear driver override in a device, intended for the cases set? > + * 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? > + 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. > + *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? We will then also avoid the need to do strlen and strchr. > - driver_override = kstrndup(buf, count, GFP_KERNEL); > - if (!driver_override) > - return -ENOMEM; > - > - cp = strchr(driver_override, '\n'); > - if (cp) > - *cp = '\0'; > - > - device_lock(dev); > - old = pdev->driver_override; > - if (strlen(driver_override)) { > - pdev->driver_override = driver_override; > - } else { > - kfree(driver_override); > - pdev->driver_override = NULL; > - } > - device_unlock(dev); > - > - kfree(old); > + ret = driver_set_override(dev, &pdev->driver_override, buf); > + if (ret) > + return ret; > > return count; > } > diff --git a/include/linux/device/driver.h b/include/linux/device/driver.h > index 15e7c5e15d62..81c0d9f65a40 100644 > --- a/include/linux/device/driver.h > +++ b/include/linux/device/driver.h > @@ -151,6 +151,7 @@ extern int __must_check driver_create_file(struct device_driver *driver, > extern void driver_remove_file(struct device_driver *driver, > const struct driver_attribute *attr); > > +int driver_set_override(struct device *dev, char **override, const char *s); > extern int __must_check driver_for_each_device(struct device_driver *drv, > struct device *start, > void *data, > diff --git a/include/linux/platform_device.h b/include/linux/platform_device.h > index 7c96f169d274..37ac14459499 100644 > --- a/include/linux/platform_device.h > +++ b/include/linux/platform_device.h > @@ -31,7 +31,11 @@ struct platform_device { > struct resource *resource; > > const struct platform_device_id *id_entry; > - char *driver_override; /* Driver name to force a match */ > + /* > + * Driver name to force a match, use > + * driver_set_override() to set or clear it. > + */ > + char *driver_override; > > /* MFD cell pointer */ > struct mfd_cell *mfd_cell; > -- > 2.32.0 _______________________________________________ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization 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 0E47CC433EF for ; Wed, 23 Feb 2022 21:34:50 +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:MIME-Version:References: Message-ID:Subject:Cc:To:From:Date:Reply-To:Content-ID:Content-Description: Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID: List-Owner; bh=W4rnjqMmrXlboo4Oz5lkedU9T0EKtm0CdxCTeCuvZwA=; b=WQsGbiNnZSsTGR Yc0N5dgbbhX/czMihFSUhoeXvk2uXygmExEP9lP1WK3LVbXaWgrPe1vzPDzhWEYU0dwBIWf9U9R6C KhW/c0i7CRgOFUXv2apZdj1TqgHq9MN8MPm6IS+QzIUk9LRh3xVcZAscEuaP6yIYEHJrj03Q0vCEq 9D911/myxRoo9PbfPaZ5oCjNvpiPYD5PCpHBK2zihLBb9xi6EXmnu1FNJG2yQOCxopYggLqCIKmfb umW7/OC/7wwIxs1n1fCvMzDysWy8r6cz/+syFbHLlGaBW5y7gtsD+558TYQNPG99Q7PNEkb0ICCj3 QWc7oisYEUx26RymoYvA==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.94.2 #2 (Red Hat Linux)) id 1nMzGM-00G71b-RE; Wed, 23 Feb 2022 21:33:31 +0000 Received: from us-smtp-delivery-124.mimecast.com ([170.10.129.124]) by bombadil.infradead.org with esmtps (Exim 4.94.2 #2 (Red Hat Linux)) id 1nMzGI-00G6zl-2k for linux-arm-kernel@lists.infradead.org; Wed, 23 Feb 2022 21:33:28 +0000 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1645652000; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: in-reply-to:in-reply-to:references:references; bh=JVrDSsHDee470/2N5kdB0CUYnm9UIRcjrM+Vqrir8YU=; b=RAzwl9Sez1TeT+nmEa1yYBZAZQ4KRu+18SqJ5YvCiLsN5ZoOFX4CZPFTlGdfYriTr4/+WA XXnR0ENpAnH0WVYQ6IDWFU8hF6VB+UXAFAgohlr+Zsldn+5r5XNapYpdZ3IxnHR1NhHHKK OVHzWqSkiwNvtpVRgw7agSTzlkdlpqo= Received: from mail-wr1-f70.google.com (mail-wr1-f70.google.com [209.85.221.70]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id us-mta-529-2N03cq-MN7OA6LUzIwgRFQ-1; Wed, 23 Feb 2022 16:33:19 -0500 X-MC-Unique: 2N03cq-MN7OA6LUzIwgRFQ-1 Received: by mail-wr1-f70.google.com with SMTP id m2-20020adff382000000b001edbdae0527so11001wro.2 for ; Wed, 23 Feb 2022 13:33:19 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to; bh=JVrDSsHDee470/2N5kdB0CUYnm9UIRcjrM+Vqrir8YU=; b=iGvKVvubICCccrKgPy7hvUOuFQ0q88O/EF5qJ0sypUdDHp6OrEgHLaZpluMwbkRqPe XR1gEJqqWWxNT8523wPFXbNWAV7MoqQwxAe62NACpu0t0eEqV42WyhlUGRAmNQKeEeqX TIcAsQyRKOvC8Q6fmMaxBmY0IjQ3VYWfyTvd89L/lzSoJSkcZcyXf/zFoyAZ/JyT5G94 qWKpbX3eTihVZmwORXKNya1Qpb0ojKw3CfKyJj0T3iISvvYNY8+5xU+qqAh/W7T5fA/Z ldDSAnao14kAoQjgGDtaMQrTGXZfUWlbYPVp0pduwqlATGM40q0Lsj7IMOM+SgeN2/rj fIsQ== X-Gm-Message-State: AOAM5304bXLUPT+f9mw6Wt8qR0GZyBLFSHtxrUU+TTgnFxTXZ7BBzWqF LpZhrNK8UgoHcjQfhyjYbatWSSB0Ul+NQTg5omj1DhFaCkD8fys+3xDq3fk9/zgoKaCZrqM7xCg BuRh1xsJG/Eq87wTtee1xzv8QVfMuffwhGkE= X-Received: by 2002:a5d:6f0a:0:b0:1e4:a354:a7e with SMTP id ay10-20020a5d6f0a000000b001e4a3540a7emr1081128wrb.423.1645651998034; Wed, 23 Feb 2022 13:33:18 -0800 (PST) X-Google-Smtp-Source: ABdhPJzVh3zKelOjcgzztRyyTOBFC5U69RZ7Dwi6phYx+UQ7pZLlXHi6mmPrXmx8tSFwquULC08JMw== X-Received: by 2002:a5d:6f0a:0:b0:1e4:a354:a7e with SMTP id ay10-20020a5d6f0a000000b001e4a3540a7emr1081114wrb.423.1645651997698; Wed, 23 Feb 2022 13:33:17 -0800 (PST) Received: from redhat.com ([2.55.166.187]) by smtp.gmail.com with ESMTPSA id l5sm677279wmq.7.2022.02.23.13.33.12 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Wed, 23 Feb 2022 13:33:17 -0800 (PST) Date: Wed, 23 Feb 2022 16:33:10 -0500 From: "Michael S. Tsirkin" To: Krzysztof Kozlowski 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 Subject: Re: [PATCH v2 01/11] driver: platform: add and use helper for safer setting of driver_override Message-ID: <20220223162538-mutt-send-email-mst@kernel.org> References: <20220223191310.347669-1-krzysztof.kozlowski@canonical.com> <20220223191310.347669-2-krzysztof.kozlowski@canonical.com> MIME-Version: 1.0 In-Reply-To: <20220223191310.347669-2-krzysztof.kozlowski@canonical.com> Authentication-Results: relay.mimecast.com; auth=pass smtp.auth=CUSA124A263 smtp.mailfrom=mst@redhat.com X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Disposition: inline X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20220223_133326_232638_213705D0 X-CRM114-Status: GOOD ( 38.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 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? Do all callers really validate that it's NUL terminated? > + * string to clear it > + * > + * Helper to setr or clear driver override in a device, intended for the cases set? > + * 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? > + 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. > + *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? We will then also avoid the need to do strlen and strchr. > - driver_override = kstrndup(buf, count, GFP_KERNEL); > - if (!driver_override) > - return -ENOMEM; > - > - cp = strchr(driver_override, '\n'); > - if (cp) > - *cp = '\0'; > - > - device_lock(dev); > - old = pdev->driver_override; > - if (strlen(driver_override)) { > - pdev->driver_override = driver_override; > - } else { > - kfree(driver_override); > - pdev->driver_override = NULL; > - } > - device_unlock(dev); > - > - kfree(old); > + ret = driver_set_override(dev, &pdev->driver_override, buf); > + if (ret) > + return ret; > > return count; > } > diff --git a/include/linux/device/driver.h b/include/linux/device/driver.h > index 15e7c5e15d62..81c0d9f65a40 100644 > --- a/include/linux/device/driver.h > +++ b/include/linux/device/driver.h > @@ -151,6 +151,7 @@ extern int __must_check driver_create_file(struct device_driver *driver, > extern void driver_remove_file(struct device_driver *driver, > const struct driver_attribute *attr); > > +int driver_set_override(struct device *dev, char **override, const char *s); > extern int __must_check driver_for_each_device(struct device_driver *drv, > struct device *start, > void *data, > diff --git a/include/linux/platform_device.h b/include/linux/platform_device.h > index 7c96f169d274..37ac14459499 100644 > --- a/include/linux/platform_device.h > +++ b/include/linux/platform_device.h > @@ -31,7 +31,11 @@ struct platform_device { > struct resource *resource; > > const struct platform_device_id *id_entry; > - char *driver_override; /* Driver name to force a match */ > + /* > + * Driver name to force a match, use > + * driver_set_override() to set or clear it. > + */ > + char *driver_override; > > /* MFD cell pointer */ > struct mfd_cell *mfd_cell; > -- > 2.32.0 _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel