| Message ID | 20170119104353.28313-1-stefanha@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 2017年01月19日 18:43, Stefan Hajnoczi wrote: > Propagate the error when vhost_vq_init_access() fails and set > vq->private_data to NULL. > > Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com> > --- > drivers/vhost/vsock.c | 13 +++++++++---- > 1 file changed, 9 insertions(+), 4 deletions(-) > > diff --git a/drivers/vhost/vsock.c b/drivers/vhost/vsock.c > index bbbf5885..ce5e63d 100644 > --- a/drivers/vhost/vsock.c > +++ b/drivers/vhost/vsock.c > @@ -373,6 +373,7 @@ static void vhost_vsock_handle_rx_kick(struct vhost_work *work) > > static int vhost_vsock_start(struct vhost_vsock *vsock) > { > + struct vhost_virtqueue *vq; > size_t i; > int ret; > > @@ -383,19 +384,20 @@ static int vhost_vsock_start(struct vhost_vsock *vsock) > goto err; > > for (i = 0; i < ARRAY_SIZE(vsock->vqs); i++) { > - struct vhost_virtqueue *vq = &vsock->vqs[i]; > + vq = &vsock->vqs[i]; > > mutex_lock(&vq->mutex); > > if (!vhost_vq_access_ok(vq)) { > ret = -EFAULT; > - mutex_unlock(&vq->mutex); > goto err_vq; > } > > if (!vq->private_data) { > vq->private_data = vsock; > - vhost_vq_init_access(vq); > + ret = vhost_vq_init_access(vq); > + if (ret) > + goto err_vq; > } > > mutex_unlock(&vq->mutex); > @@ -405,8 +407,11 @@ static int vhost_vsock_start(struct vhost_vsock *vsock) > return 0; > > err_vq: > + vq->private_data = NULL; Looks like this will be done twice? Thanks > + mutex_unlock(&vq->mutex); > + > for (i = 0; i < ARRAY_SIZE(vsock->vqs); i++) { > - struct vhost_virtqueue *vq = &vsock->vqs[i]; > + vq = &vsock->vqs[i]; > > mutex_lock(&vq->mutex); > vq->private_data = NULL; -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Jan 20, 2017 at 11:23:13AM +0800, Jason Wang wrote: > > > On 2017年01月19日 18:43, Stefan Hajnoczi wrote: > > Propagate the error when vhost_vq_init_access() fails and set > > vq->private_data to NULL. > > > > Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com> > > --- > > drivers/vhost/vsock.c | 13 +++++++++---- > > 1 file changed, 9 insertions(+), 4 deletions(-) > > > > diff --git a/drivers/vhost/vsock.c b/drivers/vhost/vsock.c > > index bbbf5885..ce5e63d 100644 > > --- a/drivers/vhost/vsock.c > > +++ b/drivers/vhost/vsock.c > > @@ -373,6 +373,7 @@ static void vhost_vsock_handle_rx_kick(struct vhost_work *work) > > static int vhost_vsock_start(struct vhost_vsock *vsock) > > { > > + struct vhost_virtqueue *vq; > > size_t i; > > int ret; > > @@ -383,19 +384,20 @@ static int vhost_vsock_start(struct vhost_vsock *vsock) > > goto err; > > for (i = 0; i < ARRAY_SIZE(vsock->vqs); i++) { > > - struct vhost_virtqueue *vq = &vsock->vqs[i]; > > + vq = &vsock->vqs[i]; > > mutex_lock(&vq->mutex); > > if (!vhost_vq_access_ok(vq)) { > > ret = -EFAULT; > > - mutex_unlock(&vq->mutex); > > goto err_vq; > > } > > if (!vq->private_data) { > > vq->private_data = vsock; > > - vhost_vq_init_access(vq); > > + ret = vhost_vq_init_access(vq); > > + if (ret) > > + goto err_vq; > > } > > mutex_unlock(&vq->mutex); > > @@ -405,8 +407,11 @@ static int vhost_vsock_start(struct vhost_vsock *vsock) > > return 0; > > err_vq: > > + vq->private_data = NULL; > > Looks like this will be done twice? Here we still hold the vq mutex. If we release the mutex then a virtqueue kick could be processed. Stefan
On 2017年01月20日 21:40, Stefan Hajnoczi wrote: > On Fri, Jan 20, 2017 at 11:23:13AM +0800, Jason Wang wrote: >> >> On 2017年01月19日 18:43, Stefan Hajnoczi wrote: >>> Propagate the error when vhost_vq_init_access() fails and set >>> vq->private_data to NULL. >>> >>> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com> >>> --- >>> drivers/vhost/vsock.c | 13 +++++++++---- >>> 1 file changed, 9 insertions(+), 4 deletions(-) >>> >>> diff --git a/drivers/vhost/vsock.c b/drivers/vhost/vsock.c >>> index bbbf5885..ce5e63d 100644 >>> --- a/drivers/vhost/vsock.c >>> +++ b/drivers/vhost/vsock.c >>> @@ -373,6 +373,7 @@ static void vhost_vsock_handle_rx_kick(struct vhost_work *work) >>> static int vhost_vsock_start(struct vhost_vsock *vsock) >>> { >>> + struct vhost_virtqueue *vq; >>> size_t i; >>> int ret; >>> @@ -383,19 +384,20 @@ static int vhost_vsock_start(struct vhost_vsock *vsock) >>> goto err; >>> for (i = 0; i < ARRAY_SIZE(vsock->vqs); i++) { >>> - struct vhost_virtqueue *vq = &vsock->vqs[i]; >>> + vq = &vsock->vqs[i]; >>> mutex_lock(&vq->mutex); >>> if (!vhost_vq_access_ok(vq)) { >>> ret = -EFAULT; >>> - mutex_unlock(&vq->mutex); >>> goto err_vq; >>> } >>> if (!vq->private_data) { >>> vq->private_data = vsock; >>> - vhost_vq_init_access(vq); >>> + ret = vhost_vq_init_access(vq); >>> + if (ret) >>> + goto err_vq; >>> } >>> mutex_unlock(&vq->mutex); >>> @@ -405,8 +407,11 @@ static int vhost_vsock_start(struct vhost_vsock *vsock) >>> return 0; >>> err_vq: >>> + vq->private_data = NULL; >> Looks like this will be done twice? > Here we still hold the vq mutex. If we release the mutex then a > virtqueue kick could be processed. > > Stefan Ok, then it's better to assign vq->private_data after vhost_vq_init_acess() succeed. Thanks
On Sun, Jan 22, 2017 at 04:16:11PM +0800, Jason Wang wrote: > > > On 2017年01月20日 21:40, Stefan Hajnoczi wrote: > > On Fri, Jan 20, 2017 at 11:23:13AM +0800, Jason Wang wrote: > > > > > > On 2017年01月19日 18:43, Stefan Hajnoczi wrote: > > > > Propagate the error when vhost_vq_init_access() fails and set > > > > vq->private_data to NULL. > > > > > > > > Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com> > > > > --- > > > > drivers/vhost/vsock.c | 13 +++++++++---- > > > > 1 file changed, 9 insertions(+), 4 deletions(-) > > > > > > > > diff --git a/drivers/vhost/vsock.c b/drivers/vhost/vsock.c > > > > index bbbf5885..ce5e63d 100644 > > > > --- a/drivers/vhost/vsock.c > > > > +++ b/drivers/vhost/vsock.c > > > > @@ -373,6 +373,7 @@ static void vhost_vsock_handle_rx_kick(struct vhost_work *work) > > > > static int vhost_vsock_start(struct vhost_vsock *vsock) > > > > { > > > > + struct vhost_virtqueue *vq; > > > > size_t i; > > > > int ret; > > > > @@ -383,19 +384,20 @@ static int vhost_vsock_start(struct vhost_vsock *vsock) > > > > goto err; > > > > for (i = 0; i < ARRAY_SIZE(vsock->vqs); i++) { > > > > - struct vhost_virtqueue *vq = &vsock->vqs[i]; > > > > + vq = &vsock->vqs[i]; > > > > mutex_lock(&vq->mutex); > > > > if (!vhost_vq_access_ok(vq)) { > > > > ret = -EFAULT; > > > > - mutex_unlock(&vq->mutex); > > > > goto err_vq; > > > > } > > > > if (!vq->private_data) { > > > > vq->private_data = vsock; > > > > - vhost_vq_init_access(vq); > > > > + ret = vhost_vq_init_access(vq); > > > > + if (ret) > > > > + goto err_vq; > > > > } > > > > mutex_unlock(&vq->mutex); > > > > @@ -405,8 +407,11 @@ static int vhost_vsock_start(struct vhost_vsock *vsock) > > > > return 0; > > > > err_vq: > > > > + vq->private_data = NULL; > > > Looks like this will be done twice? > > Here we still hold the vq mutex. If we release the mutex then a > > virtqueue kick could be processed. > > > > Stefan > > Ok, then it's better to assign vq->private_data after vhost_vq_init_acess() > succeed. No, that won't work because vhost_vq_init_acess() requires that vq->private_data is non-NULL. Stefan
On 2017年01月23日 17:56, Stefan Hajnoczi wrote: > On Sun, Jan 22, 2017 at 04:16:11PM +0800, Jason Wang wrote: >> >> On 2017年01月20日 21:40, Stefan Hajnoczi wrote: >>> On Fri, Jan 20, 2017 at 11:23:13AM +0800, Jason Wang wrote: >>>> On 2017年01月19日 18:43, Stefan Hajnoczi wrote: >>>>> Propagate the error when vhost_vq_init_access() fails and set >>>>> vq->private_data to NULL. >>>>> >>>>> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com> >>>>> --- >>>>> drivers/vhost/vsock.c | 13 +++++++++---- >>>>> 1 file changed, 9 insertions(+), 4 deletions(-) >>>>> >>>>> diff --git a/drivers/vhost/vsock.c b/drivers/vhost/vsock.c >>>>> index bbbf5885..ce5e63d 100644 >>>>> --- a/drivers/vhost/vsock.c >>>>> +++ b/drivers/vhost/vsock.c >>>>> @@ -373,6 +373,7 @@ static void vhost_vsock_handle_rx_kick(struct vhost_work *work) >>>>> static int vhost_vsock_start(struct vhost_vsock *vsock) >>>>> { >>>>> + struct vhost_virtqueue *vq; >>>>> size_t i; >>>>> int ret; >>>>> @@ -383,19 +384,20 @@ static int vhost_vsock_start(struct vhost_vsock *vsock) >>>>> goto err; >>>>> for (i = 0; i < ARRAY_SIZE(vsock->vqs); i++) { >>>>> - struct vhost_virtqueue *vq = &vsock->vqs[i]; >>>>> + vq = &vsock->vqs[i]; >>>>> mutex_lock(&vq->mutex); >>>>> if (!vhost_vq_access_ok(vq)) { >>>>> ret = -EFAULT; >>>>> - mutex_unlock(&vq->mutex); >>>>> goto err_vq; >>>>> } >>>>> if (!vq->private_data) { >>>>> vq->private_data = vsock; >>>>> - vhost_vq_init_access(vq); >>>>> + ret = vhost_vq_init_access(vq); >>>>> + if (ret) >>>>> + goto err_vq; >>>>> } >>>>> mutex_unlock(&vq->mutex); >>>>> @@ -405,8 +407,11 @@ static int vhost_vsock_start(struct vhost_vsock *vsock) >>>>> return 0; >>>>> err_vq: >>>>> + vq->private_data = NULL; >>>> Looks like this will be done twice? >>> Here we still hold the vq mutex. If we release the mutex then a >>> virtqueue kick could be processed. >>> >>> Stefan >> Ok, then it's better to assign vq->private_data after vhost_vq_init_acess() >> succeed. > No, that won't work because vhost_vq_init_acess() requires that > vq->private_data is non-NULL. > > Stefan Oh right. Then Acked-by: Jason Wang <jasowang@redhat.com>
diff --git a/drivers/vhost/vsock.c b/drivers/vhost/vsock.c index bbbf5885..ce5e63d 100644 --- a/drivers/vhost/vsock.c +++ b/drivers/vhost/vsock.c @@ -373,6 +373,7 @@ static void vhost_vsock_handle_rx_kick(struct vhost_work *work) static int vhost_vsock_start(struct vhost_vsock *vsock) { + struct vhost_virtqueue *vq; size_t i; int ret; @@ -383,19 +384,20 @@ static int vhost_vsock_start(struct vhost_vsock *vsock) goto err; for (i = 0; i < ARRAY_SIZE(vsock->vqs); i++) { - struct vhost_virtqueue *vq = &vsock->vqs[i]; + vq = &vsock->vqs[i]; mutex_lock(&vq->mutex); if (!vhost_vq_access_ok(vq)) { ret = -EFAULT; - mutex_unlock(&vq->mutex); goto err_vq; } if (!vq->private_data) { vq->private_data = vsock; - vhost_vq_init_access(vq); + ret = vhost_vq_init_access(vq); + if (ret) + goto err_vq; } mutex_unlock(&vq->mutex); @@ -405,8 +407,11 @@ static int vhost_vsock_start(struct vhost_vsock *vsock) return 0; err_vq: + vq->private_data = NULL; + mutex_unlock(&vq->mutex); + for (i = 0; i < ARRAY_SIZE(vsock->vqs); i++) { - struct vhost_virtqueue *vq = &vsock->vqs[i]; + vq = &vsock->vqs[i]; mutex_lock(&vq->mutex); vq->private_data = NULL;
Propagate the error when vhost_vq_init_access() fails and set vq->private_data to NULL. Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com> --- drivers/vhost/vsock.c | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-)