From 4b511d5bfa74b1926daefd1694205c7f1bcf677f Mon Sep 17 00:00:00 2001 From: Juergen Gross Date: Fri, 30 Jul 2021 11:26:21 +0200 Subject: [PATCH 01/11] xen: fix setting of max_pfn in shared_info Xen PV guests are specifying the highest used PFN via the max_pfn field in shared_info. This value is used by the Xen tools when saving or migrating the guest. Unfortunately this field is misnamed, as in reality it is specifying the number of pages (including any memory holes) of the guest, so it is the highest used PFN + 1. Renaming isn't possible, as this is a public Xen hypervisor interface which needs to be kept stable. The kernel will set the value correctly initially at boot time, but when adding more pages (e.g. due to memory hotplug or ballooning) a real PFN number is stored in max_pfn. This is done when expanding the p2m array, and the PFN stored there is even possibly wrong, as it should be the last possible PFN of the just added P2M frame, and not one which led to the P2M expansion. Fix that by setting shared_info->max_pfn to the last possible PFN + 1. Fixes: 98dd166ea3a3c3 ("x86/xen/p2m: hint at the last populated P2M entry") Cc: stable@vger.kernel.org Signed-off-by: Juergen Gross Reviewed-by: Jan Beulich Link: https://lore.kernel.org/r/20210730092622.9973-2-jgross@suse.com Signed-off-by: Juergen Gross --- arch/x86/xen/p2m.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/arch/x86/xen/p2m.c b/arch/x86/xen/p2m.c index ac06ca32e9ef7..5e6e236977c75 100644 --- a/arch/x86/xen/p2m.c +++ b/arch/x86/xen/p2m.c @@ -618,8 +618,8 @@ int xen_alloc_p2m_entry(unsigned long pfn) } /* Expanded the p2m? */ - if (pfn > xen_p2m_last_pfn) { - xen_p2m_last_pfn = pfn; + if (pfn >= xen_p2m_last_pfn) { + xen_p2m_last_pfn = ALIGN(pfn + 1, P2M_PER_PAGE); HYPERVISOR_shared_info->arch.max_pfn = xen_p2m_last_pfn; } From ac4c403c9036793dfbf63e75acd1772cdac778de Mon Sep 17 00:00:00 2001 From: Juergen Gross Date: Fri, 30 Jul 2021 09:18:02 +0200 Subject: [PATCH 02/11] xen: check required Xen features Linux kernel is not supported to run on Xen versions older than 4.0. Add tests for required Xen features always being present in Xen 4.0 and newer. Signed-off-by: Juergen Gross Reviewed-by: Boris Ostrovsky Link: https://lore.kernel.org/r/20210730071804.4302-2-jgross@suse.com Signed-off-by: Juergen Gross --- drivers/xen/features.c | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/drivers/xen/features.c b/drivers/xen/features.c index 25c053b096051..7b591443833c9 100644 --- a/drivers/xen/features.c +++ b/drivers/xen/features.c @@ -9,13 +9,26 @@ #include #include #include +#include #include +#include #include #include #include +/* + * Linux kernel expects at least Xen 4.0. + * + * Assume some features to be available for that reason (depending on guest + * mode, of course). + */ +#define chk_required_feature(f) { \ + if (!xen_feature(f)) \ + panic("Xen: feature %s not available!\n", #f); \ + } + u8 xen_features[XENFEAT_NR_SUBMAPS * 32] __read_mostly; EXPORT_SYMBOL_GPL(xen_features); @@ -31,4 +44,9 @@ void xen_setup_features(void) for (j = 0; j < 32; j++) xen_features[i * 32 + j] = !!(fi.submap & 1< Date: Fri, 30 Jul 2021 09:18:03 +0200 Subject: [PATCH 03/11] xen: assume XENFEAT_mmu_pt_update_preserve_ad being set for pv guests XENFEAT_mmu_pt_update_preserve_ad is always set in Xen 4.0 and newer. Remove coding assuming it might be zero. Signed-off-by: Juergen Gross Acked-by: Peter Zijlstra (Intel) Reviewed-by: Boris Ostrovsky Link: https://lore.kernel.org/r/20210730071804.4302-3-jgross@suse.com Signed-off-by: Juergen Gross --- arch/x86/xen/enlighten_pv.c | 12 ++---------- arch/x86/xen/mmu_pv.c | 4 ++-- 2 files changed, 4 insertions(+), 12 deletions(-) diff --git a/arch/x86/xen/enlighten_pv.c b/arch/x86/xen/enlighten_pv.c index 03149422dce2b..753f63734c134 100644 --- a/arch/x86/xen/enlighten_pv.c +++ b/arch/x86/xen/enlighten_pv.c @@ -116,9 +116,8 @@ static void __init xen_banner(void) HYPERVISOR_xen_version(XENVER_extraversion, &extra); pr_info("Booting paravirtualized kernel on %s\n", pv_info.name); - printk(KERN_INFO "Xen version: %d.%d%s%s\n", - version >> 16, version & 0xffff, extra.extraversion, - xen_feature(XENFEAT_mmu_pt_update_preserve_ad) ? " (preserve-AD)" : ""); + pr_info("Xen version: %d.%d%s (preserve-AD)\n", + version >> 16, version & 0xffff, extra.extraversion); } static void __init xen_pv_init_platform(void) @@ -1302,13 +1301,6 @@ asmlinkage __visible void __init xen_start_kernel(void) xen_init_apic(); #endif - if (xen_feature(XENFEAT_mmu_pt_update_preserve_ad)) { - pv_ops.mmu.ptep_modify_prot_start = - xen_ptep_modify_prot_start; - pv_ops.mmu.ptep_modify_prot_commit = - xen_ptep_modify_prot_commit; - } - machine_ops = xen_machine_ops; /* diff --git a/arch/x86/xen/mmu_pv.c b/arch/x86/xen/mmu_pv.c index ade789e73ee42..1df5f01529e59 100644 --- a/arch/x86/xen/mmu_pv.c +++ b/arch/x86/xen/mmu_pv.c @@ -2099,8 +2099,8 @@ static const struct pv_mmu_ops xen_mmu_ops __initconst = { .set_pte = xen_set_pte_init, .set_pmd = xen_set_pmd_hyper, - .ptep_modify_prot_start = __ptep_modify_prot_start, - .ptep_modify_prot_commit = __ptep_modify_prot_commit, + .ptep_modify_prot_start = xen_ptep_modify_prot_start, + .ptep_modify_prot_commit = xen_ptep_modify_prot_commit, .pte_val = PV_CALLEE_SAVE(xen_pte_val), .pgd_val = PV_CALLEE_SAVE(xen_pgd_val), From 30dcc56bba911db561c35d4131baf983a41023f8 Mon Sep 17 00:00:00 2001 From: Juergen Gross Date: Fri, 30 Jul 2021 09:18:04 +0200 Subject: [PATCH 04/11] xen: assume XENFEAT_gnttab_map_avail_bits being set for pv guests XENFEAT_gnttab_map_avail_bits is always set in Xen 4.0 and newer. Remove coding assuming it might be zero. Signed-off-by: Juergen Gross Acked-by: Peter Zijlstra (Intel) Reviewed-by: Boris Ostrovsky Link: https://lore.kernel.org/r/20210730071804.4302-4-jgross@suse.com Signed-off-by: Juergen Gross --- drivers/xen/gntdev.c | 36 ++---------------------------------- 1 file changed, 2 insertions(+), 34 deletions(-) diff --git a/drivers/xen/gntdev.c b/drivers/xen/gntdev.c index a3e7be96527d7..1e7f6b1c0c971 100644 --- a/drivers/xen/gntdev.c +++ b/drivers/xen/gntdev.c @@ -266,20 +266,13 @@ static int find_grant_ptes(pte_t *pte, unsigned long addr, void *data) { struct gntdev_grant_map *map = data; unsigned int pgnr = (addr - map->vma->vm_start) >> PAGE_SHIFT; - int flags = map->flags | GNTMAP_application_map | GNTMAP_contains_pte; + int flags = map->flags | GNTMAP_application_map | GNTMAP_contains_pte | + (1 << _GNTMAP_guest_avail0); u64 pte_maddr; BUG_ON(pgnr >= map->count); pte_maddr = arbitrary_virt_to_machine(pte).maddr; - /* - * Set the PTE as special to force get_user_pages_fast() fall - * back to the slow path. If this is not supported as part of - * the grant map, it will be done afterwards. - */ - if (xen_feature(XENFEAT_gnttab_map_avail_bits)) - flags |= (1 << _GNTMAP_guest_avail0); - gnttab_set_map_op(&map->map_ops[pgnr], pte_maddr, flags, map->grants[pgnr].ref, map->grants[pgnr].domid); @@ -288,14 +281,6 @@ static int find_grant_ptes(pte_t *pte, unsigned long addr, void *data) return 0; } -#ifdef CONFIG_X86 -static int set_grant_ptes_as_special(pte_t *pte, unsigned long addr, void *data) -{ - set_pte_at(current->mm, addr, pte, pte_mkspecial(*pte)); - return 0; -} -#endif - int gntdev_map_grant_pages(struct gntdev_grant_map *map) { int i, err = 0; @@ -1055,23 +1040,6 @@ static int gntdev_mmap(struct file *flip, struct vm_area_struct *vma) err = vm_map_pages_zero(vma, map->pages, map->count); if (err) goto out_put_map; - } else { -#ifdef CONFIG_X86 - /* - * If the PTEs were not made special by the grant map - * hypercall, do so here. - * - * This is racy since the mapping is already visible - * to userspace but userspace should be well-behaved - * enough to not touch it until the mmap() call - * returns. - */ - if (!xen_feature(XENFEAT_gnttab_map_avail_bits)) { - apply_to_page_range(vma->vm_mm, vma->vm_start, - vma->vm_end - vma->vm_start, - set_grant_ptes_as_special, NULL); - } -#endif } return 0; From 71b66243f9898d0e54296b4e7035fb33cdcb0707 Mon Sep 17 00:00:00 2001 From: Juergen Gross Date: Fri, 30 Jul 2021 12:38:52 +0200 Subject: [PATCH 05/11] xen/blkfront: read response from backend only once MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit In order to avoid problems in case the backend is modifying a response on the ring page while the frontend has already seen it, just read the response into a local buffer in one go and then operate on that buffer only. Signed-off-by: Juergen Gross Reviewed-by: Jan Beulich Acked-by: Roger Pau Monné Link: https://lore.kernel.org/r/20210730103854.12681-2-jgross@suse.com Signed-off-by: Juergen Gross --- drivers/block/xen-blkfront.c | 35 ++++++++++++++++++----------------- 1 file changed, 18 insertions(+), 17 deletions(-) diff --git a/drivers/block/xen-blkfront.c b/drivers/block/xen-blkfront.c index d83fee21f6c59..15e8402877340 100644 --- a/drivers/block/xen-blkfront.c +++ b/drivers/block/xen-blkfront.c @@ -1496,7 +1496,7 @@ static bool blkif_completion(unsigned long *id, static irqreturn_t blkif_interrupt(int irq, void *dev_id) { struct request *req; - struct blkif_response *bret; + struct blkif_response bret; RING_IDX i, rp; unsigned long flags; struct blkfront_ring_info *rinfo = (struct blkfront_ring_info *)dev_id; @@ -1513,8 +1513,9 @@ static irqreturn_t blkif_interrupt(int irq, void *dev_id) for (i = rinfo->ring.rsp_cons; i != rp; i++) { unsigned long id; - bret = RING_GET_RESPONSE(&rinfo->ring, i); - id = bret->id; + RING_COPY_RESPONSE(&rinfo->ring, i, &bret); + id = bret.id; + /* * The backend has messed up and given us an id that we would * never have given to it (we stamp it up to BLK_RING_SIZE - @@ -1522,39 +1523,39 @@ static irqreturn_t blkif_interrupt(int irq, void *dev_id) */ if (id >= BLK_RING_SIZE(info)) { WARN(1, "%s: response to %s has incorrect id (%ld)\n", - info->gd->disk_name, op_name(bret->operation), id); + info->gd->disk_name, op_name(bret.operation), id); /* We can't safely get the 'struct request' as * the id is busted. */ continue; } req = rinfo->shadow[id].request; - if (bret->operation != BLKIF_OP_DISCARD) { + if (bret.operation != BLKIF_OP_DISCARD) { /* * We may need to wait for an extra response if the * I/O request is split in 2 */ - if (!blkif_completion(&id, rinfo, bret)) + if (!blkif_completion(&id, rinfo, &bret)) continue; } if (add_id_to_freelist(rinfo, id)) { WARN(1, "%s: response to %s (id %ld) couldn't be recycled!\n", - info->gd->disk_name, op_name(bret->operation), id); + info->gd->disk_name, op_name(bret.operation), id); continue; } - if (bret->status == BLKIF_RSP_OKAY) + if (bret.status == BLKIF_RSP_OKAY) blkif_req(req)->error = BLK_STS_OK; else blkif_req(req)->error = BLK_STS_IOERR; - switch (bret->operation) { + switch (bret.operation) { case BLKIF_OP_DISCARD: - if (unlikely(bret->status == BLKIF_RSP_EOPNOTSUPP)) { + if (unlikely(bret.status == BLKIF_RSP_EOPNOTSUPP)) { struct request_queue *rq = info->rq; printk(KERN_WARNING "blkfront: %s: %s op failed\n", - info->gd->disk_name, op_name(bret->operation)); + info->gd->disk_name, op_name(bret.operation)); blkif_req(req)->error = BLK_STS_NOTSUPP; info->feature_discard = 0; info->feature_secdiscard = 0; @@ -1564,15 +1565,15 @@ static irqreturn_t blkif_interrupt(int irq, void *dev_id) break; case BLKIF_OP_FLUSH_DISKCACHE: case BLKIF_OP_WRITE_BARRIER: - if (unlikely(bret->status == BLKIF_RSP_EOPNOTSUPP)) { + if (unlikely(bret.status == BLKIF_RSP_EOPNOTSUPP)) { printk(KERN_WARNING "blkfront: %s: %s op failed\n", - info->gd->disk_name, op_name(bret->operation)); + info->gd->disk_name, op_name(bret.operation)); blkif_req(req)->error = BLK_STS_NOTSUPP; } - if (unlikely(bret->status == BLKIF_RSP_ERROR && + if (unlikely(bret.status == BLKIF_RSP_ERROR && rinfo->shadow[id].req.u.rw.nr_segments == 0)) { printk(KERN_WARNING "blkfront: %s: empty %s op failed\n", - info->gd->disk_name, op_name(bret->operation)); + info->gd->disk_name, op_name(bret.operation)); blkif_req(req)->error = BLK_STS_NOTSUPP; } if (unlikely(blkif_req(req)->error)) { @@ -1585,9 +1586,9 @@ static irqreturn_t blkif_interrupt(int irq, void *dev_id) fallthrough; case BLKIF_OP_READ: case BLKIF_OP_WRITE: - if (unlikely(bret->status != BLKIF_RSP_OKAY)) + if (unlikely(bret.status != BLKIF_RSP_OKAY)) dev_dbg(&info->xbdev->dev, "Bad return from blkdev data " - "request: %x\n", bret->status); + "request: %x\n", bret.status); break; default: From 8f5a695d99000fc3aa73934d7ced33cfc64dcdab Mon Sep 17 00:00:00 2001 From: Juergen Gross Date: Fri, 30 Jul 2021 12:38:53 +0200 Subject: [PATCH 06/11] xen/blkfront: don't take local copy of a request from the ring page MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit In order to avoid a malicious backend being able to influence the local copy of a request build the request locally first and then copy it to the ring page instead of doing it the other way round as today. Signed-off-by: Juergen Gross Reviewed-by: Jan Beulich Acked-by: Roger Pau Monné Link: https://lore.kernel.org/r/20210730103854.12681-3-jgross@suse.com Signed-off-by: Juergen Gross --- drivers/block/xen-blkfront.c | 25 +++++++++++++++---------- 1 file changed, 15 insertions(+), 10 deletions(-) diff --git a/drivers/block/xen-blkfront.c b/drivers/block/xen-blkfront.c index 15e8402877340..b7301006fb281 100644 --- a/drivers/block/xen-blkfront.c +++ b/drivers/block/xen-blkfront.c @@ -533,7 +533,7 @@ static unsigned long blkif_ring_get_request(struct blkfront_ring_info *rinfo, rinfo->shadow[id].status = REQ_WAITING; rinfo->shadow[id].associated_id = NO_ASSOCIATED_ID; - (*ring_req)->u.rw.id = id; + rinfo->shadow[id].req.u.rw.id = id; return id; } @@ -541,11 +541,12 @@ static unsigned long blkif_ring_get_request(struct blkfront_ring_info *rinfo, static int blkif_queue_discard_req(struct request *req, struct blkfront_ring_info *rinfo) { struct blkfront_info *info = rinfo->dev_info; - struct blkif_request *ring_req; + struct blkif_request *ring_req, *final_ring_req; unsigned long id; /* Fill out a communications ring structure. */ - id = blkif_ring_get_request(rinfo, req, &ring_req); + id = blkif_ring_get_request(rinfo, req, &final_ring_req); + ring_req = &rinfo->shadow[id].req; ring_req->operation = BLKIF_OP_DISCARD; ring_req->u.discard.nr_sectors = blk_rq_sectors(req); @@ -556,8 +557,8 @@ static int blkif_queue_discard_req(struct request *req, struct blkfront_ring_inf else ring_req->u.discard.flag = 0; - /* Keep a private copy so we can reissue requests when recovering. */ - rinfo->shadow[id].req = *ring_req; + /* Copy the request to the ring page. */ + *final_ring_req = *ring_req; return 0; } @@ -690,6 +691,7 @@ static int blkif_queue_rw_req(struct request *req, struct blkfront_ring_info *ri { struct blkfront_info *info = rinfo->dev_info; struct blkif_request *ring_req, *extra_ring_req = NULL; + struct blkif_request *final_ring_req, *final_extra_ring_req = NULL; unsigned long id, extra_id = NO_ASSOCIATED_ID; bool require_extra_req = false; int i; @@ -734,7 +736,8 @@ static int blkif_queue_rw_req(struct request *req, struct blkfront_ring_info *ri } /* Fill out a communications ring structure. */ - id = blkif_ring_get_request(rinfo, req, &ring_req); + id = blkif_ring_get_request(rinfo, req, &final_ring_req); + ring_req = &rinfo->shadow[id].req; num_sg = blk_rq_map_sg(req->q, req, rinfo->shadow[id].sg); num_grant = 0; @@ -785,7 +788,9 @@ static int blkif_queue_rw_req(struct request *req, struct blkfront_ring_info *ri ring_req->u.rw.nr_segments = num_grant; if (unlikely(require_extra_req)) { extra_id = blkif_ring_get_request(rinfo, req, - &extra_ring_req); + &final_extra_ring_req); + extra_ring_req = &rinfo->shadow[extra_id].req; + /* * Only the first request contains the scatter-gather * list. @@ -827,10 +832,10 @@ static int blkif_queue_rw_req(struct request *req, struct blkfront_ring_info *ri if (setup.segments) kunmap_atomic(setup.segments); - /* Keep a private copy so we can reissue requests when recovering. */ - rinfo->shadow[id].req = *ring_req; + /* Copy request(s) to the ring page. */ + *final_ring_req = *ring_req; if (unlikely(require_extra_req)) - rinfo->shadow[extra_id].req = *extra_ring_req; + *final_extra_ring_req = *extra_ring_req; if (new_persistent_gnts) gnttab_free_grant_references(setup.gref_head); From b94e4b147fd1992ad450e1fea1fdaa3738753373 Mon Sep 17 00:00:00 2001 From: Juergen Gross Date: Fri, 30 Jul 2021 12:38:54 +0200 Subject: [PATCH 07/11] xen/blkfront: don't trust the backend response data blindly MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Today blkfront will trust the backend to send only sane response data. In order to avoid privilege escalations or crashes in case of malicious backends verify the data to be within expected limits. Especially make sure that the response always references an outstanding request. Introduce a new state of the ring BLKIF_STATE_ERROR which will be switched to in case an inconsistency is being detected. Recovering from this state is possible only via removing and adding the virtual device again (e.g. via a suspend/resume cycle). Make all warning messages issued due to valid error responses rate limited in order to avoid message floods being triggered by a malicious backend. Signed-off-by: Juergen Gross Reviewed-by: Jan Beulich Acked-by: Roger Pau Monné Link: https://lore.kernel.org/r/20210730103854.12681-4-jgross@suse.com Signed-off-by: Juergen Gross --- drivers/block/xen-blkfront.c | 70 +++++++++++++++++++++++++++--------- 1 file changed, 53 insertions(+), 17 deletions(-) diff --git a/drivers/block/xen-blkfront.c b/drivers/block/xen-blkfront.c index b7301006fb281..7a5e62fae171c 100644 --- a/drivers/block/xen-blkfront.c +++ b/drivers/block/xen-blkfront.c @@ -80,6 +80,7 @@ enum blkif_state { BLKIF_STATE_DISCONNECTED, BLKIF_STATE_CONNECTED, BLKIF_STATE_SUSPENDED, + BLKIF_STATE_ERROR, }; struct grant { @@ -89,6 +90,7 @@ struct grant { }; enum blk_req_status { + REQ_PROCESSING, REQ_WAITING, REQ_DONE, REQ_ERROR, @@ -530,7 +532,7 @@ static unsigned long blkif_ring_get_request(struct blkfront_ring_info *rinfo, id = get_id_from_freelist(rinfo); rinfo->shadow[id].request = req; - rinfo->shadow[id].status = REQ_WAITING; + rinfo->shadow[id].status = REQ_PROCESSING; rinfo->shadow[id].associated_id = NO_ASSOCIATED_ID; rinfo->shadow[id].req.u.rw.id = id; @@ -559,6 +561,7 @@ static int blkif_queue_discard_req(struct request *req, struct blkfront_ring_inf /* Copy the request to the ring page. */ *final_ring_req = *ring_req; + rinfo->shadow[id].status = REQ_WAITING; return 0; } @@ -834,8 +837,11 @@ static int blkif_queue_rw_req(struct request *req, struct blkfront_ring_info *ri /* Copy request(s) to the ring page. */ *final_ring_req = *ring_req; - if (unlikely(require_extra_req)) + rinfo->shadow[id].status = REQ_WAITING; + if (unlikely(require_extra_req)) { *final_extra_ring_req = *extra_ring_req; + rinfo->shadow[extra_id].status = REQ_WAITING; + } if (new_persistent_gnts) gnttab_free_grant_references(setup.gref_head); @@ -1359,8 +1365,8 @@ static enum blk_req_status blkif_rsp_to_req_status(int rsp) static int blkif_get_final_status(enum blk_req_status s1, enum blk_req_status s2) { - BUG_ON(s1 == REQ_WAITING); - BUG_ON(s2 == REQ_WAITING); + BUG_ON(s1 < REQ_DONE); + BUG_ON(s2 < REQ_DONE); if (s1 == REQ_ERROR || s2 == REQ_ERROR) return BLKIF_RSP_ERROR; @@ -1393,7 +1399,7 @@ static bool blkif_completion(unsigned long *id, s->status = blkif_rsp_to_req_status(bret->status); /* Wait the second response if not yet here. */ - if (s2->status == REQ_WAITING) + if (s2->status < REQ_DONE) return false; bret->status = blkif_get_final_status(s->status, @@ -1512,11 +1518,17 @@ static irqreturn_t blkif_interrupt(int irq, void *dev_id) spin_lock_irqsave(&rinfo->ring_lock, flags); again: - rp = rinfo->ring.sring->rsp_prod; - rmb(); /* Ensure we see queued responses up to 'rp'. */ + rp = READ_ONCE(rinfo->ring.sring->rsp_prod); + virt_rmb(); /* Ensure we see queued responses up to 'rp'. */ + 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; + } for (i = rinfo->ring.rsp_cons; i != rp; i++) { unsigned long id; + unsigned int op; RING_COPY_RESPONSE(&rinfo->ring, i, &bret); id = bret.id; @@ -1527,14 +1539,28 @@ static irqreturn_t blkif_interrupt(int irq, void *dev_id) * look in get_id_from_freelist. */ if (id >= BLK_RING_SIZE(info)) { - WARN(1, "%s: response to %s has incorrect id (%ld)\n", - info->gd->disk_name, op_name(bret.operation), id); - /* We can't safely get the 'struct request' as - * the id is busted. */ - continue; + pr_alert("%s: response has incorrect id (%ld)\n", + info->gd->disk_name, id); + goto err; + } + if (rinfo->shadow[id].status != REQ_WAITING) { + pr_alert("%s: response references no pending request\n", + info->gd->disk_name); + goto err; } + + rinfo->shadow[id].status = REQ_PROCESSING; req = rinfo->shadow[id].request; + op = rinfo->shadow[id].req.operation; + if (op == BLKIF_OP_INDIRECT) + op = rinfo->shadow[id].req.u.indirect.indirect_op; + if (bret.operation != op) { + pr_alert("%s: response has wrong operation (%u instead of %u)\n", + info->gd->disk_name, bret.operation, op); + goto err; + } + if (bret.operation != BLKIF_OP_DISCARD) { /* * We may need to wait for an extra response if the @@ -1559,7 +1585,8 @@ static irqreturn_t blkif_interrupt(int irq, void *dev_id) case BLKIF_OP_DISCARD: if (unlikely(bret.status == BLKIF_RSP_EOPNOTSUPP)) { struct request_queue *rq = info->rq; - printk(KERN_WARNING "blkfront: %s: %s op failed\n", + + pr_warn_ratelimited("blkfront: %s: %s op failed\n", info->gd->disk_name, op_name(bret.operation)); blkif_req(req)->error = BLK_STS_NOTSUPP; info->feature_discard = 0; @@ -1571,13 +1598,13 @@ static irqreturn_t blkif_interrupt(int irq, void *dev_id) case BLKIF_OP_FLUSH_DISKCACHE: case BLKIF_OP_WRITE_BARRIER: if (unlikely(bret.status == BLKIF_RSP_EOPNOTSUPP)) { - printk(KERN_WARNING "blkfront: %s: %s op failed\n", + pr_warn_ratelimited("blkfront: %s: %s op failed\n", info->gd->disk_name, op_name(bret.operation)); blkif_req(req)->error = BLK_STS_NOTSUPP; } if (unlikely(bret.status == BLKIF_RSP_ERROR && rinfo->shadow[id].req.u.rw.nr_segments == 0)) { - printk(KERN_WARNING "blkfront: %s: empty %s op failed\n", + pr_warn_ratelimited("blkfront: %s: empty %s op failed\n", info->gd->disk_name, op_name(bret.operation)); blkif_req(req)->error = BLK_STS_NOTSUPP; } @@ -1592,8 +1619,9 @@ static irqreturn_t blkif_interrupt(int irq, void *dev_id) case BLKIF_OP_READ: case BLKIF_OP_WRITE: if (unlikely(bret.status != BLKIF_RSP_OKAY)) - dev_dbg(&info->xbdev->dev, "Bad return from blkdev data " - "request: %x\n", bret.status); + dev_dbg_ratelimited(&info->xbdev->dev, + "Bad return from blkdev data request: %#x\n", + bret.status); break; default: @@ -1619,6 +1647,14 @@ 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; + + spin_unlock_irqrestore(&rinfo->ring_lock, flags); + + pr_alert("%s disabled for further use\n", info->gd->disk_name); + return IRQ_HANDLED; } From bb70913dceca050e8f4f60ba5361fcf62460df06 Mon Sep 17 00:00:00 2001 From: Jing Yangyang Date: Tue, 24 Aug 2021 23:24:51 -0700 Subject: [PATCH 08/11] drivers/xen/xenbus/xenbus_client.c: fix bugon.cocci warnings Use BUG_ON instead of a if condition followed by BUG. Generated by: scripts/coccinelle/misc/bugon.cocci Reported-by: Zeal Robot Signed-off-by: Jing Yangyang Reviewed-by: SeongJae Park Reviewed-by: Juergen Gross Link: https://lore.kernel.org/r/20210825062451.69998-1-deng.changcheng@zte.com.cn Signed-off-by: Juergen Gross --- drivers/xen/xenbus/xenbus_client.c | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-) diff --git a/drivers/xen/xenbus/xenbus_client.c b/drivers/xen/xenbus/xenbus_client.c index 0cd728961fce9..e8bed1cb76ba2 100644 --- a/drivers/xen/xenbus/xenbus_client.c +++ b/drivers/xen/xenbus/xenbus_client.c @@ -542,8 +542,7 @@ static int __xenbus_map_ring(struct xenbus_device *dev, } } - if (HYPERVISOR_grant_table_op(GNTTABOP_unmap_grant_ref, info->unmap, j)) - BUG(); + BUG_ON(HYPERVISOR_grant_table_op(GNTTABOP_unmap_grant_ref, info->unmap, j)); *leaked = false; for (i = 0; i < j; i++) { @@ -581,8 +580,7 @@ static int xenbus_unmap_ring(struct xenbus_device *dev, grant_handle_t *handles, gnttab_set_unmap_op(&unmap[i], vaddrs[i], GNTMAP_host_map, handles[i]); - if (HYPERVISOR_grant_table_op(GNTTABOP_unmap_grant_ref, unmap, i)) - BUG(); + BUG_ON(HYPERVISOR_grant_table_op(GNTTABOP_unmap_grant_ref, unmap, i)); err = GNTST_okay; for (i = 0; i < nr_handles; i++) { @@ -778,8 +776,7 @@ static int xenbus_unmap_ring_pv(struct xenbus_device *dev, void *vaddr) unmap[i].handle = node->handles[i]; } - if (HYPERVISOR_grant_table_op(GNTTABOP_unmap_grant_ref, unmap, i)) - BUG(); + BUG_ON(HYPERVISOR_grant_table_op(GNTTABOP_unmap_grant_ref, unmap, i)); err = GNTST_okay; leaked = false; From 1a0df28c09831dc4bd336fe6e090bbe85a07e79f Mon Sep 17 00:00:00 2001 From: zhaoxiao Date: Wed, 25 Aug 2021 19:41:11 +0800 Subject: [PATCH 09/11] x86: xen: platform-pci-unplug: use pr_err() and pr_warn() instead of raw printk() Since we have the nice helpers pr_err() and pr_warn(), use them instead of raw printk(). [jgross@suse.com] Move the "#define pr_fmt" above the #includes in order to avoid build warnings due to redefinition. Signed-off-by: zhaoxiao Reviewed-by: Juergen Gross Link: https://lore.kernel.org/r/20210825114111.29009-1-zhaoxiao@uniontech.com Signed-off-by: Juergen Gross --- arch/x86/xen/platform-pci-unplug.c | 16 +++++++++------- 1 file changed, 9 insertions(+), 7 deletions(-) diff --git a/arch/x86/xen/platform-pci-unplug.c b/arch/x86/xen/platform-pci-unplug.c index 96d7f7d39cb91..62ac4898df1a7 100644 --- a/arch/x86/xen/platform-pci-unplug.c +++ b/arch/x86/xen/platform-pci-unplug.c @@ -7,6 +7,8 @@ * Copyright (c) 2010, Citrix */ +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt + #include #include #include @@ -30,13 +32,13 @@ static int check_platform_magic(void) magic = inw(XEN_IOPORT_MAGIC); if (magic != XEN_IOPORT_MAGIC_VAL) { - printk(KERN_ERR "Xen Platform PCI: unrecognised magic value\n"); + pr_err("Xen Platform PCI: unrecognised magic value\n"); return XEN_PLATFORM_ERR_MAGIC; } protocol = inb(XEN_IOPORT_PROTOVER); - printk(KERN_DEBUG "Xen Platform PCI: I/O protocol version %d\n", + pr_debug("Xen Platform PCI: I/O protocol version %d\n", protocol); switch (protocol) { @@ -44,12 +46,12 @@ static int check_platform_magic(void) outw(XEN_IOPORT_LINUX_PRODNUM, XEN_IOPORT_PRODNUM); outl(XEN_IOPORT_LINUX_DRVVER, XEN_IOPORT_DRVVER); if (inw(XEN_IOPORT_MAGIC) != XEN_IOPORT_MAGIC_VAL) { - printk(KERN_ERR "Xen Platform: blacklisted by host\n"); + pr_err("Xen Platform: blacklisted by host\n"); return XEN_PLATFORM_ERR_BLACKLIST; } break; default: - printk(KERN_WARNING "Xen Platform PCI: unknown I/O protocol version\n"); + pr_warn("Xen Platform PCI: unknown I/O protocol version\n"); return XEN_PLATFORM_ERR_PROTOCOL; } @@ -155,12 +157,12 @@ void xen_unplug_emulated_devices(void) * been compiled for this kernel (modules or built-in are both OK). */ if (!xen_emul_unplug) { if (xen_must_unplug_nics()) { - printk(KERN_INFO "Netfront and the Xen platform PCI driver have " + pr_info("Netfront and the Xen platform PCI driver have " "been compiled for this kernel: unplug emulated NICs.\n"); xen_emul_unplug |= XEN_UNPLUG_ALL_NICS; } if (xen_must_unplug_disks()) { - printk(KERN_INFO "Blkfront and the Xen platform PCI driver have " + pr_info("Blkfront and the Xen platform PCI driver have " "been compiled for this kernel: unplug emulated disks.\n" "You might have to change the root device\n" "from /dev/hd[a-d] to /dev/xvd[a-d]\n" @@ -200,7 +202,7 @@ static int __init parse_xen_emul_unplug(char *arg) else if (!strncmp(p, "never", l)) xen_emul_unplug |= XEN_UNPLUG_NEVER; else - printk(KERN_WARNING "unrecognised option '%s' " + pr_warn("unrecognised option '%s' " "in parameter 'xen_emul_unplug'\n", p); } return 0; From f956c1b0d58a50dd92ec82ae4923f7cc97e2ea52 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Sergio=20Migu=C3=A9ns=20Iglesias?= Date: Mon, 30 Aug 2021 19:53:05 +0200 Subject: [PATCH 10/11] xen/pcifront: Removed unnecessary __ref annotation MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit An unnecessary "__ref" annotation was removed from the "drivers/pci/xen_pcifront.c" file. The function where the annotation was used was "pcifront_backend_changed()", which does not call any functions annotated as "__*init" nor "__*exit". This makes "__ref" unnecessary since this annotation is used to make the compiler ignore section miss-matches when they are not happening here in the first place. In addition to the aforementioned change, some code style issues were fixed in the same file. Signed-off-by: Sergio Miguéns Iglesias Reviewed-by: Juergen Gross Link: https://lore.kernel.org/r/20210830175305.13370-1-sergio@lony.xyz Signed-off-by: Juergen Gross --- drivers/pci/xen-pcifront.c | 30 +++++++++++++++++++----------- 1 file changed, 19 insertions(+), 11 deletions(-) diff --git a/drivers/pci/xen-pcifront.c b/drivers/pci/xen-pcifront.c index b7a8f3a1921f8..427041c1e4088 100644 --- a/drivers/pci/xen-pcifront.c +++ b/drivers/pci/xen-pcifront.c @@ -115,7 +115,7 @@ static int do_pci_op(struct pcifront_device *pdev, struct xen_pci_op *op) struct xen_pci_op *active_op = &pdev->sh_info->op; unsigned long irq_flags; evtchn_port_t port = pdev->evtchn; - unsigned irq = pdev->irq; + unsigned int irq = pdev->irq; s64 ns, ns_timeout; spin_lock_irqsave(&pdev->sh_info_lock, irq_flags); @@ -153,10 +153,10 @@ static int do_pci_op(struct pcifront_device *pdev, struct xen_pci_op *op) } /* - * We might lose backend service request since we - * reuse same evtchn with pci_conf backend response. So re-schedule - * aer pcifront service. - */ + * We might lose backend service request since we + * reuse same evtchn with pci_conf backend response. So re-schedule + * aer pcifront service. + */ if (test_bit(_XEN_PCIB_active, (unsigned long *)&pdev->sh_info->flags)) { dev_err(&pdev->xdev->dev, @@ -414,7 +414,8 @@ static int pcifront_scan_bus(struct pcifront_device *pdev, struct pci_dev *d; unsigned int devfn; - /* Scan the bus for functions and add. + /* + * Scan the bus for functions and add. * We omit handling of PCI bridge attachment because pciback prevents * bridges from being exported. */ @@ -492,8 +493,10 @@ static int pcifront_scan_root(struct pcifront_device *pdev, list_add(&bus_entry->list, &pdev->root_buses); - /* pci_scan_root_bus skips devices which do not have a - * devfn==0. The pcifront_scan_bus enumerates all devfn. */ + /* + * pci_scan_root_bus skips devices which do not have a + * devfn==0. The pcifront_scan_bus enumerates all devfn. + */ err = pcifront_scan_bus(pdev, domain, bus, b); /* Claim resources before going "live" with our devices */ @@ -651,8 +654,10 @@ static void pcifront_do_aer(struct work_struct *data) pci_channel_state_t state = (pci_channel_state_t)pdev->sh_info->aer_op.err; - /*If a pci_conf op is in progress, - we have to wait until it is done before service aer op*/ + /* + * If a pci_conf op is in progress, we have to wait until it is done + * before service aer op + */ dev_dbg(&pdev->xdev->dev, "pcifront service aer bus %x devfn %x\n", pdev->sh_info->aer_op.bus, pdev->sh_info->aer_op.devfn); @@ -676,6 +681,7 @@ static void pcifront_do_aer(struct work_struct *data) static irqreturn_t pcifront_handler_aer(int irq, void *dev) { struct pcifront_device *pdev = dev; + schedule_pcifront_aer_op(pdev); return IRQ_HANDLED; } @@ -1027,6 +1033,7 @@ static int pcifront_detach_devices(struct pcifront_device *pdev) /* Find devices being detached and remove them. */ for (i = 0; i < num_devs; i++) { int l, state; + l = snprintf(str, sizeof(str), "state-%d", i); if (unlikely(l >= (sizeof(str) - 1))) { err = -ENOMEM; @@ -1078,7 +1085,7 @@ static int pcifront_detach_devices(struct pcifront_device *pdev) return err; } -static void __ref pcifront_backend_changed(struct xenbus_device *xdev, +static void pcifront_backend_changed(struct xenbus_device *xdev, enum xenbus_state be_state) { struct pcifront_device *pdev = dev_get_drvdata(&xdev->dev); @@ -1137,6 +1144,7 @@ static int pcifront_xenbus_probe(struct xenbus_device *xdev, static int pcifront_xenbus_remove(struct xenbus_device *xdev) { struct pcifront_device *pdev = dev_get_drvdata(&xdev->dev); + if (pdev) free_pdev(pdev); From 58e636039b512697554b579c2bb23774061877f5 Mon Sep 17 00:00:00 2001 From: Juergen Gross Date: Wed, 25 Aug 2021 13:31:58 +0200 Subject: [PATCH 11/11] xen: remove stray preempt_disable() from PV AP startup code In cpu_bringup() there is a call of preempt_disable() without a paired preempt_enable(). This is not needed as interrupts are off initially. Additionally this will result in early boot messages like: BUG: scheduling while atomic: swapper/1/0/0x00000002 Signed-off-by: Juergen Gross Link: https://lore.kernel.org/r/20210825113158.11716-1-jgross@suse.com Signed-off-by: Juergen Gross --- arch/x86/xen/smp_pv.c | 1 - 1 file changed, 1 deletion(-) diff --git a/arch/x86/xen/smp_pv.c b/arch/x86/xen/smp_pv.c index c2ac319f11a4b..96afadf9878ef 100644 --- a/arch/x86/xen/smp_pv.c +++ b/arch/x86/xen/smp_pv.c @@ -64,7 +64,6 @@ static void cpu_bringup(void) cr4_init(); cpu_init(); touch_softlockup_watchdog(); - preempt_disable(); /* PVH runs in ring 0 and allows us to do native syscalls. Yay! */ if (!xen_feature(XENFEAT_supervisor_mode_kernel)) {