Message ID | 20210713084656.232-1-xieyongji@bytedance.com |
---|---|
Headers | show |
Series | Introduce VDUSE - vDPA Device in Userspace | expand |
On Tue, Jul 13, 2021 at 04:46:46PM +0800, Xie Yongji wrote: > We don't need to set FAILED status bit on device index allocation > failure since the device initialization hasn't been started yet. The commit message should say what the effect of this change is to the user. Is this a bugfix? Will it have any effect on runtime at all? To me, hearing your thoughts on this is valuable even if you have to guess. "I noticed this mistake during review and I don't think it will affect runtime." regards, dan carpenter
在 2021/7/13 下午7:31, Dan Carpenter 写道: > On Tue, Jul 13, 2021 at 04:46:52PM +0800, Xie Yongji wrote: >> @@ -613,37 +618,28 @@ static void vhost_vdpa_unmap(struct vhost_vdpa *v, u64 iova, u64 size) >> } >> } >> >> -static int vhost_vdpa_process_iotlb_update(struct vhost_vdpa *v, >> - struct vhost_iotlb_msg *msg) >> +static int vhost_vdpa_pa_map(struct vhost_vdpa *v, >> + u64 iova, u64 size, u64 uaddr, u32 perm) >> { >> struct vhost_dev *dev = &v->vdev; >> - struct vhost_iotlb *iotlb = dev->iotlb; >> struct page **page_list; >> unsigned long list_size = PAGE_SIZE / sizeof(struct page *); >> unsigned int gup_flags = FOLL_LONGTERM; >> unsigned long npages, cur_base, map_pfn, last_pfn = 0; >> unsigned long lock_limit, sz2pin, nchunks, i; >> - u64 iova = msg->iova; >> + u64 start = iova; >> long pinned; >> int ret = 0; >> >> - if (msg->iova < v->range.first || >> - msg->iova + msg->size - 1 > v->range.last) >> - return -EINVAL; > This is not related to your patch, but can the "msg->iova + msg->size" > addition can have an integer overflow. From looking at the callers it > seems like it can. msg comes from: > vhost_chr_write_iter() > --> dev->msg_handler(dev, &msg); > --> vhost_vdpa_process_iotlb_msg() > --> vhost_vdpa_process_iotlb_update() Yes. > > If I'm thinking of the right thing then these are allowed to overflow to > 0 because of the " - 1" but not further than that. I believe the check > needs to be something like: > > if (msg->iova < v->range.first || > msg->iova - 1 > U64_MAX - msg->size || I guess we don't need - 1 here? Thanks > msg->iova + msg->size - 1 > v->range.last) > > But writing integer overflow check correctly is notoriously difficult. > Do you think you could send a fix for that which is separate from the > patcheset? We'd want to backport it to stable. > > regards, > dan carpenter >
On Tue, Jul 13, 2021 at 7:31 PM Dan Carpenter <dan.carpenter@oracle.com> wrote: > > On Tue, Jul 13, 2021 at 04:46:52PM +0800, Xie Yongji wrote: > > @@ -613,37 +618,28 @@ static void vhost_vdpa_unmap(struct vhost_vdpa *v, u64 iova, u64 size) > > } > > } > > > > -static int vhost_vdpa_process_iotlb_update(struct vhost_vdpa *v, > > - struct vhost_iotlb_msg *msg) > > +static int vhost_vdpa_pa_map(struct vhost_vdpa *v, > > + u64 iova, u64 size, u64 uaddr, u32 perm) > > { > > struct vhost_dev *dev = &v->vdev; > > - struct vhost_iotlb *iotlb = dev->iotlb; > > struct page **page_list; > > unsigned long list_size = PAGE_SIZE / sizeof(struct page *); > > unsigned int gup_flags = FOLL_LONGTERM; > > unsigned long npages, cur_base, map_pfn, last_pfn = 0; > > unsigned long lock_limit, sz2pin, nchunks, i; > > - u64 iova = msg->iova; > > + u64 start = iova; > > long pinned; > > int ret = 0; > > > > - if (msg->iova < v->range.first || > > - msg->iova + msg->size - 1 > v->range.last) > > - return -EINVAL; > > This is not related to your patch, but can the "msg->iova + msg->size" > addition can have an integer overflow. From looking at the callers it > seems like it can. msg comes from: > vhost_chr_write_iter() > --> dev->msg_handler(dev, &msg); > --> vhost_vdpa_process_iotlb_msg() > --> vhost_vdpa_process_iotlb_update() > > If I'm thinking of the right thing then these are allowed to overflow to > 0 because of the " - 1" but not further than that. I believe the check > needs to be something like: > > if (msg->iova < v->range.first || > msg->iova - 1 > U64_MAX - msg->size || > msg->iova + msg->size - 1 > v->range.last) > Make sense. > But writing integer overflow check correctly is notoriously difficult. > Do you think you could send a fix for that which is separate from the > patcheset? We'd want to backport it to stable. > OK, I will send a patch to fix it. Thanks, Yongji
On Wed, Jul 14, 2021 at 10:14:32AM +0800, Jason Wang wrote: > > 在 2021/7/13 下午7:31, Dan Carpenter 写道: > > On Tue, Jul 13, 2021 at 04:46:52PM +0800, Xie Yongji wrote: > > > @@ -613,37 +618,28 @@ static void vhost_vdpa_unmap(struct vhost_vdpa *v, u64 iova, u64 size) > > > } > > > } > > > -static int vhost_vdpa_process_iotlb_update(struct vhost_vdpa *v, > > > - struct vhost_iotlb_msg *msg) > > > +static int vhost_vdpa_pa_map(struct vhost_vdpa *v, > > > + u64 iova, u64 size, u64 uaddr, u32 perm) > > > { > > > struct vhost_dev *dev = &v->vdev; > > > - struct vhost_iotlb *iotlb = dev->iotlb; > > > struct page **page_list; > > > unsigned long list_size = PAGE_SIZE / sizeof(struct page *); > > > unsigned int gup_flags = FOLL_LONGTERM; > > > unsigned long npages, cur_base, map_pfn, last_pfn = 0; > > > unsigned long lock_limit, sz2pin, nchunks, i; > > > - u64 iova = msg->iova; > > > + u64 start = iova; > > > long pinned; > > > int ret = 0; > > > - if (msg->iova < v->range.first || > > > - msg->iova + msg->size - 1 > v->range.last) > > > - return -EINVAL; > > This is not related to your patch, but can the "msg->iova + msg->size" > > addition can have an integer overflow. From looking at the callers it > > seems like it can. msg comes from: > > vhost_chr_write_iter() > > --> dev->msg_handler(dev, &msg); > > --> vhost_vdpa_process_iotlb_msg() > > --> vhost_vdpa_process_iotlb_update() > > > Yes. > > > > > > If I'm thinking of the right thing then these are allowed to overflow to > > 0 because of the " - 1" but not further than that. I believe the check > > needs to be something like: > > > > if (msg->iova < v->range.first || > > msg->iova - 1 > U64_MAX - msg->size || > > > I guess we don't need - 1 here? The - 1 is important. The highest address is 0xffffffff. So it goes start + size = 0 and then start + size - 1 == 0xffffffff. I guess we could move the - 1 to the other side? msg->iova > U64_MAX - msg->size + 1 || regards, dan carpenter
在 2021/7/14 下午4:05, Dan Carpenter 写道: > On Wed, Jul 14, 2021 at 10:14:32AM +0800, Jason Wang wrote: >> 在 2021/7/13 下午7:31, Dan Carpenter 写道: >>> On Tue, Jul 13, 2021 at 04:46:52PM +0800, Xie Yongji wrote: >>>> @@ -613,37 +618,28 @@ static void vhost_vdpa_unmap(struct vhost_vdpa *v, u64 iova, u64 size) >>>> } >>>> } >>>> -static int vhost_vdpa_process_iotlb_update(struct vhost_vdpa *v, >>>> - struct vhost_iotlb_msg *msg) >>>> +static int vhost_vdpa_pa_map(struct vhost_vdpa *v, >>>> + u64 iova, u64 size, u64 uaddr, u32 perm) >>>> { >>>> struct vhost_dev *dev = &v->vdev; >>>> - struct vhost_iotlb *iotlb = dev->iotlb; >>>> struct page **page_list; >>>> unsigned long list_size = PAGE_SIZE / sizeof(struct page *); >>>> unsigned int gup_flags = FOLL_LONGTERM; >>>> unsigned long npages, cur_base, map_pfn, last_pfn = 0; >>>> unsigned long lock_limit, sz2pin, nchunks, i; >>>> - u64 iova = msg->iova; >>>> + u64 start = iova; >>>> long pinned; >>>> int ret = 0; >>>> - if (msg->iova < v->range.first || >>>> - msg->iova + msg->size - 1 > v->range.last) >>>> - return -EINVAL; >>> This is not related to your patch, but can the "msg->iova + msg->size" >>> addition can have an integer overflow. From looking at the callers it >>> seems like it can. msg comes from: >>> vhost_chr_write_iter() >>> --> dev->msg_handler(dev, &msg); >>> --> vhost_vdpa_process_iotlb_msg() >>> --> vhost_vdpa_process_iotlb_update() >> >> Yes. >> >> >>> If I'm thinking of the right thing then these are allowed to overflow to >>> 0 because of the " - 1" but not further than that. I believe the check >>> needs to be something like: >>> >>> if (msg->iova < v->range.first || >>> msg->iova - 1 > U64_MAX - msg->size || >> >> I guess we don't need - 1 here? > The - 1 is important. The highest address is 0xffffffff. So it goes > start + size = 0 and then start + size - 1 == 0xffffffff. Right, so actually msg->iova = 0xfffffffe, msg->size=2 is valid. Thanks > > I guess we could move the - 1 to the other side? > > msg->iova > U64_MAX - msg->size + 1 || > > regards, > dan carpenter > >
On Wed, Jul 14, 2021 at 05:41:54PM +0800, Jason Wang wrote: > > 在 2021/7/14 下午4:05, Dan Carpenter 写道: > > On Wed, Jul 14, 2021 at 10:14:32AM +0800, Jason Wang wrote: > > > 在 2021/7/13 下午7:31, Dan Carpenter 写道: > > > > On Tue, Jul 13, 2021 at 04:46:52PM +0800, Xie Yongji wrote: > > > > > @@ -613,37 +618,28 @@ static void vhost_vdpa_unmap(struct vhost_vdpa *v, u64 iova, u64 size) > > > > > } > > > > > } > > > > > -static int vhost_vdpa_process_iotlb_update(struct vhost_vdpa *v, > > > > > - struct vhost_iotlb_msg *msg) > > > > > +static int vhost_vdpa_pa_map(struct vhost_vdpa *v, > > > > > + u64 iova, u64 size, u64 uaddr, u32 perm) > > > > > { > > > > > struct vhost_dev *dev = &v->vdev; > > > > > - struct vhost_iotlb *iotlb = dev->iotlb; > > > > > struct page **page_list; > > > > > unsigned long list_size = PAGE_SIZE / sizeof(struct page *); > > > > > unsigned int gup_flags = FOLL_LONGTERM; > > > > > unsigned long npages, cur_base, map_pfn, last_pfn = 0; > > > > > unsigned long lock_limit, sz2pin, nchunks, i; > > > > > - u64 iova = msg->iova; > > > > > + u64 start = iova; > > > > > long pinned; > > > > > int ret = 0; > > > > > - if (msg->iova < v->range.first || > > > > > - msg->iova + msg->size - 1 > v->range.last) > > > > > - return -EINVAL; > > > > This is not related to your patch, but can the "msg->iova + msg->size" > > > > addition can have an integer overflow. From looking at the callers it > > > > seems like it can. msg comes from: > > > > vhost_chr_write_iter() > > > > --> dev->msg_handler(dev, &msg); > > > > --> vhost_vdpa_process_iotlb_msg() > > > > --> vhost_vdpa_process_iotlb_update() > > > > > > Yes. > > > > > > > > > > If I'm thinking of the right thing then these are allowed to overflow to > > > > 0 because of the " - 1" but not further than that. I believe the check > > > > needs to be something like: > > > > > > > > if (msg->iova < v->range.first || > > > > msg->iova - 1 > U64_MAX - msg->size || > > > > > > I guess we don't need - 1 here? > > The - 1 is important. The highest address is 0xffffffff. So it goes > > start + size = 0 and then start + size - 1 == 0xffffffff. > > > Right, so actually > > msg->iova = 0xfffffffe, msg->size=2 is valid. I believe so, yes. It's inclusive of 0xfffffffe and 0xffffffff. (Not an expert). regards, dan carpenter
在 2021/7/14 下午5:57, Dan Carpenter 写道: > On Wed, Jul 14, 2021 at 05:41:54PM +0800, Jason Wang wrote: >> 在 2021/7/14 下午4:05, Dan Carpenter 写道: >>> On Wed, Jul 14, 2021 at 10:14:32AM +0800, Jason Wang wrote: >>>> 在 2021/7/13 下午7:31, Dan Carpenter 写道: >>>>> On Tue, Jul 13, 2021 at 04:46:52PM +0800, Xie Yongji wrote: >>>>>> @@ -613,37 +618,28 @@ static void vhost_vdpa_unmap(struct vhost_vdpa *v, u64 iova, u64 size) >>>>>> } >>>>>> } >>>>>> -static int vhost_vdpa_process_iotlb_update(struct vhost_vdpa *v, >>>>>> - struct vhost_iotlb_msg *msg) >>>>>> +static int vhost_vdpa_pa_map(struct vhost_vdpa *v, >>>>>> + u64 iova, u64 size, u64 uaddr, u32 perm) >>>>>> { >>>>>> struct vhost_dev *dev = &v->vdev; >>>>>> - struct vhost_iotlb *iotlb = dev->iotlb; >>>>>> struct page **page_list; >>>>>> unsigned long list_size = PAGE_SIZE / sizeof(struct page *); >>>>>> unsigned int gup_flags = FOLL_LONGTERM; >>>>>> unsigned long npages, cur_base, map_pfn, last_pfn = 0; >>>>>> unsigned long lock_limit, sz2pin, nchunks, i; >>>>>> - u64 iova = msg->iova; >>>>>> + u64 start = iova; >>>>>> long pinned; >>>>>> int ret = 0; >>>>>> - if (msg->iova < v->range.first || >>>>>> - msg->iova + msg->size - 1 > v->range.last) >>>>>> - return -EINVAL; >>>>> This is not related to your patch, but can the "msg->iova + msg->size" >>>>> addition can have an integer overflow. From looking at the callers it >>>>> seems like it can. msg comes from: >>>>> vhost_chr_write_iter() >>>>> --> dev->msg_handler(dev, &msg); >>>>> --> vhost_vdpa_process_iotlb_msg() >>>>> --> vhost_vdpa_process_iotlb_update() >>>> Yes. >>>> >>>> >>>>> If I'm thinking of the right thing then these are allowed to overflow to >>>>> 0 because of the " - 1" but not further than that. I believe the check >>>>> needs to be something like: >>>>> >>>>> if (msg->iova < v->range.first || >>>>> msg->iova - 1 > U64_MAX - msg->size || >>>> I guess we don't need - 1 here? >>> The - 1 is important. The highest address is 0xffffffff. So it goes >>> start + size = 0 and then start + size - 1 == 0xffffffff. >> >> Right, so actually >> >> msg->iova = 0xfffffffe, msg->size=2 is valid. > I believe so, yes. It's inclusive of 0xfffffffe and 0xffffffff. > (Not an expert). I think so, and we probably need to fix vhost_overflow() as well which did: static bool vhost_overflow(u64 uaddr, u64 size) { /* Make sure 64 bit math will not overflow. */ return uaddr > ULONG_MAX || size > ULONG_MAX || uaddr > ULONG_MAX - size; } Thanks > > regards, > dan carpenter >