Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Subject: Re: [PATCH v2] nvmem: core: fix possibly memleak when use
 nvmem_cell_info_to_nvmem_cell()
To:     Vadym Kochan <vadym.kochan@plvision.eu>
Cc:     Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        linux-kernel@vger.kernel.org
References: <20200923135343.16565-1-vadym.kochan@plvision.eu>
 <7a072204-a4b8-e5ef-9ce2-4db176f70366@linaro.org>
 <20200923141334.GA16798@plvision.eu>
 <1f65dc6a-50f3-d4e5-f1ce-7a68fddde287@linaro.org>
 <20200923145107.GB16798@plvision.eu>
 <19c5fa23-41c9-f5c7-beaf-aeb9655bfefd@linaro.org>
 <20200923162314.GF16798@plvision.eu>
From:   Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
Message-ID: <cec8a363-0c2b-2746-d986-f239f74eaff7@linaro.org>
Date:   Wed, 23 Sep 2020 17:25:05 +0100
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101
 Thunderbird/60.8.0
MIME-Version: 1.0
In-Reply-To: <20200923162314.GF16798@plvision.eu>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Precedence: bulk


On 23/09/2020 17:23, Vadym Kochan wrote:
> On Wed, Sep 23, 2020 at 04:51:06PM +0100, Srinivas Kandagatla wrote:
>>
>>
>> On 23/09/2020 15:51, Vadym Kochan wrote:
>>>> -       return nvmem_cell_write(&cell, buf, cell.bytes);
>>>> +       rc = nvmem_cell_write(&cell, buf, cell.bytes);
>>>> +       if (rc)
>>>> +               kfree_const(cell->name);
>>>> +
>>>> +       return rc;
>>>>    }
>>>>    EXPORT_SYMBOL_GPL(nvmem_device_cell_write);
>>>>    ------------------------>cut<---------------------------
>>>>
>>>> --srini
>>>>
>>> But is it really needed to kstrdup(cell->name) for nvmem_device_cell_{read,write} ?
>> This boils down to if we want to use same api to parse nvmem_cell_info or
>> not!
>>
>> If we want to keep this simple, we can either explicitly add free for
>> successful caller to nvmem_cell_info_to_nvmem_cell()!
>>
>> Or
>>
>> use something like what you did, but new api needs more clarity!
>> May be renaming __nvmem_cell_info_to_nvmem_cell to
>> nvmem_cell_info_to_nvmem_cell_no_alloc would clarify that a bit!
>>
> 
> Naming is most difficult thing, what about __nvmem_cell_info_to_nvmem_cell_{unsafe,nodup}() ?
> At least this is an indication to be carefully here.

nvmem_cell_info_to_nvmem_cell_nodup() should be good!

--srini
> 
>> Also can you make sure that linewrapping on function names be inline with
>> existing code.
>>
>> Please send v3 with that changes!
>>
>>
>> --srini
>>> It is used only for log error in case the unaligned access did not
>>> pass the check