diff mbox series

[07/11] vfio/platform: Remove dead assignment in vfio_intp_interrupt()

Message ID 20200813073712.4001404-8-kuhn.chenqun@huawei.com
State New
Headers show
Series trivial patchs for static code analyzer fixes | expand

Commit Message

Chenqun (kuhn) Aug. 13, 2020, 7:37 a.m. UTC
Clang static code analyzer show warning:
hw/vfio/platform.c:239:9: warning: Value stored to 'ret' is never read
        ret = event_notifier_test_and_clear(intp->interrupt);
        ^     ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

Reported-by: Euler Robot <euler.robot@huawei.com>
Signed-off-by: Chen Qun <kuhn.chenqun@huawei.com>
---
Cc: Alex Williamson <alex.williamson@redhat.com>
Cc: Eric Auger <eric.auger@redhat.com>
---
 hw/vfio/platform.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Auger Eric Aug. 13, 2020, 9:25 a.m. UTC | #1
Hi,

On 8/13/20 9:37 AM, Chen Qun wrote:
> Clang static code analyzer show warning:
> hw/vfio/platform.c:239:9: warning: Value stored to 'ret' is never read
>         ret = event_notifier_test_and_clear(intp->interrupt);
>         ^     ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> 
> Reported-by: Euler Robot <euler.robot@huawei.com>
> Signed-off-by: Chen Qun <kuhn.chenqun@huawei.com>
Reviewed-by: Eric Auger <eric.auger@redhat.com>

Thanks

Eric

> ---
> Cc: Alex Williamson <alex.williamson@redhat.com>
> Cc: Eric Auger <eric.auger@redhat.com>
> ---
>  hw/vfio/platform.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/hw/vfio/platform.c b/hw/vfio/platform.c
> index ac2cefc9b1..869ed2c39d 100644
> --- a/hw/vfio/platform.c
> +++ b/hw/vfio/platform.c
> @@ -236,7 +236,7 @@ static void vfio_intp_interrupt(VFIOINTp *intp)
>          trace_vfio_intp_interrupt_set_pending(intp->pin);
>          QSIMPLEQ_INSERT_TAIL(&vdev->pending_intp_queue,
>                               intp, pqnext);
> -        ret = event_notifier_test_and_clear(intp->interrupt);
> +        event_notifier_test_and_clear(intp->interrupt);
>          return;
>      }
>  
>
Alex Williamson Aug. 13, 2020, 4:59 p.m. UTC | #2
On Thu, 13 Aug 2020 15:37:08 +0800
Chen Qun <kuhn.chenqun@huawei.com> wrote:

> Clang static code analyzer show warning:
> hw/vfio/platform.c:239:9: warning: Value stored to 'ret' is never read
>         ret = event_notifier_test_and_clear(intp->interrupt);
>         ^     ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> 
> Reported-by: Euler Robot <euler.robot@huawei.com>
> Signed-off-by: Chen Qun <kuhn.chenqun@huawei.com>
> ---
> Cc: Alex Williamson <alex.williamson@redhat.com>
> Cc: Eric Auger <eric.auger@redhat.com>
> ---
>  hw/vfio/platform.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/hw/vfio/platform.c b/hw/vfio/platform.c
> index ac2cefc9b1..869ed2c39d 100644
> --- a/hw/vfio/platform.c
> +++ b/hw/vfio/platform.c
> @@ -236,7 +236,7 @@ static void vfio_intp_interrupt(VFIOINTp *intp)
>          trace_vfio_intp_interrupt_set_pending(intp->pin);
>          QSIMPLEQ_INSERT_TAIL(&vdev->pending_intp_queue,
>                               intp, pqnext);
> -        ret = event_notifier_test_and_clear(intp->interrupt);
> +        event_notifier_test_and_clear(intp->interrupt);
>          return;
>      }

Testing that an event is pending in our notifier is generally a
prerequisite to doing anything in the interrupt handler, I don't
understand why we're just consuming it and ignoring the return value.
The above is in the delayed handling branch of the function, but the
normal non-delayed path would only go on to error_report() if the
notifier is not pending and then inject an interrupt anyway.  This all
seems rather suspicious and it's a unique pattern among the vfio
callers of this function.  Is there a more fundamental bug that this
function should perform this test once and return without doing
anything if it's called spuriously, ie. without a notifier pending?
Thanks,

Alex
Auger Eric Aug. 13, 2020, 6:02 p.m. UTC | #3
Hi Alex,

On 8/13/20 6:59 PM, Alex Williamson wrote:
> On Thu, 13 Aug 2020 15:37:08 +0800
> Chen Qun <kuhn.chenqun@huawei.com> wrote:
> 
>> Clang static code analyzer show warning:
>> hw/vfio/platform.c:239:9: warning: Value stored to 'ret' is never read
>>         ret = event_notifier_test_and_clear(intp->interrupt);
>>         ^     ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>>
>> Reported-by: Euler Robot <euler.robot@huawei.com>
>> Signed-off-by: Chen Qun <kuhn.chenqun@huawei.com>
>> ---
>> Cc: Alex Williamson <alex.williamson@redhat.com>
>> Cc: Eric Auger <eric.auger@redhat.com>
>> ---
>>  hw/vfio/platform.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/hw/vfio/platform.c b/hw/vfio/platform.c
>> index ac2cefc9b1..869ed2c39d 100644
>> --- a/hw/vfio/platform.c
>> +++ b/hw/vfio/platform.c
>> @@ -236,7 +236,7 @@ static void vfio_intp_interrupt(VFIOINTp *intp)
>>          trace_vfio_intp_interrupt_set_pending(intp->pin);
>>          QSIMPLEQ_INSERT_TAIL(&vdev->pending_intp_queue,
>>                               intp, pqnext);
>> -        ret = event_notifier_test_and_clear(intp->interrupt);
>> +        event_notifier_test_and_clear(intp->interrupt);
>>          return;
>>      }
> 
> Testing that an event is pending in our notifier is generally a
> prerequisite to doing anything in the interrupt handler, I don't
> understand why we're just consuming it and ignoring the return value.
> The above is in the delayed handling branch of the function, but the
> normal non-delayed path would only go on to error_report() if the
> notifier is not pending and then inject an interrupt anyway.  This all
> seems rather suspicious and it's a unique pattern among the vfio
> callers of this function.  Is there a more fundamental bug that this
> function should perform this test once and return without doing
> anything if it's called spuriously, ie. without a notifier pending?
> Thanks,

Hum that's correct that other VFIO call sites do the check. My
understanding was that this could not fail in this case as, if we
entered the handler there was something to be cleared. In which
situation can this fail?

Thanks

Eric
> 
> Alex
> 
>
Alex Williamson Aug. 13, 2020, 7:15 p.m. UTC | #4
On Thu, 13 Aug 2020 20:02:45 +0200
Auger Eric <eric.auger@redhat.com> wrote:

> Hi Alex,
> 
> On 8/13/20 6:59 PM, Alex Williamson wrote:
> > On Thu, 13 Aug 2020 15:37:08 +0800
> > Chen Qun <kuhn.chenqun@huawei.com> wrote:
> >   
> >> Clang static code analyzer show warning:
> >> hw/vfio/platform.c:239:9: warning: Value stored to 'ret' is never read
> >>         ret = event_notifier_test_and_clear(intp->interrupt);
> >>         ^     ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> >>
> >> Reported-by: Euler Robot <euler.robot@huawei.com>
> >> Signed-off-by: Chen Qun <kuhn.chenqun@huawei.com>
> >> ---
> >> Cc: Alex Williamson <alex.williamson@redhat.com>
> >> Cc: Eric Auger <eric.auger@redhat.com>
> >> ---
> >>  hw/vfio/platform.c | 2 +-
> >>  1 file changed, 1 insertion(+), 1 deletion(-)
> >>
> >> diff --git a/hw/vfio/platform.c b/hw/vfio/platform.c
> >> index ac2cefc9b1..869ed2c39d 100644
> >> --- a/hw/vfio/platform.c
> >> +++ b/hw/vfio/platform.c
> >> @@ -236,7 +236,7 @@ static void vfio_intp_interrupt(VFIOINTp *intp)
> >>          trace_vfio_intp_interrupt_set_pending(intp->pin);
> >>          QSIMPLEQ_INSERT_TAIL(&vdev->pending_intp_queue,
> >>                               intp, pqnext);
> >> -        ret = event_notifier_test_and_clear(intp->interrupt);
> >> +        event_notifier_test_and_clear(intp->interrupt);
> >>          return;
> >>      }  
> > 
> > Testing that an event is pending in our notifier is generally a
> > prerequisite to doing anything in the interrupt handler, I don't
> > understand why we're just consuming it and ignoring the return value.
> > The above is in the delayed handling branch of the function, but the
> > normal non-delayed path would only go on to error_report() if the
> > notifier is not pending and then inject an interrupt anyway.  This all
> > seems rather suspicious and it's a unique pattern among the vfio
> > callers of this function.  Is there a more fundamental bug that this
> > function should perform this test once and return without doing
> > anything if it's called spuriously, ie. without a notifier pending?
> > Thanks,  
> 
> Hum that's correct that other VFIO call sites do the check. My
> understanding was that this could not fail in this case as, if we
> entered the handler there was something to be cleared. In which
> situation can this fail?

I'm not sure what the right answer is, I see examples either way
looking outside of vfio code.  On one hand, maybe we never get called
spuriously, on the other if it's the callee's responsibility to drain
events from the fd and we have it readily accessible whether there were
any events pending, why would we inject an interrupt if the result that
we have in hand shows no pending events?  The overhead of returning
based on that result is minuscule.

qemu_set_fd_handler() is a wrapper for aio_set_fd_handler().  Stefan is
a possible defacto maintainer of some of the aio code.  Stefan, do you
have thoughts on whether callbacks from event notifier fds should
consider spurious events?  Thanks,

Alex
Auger Eric Aug. 13, 2020, 7:18 p.m. UTC | #5
Hi Alex,

On 8/13/20 9:15 PM, Alex Williamson wrote:
> On Thu, 13 Aug 2020 20:02:45 +0200
> Auger Eric <eric.auger@redhat.com> wrote:
> 
>> Hi Alex,
>>
>> On 8/13/20 6:59 PM, Alex Williamson wrote:
>>> On Thu, 13 Aug 2020 15:37:08 +0800
>>> Chen Qun <kuhn.chenqun@huawei.com> wrote:
>>>   
>>>> Clang static code analyzer show warning:
>>>> hw/vfio/platform.c:239:9: warning: Value stored to 'ret' is never read
>>>>         ret = event_notifier_test_and_clear(intp->interrupt);
>>>>         ^     ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>>>>
>>>> Reported-by: Euler Robot <euler.robot@huawei.com>
>>>> Signed-off-by: Chen Qun <kuhn.chenqun@huawei.com>
>>>> ---
>>>> Cc: Alex Williamson <alex.williamson@redhat.com>
>>>> Cc: Eric Auger <eric.auger@redhat.com>
>>>> ---
>>>>  hw/vfio/platform.c | 2 +-
>>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>>
>>>> diff --git a/hw/vfio/platform.c b/hw/vfio/platform.c
>>>> index ac2cefc9b1..869ed2c39d 100644
>>>> --- a/hw/vfio/platform.c
>>>> +++ b/hw/vfio/platform.c
>>>> @@ -236,7 +236,7 @@ static void vfio_intp_interrupt(VFIOINTp *intp)
>>>>          trace_vfio_intp_interrupt_set_pending(intp->pin);
>>>>          QSIMPLEQ_INSERT_TAIL(&vdev->pending_intp_queue,
>>>>                               intp, pqnext);
>>>> -        ret = event_notifier_test_and_clear(intp->interrupt);
>>>> +        event_notifier_test_and_clear(intp->interrupt);
>>>>          return;
>>>>      }  
>>>
>>> Testing that an event is pending in our notifier is generally a
>>> prerequisite to doing anything in the interrupt handler, I don't
>>> understand why we're just consuming it and ignoring the return value.
>>> The above is in the delayed handling branch of the function, but the
>>> normal non-delayed path would only go on to error_report() if the
>>> notifier is not pending and then inject an interrupt anyway.  This all
>>> seems rather suspicious and it's a unique pattern among the vfio
>>> callers of this function.  Is there a more fundamental bug that this
>>> function should perform this test once and return without doing
>>> anything if it's called spuriously, ie. without a notifier pending?
>>> Thanks,  
>>
>> Hum that's correct that other VFIO call sites do the check. My
>> understanding was that this could not fail in this case as, if we
>> entered the handler there was something to be cleared. In which
>> situation can this fail?
> 
> I'm not sure what the right answer is, I see examples either way
> looking outside of vfio code.  On one hand, maybe we never get called
> spuriously, on the other if it's the callee's responsibility to drain
> events from the fd and we have it readily accessible whether there were
> any events pending, why would we inject an interrupt if the result that
> we have in hand shows no pending events?  The overhead of returning
> based on that result is minuscule.

I agree
> 
> qemu_set_fd_handler() is a wrapper for aio_set_fd_handler().  Stefan is
> a possible defacto maintainer of some of the aio code.  Stefan, do you
> have thoughts on whether callbacks from event notifier fds should
> consider spurious events?  Thanks,

Indeed I saw that for instance block/nvme.c nvme_handle_event is not
checking the result.

Let's wait for Stefan's answer ...

Thanks

Eric
> 
> Alex
>
Stefan Hajnoczi Aug. 18, 2020, 12:54 p.m. UTC | #6
On Thu, Aug 13, 2020 at 09:18:59PM +0200, Auger Eric wrote:
> Hi Alex,
> 
> On 8/13/20 9:15 PM, Alex Williamson wrote:
> > On Thu, 13 Aug 2020 20:02:45 +0200
> > Auger Eric <eric.auger@redhat.com> wrote:
> > 
> >> Hi Alex,
> >>
> >> On 8/13/20 6:59 PM, Alex Williamson wrote:
> >>> On Thu, 13 Aug 2020 15:37:08 +0800
> >>> Chen Qun <kuhn.chenqun@huawei.com> wrote:
> >>>   
> >>>> Clang static code analyzer show warning:
> >>>> hw/vfio/platform.c:239:9: warning: Value stored to 'ret' is never read
> >>>>         ret = event_notifier_test_and_clear(intp->interrupt);
> >>>>         ^     ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> >>>>
> >>>> Reported-by: Euler Robot <euler.robot@huawei.com>
> >>>> Signed-off-by: Chen Qun <kuhn.chenqun@huawei.com>
> >>>> ---
> >>>> Cc: Alex Williamson <alex.williamson@redhat.com>
> >>>> Cc: Eric Auger <eric.auger@redhat.com>
> >>>> ---
> >>>>  hw/vfio/platform.c | 2 +-
> >>>>  1 file changed, 1 insertion(+), 1 deletion(-)
> >>>>
> >>>> diff --git a/hw/vfio/platform.c b/hw/vfio/platform.c
> >>>> index ac2cefc9b1..869ed2c39d 100644
> >>>> --- a/hw/vfio/platform.c
> >>>> +++ b/hw/vfio/platform.c
> >>>> @@ -236,7 +236,7 @@ static void vfio_intp_interrupt(VFIOINTp *intp)
> >>>>          trace_vfio_intp_interrupt_set_pending(intp->pin);
> >>>>          QSIMPLEQ_INSERT_TAIL(&vdev->pending_intp_queue,
> >>>>                               intp, pqnext);
> >>>> -        ret = event_notifier_test_and_clear(intp->interrupt);
> >>>> +        event_notifier_test_and_clear(intp->interrupt);
> >>>>          return;
> >>>>      }  
> >>>
> >>> Testing that an event is pending in our notifier is generally a
> >>> prerequisite to doing anything in the interrupt handler, I don't
> >>> understand why we're just consuming it and ignoring the return value.
> >>> The above is in the delayed handling branch of the function, but the
> >>> normal non-delayed path would only go on to error_report() if the
> >>> notifier is not pending and then inject an interrupt anyway.  This all
> >>> seems rather suspicious and it's a unique pattern among the vfio
> >>> callers of this function.  Is there a more fundamental bug that this
> >>> function should perform this test once and return without doing
> >>> anything if it's called spuriously, ie. without a notifier pending?
> >>> Thanks,  
> >>
> >> Hum that's correct that other VFIO call sites do the check. My
> >> understanding was that this could not fail in this case as, if we
> >> entered the handler there was something to be cleared. In which
> >> situation can this fail?
> > 
> > I'm not sure what the right answer is, I see examples either way
> > looking outside of vfio code.  On one hand, maybe we never get called
> > spuriously, on the other if it's the callee's responsibility to drain
> > events from the fd and we have it readily accessible whether there were
> > any events pending, why would we inject an interrupt if the result that
> > we have in hand shows no pending events?  The overhead of returning
> > based on that result is minuscule.
> 
> I agree
> > 
> > qemu_set_fd_handler() is a wrapper for aio_set_fd_handler().  Stefan is
> > a possible defacto maintainer of some of the aio code.  Stefan, do you
> > have thoughts on whether callbacks from event notifier fds should
> > consider spurious events?  Thanks,
> 
> Indeed I saw that for instance block/nvme.c nvme_handle_event is not
> checking the result.
> 
> Let's wait for Stefan's answer ...

vfio_intp_interrupt() will always read a non-zero eventfd value, based
on these assumptions:

intp->interrupt is "readable" since vfio_intp_interrupt() is called by
the AioContext (event loop). "readable" does not guarantee that data can
actually be read because it also includes error events:

  new_node->pfd.events = (io_read ? G_IO_IN | G_IO_HUP | G_IO_ERR : 0);

However, I think we can exclude the error case for the VFIO interrupt
eventfds because there are no error cases for eventfds (unlike socket
disconnection, for example).

The other important assumption is that only one thread on the host is
monitoring the eventfd for activity.

Stefan
Auger Eric Aug. 18, 2020, 1:42 p.m. UTC | #7
Hi Stefan,

On 8/18/20 2:54 PM, Stefan Hajnoczi wrote:
> On Thu, Aug 13, 2020 at 09:18:59PM +0200, Auger Eric wrote:
>> Hi Alex,
>>
>> On 8/13/20 9:15 PM, Alex Williamson wrote:
>>> On Thu, 13 Aug 2020 20:02:45 +0200
>>> Auger Eric <eric.auger@redhat.com> wrote:
>>>
>>>> Hi Alex,
>>>>
>>>> On 8/13/20 6:59 PM, Alex Williamson wrote:
>>>>> On Thu, 13 Aug 2020 15:37:08 +0800
>>>>> Chen Qun <kuhn.chenqun@huawei.com> wrote:
>>>>>   
>>>>>> Clang static code analyzer show warning:
>>>>>> hw/vfio/platform.c:239:9: warning: Value stored to 'ret' is never read
>>>>>>         ret = event_notifier_test_and_clear(intp->interrupt);
>>>>>>         ^     ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>>>>>>
>>>>>> Reported-by: Euler Robot <euler.robot@huawei.com>
>>>>>> Signed-off-by: Chen Qun <kuhn.chenqun@huawei.com>
>>>>>> ---
>>>>>> Cc: Alex Williamson <alex.williamson@redhat.com>
>>>>>> Cc: Eric Auger <eric.auger@redhat.com>
>>>>>> ---
>>>>>>  hw/vfio/platform.c | 2 +-
>>>>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>>>>
>>>>>> diff --git a/hw/vfio/platform.c b/hw/vfio/platform.c
>>>>>> index ac2cefc9b1..869ed2c39d 100644
>>>>>> --- a/hw/vfio/platform.c
>>>>>> +++ b/hw/vfio/platform.c
>>>>>> @@ -236,7 +236,7 @@ static void vfio_intp_interrupt(VFIOINTp *intp)
>>>>>>          trace_vfio_intp_interrupt_set_pending(intp->pin);
>>>>>>          QSIMPLEQ_INSERT_TAIL(&vdev->pending_intp_queue,
>>>>>>                               intp, pqnext);
>>>>>> -        ret = event_notifier_test_and_clear(intp->interrupt);
>>>>>> +        event_notifier_test_and_clear(intp->interrupt);
>>>>>>          return;
>>>>>>      }  
>>>>>
>>>>> Testing that an event is pending in our notifier is generally a
>>>>> prerequisite to doing anything in the interrupt handler, I don't
>>>>> understand why we're just consuming it and ignoring the return value.
>>>>> The above is in the delayed handling branch of the function, but the
>>>>> normal non-delayed path would only go on to error_report() if the
>>>>> notifier is not pending and then inject an interrupt anyway.  This all
>>>>> seems rather suspicious and it's a unique pattern among the vfio
>>>>> callers of this function.  Is there a more fundamental bug that this
>>>>> function should perform this test once and return without doing
>>>>> anything if it's called spuriously, ie. without a notifier pending?
>>>>> Thanks,  
>>>>
>>>> Hum that's correct that other VFIO call sites do the check. My
>>>> understanding was that this could not fail in this case as, if we
>>>> entered the handler there was something to be cleared. In which
>>>> situation can this fail?
>>>
>>> I'm not sure what the right answer is, I see examples either way
>>> looking outside of vfio code.  On one hand, maybe we never get called
>>> spuriously, on the other if it's the callee's responsibility to drain
>>> events from the fd and we have it readily accessible whether there were
>>> any events pending, why would we inject an interrupt if the result that
>>> we have in hand shows no pending events?  The overhead of returning
>>> based on that result is minuscule.
>>
>> I agree
>>>
>>> qemu_set_fd_handler() is a wrapper for aio_set_fd_handler().  Stefan is
>>> a possible defacto maintainer of some of the aio code.  Stefan, do you
>>> have thoughts on whether callbacks from event notifier fds should
>>> consider spurious events?  Thanks,
>>
>> Indeed I saw that for instance block/nvme.c nvme_handle_event is not
>> checking the result.
>>
>> Let's wait for Stefan's answer ...
> 
> vfio_intp_interrupt() will always read a non-zero eventfd value, based
> on these assumptions:
> 
> intp->interrupt is "readable" since vfio_intp_interrupt() is called by
> the AioContext (event loop). "readable" does not guarantee that data can
> actually be read because it also includes error events:
> 
>   new_node->pfd.events = (io_read ? G_IO_IN | G_IO_HUP | G_IO_ERR : 0);
> 
> However, I think we can exclude the error case for the VFIO interrupt
> eventfds because there are no error cases for eventfds (unlike socket
> disconnection, for example).
> 
> The other important assumption is that only one thread on the host is
> monitoring the eventfd for activity.

Thank for your the confirmation. So this patch should be safe.

Best Regards

Eric
> 
> Stefan
>
diff mbox series

Patch

diff --git a/hw/vfio/platform.c b/hw/vfio/platform.c
index ac2cefc9b1..869ed2c39d 100644
--- a/hw/vfio/platform.c
+++ b/hw/vfio/platform.c
@@ -236,7 +236,7 @@  static void vfio_intp_interrupt(VFIOINTp *intp)
         trace_vfio_intp_interrupt_set_pending(intp->pin);
         QSIMPLEQ_INSERT_TAIL(&vdev->pending_intp_queue,
                              intp, pqnext);
-        ret = event_notifier_test_and_clear(intp->interrupt);
+        event_notifier_test_and_clear(intp->interrupt);
         return;
     }