[Xen-devel] [bug report] xen/pvcalls: implement release command
Hello Stefano Stabellini,
The patch 235a71c53903: "xen/pvcalls: implement release command" from
Oct 30, 2017, leads to the following static checker warning:
drivers/xen/pvcalls-front.c:1051 pvcalls_front_release()
error: double lock 'mutex:&map->active.in_mutex'
drivers/xen/pvcalls-front.c
1037 if (map->active_socket) {
1038 /*
1039 * Set in_error and wake up inflight_conn_req to force
1040 * recvmsg waiters to exit.
1041 */
1042 map->active.ring->in_error = -EBADF;
1043 wake_up_interruptible(&map->active.inflight_conn_req);
1044
1045 /*
1046 * Wait until there are no more waiters on the mutexes.
1047 * We know that no new waiters can be added because
sk_send_head
1048 * is set to NULL -- we only need to wait for the
existing
1049 * waiters to return.
1050 */
1051 while (!mutex_trylock(&map->active.in_mutex) ||
1052 !mutex_trylock(&map->active.out_mutex))
1053 cpu_relax();
mutex_trylock() returns 1 if you take the lock and 0 if not. So I think
the static checker is right that this code has an issue.
Assume you take in_mutex on the first try, but you can't take out_mutex.
That means you the next times you call mutex_trylock() in_mutex is going
to fail. So it's an endless loop.
But it could also be that I haven't slept well recently and my eyes are
cross eyed.
1054
1055 pvcalls_front_free_map(bedata, map);
1056 } else {
regards,
dan carpenter
___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
[Xen-devel] [PATCH] xen/pvcalls: NULL dereference in error handling
We accidentally dereference "map" when it's NULL.
Fixes: b535e2b9b78a ("xen/pvcalls: implement connect command")
Signed-off-by: Dan Carpenter
diff --git a/drivers/xen/pvcalls-back.c b/drivers/xen/pvcalls-back.c
index d6c4c4aecb41..01b690e1e555 100644
--- a/drivers/xen/pvcalls-back.c
+++ b/drivers/xen/pvcalls-back.c
@@ -424,7 +424,7 @@ static int pvcalls_back_connect(struct xenbus_device *dev,
sock);
if (!map) {
ret = -EFAULT;
- sock_release(map->sock);
+ sock_release(sock);
}
out:
___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH] xen-scsifront: Add a missing call to kfree
Oops. Sorry for the noise. regards, dan carpenter ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH] xen-scsifront: Add a missing call to kfree
On Mon, Nov 21, 2016 at 07:01:36AM +0100, Juergen Gross wrote: > On 19/11/16 19:22, Quentin Lambert wrote: > > Most error branches following the call to kmalloc contain > > a call to kfree. This patch add these calls where they are > > missing. > > > > This issue was found with Hector. > > > > Signed-off-by: Quentin Lambert > > Nice catch. I think this will need some more work, I'll do a > follow-on patch. > The error handling is really weird. Could you send your follow on to this thread? regards, dan carpenter ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH] xen-scsifront: Add a missing call to kfree
On Mon, Nov 21, 2016 at 07:01:36AM +0100, Juergen Gross wrote: > On 19/11/16 19:22, Quentin Lambert wrote: > > Most error branches following the call to kmalloc contain > > a call to kfree. This patch add these calls where they are > > missing. > > > > This issue was found with Hector. > > > > Signed-off-by: Quentin Lambert > > Nice catch. I think this will need some more work, I'll do a > follow-on patch. Yeah. It's weird how we free it on the success path and all the failure paths except one. But it looks so deliberate. What's going on with that? Could you send your follow on patch as a reply to the thread? regards, dan carpenter ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] xen-netback: add control protocol implementation
Hello Paul Durrant,
The patch 40d8abdee806: "xen-netback: add control protocol
implementation" from May 13, 2016, leads to the following static
checker warning:
drivers/net/xen-netback/hash.c:362 xenvif_set_hash_mapping()
warn: should this be 'len == -1'
drivers/net/xen-netback/hash.c
341 u32 xenvif_set_hash_mapping(struct xenvif *vif, u32 gref, u32 len,
342 u32 off)
343 {
344 u32 *mapping = &vif->hash.mapping[off];
345 struct gnttab_copy copy_op = {
346 .source.u.ref = gref,
347 .source.domid = vif->domid,
348 .dest.u.gmfn = virt_to_gfn(mapping),
349 .dest.domid = DOMID_SELF,
350 .dest.offset = xen_offset_in_page(mapping),
351 .len = len * sizeof(u32),
352 .flags = GNTCOPY_source_gref
353 };
354
355 if ((off + len > vif->hash.size) || copy_op.len > XEN_PAGE_SIZE)
356 return XEN_NETIF_CTRL_STATUS_INVALID_PARAMETER;
357
358 while (len-- != 0)
359 if (mapping[off++] >= vif->num_queues)
360 return XEN_NETIF_CTRL_STATUS_INVALID_PARAMETER;
361
362 if (len != 0) {
We know that "len" is always UINT_MAX here meaning this is always true.
What are trying to test?
363 gnttab_batch_copy(©_op, 1);
364
365 if (copy_op.status != GNTST_okay)
366 return XEN_NETIF_CTRL_STATUS_INVALID_PARAMETER;
367 }
368
369 return XEN_NETIF_CTRL_STATUS_SUCCESS;
370 }
regards,
dan carpenter
___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
Re: [Xen-devel] xen/gntdev: add ioctl for grant copy
Hello David Vrabel,
The patch a4cdb556cae0: "xen/gntdev: add ioctl for grant copy" from
Dec 2, 2014, leads to the following static checker warning:
drivers/xen/gntdev.c:775 gntdev_get_page()
warn: mask and shift to zero
drivers/xen/gntdev.c
761 static int gntdev_get_page(struct gntdev_copy_batch *batch, void __user
*virt,
762 bool writeable, unsigned long *gfn)
763 {
764 unsigned long addr = (unsigned long)virt;
765 struct page *page;
766 unsigned long xen_pfn;
767 int ret;
768
769 ret = get_user_pages_fast(addr, 1, writeable, &page);
770 if (ret < 0)
771 return ret;
772
773 batch->pages[batch->nr_pages++] = page;
774
775 xen_pfn = page_to_xen_pfn(page) + XEN_PFN_DOWN(addr &
~PAGE_MASK);
^^^
This is zero most of the time. Did you intend the bitwise negate?
776 *gfn = pfn_to_gfn(xen_pfn);
777
778 return 0;
779 }
regards,
dan carpenter
___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH 0/5] xen-netback: Fine-tuning for three function implementations
The original code is fine. regards, dan carpenter ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] xen/mmu: Copy and revector the P2M tree.
On Mon, Jul 20, 2015 at 01:03:52PM +0300, Dan Carpenter wrote:
> Hello Konrad Rzeszutek Wilk,
>
> The patch 7f9140626c75: "xen/mmu: Copy and revector the P2M tree."
> from Jul 26, 2012, leads to the following static checker warning:
>
> arch/x86/xen/mmu.c:1105 xen_cleanhighmap()
> warn: potential pointer math issue ('level2_kernel_pgt' is pointer to
> unsigned long)
>
> arch/x86/xen/mmu.c
> 1096 #ifdef CONFIG_X86_64
> 1097 static void __init xen_cleanhighmap(unsigned long vaddr,
> 1098 unsigned long vaddr_end)
> 1099 {
> 1100 unsigned long kernel_end = roundup((unsigned long)_brk_end,
> PMD_SIZE) - 1;
> 1101 pmd_t *pmd = level2_kernel_pgt + pmd_index(vaddr);
> 1102
> 1103 /* NOTE: The loop is more greedy than the cleanup_highmap
> variant.
> 1104 * We include the PMD passed in on _both_ boundaries. */
> 1105 for (; vaddr <= vaddr_end && (pmd < (level2_kernel_pgt +
> PAGE_SIZE));
>
> ^
> This pointer math is weird because we typically think of PAGE_SIZE as
> a number of bytes but since level2_kernel_pgt is a pointer to unsigned
> long, it looks like this loop can go through more iterations than
> intended.
>
Yeah. This condition doesn't make sense at all. level2_kernel_pgt is
an array that has 512 elements but PAGE_SIZE is 4096 so we would be well
past the end of the array.
I suspect that this works because it is only called when DEBUG in
defined or because of the "vaddr <= vaddr_end" condition.
> 1106 pmd++, vaddr += PMD_SIZE) {
> 1107 if (pmd_none(*pmd))
> 1108 continue;
> 1109 if (vaddr < (unsigned long) _text || vaddr >
> kernel_end)
> 1110 set_pmd(pmd, __pmd(0));
> }
> 1112 /* In case we did something silly, we should crash in this
> function
> 1113 * instead of somewhere later and be confusing. */
> 1114 xen_mc_flush();
> 1115 }
>
> regards,
> dan carpenter
___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
Re: [Xen-devel] xen/mmu: Copy and revector the P2M tree.
Hello Konrad Rzeszutek Wilk,
The patch 7f9140626c75: "xen/mmu: Copy and revector the P2M tree."
from Jul 26, 2012, leads to the following static checker warning:
arch/x86/xen/mmu.c:1105 xen_cleanhighmap()
warn: potential pointer math issue ('level2_kernel_pgt' is pointer to
unsigned long)
arch/x86/xen/mmu.c
1096 #ifdef CONFIG_X86_64
1097 static void __init xen_cleanhighmap(unsigned long vaddr,
1098 unsigned long vaddr_end)
1099 {
1100 unsigned long kernel_end = roundup((unsigned long)_brk_end,
PMD_SIZE) - 1;
1101 pmd_t *pmd = level2_kernel_pgt + pmd_index(vaddr);
1102
1103 /* NOTE: The loop is more greedy than the cleanup_highmap
variant.
1104 * We include the PMD passed in on _both_ boundaries. */
1105 for (; vaddr <= vaddr_end && (pmd < (level2_kernel_pgt +
PAGE_SIZE));
^
This pointer math is weird because we typically think of PAGE_SIZE as
a number of bytes but since level2_kernel_pgt is a pointer to unsigned
long, it looks like this loop can go through more iterations than
intended.
1106 pmd++, vaddr += PMD_SIZE) {
1107 if (pmd_none(*pmd))
1108 continue;
1109 if (vaddr < (unsigned long) _text || vaddr > kernel_end)
1110 set_pmd(pmd, __pmd(0));
}
1112 /* In case we did something silly, we should crash in this
function
1113 * instead of somewhere later and be confusing. */
1114 xen_mc_flush();
1115 }
regards,
dan carpenter
___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
[Xen-devel] [patch] net/xen-netback: off by one in BUG_ON() condition
The > should be >=. I also added spaces around the '-' operations so
the code is a little more consistent and matches the condition better.
Fixes: f53c3fe8dad7 ('xen-netback: Introduce TX grant mapping')
Signed-off-by: Dan Carpenter
diff --git a/drivers/net/xen-netback/netback.c
b/drivers/net/xen-netback/netback.c
index 880d0d6..7d50711 100644
--- a/drivers/net/xen-netback/netback.c
+++ b/drivers/net/xen-netback/netback.c
@@ -1566,13 +1566,13 @@ static inline void xenvif_tx_dealloc_action(struct
xenvif_queue *queue)
smp_rmb();
while (dc != dp) {
- BUG_ON(gop - queue->tx_unmap_ops > MAX_PENDING_REQS);
+ BUG_ON(gop - queue->tx_unmap_ops >= MAX_PENDING_REQS);
pending_idx =
queue->dealloc_ring[pending_index(dc++)];
- pending_idx_release[gop-queue->tx_unmap_ops] =
+ pending_idx_release[gop - queue->tx_unmap_ops] =
pending_idx;
- queue->pages_to_unmap[gop-queue->tx_unmap_ops] =
+ queue->pages_to_unmap[gop - queue->tx_unmap_ops] =
queue->mmap_pages[pending_idx];
gnttab_set_unmap_op(gop,
idx_to_kaddr(queue, pending_idx),
___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
[Xen-devel] [patch] xen/mce: fix up xen_late_init_mcelog() error handling
Static checkers complain about the missing call to misc_deregister() if
bind_virq_for_mce() fails.
Also I reversed the tests so that we do error handling instead of
success handling. That way we just have a series of function calls
instead of the more complicated nested if statements in the original
code. Let's preserve the error codes as well.
Signed-off-by: Dan Carpenter
diff --git a/drivers/xen/mcelog.c b/drivers/xen/mcelog.c
index 6ab6a79..a493c7315 100644
--- a/drivers/xen/mcelog.c
+++ b/drivers/xen/mcelog.c
@@ -393,14 +393,25 @@ static int bind_virq_for_mce(void)
static int __init xen_late_init_mcelog(void)
{
+ int ret;
+
/* Only DOM0 is responsible for MCE logging */
- if (xen_initial_domain()) {
- /* register character device /dev/mcelog for xen mcelog */
- if (misc_register(&xen_mce_chrdev_device))
- return -ENODEV;
- return bind_virq_for_mce();
- }
+ if (!xen_initial_domain())
+ return -ENODEV;
+
+ /* register character device /dev/mcelog for xen mcelog */
+ ret = misc_register(&xen_mce_chrdev_device);
+ if (ret)
+ return ret;
+
+ ret = bind_virq_for_mce();
+ if (ret)
+ goto deregister;
- return -ENODEV;
+ return 0;
+
+deregister:
+ misc_deregister(&xen_mce_chrdev_device);
+ return ret;
}
device_initcall(xen_late_init_mcelog);
___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
