| Message ID | 5ab04420c3de11ae4a573b08b53584a2a0c5dd00.1720046570.git.mst@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | [PULL,v3,01/85] vhost: dirty log should be per backend type | expand |
On Wed, 3 Jul 2024 at 23:48, Michael S. Tsirkin <mst@redhat.com> wrote: > > From: Stefano Garzarella <sgarzare@redhat.com> > > Let's replace the calls to le*toh() and htole*() with qemu/bswap.h > helpers to make the code more portable. > > Suggested-by: Philippe Mathieu-Daudé <philmd@linaro.org> > Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org> > Tested-by: Philippe Mathieu-Daudé <philmd@linaro.org> > Acked-by: Stefan Hajnoczi <stefanha@redhat.com> > Reviewed-by: David Hildenbrand <david@redhat.com> > Signed-off-by: Stefano Garzarella <sgarzare@redhat.com> > Message-Id: <20240618100447.145697-1-sgarzare@redhat.com> > Reviewed-by: Michael S. Tsirkin <mst@redhat.com> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com> > --- > contrib/vhost-user-blk/vhost-user-blk.c | 9 +++++---- > contrib/vhost-user-input/main.c | 16 ++++++++-------- > 2 files changed, 13 insertions(+), 12 deletions(-) > > diff --git a/contrib/vhost-user-blk/vhost-user-blk.c b/contrib/vhost-user-blk/vhost-user-blk.c > index a8ab9269a2..9492146855 100644 > --- a/contrib/vhost-user-blk/vhost-user-blk.c > +++ b/contrib/vhost-user-blk/vhost-user-blk.c > @@ -16,6 +16,7 @@ > */ > > #include "qemu/osdep.h" > +#include "qemu/bswap.h" > #include "standard-headers/linux/virtio_blk.h" > #include "libvhost-user-glib.h" > > @@ -194,8 +195,8 @@ vub_discard_write_zeroes(VubReq *req, struct iovec *iov, uint32_t iovcnt, > #if defined(__linux__) && defined(BLKDISCARD) && defined(BLKZEROOUT) > VubDev *vdev_blk = req->vdev_blk; > desc = buf; > - uint64_t range[2] = { le64toh(desc->sector) << 9, > - le32toh(desc->num_sectors) << 9 }; > + uint64_t range[2] = { le64_to_cpu(desc->sector) << 9, > + le32_to_cpu(desc->num_sectors) << 9 }; Hi; Coverity points out that this does a 32-bit shift, not a 64-bit one, so it could unintentionally chop the high bits off if desc->num_sectors is big enough (CID 1549454). We could fix this by making it (uint64_t)le32_to_cpu(desc->num_sectors) << 9 I think. (It looks like the issue was already there before, so Coverity has just noticed it because of the code change here.) thanks -- PMM
On Fri, Jul 12, 2024 at 03:24:47PM GMT, Peter Maydell wrote: >On Wed, 3 Jul 2024 at 23:48, Michael S. Tsirkin <mst@redhat.com> wrote: >> >> From: Stefano Garzarella <sgarzare@redhat.com> >> >> Let's replace the calls to le*toh() and htole*() with qemu/bswap.h >> helpers to make the code more portable. >> >> Suggested-by: Philippe Mathieu-Daudé <philmd@linaro.org> >> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org> >> Tested-by: Philippe Mathieu-Daudé <philmd@linaro.org> >> Acked-by: Stefan Hajnoczi <stefanha@redhat.com> >> Reviewed-by: David Hildenbrand <david@redhat.com> >> Signed-off-by: Stefano Garzarella <sgarzare@redhat.com> >> Message-Id: <20240618100447.145697-1-sgarzare@redhat.com> >> Reviewed-by: Michael S. Tsirkin <mst@redhat.com> >> Signed-off-by: Michael S. Tsirkin <mst@redhat.com> >> --- >> contrib/vhost-user-blk/vhost-user-blk.c | 9 +++++---- >> contrib/vhost-user-input/main.c | 16 ++++++++-------- >> 2 files changed, 13 insertions(+), 12 deletions(-) >> >> diff --git a/contrib/vhost-user-blk/vhost-user-blk.c b/contrib/vhost-user-blk/vhost-user-blk.c >> index a8ab9269a2..9492146855 100644 >> --- a/contrib/vhost-user-blk/vhost-user-blk.c >> +++ b/contrib/vhost-user-blk/vhost-user-blk.c >> @@ -16,6 +16,7 @@ >> */ >> >> #include "qemu/osdep.h" >> +#include "qemu/bswap.h" >> #include "standard-headers/linux/virtio_blk.h" >> #include "libvhost-user-glib.h" >> >> @@ -194,8 +195,8 @@ vub_discard_write_zeroes(VubReq *req, struct iovec *iov, uint32_t iovcnt, >> #if defined(__linux__) && defined(BLKDISCARD) && defined(BLKZEROOUT) >> VubDev *vdev_blk = req->vdev_blk; >> desc = buf; >> - uint64_t range[2] = { le64toh(desc->sector) << 9, >> - le32toh(desc->num_sectors) << 9 }; >> + uint64_t range[2] = { le64_to_cpu(desc->sector) << 9, >> + le32_to_cpu(desc->num_sectors) << 9 }; > >Hi; Coverity points out that this does a 32-bit shift, not a >64-bit one, so it could unintentionally chop the high bits off >if desc->num_sectors is big enough (CID 1549454). >We could fix this by making it > (uint64_t)le32_to_cpu(desc->num_sectors) << 9 >I think. Yep, I think so! I'll send a patch. > >(It looks like the issue was already there before, so Yes, it is pre-existing to this patch, introduced from the beginning with commit caa1ee4313 ("vhost-user-blk: add discard/write zeroes features support") >Coverity has just noticed it because of the code change here.) Ah, I thought it ran on all the code, not just the changes. Thanks, Stefano
On Fri, 12 Jul 2024 at 16:18, Stefano Garzarella <sgarzare@redhat.com> wrote: > > On Fri, Jul 12, 2024 at 03:24:47PM GMT, Peter Maydell wrote: > >On Wed, 3 Jul 2024 at 23:48, Michael S. Tsirkin <mst@redhat.com> wrote: > >> #if defined(__linux__) && defined(BLKDISCARD) && defined(BLKZEROOUT) > >> VubDev *vdev_blk = req->vdev_blk; > >> desc = buf; > >> - uint64_t range[2] = { le64toh(desc->sector) << 9, > >> - le32toh(desc->num_sectors) << 9 }; > >> + uint64_t range[2] = { le64_to_cpu(desc->sector) << 9, > >> + le32_to_cpu(desc->num_sectors) << 9 }; > > > >Hi; Coverity points out that this does a 32-bit shift, not a > >64-bit one, so it could unintentionally chop the high bits off > >if desc->num_sectors is big enough (CID 1549454). > >We could fix this by making it > > (uint64_t)le32_to_cpu(desc->num_sectors) << 9 > >I think. > > Yep, I think so! I'll send a patch. > > > > >(It looks like the issue was already there before, so > > Yes, it is pre-existing to this patch, introduced from the beginning > with commit caa1ee4313 ("vhost-user-blk: add discard/write zeroes > features support") > > >Coverity has just noticed it because of the code change here.) > > Ah, I thought it ran on all the code, not just the changes. It does run on all the code, but if the code changes enough that can cause it to close out the old issue on the old code and create a new issue in the system for the new code (which I then notice because I look at newly-found things to triage them). So things like refactorings and moving code around can cause issues to show up. The other reason this might have shown up now is that they seem to have added a new check type which flags up a lot of "possible overflow" errors, so there's a huge pile of new issues for old code that I'm gradually going through to see which are false positives and which we should look at. -- PMM
diff --git a/contrib/vhost-user-blk/vhost-user-blk.c b/contrib/vhost-user-blk/vhost-user-blk.c index a8ab9269a2..9492146855 100644 --- a/contrib/vhost-user-blk/vhost-user-blk.c +++ b/contrib/vhost-user-blk/vhost-user-blk.c @@ -16,6 +16,7 @@ */ #include "qemu/osdep.h" +#include "qemu/bswap.h" #include "standard-headers/linux/virtio_blk.h" #include "libvhost-user-glib.h" @@ -194,8 +195,8 @@ vub_discard_write_zeroes(VubReq *req, struct iovec *iov, uint32_t iovcnt, #if defined(__linux__) && defined(BLKDISCARD) && defined(BLKZEROOUT) VubDev *vdev_blk = req->vdev_blk; desc = buf; - uint64_t range[2] = { le64toh(desc->sector) << 9, - le32toh(desc->num_sectors) << 9 }; + uint64_t range[2] = { le64_to_cpu(desc->sector) << 9, + le32_to_cpu(desc->num_sectors) << 9 }; if (type == VIRTIO_BLK_T_DISCARD) { if (ioctl(vdev_blk->blk_fd, BLKDISCARD, range) == 0) { g_free(buf); @@ -267,13 +268,13 @@ static int vub_virtio_process_req(VubDev *vdev_blk, req->in = (struct virtio_blk_inhdr *)elem->in_sg[in_num - 1].iov_base; in_num--; - type = le32toh(req->out->type); + type = le32_to_cpu(req->out->type); switch (type & ~VIRTIO_BLK_T_BARRIER) { case VIRTIO_BLK_T_IN: case VIRTIO_BLK_T_OUT: { ssize_t ret = 0; bool is_write = type & VIRTIO_BLK_T_OUT; - req->sector_num = le64toh(req->out->sector); + req->sector_num = le64_to_cpu(req->out->sector); if (is_write) { ret = vub_writev(req, &elem->out_sg[1], out_num); } else { diff --git a/contrib/vhost-user-input/main.c b/contrib/vhost-user-input/main.c index 081230da54..f3362d41ac 100644 --- a/contrib/vhost-user-input/main.c +++ b/contrib/vhost-user-input/main.c @@ -51,8 +51,8 @@ static void vi_input_send(VuInput *vi, struct virtio_input_event *event) vi->queue[vi->qindex++].event = *event; /* ... until we see a report sync ... */ - if (event->type != htole16(EV_SYN) || - event->code != htole16(SYN_REPORT)) { + if (event->type != cpu_to_le16(EV_SYN) || + event->code != cpu_to_le16(SYN_REPORT)) { return; } @@ -103,9 +103,9 @@ vi_evdev_watch(VuDev *dev, int condition, void *data) g_debug("input %d %d %d", evdev.type, evdev.code, evdev.value); - virtio.type = htole16(evdev.type); - virtio.code = htole16(evdev.code); - virtio.value = htole32(evdev.value); + virtio.type = cpu_to_le16(evdev.type); + virtio.code = cpu_to_le16(evdev.code); + virtio.value = cpu_to_le32(evdev.value); vi_input_send(vi, &virtio); } } @@ -124,9 +124,9 @@ static void vi_handle_status(VuInput *vi, virtio_input_event *event) evdev.input_event_sec = tval.tv_sec; evdev.input_event_usec = tval.tv_usec; - evdev.type = le16toh(event->type); - evdev.code = le16toh(event->code); - evdev.value = le32toh(event->value); + evdev.type = le16_to_cpu(event->type); + evdev.code = le16_to_cpu(event->code); + evdev.value = le32_to_cpu(event->value); rc = write(vi->evdevfd, &evdev, sizeof(evdev)); if (rc == -1) {