Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1684652

Re: [PATCH v2 1/1] mux: mux-core: Add NULL check for dev->of_node

Show all headers | View raw

Hi,


On 07/10/2017 12:40 AM, Peter Rosin wrote:
> On 2017-07-09 09:35, Kuppuswamy, Sathyanarayanan wrote:
>> Hi,
>>
>>
>> On 7/9/2017 12:07 AM, Peter Rosin wrote:
>>> On 2017-07-09 01:12, Kuppuswamy, Sathyanarayanan wrote:
>>>> Hi Peter,
>>>>
>>>>
>>>> On 7/8/2017 2:00 PM, Peter Rosin wrote:
>>>>> On 2017-07-07 23:46, sathyanarayanan.kuppuswamy@linux.intel.com wrote:
>>>>>> From: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
>>>>>>
>>>>>> If dev->of_node is NULL, then calling mux_control_get()
>>>>>> function can lead to NULL pointer exception. So adding
>>>>>> a NULL check for dev->of_node.
>>>>>>
>>>>>> Signed-off-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
>>>>> Do you have a driver that might call mux_control_get and not have any
>>>>> of_node?
>>>> For non-device tree drivers, this case is valid. I hit this issue when I
>>>> was working on Intel USB MUX driver.
>>>>>     If not, I don't see the point of this check.
>>>> Since this is an API for other consumers, I think its better to have
>>>> some sanity checks.
>>>>
>>>> If a non device tree driver call this API , I think its better to fail
>>>> with some error no instead of creating null pointer exception.
>>> Is it? When authoring a new driver, and you make some error like this, why
>>> is a "nice" error better than a big fat fail? If you get a null deref,
>>> you will presumably also get a call stack etc, which will help you find
>>> where you made the error, w/o adding a bunch of traces to find out exactly
>>> what you did wrong.
>> In this case, I think the error can happen even if there is any
>> configuration mismatch in device tree blob. So its not only
> No, it cannot happen for any of the existing consumers. And seeing crashes
> if the DT blob is faulty isn't unexpected. I'm sure the ways to provoke a
> crash in that situation are abundant.
If we can prevent the crash and fail with some meaning full message, I think
we should adapt to it. if you want to leave a strong message, my 
suggestion is to
use BUG or WARN_ON.
>
>> about API usage in driver. If you think that you need to fail the system
>> if the API is used without proper dt node configuration,
>>    then we should use something like BUG or WARN_ON to explicitly mention
>> this dependency.  But I think its better to
> But this is something that *cannot* happen unless you add some new code
> somewhere else. Why check at all?
Its not just kernel code here, DT tables are also involved.
>
>> leave this decision to the MUX consumers because there use cases where
>> MUX control can be optional
> This is only future-proofing for consumers that does not exist, and does
> nothing for the existing code. And the future where this is needed might
> never happen.
>
> Still skeptic...
I agree with philosophy of not bloating the kernel by merging un-used
code. But I think this concept should be limited to adding new APIs or
drivers. But for bugs in existing code I am not sure whether we should
follow the same principle.

I have checked some use cases of dev->of_node in kernel, and most of 
them who uses it in
kernel APIs have some form of protection check for it. For example, take 
a look at the
following functions.

int platform_get_irq(struct platform_device *dev, unsigned int num)
bool device_dma_supported(struct device *dev)
struct pwm_device *pwm_get(struct device *dev, const char *con_id)

These APIs have checks like,

if (IS_ENABLED(CONFIG_OF) &&  dev->of_node)

If you are still skeptic, I am out of arguments -:)
>
> Cheers,
> peda
>
>>> So, I'm skeptic...
>>>
>>> Cheers,
>>> peda
>>>
>>>>> Cheers,
>>>>> peda
>>>>>
>>>>>> ---
>>>>>>     drivers/mux/mux-core.c | 3 +++
>>>>>>     1 file changed, 3 insertions(+)
>>>>>>
>>>>>> Changes since v1:
>>>>>>     * Removed dummy new line.
>>>>>>
>>>>>> diff --git a/drivers/mux/mux-core.c b/drivers/mux/mux-core.c
>>>>>> index 90b8995..924c983 100644
>>>>>> --- a/drivers/mux/mux-core.c
>>>>>> +++ b/drivers/mux/mux-core.c
>>>>>> @@ -438,6 +438,9 @@ struct mux_control *mux_control_get(struct device *dev, const char *mux_name)
>>>>>>     	int index = 0;
>>>>>>     	int ret;
>>>>>>     
>>>>>> +	if (!np)
>>>>>> +		return ERR_PTR(-ENODEV);
>>>>>> +
>>>>>>     	if (mux_name) {
>>>>>>     		index = of_property_match_string(np, "mux-control-names",
>>>>>>     						 mux_name);
>>>>>>
>

-- 
Sathyanarayanan Kuppuswamy
Linux kernel developer

Back to linux.kernel | Previous | Next — Previous in thread | Find similar | Unroll thread

Thread

[PATCH v2 1/1] mux: mux-core: Add NULL check for dev->of_node sathyanarayanan.kuppuswamy@linux.intel.com - 2017-07-07 23:50 +0200
  Re: [PATCH v2 1/1] mux: mux-core: Add NULL check for dev->of_node "Kuppuswamy, Sathyanarayanan" <sathyaosid@gmail.com> - 2017-07-09 01:20 +0200
    Re: [PATCH v2 1/1] mux: mux-core: Add NULL check for dev->of_node "Kuppuswamy, Sathyanarayanan" <sathyaosid@gmail.com> - 2017-07-09 09:40 +0200
      Re: [PATCH v2 1/1] mux: mux-core: Add NULL check for dev->of_node sathyanarayanan kuppuswamy          <sathyanarayanan.kuppuswamy@linux.intel.com> - 2017-07-10 23:40 +0200

csiph-web