[PATCH] accel/tcg: assert insn_idx will always be valid before plugin_inject_cb

Alex Bennée posted 1 patch 2 years, 6 months ago
Test checkpatch passed
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/20210903145938.1321571-1-alex.bennee@linaro.org
accel/tcg/plugin-gen.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
[PATCH] accel/tcg: assert insn_idx will always be valid before plugin_inject_cb
Posted by Alex Bennée 2 years, 6 months ago
Coverity doesn't know enough about how we have arranged our plugin TCG
ops to know we will always have incremented insn_idx before injecting
the callback. Let us assert it for the benefit of Coverity and protect
ourselves from accidentally breaking the assumption and triggering
harder to grok errors deeper in the code if we attempt a negative
indexed array lookup.

Fixes: Coverity 1459509
Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
---
 accel/tcg/plugin-gen.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/accel/tcg/plugin-gen.c b/accel/tcg/plugin-gen.c
index 88e25c6df9..b38aa1bb36 100644
--- a/accel/tcg/plugin-gen.c
+++ b/accel/tcg/plugin-gen.c
@@ -820,10 +820,9 @@ static void pr_ops(void)
 static void plugin_gen_inject(const struct qemu_plugin_tb *plugin_tb)
 {
     TCGOp *op;
-    int insn_idx;
+    int insn_idx = -1;
 
     pr_ops();
-    insn_idx = -1;
     QSIMPLEQ_FOREACH(op, &tcg_ctx->plugin_ops, plugin_link) {
         enum plugin_gen_from from = op->args[0];
         enum plugin_gen_cb type = op->args[1];
@@ -834,6 +833,7 @@ static void plugin_gen_inject(const struct qemu_plugin_tb *plugin_tb)
             type == PLUGIN_GEN_ENABLE_MEM_HELPER) {
             insn_idx++;
         }
+        g_assert(from == PLUGIN_GEN_FROM_TB || insn_idx >= 0);
         plugin_inject_cb(plugin_tb, op, insn_idx);
     }
     pr_ops();
-- 
2.30.2


Re: [PATCH] accel/tcg: assert insn_idx will always be valid before plugin_inject_cb
Posted by Richard Henderson 2 years, 6 months ago
On 9/3/21 7:59 AM, Alex Bennée wrote:
> Coverity doesn't know enough about how we have arranged our plugin TCG
> ops to know we will always have incremented insn_idx before injecting
> the callback. Let us assert it for the benefit of Coverity and protect
> ourselves from accidentally breaking the assumption and triggering
> harder to grok errors deeper in the code if we attempt a negative
> indexed array lookup.
> 
> Fixes: Coverity 1459509
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> ---
>   accel/tcg/plugin-gen.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/accel/tcg/plugin-gen.c b/accel/tcg/plugin-gen.c
> index 88e25c6df9..b38aa1bb36 100644
> --- a/accel/tcg/plugin-gen.c
> +++ b/accel/tcg/plugin-gen.c
> @@ -820,10 +820,9 @@ static void pr_ops(void)
>   static void plugin_gen_inject(const struct qemu_plugin_tb *plugin_tb)
>   {
>       TCGOp *op;
> -    int insn_idx;
> +    int insn_idx = -1;
>   
>       pr_ops();
> -    insn_idx = -1;
>       QSIMPLEQ_FOREACH(op, &tcg_ctx->plugin_ops, plugin_link) {
>           enum plugin_gen_from from = op->args[0];
>           enum plugin_gen_cb type = op->args[1];
> @@ -834,6 +833,7 @@ static void plugin_gen_inject(const struct qemu_plugin_tb *plugin_tb)
>               type == PLUGIN_GEN_ENABLE_MEM_HELPER) {
>               insn_idx++;
>           }
> +        g_assert(from == PLUGIN_GEN_FROM_TB || insn_idx >= 0);
>           plugin_inject_cb(plugin_tb, op, insn_idx);

Hmm.  This is the single caller of plugin_inject_cb.

I think we could simplify all of this by inlining it, so that we can put these blocks into 
their proper place within the switch.

Also, existing strageness in insn_idx being incremented for non-insns?  Should it be named 
something else?  I haven't looked at how it's really used in the end.


r~

Re: [PATCH] accel/tcg: assert insn_idx will always be valid before plugin_inject_cb
Posted by Alex Bennée 2 years, 6 months ago
Richard Henderson <richard.henderson@linaro.org> writes:

> On 9/3/21 7:59 AM, Alex Bennée wrote:
>> Coverity doesn't know enough about how we have arranged our plugin TCG
>> ops to know we will always have incremented insn_idx before injecting
>> the callback. Let us assert it for the benefit of Coverity and protect
>> ourselves from accidentally breaking the assumption and triggering
>> harder to grok errors deeper in the code if we attempt a negative
>> indexed array lookup.
>> Fixes: Coverity 1459509
>> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
>> ---
>>   accel/tcg/plugin-gen.c | 4 ++--
>>   1 file changed, 2 insertions(+), 2 deletions(-)
>> diff --git a/accel/tcg/plugin-gen.c b/accel/tcg/plugin-gen.c
>> index 88e25c6df9..b38aa1bb36 100644
>> --- a/accel/tcg/plugin-gen.c
>> +++ b/accel/tcg/plugin-gen.c
>> @@ -820,10 +820,9 @@ static void pr_ops(void)
>>   static void plugin_gen_inject(const struct qemu_plugin_tb *plugin_tb)
>>   {
>>       TCGOp *op;
>> -    int insn_idx;
>> +    int insn_idx = -1;
>>         pr_ops();
>> -    insn_idx = -1;
>>       QSIMPLEQ_FOREACH(op, &tcg_ctx->plugin_ops, plugin_link) {
>>           enum plugin_gen_from from = op->args[0];
>>           enum plugin_gen_cb type = op->args[1];
>> @@ -834,6 +833,7 @@ static void plugin_gen_inject(const struct qemu_plugin_tb *plugin_tb)
>>               type == PLUGIN_GEN_ENABLE_MEM_HELPER) {
>>               insn_idx++;
>>           }
>> +        g_assert(from == PLUGIN_GEN_FROM_TB || insn_idx >= 0);
>>           plugin_inject_cb(plugin_tb, op, insn_idx);
>
> Hmm.  This is the single caller of plugin_inject_cb.
>
> I think we could simplify all of this by inlining it, so that we can
> put these blocks into their proper place within the switch.

I guess. This was the simplest form for calming coverity but I can
experiment with more refactoring.

> Also, existing strageness in insn_idx being incremented for non-insns?

It shouldn't be - it's just using the presence of the memory
instrumentation as a proxy for the start of a instruction and dealing
with the slightly different start of block boundary. 

> Should it be named something else?  I haven't looked at how it's
> really used in the end.

We need the insn idx to find the registered callbacks for a given
instruction later. We could maybe embed a metadata TCGOp that could
track this for us but that might make TCG a bit more confusing as it
doesn't really need that information?

>
>
> r~


-- 
Alex Bennée

Re: [PATCH] accel/tcg: assert insn_idx will always be valid before plugin_inject_cb
Posted by Richard Henderson 2 years, 6 months ago
On 9/13/21 3:06 AM, Alex Bennée wrote:
>> Also, existing strageness in insn_idx being incremented for non-insns?
> 
> It shouldn't be - it's just using the presence of the memory
> instrumentation as a proxy for the start of a instruction and dealing
> with the slightly different start of block boundary.
> 
>> Should it be named something else?  I haven't looked at how it's
>> really used in the end.
> 
> We need the insn idx to find the registered callbacks for a given
> instruction later. We could maybe embed a metadata TCGOp that could
> track this for us but that might make TCG a bit more confusing as it
> doesn't really need that information?

We have a metadata op for marking instruction boundaries already: INDEX_op_insn_start.


r~


Re: [PATCH] accel/tcg: assert insn_idx will always be valid before plugin_inject_cb
Posted by Alex Bennée 2 years, 6 months ago
Richard Henderson <richard.henderson@linaro.org> writes:

> On 9/13/21 3:06 AM, Alex Bennée wrote:
>>> Also, existing strageness in insn_idx being incremented for non-insns?
>> It shouldn't be - it's just using the presence of the memory
>> instrumentation as a proxy for the start of a instruction and dealing
>> with the slightly different start of block boundary.
>> 
>>> Should it be named something else?  I haven't looked at how it's
>>> really used in the end.
>> We need the insn idx to find the registered callbacks for a given
>> instruction later. We could maybe embed a metadata TCGOp that could
>> track this for us but that might make TCG a bit more confusing as it
>> doesn't really need that information?
>
> We have a metadata op for marking instruction boundaries already:
> INDEX_op_insn_start.

Hmm so we have a separate list for speedy access:

    /* list to quickly access the injected ops */
    QSIMPLEQ_HEAD(, TCGOp) plugin_ops;

I wonder if we should drop that and just scan QTAILQ_HEAD(, TCGOp) ops
so we can be properly aligned with the current instruction.
Alternatively we could just emit INDEX_op_insn_start to the plugin list
as well?

>
>
> r~


-- 
Alex Bennée

Re: [PATCH] accel/tcg: assert insn_idx will always be valid before plugin_inject_cb
Posted by Richard Henderson 2 years, 6 months ago
On 9/13/21 7:06 AM, Alex Bennée wrote:
> Hmm so we have a separate list for speedy access:
> 
>      /* list to quickly access the injected ops */
>      QSIMPLEQ_HEAD(, TCGOp) plugin_ops;
> 
> I wonder if we should drop that and just scan QTAILQ_HEAD(, TCGOp) ops
> so we can be properly aligned with the current instruction.
> Alternatively we could just emit INDEX_op_insn_start to the plugin list
> as well?

I suspect that just scanning the whole list would be easiest.  Then you don't need to 
maintain your own separate list.


r~