linux-kernel - Re: [RFC PATCH v7 6/9] media: tegra: Add Tegra210 Video input driver

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] [thread-next>] [day] [month] [year] [list]

Date:   Wed, 15 Apr 2020 10:48:42 -0700
From:   Sowjanya Komatineni <skomatineni@...dia.com>
To:     Dmitry Osipenko <digetx@...il.com>, <thierry.reding@...il.com>,
        <jonathanh@...dia.com>, <frankc@...dia.com>, <hverkuil@...all.nl>,
        <sakari.ailus@....fi>, <helen.koike@...labora.com>
CC:     <sboyd@...nel.org>, <linux-media@...r.kernel.org>,
        <devicetree@...r.kernel.org>, <linux-clk@...r.kernel.org>,
        <linux-tegra@...r.kernel.org>, <linux-kernel@...r.kernel.org>
Subject: Re: [RFC PATCH v7 6/9] media: tegra: Add Tegra210 Video input driver


On 4/15/20 10:47 AM, Sowjanya Komatineni wrote:
>
> On 4/15/20 10:21 AM, Sowjanya Komatineni wrote:
>>
>> On 4/15/20 9:54 AM, Sowjanya Komatineni wrote:
>>>
>>> On 4/15/20 7:22 AM, Dmitry Osipenko wrote:
>>>> External email: Use caution opening links or attachments
>>>>
>>>>
>>>> 15.04.2020 05:57, Sowjanya Komatineni пишет:
>>>>> +static int tegra_csi_remove(struct platform_device *pdev)
>>>>> +{
>>>>> +     struct tegra_csi *csi = platform_get_drvdata(pdev);
>>>>> +     int err;
>>>>> +
>>>>> +     err = host1x_client_unregister(&csi->client);
>>>>> +     if (err < 0) {
>>>>> +             dev_err(csi->dev,
>>>>> +                     "failed to unregister host1x client: %d\n", 
>>>>> err);
>>>>> +             return err;
>>>>> +     }
>>>>> +
>>>>> +     pm_runtime_disable(csi->dev);
>>>>> +     kfree(csi);
>>>> IIRC, the driver removal is invoked on the unbinding. Hence, I'm not
>>>> sure how moving away from the resource-managed API helps here. 
>>>> Could you
>>>> please explain in a more details?
>>>>
>>>> Have you tried to test this driver under KASAN? I suspect that you 
>>>> just
>>>> masked the problem, instead of fixing it.
>>> Using devm_kzalloc for vi/csi structures based on prior feedback 
>>> request to switch to use kzalloc all over this driver.
>>>
>>> Hi Hans,
>>>
>>> video devices lifetime is till video device nodes are released. So, 
>>> v4l2 device release callback does the release of tegra channel 
>>> allocation which hold video device.
>>>
>>> Below are the 3 possible cases of unbind/unload,
>>>
>>> 1. during tegra-video module unload, if v4l2 device refcnt is not 0 
>>> which is the case when any of video device node handle is kept 
>>> opened then unloading module will not happen and module refcnt is 
>>> also non-zero and unloading tegra-video module reports module in use.
>>>
>>> 2. during tegra-video driver unbind, tegra-video driver removal will 
>>> do vi/csi clients exit ops which unregisters video device allocated 
>>> memory during release callback of v4l2 device. vi/csi structure 
>>> allocation remains same as vi/csi driver removal will not happen in 
>>> this case.
>>>
>>>
>>> 3. during direct host1x client drivers vi/csi unbind, both 
>>> host1x_clients vi/csi gets unregistered, deletes host1x logical 
>>> device which executes tegra-video driver removal() -> vi/csi exit() 
>>> before vi/csi memory gets freed in vi/csi driver remove().
>>>
>>> So, any active streaming will stop and video devices are 
>>> unregistered during direct client driver unbind prior to freeing 
>>> vi/csi memory.
>>>
>>> Also vi/csi driver remove does explicit free vi/csi as its allocated 
>>> with kzalloc. So not sure how using kzalloc is different to 
>>> devm_kzalloc for vi/csi structure in terms of when vi/csi memory 
>>> gets freed?
>>>
>>> Except for channel allocation which holds video device and as video 
>>> device life time is beyond tegra-video module unbind->vi exit(), 
>>> looks like we can use devm_kzalloc for vi/csi.
>>>
>>>
>>> Can you please comment if you still think we need to use kzalloc 
>>> rather than devm_kzalloc for vi/csi structure allocation?
>>>
>>> Thanks
>>>
>>> Sowjanya
>>>
>> One more case is when video device node is kept opened with v4l2-ctl 
>> sleep (rather than streaming), where it will keep device node open 
>> for specified time and if direct vi client driver unbind happens then 
>> vi driver remove() will free vi memory before v4l2 device release 
>> happens.
>>
>> But I don't see any crash or errors with this case.
>>
>> Also if we allow direct client driver unbind, then vi structure 
>> memory lifetime should also be till v4l2 device release happens.
>>
>> But we can free vi in v4l2 device release callback as in case when 
>> device node is not kept opened, video device release happens 
>> immediate and we cant free vi that early.
>
> typo fix:
>
> But we can't free vi structure memory allocation in v4l2 device 
> release callback as in case when device node is not kept opened, 
> device release happens immediate and we can't free vi structure memory 
> that early.
>
>>
>> Hans/Thierry, Can you please comment on this case?
>>
>> Thanks
>>
>> Sowjanya
>>
Also, Can you please help explain on cases where we do/need direct 
host1x clients vi/csi drivers unbind?