| Message ID | 20201005125527.429187-1-pbonzini@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | ide: clean up ahci_populate_sglist | expand |
On 10/5/20 2:55 PM, Paolo Bonzini wrote: > Alex reported an uninitialized variable warning in ahci_populate_sglist. > Even though the warning is bogus and happens only because of -Og, the > code in the function leaves something to be desired; the condition that > triggers the warning is easily shown to be entirely redundant. > > In particular, the loop's "if" condition can be rewritten from > "offset < sum + tbl_entry_size" to "offset - sum < tbl_entry_size"; > this is safe since the LHS cannot underflow. Because off_pos is > exactly "offset - sum" it is clear that it can never be less than > zero or greater than tbl_entry_size. We can therefore keep the off_idx > check only and, for documentation purposes, reduce off_pos to an unsigned > 32-bit integer. > > The tracepoint also is not particularly useful at this point, since > we know that (if it ever triggers) off_idx will be -1 and off_pos > uninitialized. Instead, include the requested offset and the total PRDT > length, which will be smaller than the offset. > Reported-by: Alex Bennée <alex.bennee@linaro.org> so we know which 'Alex', and: Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com> > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> > --- > hw/ide/ahci.c | 12 +++++------- > hw/ide/trace-events | 2 +- > 2 files changed, 6 insertions(+), 8 deletions(-) > > diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c > index 680304a24c..997b67a6fc 100644 > --- a/hw/ide/ahci.c > +++ b/hw/ide/ahci.c > @@ -924,8 +924,7 @@ static int ahci_populate_sglist(AHCIDevice *ad, QEMUSGList *sglist, > int r = 0; > uint64_t sum = 0; > int off_idx = -1; > - int64_t off_pos = -1; > - int tbl_entry_size; > + uint32_t off_pos = 0; > IDEBus *bus = &ad->port; > BusState *qbus = BUS(bus); > > @@ -952,19 +951,18 @@ static int ahci_populate_sglist(AHCIDevice *ad, QEMUSGList *sglist, > /* Get entries in the PRDT, init a qemu sglist accordingly */ > if (prdtl > 0) { > AHCI_SG *tbl = (AHCI_SG *)prdt; > - sum = 0; > for (i = 0; i < prdtl; i++) { > - tbl_entry_size = prdt_tbl_entry_size(&tbl[i]); > - if (offset < (sum + tbl_entry_size)) { > + uint32_t tbl_entry_size = prdt_tbl_entry_size(&tbl[i]); > + if (offset - sum < tbl_entry_size) { > off_idx = i; > off_pos = offset - sum; > break; > } > sum += tbl_entry_size; > } > - if ((off_idx == -1) || (off_pos < 0) || (off_pos > tbl_entry_size)) { > + if (off_idx == -1) { > trace_ahci_populate_sglist_bad_offset(ad->hba, ad->port_no, > - off_idx, off_pos); > + sum, offset); > r = -1; > goto out; > } > diff --git a/hw/ide/trace-events b/hw/ide/trace-events > index 6e357685f9..81706efe80 100644 > --- a/hw/ide/trace-events > +++ b/hw/ide/trace-events > @@ -88,7 +88,7 @@ ahci_populate_sglist(void *s, int port) "ahci(%p)[%d]" > ahci_populate_sglist_no_prdtl(void *s, int port, uint16_t opts) "ahci(%p)[%d]: no sg list given by guest: 0x%04x" > ahci_populate_sglist_no_map(void *s, int port) "ahci(%p)[%d]: DMA mapping failed" > ahci_populate_sglist_short_map(void *s, int port) "ahci(%p)[%d]: mapped less than expected" > -ahci_populate_sglist_bad_offset(void *s, int port, int off_idx, int64_t off_pos) "ahci(%p)[%d]: Incorrect offset! off_idx: %d, off_pos: %"PRId64 > +ahci_populate_sglist_bad_offset(void *s, int port, uint64_t sum, uint64_t offset) "ahci(%p)[%d]: Incorrect offset! total PRDT length %"PRIu64", offset: %"PRIu64 > ncq_finish(void *s, int port, uint8_t tag) "ahci(%p)[%d][tag:%d]: NCQ transfer finished" > execute_ncq_command_read(void *s, int port, uint8_t tag, int count, int64_t lba) "ahci(%p)[%d][tag:%d]: NCQ reading %d sectors from LBA %"PRId64 > execute_ncq_command_unsup(void *s, int port, uint8_t tag, uint8_t cmd) "ahci(%p)[%d][tag:%d]: error: unsupported NCQ command (0x%02x) received" >
diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c index 680304a24c..997b67a6fc 100644 --- a/hw/ide/ahci.c +++ b/hw/ide/ahci.c @@ -924,8 +924,7 @@ static int ahci_populate_sglist(AHCIDevice *ad, QEMUSGList *sglist, int r = 0; uint64_t sum = 0; int off_idx = -1; - int64_t off_pos = -1; - int tbl_entry_size; + uint32_t off_pos = 0; IDEBus *bus = &ad->port; BusState *qbus = BUS(bus); @@ -952,19 +951,18 @@ static int ahci_populate_sglist(AHCIDevice *ad, QEMUSGList *sglist, /* Get entries in the PRDT, init a qemu sglist accordingly */ if (prdtl > 0) { AHCI_SG *tbl = (AHCI_SG *)prdt; - sum = 0; for (i = 0; i < prdtl; i++) { - tbl_entry_size = prdt_tbl_entry_size(&tbl[i]); - if (offset < (sum + tbl_entry_size)) { + uint32_t tbl_entry_size = prdt_tbl_entry_size(&tbl[i]); + if (offset - sum < tbl_entry_size) { off_idx = i; off_pos = offset - sum; break; } sum += tbl_entry_size; } - if ((off_idx == -1) || (off_pos < 0) || (off_pos > tbl_entry_size)) { + if (off_idx == -1) { trace_ahci_populate_sglist_bad_offset(ad->hba, ad->port_no, - off_idx, off_pos); + sum, offset); r = -1; goto out; } diff --git a/hw/ide/trace-events b/hw/ide/trace-events index 6e357685f9..81706efe80 100644 --- a/hw/ide/trace-events +++ b/hw/ide/trace-events @@ -88,7 +88,7 @@ ahci_populate_sglist(void *s, int port) "ahci(%p)[%d]" ahci_populate_sglist_no_prdtl(void *s, int port, uint16_t opts) "ahci(%p)[%d]: no sg list given by guest: 0x%04x" ahci_populate_sglist_no_map(void *s, int port) "ahci(%p)[%d]: DMA mapping failed" ahci_populate_sglist_short_map(void *s, int port) "ahci(%p)[%d]: mapped less than expected" -ahci_populate_sglist_bad_offset(void *s, int port, int off_idx, int64_t off_pos) "ahci(%p)[%d]: Incorrect offset! off_idx: %d, off_pos: %"PRId64 +ahci_populate_sglist_bad_offset(void *s, int port, uint64_t sum, uint64_t offset) "ahci(%p)[%d]: Incorrect offset! total PRDT length %"PRIu64", offset: %"PRIu64 ncq_finish(void *s, int port, uint8_t tag) "ahci(%p)[%d][tag:%d]: NCQ transfer finished" execute_ncq_command_read(void *s, int port, uint8_t tag, int count, int64_t lba) "ahci(%p)[%d][tag:%d]: NCQ reading %d sectors from LBA %"PRId64 execute_ncq_command_unsup(void *s, int port, uint8_t tag, uint8_t cmd) "ahci(%p)[%d][tag:%d]: error: unsupported NCQ command (0x%02x) received"
Alex reported an uninitialized variable warning in ahci_populate_sglist. Even though the warning is bogus and happens only because of -Og, the code in the function leaves something to be desired; the condition that triggers the warning is easily shown to be entirely redundant. In particular, the loop's "if" condition can be rewritten from "offset < sum + tbl_entry_size" to "offset - sum < tbl_entry_size"; this is safe since the LHS cannot underflow. Because off_pos is exactly "offset - sum" it is clear that it can never be less than zero or greater than tbl_entry_size. We can therefore keep the off_idx check only and, for documentation purposes, reduce off_pos to an unsigned 32-bit integer. The tracepoint also is not particularly useful at this point, since we know that (if it ever triggers) off_idx will be -1 and off_pos uninitialized. Instead, include the requested offset and the total PRDT length, which will be smaller than the offset. Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> --- hw/ide/ahci.c | 12 +++++------- hw/ide/trace-events | 2 +- 2 files changed, 6 insertions(+), 8 deletions(-)