Re: [PATCH] vhost: It's better to use size_t for the 3rd parameter of vhost_exceeds_weight()

[Jason Wang]

On 2019/9/23 下午5:12, wangxu (AE) wrote:

Hi Michael

        Thanks for your fast reply.

        As the following code, the 2nd branch of iov_iter_advance() does not check if 
i->count < size, when this happens, i->count -= size may cause len exceed 
INT_MAX, and then total_len exceed INT_MAX.

        handle_tx_copy() ->
                get_tx_bufs(..., &len, ...) ->
                        init_iov_iter() ->
                                iov_iter_advance(iter, ...)     // has 3 
branches:
                                        pipe_advance()          // has checked the size: 
if (unlikely(i->count < size)) size = i->count;
                                        iov_iter_is_discard() ...       // no 
check.

Yes, but I don't think we use ITER_DISCARD.

Thanks

                                        iterate_and_advance()   //has checked: if 
(unlikely(i->count < n)) n = i->count;
                                return iov_iter_count(iter);

-----Original Message-----
From: Michael S. Tsirkin [mailto:m...@redhat.com]
Sent: Monday, September 23, 2019 4:07 PM
To: wangxu (AE) <wangx...@huawei.com>
Cc: jasow...@redhat.com; k...@vger.kernel.org; 
virtualizat...@lists.linux-foundation.org; net...@vger.kernel.org; 
linux-kernel@vger.kernel.org
Subject: Re: [PATCH] vhost: It's better to use size_t for the 3rd parameter of 
vhost_exceeds_weight()

On Mon, Sep 23, 2019 at 03:46:41PM +0800, wangxu wrote:

From: Wang Xu <wangx...@huawei.com>

Caller of vhost_exceeds_weight(..., total_len) in drivers/vhost/net.c
usually pass size_t total_len, which may be affected by rx/tx package.

Signed-off-by: Wang Xu <wangx...@huawei.com>

Puts a bit more pressure on the register file ...
why do we care? Is there some way that it can exceed INT_MAX?

---
  drivers/vhost/vhost.c | 4 ++--
  drivers/vhost/vhost.h | 7 ++++---
  2 files changed, 6 insertions(+), 5 deletions(-)

diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c index
36ca2cf..159223a 100644
--- a/drivers/vhost/vhost.c
+++ b/drivers/vhost/vhost.c
@@ -412,7 +412,7 @@ static void vhost_dev_free_iovecs(struct vhost_dev
*dev)  }

bool vhost_exceeds_weight(struct vhost_virtqueue *vq,

-                         int pkts, int total_len)
+                         int pkts, size_t total_len)
  {
        struct vhost_dev *dev = vq->dev;

@@ -454,7 +454,7 @@ static size_t vhost_get_desc_size(struct

vhost_virtqueue *vq,

void vhost_dev_init(struct vhost_dev *dev,

                    struct vhost_virtqueue **vqs, int nvqs,
-                   int iov_limit, int weight, int byte_weight)
+                   int iov_limit, int weight, size_t byte_weight)
  {
        struct vhost_virtqueue *vq;
        int i;
diff --git a/drivers/vhost/vhost.h b/drivers/vhost/vhost.h index
e9ed272..8d80389d 100644
--- a/drivers/vhost/vhost.h
+++ b/drivers/vhost/vhost.h
@@ -172,12 +172,13 @@ struct vhost_dev {
        wait_queue_head_t wait;
        int iov_limit;
        int weight;
-       int byte_weight;
+       size_t byte_weight;
  };

This just costs extra memory, and value is never large, so I don't think this 
matters.

-bool vhost_exceeds_weight(struct vhost_virtqueue *vq, int pkts, int
total_len);
+bool vhost_exceeds_weight(struct vhost_virtqueue *vq, int pkts,
+                         size_t total_len);
  void vhost_dev_init(struct vhost_dev *, struct vhost_virtqueue **vqs,
-                   int nvqs, int iov_limit, int weight, int byte_weight);
+                   int nvqs, int iov_limit, int weight, size_t byte_weight);
  long vhost_dev_set_owner(struct vhost_dev *dev);  bool
vhost_dev_has_owner(struct vhost_dev *dev);  long
vhost_dev_check_owner(struct vhost_dev *);
--
1.8.5.6