On Mon, 20 Nov 2023 11:17:52 +0800
JianChunfu <chunfu.jian@xxxxxxxxxxxx> wrote:
> It seems a little unclear when dealing with vfio_intx_set_signal()
> because of vfio_pci_device which is irq_none,
> so separate the two situations.
>
> Signed-off-by: JianChunfu <chunfu.jian@xxxxxxxxxxxx>
> ---
> drivers/vfio/pci/vfio_pci_intrs.c | 31 +++++++++++++++----------------
> 1 file changed, 15 insertions(+), 16 deletions(-)
>
> diff --git a/drivers/vfio/pci/vfio_pci_intrs.c b/drivers/vfio/pci/vfio_pci_intrs.c
> index 6069a11fb51a..b6d126c48393 100644
> --- a/drivers/vfio/pci/vfio_pci_intrs.c
> +++ b/drivers/vfio/pci/vfio_pci_intrs.c
> @@ -468,6 +468,8 @@ static int vfio_pci_set_intx_trigger(struct vfio_pci_core_device *vdev,
> unsigned index, unsigned start,
> unsigned count, uint32_t flags, void *data)
> {
> + int32_t fd = *(int32_t *)data;
This is a null pointer dereference if anyone were to use
VFIO_IRQ_SET_DATA_NONE. Note this is also invalid for
VFIO_IRQ_SET_DATA_BOOL. I think this is largely why the function has
the current layout.
> +
> if (is_intx(vdev) && !count && (flags & VFIO_IRQ_SET_DATA_NONE)) {
> vfio_intx_disable(vdev);
> return 0;
> @@ -476,28 +478,25 @@ static int vfio_pci_set_intx_trigger(struct vfio_pci_core_device *vdev,
> if (!(is_intx(vdev) || is_irq_none(vdev)) || start != 0 || count != 1)
> return -EINVAL;
>
> - if (flags & VFIO_IRQ_SET_DATA_EVENTFD) {
> - int32_t fd = *(int32_t *)data;
> + if (!is_intx(vdev)) {
> int ret;
> + if (flags & VFIO_IRQ_SET_DATA_EVENTFD) {
@ret should be scoped within this branch with this layout and there
should be a blank line after variable declaration.
> + ret = vfio_intx_enable(vdev);
> + if (ret)
> + return ret;
>
> - if (is_intx(vdev))
> - return vfio_intx_set_signal(vdev, fd);
> + ret = vfio_intx_set_signal(vdev, fd);
> + if (ret)
> + vfio_intx_disable(vdev);
>
> - ret = vfio_intx_enable(vdev);
> - if (ret)
> return ret;
> -
> - ret = vfio_intx_set_signal(vdev, fd);
> - if (ret)
> - vfio_intx_disable(vdev);
> -
> - return ret;
> + } else
> + return -EINVAL;
Single line branches also get braces if the previous branch required
braces. Thanks,
Alex
> }
>
> - if (!is_intx(vdev))
> - return -EINVAL;
> -
> - if (flags & VFIO_IRQ_SET_DATA_NONE) {
> + if (flags & VFIO_IRQ_SET_DATA_EVENTFD) {
> + return vfio_intx_set_signal(vdev, fd);
> + } else if (flags & VFIO_IRQ_SET_DATA_NONE) {
> vfio_send_intx_eventfd(vdev, NULL);
> } else if (flags & VFIO_IRQ_SET_DATA_BOOL) {
> uint8_t trigger = *(uint8_t *)data;
