Re: [PATCH 26/31] staging/comedi/das16: Make timer initialization unconditional

[Kees Cook]

On Fri, Sep 1, 2017 at 2:38 AM, Ian Abbott <abbotti@xxxxxxxxx> wrote:
> On 01/09/17 10:29, Ian Abbott wrote:
>>
>> On 01/09/17 00:29, Kees Cook wrote:
>>>
>>> With timer initialization made unconditional, there is no reason to
>>> make del_timer_sync() calls conditionally, there by removing the test
>>> of the .data field.
>>>
>>> Cc: Ian Abbott <abbotti@xxxxxxxxx>
>>> Cc: H Hartley Sweeten <hsweeten@xxxxxxxxxxxxxxxxxxx>
>>> Cc: Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx>
>>> Cc: devel@xxxxxxxxxxxxxxxxxxxx
>>> Signed-off-by: Kees Cook <keescook@xxxxxxxxxxxx>
>>> ---
>>>   drivers/staging/comedi/drivers/das16.c | 9 +++------
>>>   1 file changed, 3 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/drivers/staging/comedi/drivers/das16.c
>>> b/drivers/staging/comedi/drivers/das16.c
>>> index 5d157951f63f..4514179b2007 100644
>>> --- a/drivers/staging/comedi/drivers/das16.c
>>> +++ b/drivers/staging/comedi/drivers/das16.c
>>> @@ -941,10 +941,8 @@ static void das16_alloc_dma(struct comedi_device
>>> *dev, unsigned int dma_chan)
>>>       /* DMA uses two buffers */
>>>       devpriv->dma = comedi_isadma_alloc(dev, 2, dma_chan, dma_chan,
>>>                          DAS16_DMA_SIZE, COMEDI_ISADMA_READ);
>>> -    if (devpriv->dma) {
>>> -        setup_timer(&devpriv->timer, das16_timer_interrupt,
>>> -                (unsigned long)dev);
>>> -    }
>>> +    setup_timer(&devpriv->timer, das16_timer_interrupt,
>>> +            (unsigned long)dev);
>>>   }
>>
>>
>> das16_alloc_dma() returns before the call to comedi_isadma_alloc() if the
>> dma_chan parameter is not one of the values 1 or 3, so setup_timer() will
>> not be called in that case.
>>
>>>   static void das16_free_dma(struct comedi_device *dev)
>>> @@ -952,8 +950,7 @@ static void das16_free_dma(struct comedi_device *dev)
>>>       struct das16_private_struct *devpriv = dev->private;
>>>       if (devpriv) {
>>> -        if (devpriv->timer.data)
>>> -            del_timer_sync(&devpriv->timer);
>>> +        del_timer_sync(&devpriv->timer);
>>
>>
>> If setup_timer() has not been called (see remark above), this change will
>> break.
>>
>>>           comedi_isadma_free(devpriv->dma);
>>>       }
>>>   }
>>>
>
> If you want to avoid testing devpriv->timer.data for some reason, you could
> make the calls to setup_timer() and del_timer_sync() depend on devpriv->dma.

Thanks for checking this! I think the cleanest would be to just move
setup_timer() to the start of das16_alloc_dma(). I'll make that
adjustment.

-Kees

-- 
Kees Cook
Pixel Security