| Message ID | 20240618172527.3450098-1-nabihestefan@google.com |
|---|---|
| State | New |
| Headers | show |
| Series | hw/net: Fix Coverity Issue for npcm-gmac | expand |
Nabih Estefan <nabihestefan@google.com> writes: > There is an extra `buf=` set that is not used by npcm-gmac. Remove it > for coverity to be happy. Have you go the coverity reference to include in the commit message? > > Signed-off-by: Nabih Estefan <nabihestefan@google.com> > --- > hw/net/npcm_gmac.c | 1 - > 1 file changed, 1 deletion(-) > > diff --git a/hw/net/npcm_gmac.c b/hw/net/npcm_gmac.c > index 1b71e2526e..b397fd5064 100644 > --- a/hw/net/npcm_gmac.c > +++ b/hw/net/npcm_gmac.c > @@ -614,7 +614,6 @@ static void gmac_try_send_next_packet(NPCMGMACState *gmac) > net_checksum_calculate(tx_send_buffer, length, csum); > qemu_send_packet(qemu_get_queue(gmac->nic), tx_send_buffer, length); > trace_npcm_gmac_packet_sent(DEVICE(gmac)->canonical_path, length); > - buf = tx_send_buffer; So coverity is saying that buf starts at tx_send_buffer and none of the other legs that can mess with it are possible for the tx_desc.tdes1 & TX_DESC_TDES1_LAST_SEG_MASK leg? Or that buf should always start at tx_send_buffer and only ever advance if we grow the size of the tx_send_buffer? > length = 0; > }
On Wed, 19 Jun 2024 at 10:13, Alex Bennée <alex.bennee@linaro.org> wrote: > > Nabih Estefan <nabihestefan@google.com> writes: > > > There is an extra `buf=` set that is not used by npcm-gmac. Remove it > > for coverity to be happy. By the way, Nabih, it looks like the mailing list received five copies of this patch email. You might want to look at what happened on your end that resulted in all the duplicates. > Have you go the coverity reference to include in the commit message? This is CID 1534027. > > Signed-off-by: Nabih Estefan <nabihestefan@google.com> > > --- > > hw/net/npcm_gmac.c | 1 - > > 1 file changed, 1 deletion(-) > > > > diff --git a/hw/net/npcm_gmac.c b/hw/net/npcm_gmac.c > > index 1b71e2526e..b397fd5064 100644 > > --- a/hw/net/npcm_gmac.c > > +++ b/hw/net/npcm_gmac.c > > @@ -614,7 +614,6 @@ static void gmac_try_send_next_packet(NPCMGMACState *gmac) > > net_checksum_calculate(tx_send_buffer, length, csum); > > qemu_send_packet(qemu_get_queue(gmac->nic), tx_send_buffer, length); > > trace_npcm_gmac_packet_sent(DEVICE(gmac)->canonical_path, length); > > - buf = tx_send_buffer; > > So coverity is saying that buf starts at tx_send_buffer and none of the > other legs that can mess with it are possible for the tx_desc.tdes1 & > TX_DESC_TDES1_LAST_SEG_MASK leg? Coverity is saying that in the loop body, we unconditionally (in "step 4") set "buf = &tx_send_buffer[prev_buf_size]" before we ever try to use "buf". This assignment "buf = tx_send_buffer" happens later in the loop body, but there is no further reference to buf either inside the loop body or after the loop ends. So we will never look at the value we assign to "buf" here (either we finish the loop and the function, or else we loop back around again and overwrite this value), and this assignment is dead code. What I'm wondering is whether this code for "last segment, send the packet" should be setting "prev_buf_size = 0" instead of "buf = tx_send_buffer" (meaning, I think "we've sent this packet, there is nothing currently in the tx_send_buffer, the next descriptor can start filling tx_send_buffer from byte 0".) Otherwise I think we will continue to accumulate data from the following descriptor into tx_send_buffer after the data from this packet, but when we send that second packet we will do it from the start of tx_send_buffer and so we will send the wrong data. thanks -- PMM
diff --git a/hw/net/npcm_gmac.c b/hw/net/npcm_gmac.c index 1b71e2526e..b397fd5064 100644 --- a/hw/net/npcm_gmac.c +++ b/hw/net/npcm_gmac.c @@ -614,7 +614,6 @@ static void gmac_try_send_next_packet(NPCMGMACState *gmac) net_checksum_calculate(tx_send_buffer, length, csum); qemu_send_packet(qemu_get_queue(gmac->nic), tx_send_buffer, length); trace_npcm_gmac_packet_sent(DEVICE(gmac)->canonical_path, length); - buf = tx_send_buffer; length = 0; }
There is an extra `buf=` set that is not used by npcm-gmac. Remove it for coverity to be happy. Signed-off-by: Nabih Estefan <nabihestefan@google.com> --- hw/net/npcm_gmac.c | 1 - 1 file changed, 1 deletion(-)