linux-kernel - Re: [PATCH] spmi: Add check for remove callback in spmi_drv

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <e63e27df-7e46-337c-f22a-ccd1bbcd0c28@quicinc.com>
Date:   Wed, 21 Dec 2022 11:09:03 +0530
From:   Jishnu Prakash <quic_jprakash@...cinc.com>
To:     Greg KH <gregkh@...uxfoundation.org>
CC:     <agross@...nel.org>, <devicetree@...r.kernel.org>,
        <robh+dt@...nel.org>, <linus.walleij@...aro.org>,
        <quic_kamalw@...cinc.com>, <sboyd@...nel.org>,
        <quic_subbaram@...cinc.com>, <quic_collinsd@...cinc.com>,
        <linux-kernel@...r.kernel.org>, <linux-arm-msm@...r.kernel.org>,
        <linux-arm-msm-owner@...r.kernel.org>
Subject: Re: [PATCH] spmi: Add check for remove callback in spmi_drv_remove
 API

Hi Greg

On 12/13/2022 8:39 PM, Greg KH wrote:
> On Tue, Dec 13, 2022 at 07:12:10PM +0530, Jishnu Prakash wrote:
>> Hi Greg
> 
> Hi, please do not top-post :(
> 
>> These are two SPMI drivers without remove callbacks defined:
>>
>> drivers/mfd/qcom-spmi-pmic.c
>> drivers/mfd/hi6421-spmi-pmic.c
> 
> Great, they should be fixed up now, right?
> 

Our QCOM SPMI PMIC driver allocates resources in its probe using only 
devm_() APIs and does not require any other cleanup. It doesn't seem 
right to add an empty remove callback to it just to avoid this crash, it
seems the better solution architecturally is to call the remove function 
only if it's defined.

In addition, I could see that other buses like PCI and AMBA do check for 
the remove API being defined for their drivers before calling it:

AMBA example: 
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/amba/bus.c#n328

PCI example: 
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/pci/pci-driver.c#n474


>> We made this change after noticing an issue internally with the first one
>> above, there was a crash when trying to remove it with rmmod, which is fixed
>> by this change.
> 
> Then please say that in the changelog text, otherwise we have no idea
> _why_ this is needed.  All you said was "add this new check _IF_" and we
> have no idea what the answer to "if" is :(
> 

I have uploaded the change with an updated title and commit text, can 
you please have a look?

> thanks,
> 
> greg k-h
Thanks,
Jishnu