Re: [PATCH 4/8] xen/blkfront: don't trust the backend response data blindly

[Jan Beulich <jbeulich@xxxxxxxx>]

On 13.05.2021 12:02, Juergen Gross wrote:
> @@ -1574,10 +1580,16 @@ static irqreturn_t blkif_interrupt(int irq, void 
> *dev_id)
>       spin_lock_irqsave(&rinfo->ring_lock, flags);
>   again:
>       rp = rinfo->ring.sring->rsp_prod;
> +     if (RING_RESPONSE_PROD_OVERFLOW(&rinfo->ring, rp)) {
> +             pr_alert("%s: illegal number of responses %u\n",
> +                      info->gd->disk_name, rp - rinfo->ring.rsp_cons);
> +             goto err;
> +     }
>       rmb(); /* Ensure we see queued responses up to 'rp'. */

I think you want to insert after the barrier.

> @@ -1680,6 +1707,11 @@ static irqreturn_t blkif_interrupt(int irq, void 
> *dev_id)
>       spin_unlock_irqrestore(&rinfo->ring_lock, flags);
>  
>       return IRQ_HANDLED;
> +
> + err:
> +     info->connected = BLKIF_STATE_ERROR;
> +     pr_alert("%s disabled for further use\n", info->gd->disk_name);
> +     return IRQ_HANDLED;
>  }

Am I understanding that a suspend (and then resume) can be used to
recover from error state? If so - is this intentional? If so in turn,
would it make sense to spell this out in the description?

Jan