Re: [PATCH] media: isif: reset global state

[Evgeny Novikov <novikov@ispras.ru>]

Hi Lad,

I will try to answer your question below.

17.07.2020, 12:55, "Lad, Prabhakar" <prabhakar.csengg@gmail.com>:
> HI Evgeny,
>
> Thank you for the patch.
>
> On Tue, Jul 14, 2020 at 6:20 PM Evgeny Novikov <novikov@ispras.ru> wrote:
>>  isif_probe() invokes iounmap() on error handling paths, but it does not
>>  reset the global state. So, later it can invoke iounmap() even when
>>  ioremap() fails. This is the case also for isif_remove(). The patch
>>  resets the global state after invoking iounmap() to avoid this.
>>
>>  Found by Linux Driver Verification project (linuxtesting.org).
>>
>>  Signed-off-by: Evgeny Novikov <novikov@ispras.ru>
>>  ---
>>   drivers/media/platform/davinci/isif.c | 11 +++++++++--
>>   1 file changed, 9 insertions(+), 2 deletions(-)
>>
>>  diff --git a/drivers/media/platform/davinci/isif.c b/drivers/media/platform/davinci/isif.c
>>  index c98edb67cfb2..c53cecd072b1 100644
>>  --- a/drivers/media/platform/davinci/isif.c
>>  +++ b/drivers/media/platform/davinci/isif.c
>>  @@ -1075,10 +1075,14 @@ static int isif_probe(struct platform_device *pdev)
>>          release_mem_region(res->start, resource_size(res));
>>          i--;
>>   fail_nobase_res:
>>  - if (isif_cfg.base_addr)
>>  + if (isif_cfg.base_addr) {
>>                  iounmap(isif_cfg.base_addr);
>>  - if (isif_cfg.linear_tbl0_addr)
>>  + isif_cfg.base_addr = NULL;
>>  + }
>>  + if (isif_cfg.linear_tbl0_addr) {
>>                  iounmap(isif_cfg.linear_tbl0_addr);
>>  + isif_cfg.linear_tbl0_addr = NULL;
>>  + }
>
> As the probe itself is failing I don't see a benefit in setting the
> pointers to NULL. Unless I'm missing something.

Unfortunately, I can not provide you easily with a nice visualization of a corresponding error trace, but I will try to explain it in a text form. Our environment model for device drivers infinitely invokes their probe and remove callbacks. In particular, when probe fails, it does not invoke remove, but it can call probe again.

Before the fix the global state (isif_cfg.base_addr and isif_cfg.linear_tbl0_addr) was not set to NULL when first probe fails. Assuming some failures during the second invocation of probe, e.g. ioremap_nocache() can fail, the driver proceeds to iounmap() because corresponding pointers are not NULL. Of course, one can hardly imagine that this is possible in practice but static analysis treats all possible paths.

Perhaps, our environment model is not accurate enough in this aspect. But our tool does not report many similar warnings when we check thousands of drivers, so, we can assume that everything is okay. If you have another opinion, I would be glad to hear it.  

-- 
Evgeny Novikov
Linux Verification Center, ISP RAS
http://linuxtesting.org

>
> Cheers,
> --Prabhakar
>
>>          while (i >= 0) {
>>                  res = platform_get_resource(pdev, IORESOURCE_MEM, i);
>>  @@ -1096,8 +1100,11 @@ static int isif_remove(struct platform_device *pdev)
>>          int i = 0;
>>
>>          iounmap(isif_cfg.base_addr);
>>  + isif_cfg.base_addr = NULL;
>>          iounmap(isif_cfg.linear_tbl0_addr);
>>  + isif_cfg.linear_tbl0_addr = NULL;
>>          iounmap(isif_cfg.linear_tbl1_addr);
>>  + isif_cfg.linear_tbl1_addr = NULL;
>>          while (i < 3) {
>>                  res = platform_get_resource(pdev, IORESOURCE_MEM, i);
>>                  if (res)
>>  --
>>  2.16.4