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 58484C4332F for ; Mon, 4 Apr 2022 09:35:00 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1354324AbiDDJgy (ORCPT ); Mon, 4 Apr 2022 05:36:54 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:37540 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1354242AbiDDJgx (ORCPT ); Mon, 4 Apr 2022 05:36:53 -0400 Received: from mail-wm1-x32a.google.com (mail-wm1-x32a.google.com [IPv6:2a00:1450:4864:20::32a]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id E58E427179 for ; Mon, 4 Apr 2022 02:34:55 -0700 (PDT) Received: by mail-wm1-x32a.google.com with SMTP id h16so5544943wmd.0 for ; Mon, 04 Apr 2022 02:34:55 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=message-id:date:mime-version:user-agent:subject:content-language:to :cc:references:from:in-reply-to:content-transfer-encoding; bh=xry0fvSIyDxjB2H9TftOM1VVr7Z84GMJx65XGE7l1g8=; b=QyIkbrvP3PXHYn8SZb3YFGNOU4YEWKwijKbTOMes2v3n/iFjy3E0l83+3PU1OyKdiC 28W9oIBBI4K+hraVFUC4rWiUr33rPk2ySKcvsAeR22WUXi4iM2aHnZd8Fn1GeAeirYrD vpYCRqjh9PypWsECcQ9s8YJ0IOwuqrtnIou1JtT8aMLsJeQiP8+/F14BOx45Pzoc5hOs zFVa0z20WuRvfR9IM3W6B7NXFHL5NIRr3q2/9wWYRFFUZeBeGzMux4sUyMGNAuZjF3t0 mwwwBGsWFJCRgizWUZXY7qrl7cdc2T3DX58d8hKruK4mcWNDpPeP1QmPVvKj4hi4kGzf uymg== 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=xry0fvSIyDxjB2H9TftOM1VVr7Z84GMJx65XGE7l1g8=; b=eMPlxmQQa0oRZvAdCRaSJ51eb/v9QT4GXocwvWZ0Sbh0SSFz8XB2A1WzMirISIefKG bi6/iyh6lJmaEAcAw6Ymdzqdpgh8Bm7VH0NKs2N8ksAyVnwdy/ge9b4cBn1uNU8brcH9 ozflgM9XFtBNmN5WGD2R6ZYtg+zAeiNV/xiSE9ASj78hE4LXiUmiQlhq9ozafmPHP78W uZuO7Ht34KCaXaLvahmftxLsyasdO7Hi8Z7V0hrFCAyyFHwko3hF9f/NburVHtGvEbgX hnGHSgP1WV5bGjgSajnso9bglUIJXGJr1zUMjVc+72p2mZaet3JwralEuzy5L7Sn0JBU HShg== X-Gm-Message-State: AOAM531TAxHIN+Kh0hpxvROpjt2v+QhIi3Ll9RsqYtdPYuNEur/R324i 2sh9fllPEsVjIJ4hJmpaHUGIDQ== X-Google-Smtp-Source: ABdhPJw+vbnPc6vft6RqA6zSnbRtvDlS1NwlZFp269pvENQ083BNJ7dEdI8BO5JpR9Ta6ymEkaX/jQ== X-Received: by 2002:a05:600c:3487:b0:38c:9a42:4d49 with SMTP id a7-20020a05600c348700b0038c9a424d49mr18900226wmq.29.1649064894420; Mon, 04 Apr 2022 02:34:54 -0700 (PDT) Received: from [192.168.0.173] (xdsl-188-155-201-27.adslplus.ch. [188.155.201.27]) by smtp.gmail.com with ESMTPSA id o4-20020a5d6484000000b002057ad822d4sm9143811wri.48.2022.04.04.02.34.53 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Mon, 04 Apr 2022 02:34:54 -0700 (PDT) Message-ID: <2976f4f9-4fda-c04f-45cf-351518f88ec0@linaro.org> Date: Mon, 4 Apr 2022 11:34:52 +0200 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101 Thunderbird/91.7.0 Subject: Re: [PATCH v6 01/12] driver: platform: Add helper for safer setting of driver_override Content-Language: en-US To: Andy Shevchenko Cc: Greg Kroah-Hartman , "Rafael J. Wysocki" , Stuart Yoder , "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 , 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 References: <20220403183758.192236-1-krzysztof.kozlowski@linaro.org> <20220403183758.192236-2-krzysztof.kozlowski@linaro.org> From: Krzysztof Kozlowski In-Reply-To: Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit Precedence: bulk List-ID: X-Mailing-List: linux-remoteproc@vger.kernel.org On 04/04/2022 11:16, Andy Shevchenko wrote: > On Sun, Apr 3, 2022 at 9:38 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) >> >> Provide a helper which clearly documents the usage of driver_override. >> This will allow later to reuse the helper and reduce the amount of >> duplicated code. >> >> Convert the platform driver to use a new helper and make the >> driver_override field const char (it is not modified by the core). > > ... > >> +int driver_set_override(struct device *dev, const char **override, >> + const char *s, size_t len) >> +{ >> + const char *new, *old; >> + char *cp; > >> + if (!override || !s) >> + return -EINVAL; > > Still not sure if we should distinguish (s == NULL && len == 0) from > (s != NULL && len == 0). > Supplying the latter seems confusing (yes, I see that in the old code). Perhaps > !s test, in case you want to leave it, should be also commented. The old semantics were focused on sysfs usage, so clearing is by passing an empty string. In the case of sysfs empty string is actually "\n". I intend to keep the semantics also for the in-kernel usage and in such case empty string can be also "". If I understand your comment correctly, you propose to change it to NULL for in-kernel usage, but that would change the semantics. > Another approach is to split above to two checks and move !s after !len. I don't follow why... The !override and !s are invalid uses. !len is a valid user for internal callers, just like "\n" is for sysfs. > >> + /* >> + * 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; > > Perhaps explain the case in the comment here? You mean the case we discuss here (to clear override with "")? Sure. > >> + if (!len) { >> + device_lock(dev); >> + old = *override; >> + *override = NULL; > >> + device_unlock(dev); >> + goto out_free; > > You may deduplicate this one, by > > goto out_unlock_free; > > But I understand your intention to keep lock-unlock in one place, so > perhaps dropping that label would be even better in this case and > keeping it Yes, exactly. > > kfree(old); > return 0; > > here instead of goto. Slightly more code, but indeed maybe easier to follow. I'll do like this. Best regards, Krzysztof 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 267C6C433EF for ; Mon, 4 Apr 2022 09:36:02 +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=kVb8TMw/45Tz6SOBlqAcajd1p2M5hUwu3bqFEjIQy3M=; b=T4DI7nvgd5lo12 ar4stoHTb9fNK87qmg+I5ibWhpQbXgcx6dofnBsNzCTrw8mPEm6a+wRaFGchaGXolweB69Wp/4B65 EetazgXFJoBCb7WIUSljAwo89WSrkhC/r4QlLfrQjG83HGl4Uh/LiAA0kdbkBNOFUOp/lb8pJZBW5 IjAvcpvZfUjSVamjxB61EeBlQp8I+5F8WJf+dpo+nAHne6kuutS2h58UJX3WHGeJjwBvIO9tHh1zA nxZnfGrUUW70PJMw+Ovg1tDbDLru5oikEkjl1hRYseRx5mUUc83CMEjkj8PFhv48UXJMU9rhgG0jd metELf6oMW4YJt+4gwKA==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.94.2 #2 (Red Hat Linux)) id 1nbJ6z-00EDDk-CQ; Mon, 04 Apr 2022 09:35:01 +0000 Received: from mail-wm1-x32c.google.com ([2a00:1450:4864:20::32c]) by bombadil.infradead.org with esmtps (Exim 4.94.2 #2 (Red Hat Linux)) id 1nbJ6w-00EDBq-94 for linux-arm-kernel@lists.infradead.org; Mon, 04 Apr 2022 09:34:59 +0000 Received: by mail-wm1-x32c.google.com with SMTP id n63-20020a1c2742000000b0038d0c31db6eso5221336wmn.1 for ; Mon, 04 Apr 2022 02:34:55 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=message-id:date:mime-version:user-agent:subject:content-language:to :cc:references:from:in-reply-to:content-transfer-encoding; bh=xry0fvSIyDxjB2H9TftOM1VVr7Z84GMJx65XGE7l1g8=; b=QyIkbrvP3PXHYn8SZb3YFGNOU4YEWKwijKbTOMes2v3n/iFjy3E0l83+3PU1OyKdiC 28W9oIBBI4K+hraVFUC4rWiUr33rPk2ySKcvsAeR22WUXi4iM2aHnZd8Fn1GeAeirYrD vpYCRqjh9PypWsECcQ9s8YJ0IOwuqrtnIou1JtT8aMLsJeQiP8+/F14BOx45Pzoc5hOs zFVa0z20WuRvfR9IM3W6B7NXFHL5NIRr3q2/9wWYRFFUZeBeGzMux4sUyMGNAuZjF3t0 mwwwBGsWFJCRgizWUZXY7qrl7cdc2T3DX58d8hKruK4mcWNDpPeP1QmPVvKj4hi4kGzf uymg== 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=xry0fvSIyDxjB2H9TftOM1VVr7Z84GMJx65XGE7l1g8=; b=P8utyM9MCG8e2KfLzQs9/wnKM05IU2yRFp8R+rRJAmxV3k4SIm4bWS5PdQijpXLEZL Ec8Se3LZhbG2aO/q16v6vTOIR2l77tBH8BnFof8Decqx4chKF5GTvy8rLKWldZYu+euy ZdYGIEwDZ1Ofg8KQmr/9X7poCBfhQXAXPHOFfbRonzFveBQbirR5NasQK5hkDl2bHYbL KOAj1/yOTZPIDYpm6R4/EHKYZa8gik+NE1gyLQPHXC8IiIBxMiGsfeiqmOLG1KgtnbXz aobJXMXAPtQxNTMEXn9FYUbhOHDM66P7jtu57UMdVAEkITUwl3gdOHhAk+/mN6C9lEfl J62Q== X-Gm-Message-State: AOAM532eVp/fFhiIgDOmpvUCyRtmtdp9hFK07QCu4wK+J45SvLUkpekk UJeEg4ySTIkLkyvEtd8i8p4KxA== X-Google-Smtp-Source: ABdhPJw+vbnPc6vft6RqA6zSnbRtvDlS1NwlZFp269pvENQ083BNJ7dEdI8BO5JpR9Ta6ymEkaX/jQ== X-Received: by 2002:a05:600c:3487:b0:38c:9a42:4d49 with SMTP id a7-20020a05600c348700b0038c9a424d49mr18900226wmq.29.1649064894420; Mon, 04 Apr 2022 02:34:54 -0700 (PDT) Received: from [192.168.0.173] (xdsl-188-155-201-27.adslplus.ch. [188.155.201.27]) by smtp.gmail.com with ESMTPSA id o4-20020a5d6484000000b002057ad822d4sm9143811wri.48.2022.04.04.02.34.53 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Mon, 04 Apr 2022 02:34:54 -0700 (PDT) Message-ID: <2976f4f9-4fda-c04f-45cf-351518f88ec0@linaro.org> Date: Mon, 4 Apr 2022 11:34:52 +0200 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101 Thunderbird/91.7.0 Subject: Re: [PATCH v6 01/12] driver: platform: Add helper for safer setting of driver_override Content-Language: en-US To: Andy Shevchenko Cc: Greg Kroah-Hartman , "Rafael J. Wysocki" , Stuart Yoder , "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 , 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 References: <20220403183758.192236-1-krzysztof.kozlowski@linaro.org> <20220403183758.192236-2-krzysztof.kozlowski@linaro.org> From: Krzysztof Kozlowski In-Reply-To: X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20220404_023458_385532_D114E52E X-CRM114-Status: GOOD ( 30.19 ) 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 04/04/2022 11:16, Andy Shevchenko wrote: > On Sun, Apr 3, 2022 at 9:38 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) >> >> Provide a helper which clearly documents the usage of driver_override. >> This will allow later to reuse the helper and reduce the amount of >> duplicated code. >> >> Convert the platform driver to use a new helper and make the >> driver_override field const char (it is not modified by the core). > > ... > >> +int driver_set_override(struct device *dev, const char **override, >> + const char *s, size_t len) >> +{ >> + const char *new, *old; >> + char *cp; > >> + if (!override || !s) >> + return -EINVAL; > > Still not sure if we should distinguish (s == NULL && len == 0) from > (s != NULL && len == 0). > Supplying the latter seems confusing (yes, I see that in the old code). Perhaps > !s test, in case you want to leave it, should be also commented. The old semantics were focused on sysfs usage, so clearing is by passing an empty string. In the case of sysfs empty string is actually "\n". I intend to keep the semantics also for the in-kernel usage and in such case empty string can be also "". If I understand your comment correctly, you propose to change it to NULL for in-kernel usage, but that would change the semantics. > Another approach is to split above to two checks and move !s after !len. I don't follow why... The !override and !s are invalid uses. !len is a valid user for internal callers, just like "\n" is for sysfs. > >> + /* >> + * 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; > > Perhaps explain the case in the comment here? You mean the case we discuss here (to clear override with "")? Sure. > >> + if (!len) { >> + device_lock(dev); >> + old = *override; >> + *override = NULL; > >> + device_unlock(dev); >> + goto out_free; > > You may deduplicate this one, by > > goto out_unlock_free; > > But I understand your intention to keep lock-unlock in one place, so > perhaps dropping that label would be even better in this case and > keeping it Yes, exactly. > > kfree(old); > return 0; > > here instead of goto. Slightly more code, but indeed maybe easier to follow. I'll do like this. Best regards, Krzysztof _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel 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 719C2C433F5 for ; Fri, 8 Apr 2022 07:50:30 +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 BEC9D18E6; Fri, 8 Apr 2022 09:49:38 +0200 (CEST) DKIM-Filter: OpenDKIM Filter v2.11.0 alsa0.perex.cz BEC9D18E6 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=alsa-project.org; s=default; t=1649404228; bh=/hx8Yrf99eaTbqv/H/8PL5wwo9NJFVFS2q4y8yzBWXw=; h=Date:Subject:To:References:From:In-Reply-To:Cc:List-Id: List-Unsubscribe:List-Archive:List-Post:List-Help:List-Subscribe: From; b=mQIq3qtztVwFi04tgMrld0D+atvbIiZk3JC0jM3w9kRaudtW+H4ylCLiwoedimc07 HRzXdaOyOxXPG/wEOtAbbYHKwJykXN1A89cKsjiEeqytny+aTySln8JV5+Ke8qqBj9 Ei8Bo+uGm7oUmmg/zNfqAmq4TuhhrmCaCKQE70N0= Received: from alsa1.perex.cz (localhost.localdomain [127.0.0.1]) by alsa1.perex.cz (Postfix) with ESMTP id 5B711F80527; Fri, 8 Apr 2022 09:48:17 +0200 (CEST) Received: by alsa1.perex.cz (Postfix, from userid 50401) id 472F4F800D1; Mon, 4 Apr 2022 11:35:03 +0200 (CEST) Received: from mail-wm1-x329.google.com (mail-wm1-x329.google.com [IPv6:2a00:1450:4864:20::329]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by alsa1.perex.cz (Postfix) with ESMTPS id D9585F800D1 for ; Mon, 4 Apr 2022 11:34:56 +0200 (CEST) DKIM-Filter: OpenDKIM Filter v2.11.0 alsa1.perex.cz D9585F800D1 Authentication-Results: alsa1.perex.cz; dkim=pass (2048-bit key) header.d=linaro.org header.i=@linaro.org header.b="QyIkbrvP" Received: by mail-wm1-x329.google.com with SMTP id r7so5514278wmq.2 for ; Mon, 04 Apr 2022 02:34:56 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=message-id:date:mime-version:user-agent:subject:content-language:to :cc:references:from:in-reply-to:content-transfer-encoding; bh=xry0fvSIyDxjB2H9TftOM1VVr7Z84GMJx65XGE7l1g8=; b=QyIkbrvP3PXHYn8SZb3YFGNOU4YEWKwijKbTOMes2v3n/iFjy3E0l83+3PU1OyKdiC 28W9oIBBI4K+hraVFUC4rWiUr33rPk2ySKcvsAeR22WUXi4iM2aHnZd8Fn1GeAeirYrD vpYCRqjh9PypWsECcQ9s8YJ0IOwuqrtnIou1JtT8aMLsJeQiP8+/F14BOx45Pzoc5hOs zFVa0z20WuRvfR9IM3W6B7NXFHL5NIRr3q2/9wWYRFFUZeBeGzMux4sUyMGNAuZjF3t0 mwwwBGsWFJCRgizWUZXY7qrl7cdc2T3DX58d8hKruK4mcWNDpPeP1QmPVvKj4hi4kGzf uymg== 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=xry0fvSIyDxjB2H9TftOM1VVr7Z84GMJx65XGE7l1g8=; b=NTxAv77u8nvXhgjYPq425F62cx8b7mo5XTgdq/lB+0RFjAj7GunRxvbwhL4Ju0CX3P GHvZduhrPL97/b3ecP3QnCwqth9DofF0uJFxtrrDtS3aQqRx91iENhQNBkHJiUcB626P nK0ft1/UiJexKrVMbZCN0L9W05fsmGLoCEiVQiKB6fm9aBxyCpcogQoQBOapWAlPXxbX P4hWmxSWYsrlY2IheIN6lqImNufXWcX5EQrldO/00hoX6aAA2P7tVr8ybm0dedDKLDbt F8ts4m9L6ynRQfs7E5RXnisQw03k3fT/NWx2IUK9E52Fj3M4P9JSJIb1Qlr76XHIqyzs glug== X-Gm-Message-State: AOAM5339ScRMr5526MUMAqtB14vSsPpfLeZEMSnf2IHQY2agHFDYpERG kIiVdBi4B7VxN9imYa83hKDIAw== X-Google-Smtp-Source: ABdhPJw+vbnPc6vft6RqA6zSnbRtvDlS1NwlZFp269pvENQ083BNJ7dEdI8BO5JpR9Ta6ymEkaX/jQ== X-Received: by 2002:a05:600c:3487:b0:38c:9a42:4d49 with SMTP id a7-20020a05600c348700b0038c9a424d49mr18900226wmq.29.1649064894420; Mon, 04 Apr 2022 02:34:54 -0700 (PDT) Received: from [192.168.0.173] (xdsl-188-155-201-27.adslplus.ch. [188.155.201.27]) by smtp.gmail.com with ESMTPSA id o4-20020a5d6484000000b002057ad822d4sm9143811wri.48.2022.04.04.02.34.53 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Mon, 04 Apr 2022 02:34:54 -0700 (PDT) Message-ID: <2976f4f9-4fda-c04f-45cf-351518f88ec0@linaro.org> Date: Mon, 4 Apr 2022 11:34:52 +0200 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101 Thunderbird/91.7.0 Subject: Re: [PATCH v6 01/12] driver: platform: Add helper for safer setting of driver_override Content-Language: en-US To: Andy Shevchenko References: <20220403183758.192236-1-krzysztof.kozlowski@linaro.org> <20220403183758.192236-2-krzysztof.kozlowski@linaro.org> From: Krzysztof Kozlowski In-Reply-To: Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit X-Mailman-Approved-At: Fri, 08 Apr 2022 09:48:12 +0200 Cc: Linux on Hyper-V List , Stuart Yoder , "Rafael J. Wysocki" , linux-pci , linux-remoteproc@vger.kernel.org, ALSA Development Mailing List , Bjorn Andersson , Vineeth Vijayan , Alexander Gordeev , "K. Y. Srinivasan" , linux-clk , linux-s390@vger.kernel.org, Wei Liu , Stephen Hemminger , Dexuan Cui , Andy Gross , NXP Linux Team , Christian Borntraeger , Heiko Carstens , Vasily Gorbik , linux-arm-msm@vger.kernel.org, Haiyang Zhang , linux-spi , Rasmus Villemoes , Bjorn Helgaas , virtualization@lists.linux-foundation.org, linux-arm Mailing List , Mathieu Poirier , Greg Kroah-Hartman , Peter Oberparleiter , Linux Kernel Mailing List , Sven Schnelle , Linus Torvalds 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 04/04/2022 11:16, Andy Shevchenko wrote: > On Sun, Apr 3, 2022 at 9:38 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) >> >> Provide a helper which clearly documents the usage of driver_override. >> This will allow later to reuse the helper and reduce the amount of >> duplicated code. >> >> Convert the platform driver to use a new helper and make the >> driver_override field const char (it is not modified by the core). > > ... > >> +int driver_set_override(struct device *dev, const char **override, >> + const char *s, size_t len) >> +{ >> + const char *new, *old; >> + char *cp; > >> + if (!override || !s) >> + return -EINVAL; > > Still not sure if we should distinguish (s == NULL && len == 0) from > (s != NULL && len == 0). > Supplying the latter seems confusing (yes, I see that in the old code). Perhaps > !s test, in case you want to leave it, should be also commented. The old semantics were focused on sysfs usage, so clearing is by passing an empty string. In the case of sysfs empty string is actually "\n". I intend to keep the semantics also for the in-kernel usage and in such case empty string can be also "". If I understand your comment correctly, you propose to change it to NULL for in-kernel usage, but that would change the semantics. > Another approach is to split above to two checks and move !s after !len. I don't follow why... The !override and !s are invalid uses. !len is a valid user for internal callers, just like "\n" is for sysfs. > >> + /* >> + * 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; > > Perhaps explain the case in the comment here? You mean the case we discuss here (to clear override with "")? Sure. > >> + if (!len) { >> + device_lock(dev); >> + old = *override; >> + *override = NULL; > >> + device_unlock(dev); >> + goto out_free; > > You may deduplicate this one, by > > goto out_unlock_free; > > But I understand your intention to keep lock-unlock in one place, so > perhaps dropping that label would be even better in this case and > keeping it Yes, exactly. > > kfree(old); > return 0; > > here instead of goto. Slightly more code, but indeed maybe easier to follow. I'll do like this. Best regards, Krzysztof