Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <fb2b05f0-7609-1fa7-4d69-c64e082e2e2b@linaro.org>
Date:   Sun, 3 Apr 2022 20:26:51 +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 v5 01/11] driver: platform: Add helper for safer setting
 of driver_override
Content-Language: en-US
To:     Andy Shevchenko <andy.shevchenko@gmail.com>,
        Krzysztof Kozlowski <krzysztof.kozlowski@canonical.com>
Cc:     Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        "Rafael J. Wysocki" <rafael@kernel.org>,
        Stuart Yoder <stuyoder@gmail.com>,
        "K. Y. Srinivasan" <kys@microsoft.com>,
        Haiyang Zhang <haiyangz@microsoft.com>,
        Stephen Hemminger <sthemmin@microsoft.com>,
        Wei Liu <wei.liu@kernel.org>, Dexuan Cui <decui@microsoft.com>,
        Bjorn Helgaas <bhelgaas@google.com>,
        Bjorn Andersson <bjorn.andersson@linaro.org>,
        Mathieu Poirier <mathieu.poirier@linaro.org>,
        Vineeth Vijayan <vneethv@linux.ibm.com>,
        Peter Oberparleiter <oberpar@linux.ibm.com>,
        Heiko Carstens <hca@linux.ibm.com>,
        Vasily Gorbik <gor@linux.ibm.com>,
        Alexander Gordeev <agordeev@linux.ibm.com>,
        Christian Borntraeger <borntraeger@linux.ibm.com>,
        Sven Schnelle <svens@linux.ibm.com>,
        Andy Gross <agross@kernel.org>,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
        linux-clk <linux-clk@vger.kernel.org>,
        NXP Linux Team <linux-imx@nxp.com>,
        linux-arm Mailing List <linux-arm-kernel@lists.infradead.org>,
        Linux on Hyper-V List <linux-hyperv@vger.kernel.org>,
        linux-pci <linux-pci@vger.kernel.org>,
        linux-remoteproc@vger.kernel.org, linux-s390@vger.kernel.org,
        linux-arm-msm@vger.kernel.org,
        ALSA Development Mailing List <alsa-devel@alsa-project.org>,
        linux-spi <linux-spi@vger.kernel.org>,
        virtualization@lists.linux-foundation.org,
        Linus Torvalds <torvalds@linux-foundation.org>,
        Rasmus Villemoes <linux@rasmusvillemoes.dk>
References: <20220316150533.421349-1-krzysztof.kozlowski@canonical.com>
 <20220316150533.421349-2-krzysztof.kozlowski@canonical.com>
 <CAHp75VeaQdzUKJSKzH9FjbmON5asqH799AS8OzHGoDiRnJifNw@mail.gmail.com>
From:   Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
In-Reply-To: <CAHp75VeaQdzUKJSKzH9FjbmON5asqH799AS8OzHGoDiRnJifNw@mail.gmail.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Precedence: bulk

On 16/03/2022 16:54, Andy Shevchenko wrote:
> On Wed, Mar 16, 2022 at 5:06 PM Krzysztof Kozlowski
> <krzysztof.kozlowski@canonical.com> wrote:
> 
> ...
> 
>> +int driver_set_override(struct device *dev, const char **override,
>> +                       const char *s, size_t len)
>> +{
>> +       const char *new, *old;
>> +       char *cp;

I focused on some other topics, so I did not respond to your questions
for some time.

> 
>> +       if (!dev || !override || !s)
>> +               return -EINVAL;
> 
> Sorry, I didn't pay much attention on this. First of all, I would drop
> dev checks and simply require that dev should be valid. Do you expect
> this can be called when dev is invalid?

No, I can skip the check.

> I would like to hear if it's
> anything but theoretical. Second one, is the !s requirement. Do I
> understand correctly that the string must be always present? But then
> how we NULify the override? Is it possible?

I did not change the convention of this sysfs hook, so removing of
override is passing empty string "". Have in mind that his interface is
mainly for sysfs, not for other drivers.

> Third one is absence of
> len check. See below.
> 
>> +       /*
>> +        * 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;
> 
> I would relax this to make sure we can use it if \n is within this limit.

Relax in what way? Store more than PAGE_SIZE? This is a user-visible
string and the name of a driver.

> 
>> +       cp = strnchr(s, len, '\n');
>> +       if (cp)
>> +               len = cp - s;
>> +
>> +       new = kstrndup(s, len, GFP_KERNEL);
> 
> Here is a word about the len check.
> 
>> +       if (!new)
> 
> If len == 0, this won't trigger and you have something very
> interesting as a result.

True, empty string would be set as override. The API says that empty
string clears the override, so len==0 should be valid (just like "\n" is
ok).

> 
> One way is to use ZERO_PTR_OR_NULL() another is explicitly check for 0
> and issue a (different?) error code.


Best regards,
Krzysztof