Re: kern/53506: Easy trigger of "panic: biodone2 already" on NetBSD 8.0 Xen domU

[Emmanuel Dreyfus <manu%netbsd.org@localhost>]

The following reply was made to PR port-xen/53506; it has been noted by GNATS.

From: Emmanuel Dreyfus <manu%netbsd.org@localhost>
To: gnats-bugs%NetBSD.org@localhost
Cc: 
Subject: Re: kern/53506: Easy trigger of "panic: biodone2 already" on NetBSD
 8.0 Xen domU
Date: Mon, 20 Aug 2018 01:38:59 +0000

 --/04w6evG8XlLl3ft
 Content-Type: text/plain; charset=us-ascii
 Content-Disposition: inline
 
 The attached patch clears the struct xbd_req content between usage, 
 while there adds a few sanity checks. At mine it is enough to 
 prevent any panic on biodone2 already.
 
 If it looks good enough, I think we should pull it up to netbsd-8 so
 that backups are possible again. The code can be more heavily 
 reworked on -current afterwards.
 
 --  
 Emmanuel Dreyfus
 manu%netbsd.org@localhost
 
 --/04w6evG8XlLl3ft
 Content-Type: text/plain; charset=us-ascii
 Content-Disposition: attachment; filename=patch-biodone2already
 
 --- ./sys/arch/xen/xen/xbd_xenbus.c.orig
 +++ ./sys/arch/xen/xen/xbd_xenbus.c
 @@ -222,8 +222,21 @@
  	.d_close = xbdclose,
  	.d_diskstart = xbd_diskstart,
  };
  
 +static inline void
 +xbd_xbdreq_clear(struct xbd_xenbus_softc *sc, struct xbd_req *xbdreq)
 +{       
 +	int i = xbdreq - sc->sc_reqs;
 +
 +	KASSERT(xbdreq->req_id == i);
 +	memset(xbdreq, 0, sizeof(*xbdreq));
 +	xbdreq->req_id = i;
 +
 +	return;
 +}
 +
 +
  static int
  xbd_xenbus_match(device_t parent, cfdata_t match, void *aux)
  {
  	struct xenbusdev_attach_args *xa = aux;
 @@ -652,8 +665,9 @@
  	for (i = sc->sc_ring.rsp_cons; i != resp_prod; i++) {
  		blkif_response_t *rep = RING_GET_RESPONSE(&sc->sc_ring, i);
  		struct xbd_req *xbdreq = &sc->sc_reqs[rep->id];
  		bp = xbdreq->req_bp;
 +		KASSERT(bp != NULL);
  		DPRINTF(("xbd_handler(%p): b_bcount = %ld\n",
  		    xbdreq->req_bp, (long)bp->b_bcount));
  		if (rep->operation == BLKIF_OP_FLUSH_DISKCACHE) {
  			xbdreq->req_sync.s_error = rep->status;
 @@ -693,8 +707,9 @@
  			xbd_unmap_align(xbdreq);
  
  		dk_done(&sc->sc_dksc, bp);
  
 +		xbdreq->req_bp = NULL; /* Do not reuse */
  		SLIST_INSERT_HEAD(&sc->sc_xbdreq_head, xbdreq, req_next);
  	}
  done:
  	xen_rmb();
 @@ -854,8 +869,9 @@
  		if (__predict_false(xbdreq == NULL)) {
  			DPRINTF(("xbdioctl: no req\n"));
  			error = ENOMEM;
  		} else {
 +			xbd_xbdreq_clear(sc, xbdreq);
  			SLIST_REMOVE_HEAD(&sc->sc_xbdreq_head, req_next);
  			req = RING_GET_REQUEST(&sc->sc_ring,
  			    sc->sc_ring.req_prod_pvt);
  			req->id = xbdreq->req_id;
 @@ -952,8 +968,9 @@
  		error = EAGAIN;
  		goto out;
  	}
  
 +	xbd_xbdreq_clear(sc, xbdreq);
  	xbdreq->req_bp = bp;
  	xbdreq->req_data = bp->b_data;
  	if ((vaddr_t)bp->b_data & (XEN_BSIZE - 1)) {
  		if (__predict_false(xbd_map_align(xbdreq) != 0)) {
 
 --/04w6evG8XlLl3ft--