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 2F317C433EF for ; Tue, 15 Mar 2022 18:01:14 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1350191AbiCOSCY (ORCPT ); Tue, 15 Mar 2022 14:02:24 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:41014 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S243063AbiCOSCY (ORCPT ); Tue, 15 Mar 2022 14:02:24 -0400 Received: from mail-ed1-x52f.google.com (mail-ed1-x52f.google.com [IPv6:2a00:1450:4864:20::52f]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id A329D58E6E; Tue, 15 Mar 2022 11:01:11 -0700 (PDT) Received: by mail-ed1-x52f.google.com with SMTP id h13so25237638ede.5; Tue, 15 Mar 2022 11:01:11 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20210112; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=9egEby/kGDQx/H2AywacJdMEq9UWb27T57iBP6MKY/M=; b=X459zaz8rBpHmz1eWRM/nXKPoCcWwgv93mgo1kUIqKJmmrFSVbFmtlBkSyc7oZbqEA 9iE/wpN2ldzWFyDARevE8q4aWwftUcTWrj/hZGCp5c/nWOErfTWiotj502O2E3wRub8x fAvwgbWwEpPPMZKlHleAIZ5b3NuCDHQaUCu3o6zMz3Pk9oBOWUhxp7FqU8z4OYlvff33 vjwTCIZda48THwgM2za8HjIs/dPh3/+wx8u7uUvYGGau/xbCmqh8QSdYU72RKq3dM+TV 7s/9EfIhX6BorS0N3xa1MT7AdVO4JDq0rPwvec+a1/gkrmCnUuEgVF5WsgvwobwSbCSf ZFuQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=9egEby/kGDQx/H2AywacJdMEq9UWb27T57iBP6MKY/M=; b=XzfM2y1/piwXOHyvUJCRTcLMyV5F25NwyAJ3OpzE3GPeEYCL6U9QtqD23JMPJV0Dlm 7Cd9roX0kyKTR3QEyPNBz7P1CTQQdguanEMw4FSfhmX7UWjfUgLeQiCFa+zIQZPXBRbP fVruSaKpdK9Nj2pQUqrEVlgMh/AZYNyEU8/ABFX10M8ukj2vAlNcIGthtma/HVPMpqFG eqHaWFywtWxx7W7kKn/3gr//Ca7cgl6MKQVOIjdnR0sT7WwYJmwA+p+LxEmrnryMr8zr 3zGqTbm/cgWVzJypTcsv1YoAaYPodndYKENU1w65gMnmadBR4NPRWrr/poSTTz9kAVwH +/lQ== X-Gm-Message-State: AOAM530LkyqLKpknuIcsqFSJWgP9d+kI2fTmX20AJM3dDQJS36PD0aSm M3ETKmWVYbHCuaFa4FQN0K7Dpq3rsDCIzIWhgY0= X-Google-Smtp-Source: ABdhPJz09b9Fm75ixFJtzUGUkS6dUid3EAUCrWF+jfN6zd2zyejpSwybW8/2BkPFf2PjWP6mgqXTNKVgn2eTNF92NTk= X-Received: by 2002:a05:6402:3589:b0:416:7de7:cdde with SMTP id y9-20020a056402358900b004167de7cddemr26440456edc.218.1647367269998; Tue, 15 Mar 2022 11:01:09 -0700 (PDT) MIME-Version: 1.0 References: <20220312132856.65163-1-krzysztof.kozlowski@canonical.com> <20220312132856.65163-2-krzysztof.kozlowski@canonical.com> In-Reply-To: <20220312132856.65163-2-krzysztof.kozlowski@canonical.com> From: Andy Shevchenko Date: Tue, 15 Mar 2022 19:59:56 +0200 Message-ID: Subject: Re: [PATCH v4 01/11] driver: platform: Add helper for safer setting of driver_override 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 , "Michael S. Tsirkin" , Jason Wang , Linux Kernel Mailing List , linux-clk , NXP Linux Team , linux-arm Mailing List , Linux on Hyper-V List , linux-pci , linux-remoteproc@vger.kernel.org, linux-s390@vger.kernel.org, linux-arm-msm@vger.kernel.org, ALSA Development Mailing List , linux-spi , virtualization@lists.linux-foundation.org, Linus Torvalds , Rasmus Villemoes Content-Type: text/plain; charset="UTF-8" Precedence: bulk List-ID: X-Mailing-List: linux-pci@vger.kernel.org On Sat, Mar 12, 2022 at 5:16 PM 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) I believe you may remove these three. > Provide a helper which clearly documents the usage of driver_override. > This will allow later to reuse the helper and reduce amount of the amount > duplicated code. > Convert the platform driver to use new helper and make the a new > driver_override field const char (it is not modified by the core). ... > +/** > + * driver_set_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: NUL terminated string, new driver name to force a match, pass empty NUL-terminated? (44 vs 115 occurrences) > + * string to clear it > + * @len: length of @s > + * > + * Helper to set or clear driver override in a device, intended for the cases > + * 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, const char **override, > + const char *s, size_t len) > +{ > + const char *new, *old; > + char *cp; > + > + if (!dev || !override || !s) > + return -EINVAL; > + > + /* > + * The stored value will be used in sysfs show callback (sysfs_emit()), > + * which has a length limit of PAGE_SIZE and adds a trailing newline. > + * Thus we can store one character less to avoid truncation during sysfs > + * show. > + */ > + if (len >= (PAGE_SIZE - 1)) > + return -EINVAL; > + > + new = kstrndup(s, len, GFP_KERNEL); > + if (!new) > + return -ENOMEM; > + > + cp = strchr(new, '\n'); > + if (cp) > + *cp = '\0'; AFAIU you may reduce memory footprint by cp = strnchr(new, len, '\n'); if (cp) len = s - cp; new = kstrndup(...); > + device_lock(dev); > + old = *override; > + if (cp != new) { > + *override = new; > + } else { > + kfree(new); > + *override = NULL; > + } > + device_unlock(dev); > + > + kfree(old); > + > + return 0; > +} -- With Best Regards, Andy Shevchenko