diff mbox

[dpdk-dev] vhost: fix off-by-one error on nr_desc check

Message ID 1469455798-19790-1-git-send-email-maxime.coquelin@redhat.com (mailing list archive)
State Accepted, archived
Headers

Commit Message

Maxime Coquelin July 25, 2016, 2:09 p.m. UTC 
  nr_desc is not an index but the number of descriptors,
so can be equal to the virtqueue size.

Fixes: a436f53ebfeb ("vhost: avoid dead loop chain")

Cc: Yuanhan Liu <yuanhan.liu@linux.intel.com>
Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
---
Hi Yuanhan,

I faced the bug while testing my indirect descriptor patch, it happens
as soon as the number of chained descritors is above 2.

But the bug may in theory also be faced with normal descriptors, so it might
be good to have it 16.07?

Regards,
Maxime

---
 lib/librte_vhost/vhost_rxtx.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Yuanhan Liu July 25, 2016, 3:24 p.m. UTC | #1 
On Mon, Jul 25, 2016 at 04:09:58PM +0200, Maxime Coquelin wrote:
> nr_desc is not an index but the number of descriptors,
> so can be equal to the virtqueue size.
> 
> Fixes: a436f53ebfeb ("vhost: avoid dead loop chain")
> 
> Cc: Yuanhan Liu <yuanhan.liu@linux.intel.com>
> Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>

Thanks for catching it!

> ---
> Hi Yuanhan,
> 
> I faced the bug while testing my indirect descriptor patch, it happens
> as soon as the number of chained descritors is above 2.
> 
> But the bug may in theory also be faced with normal descriptors,

In theory, yes, and only in one case, that there is a Tx has 256
descriptors chained. If that happens, I doubt things work well.
So I would say it just happens __in theory__.

> so it might
> be good to have it 16.07?

Even though, it apparently fixes a bug, so I think we could have it
for 16.07.

Acked-by: Yuanhan Liu <yuanhan.liu@linux.intel.com>

	--yliu
Maxime Coquelin July 25, 2016, 3:28 p.m. UTC | #2 
On 07/25/2016 05:24 PM, Yuanhan Liu wrote:
> On Mon, Jul 25, 2016 at 04:09:58PM +0200, Maxime Coquelin wrote:
>> nr_desc is not an index but the number of descriptors,
>> so can be equal to the virtqueue size.
>>
>> Fixes: a436f53ebfeb ("vhost: avoid dead loop chain")
>>
>> Cc: Yuanhan Liu <yuanhan.liu@linux.intel.com>
>> Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
>
> Thanks for catching it!
>
>> ---
>> Hi Yuanhan,
>>
>> I faced the bug while testing my indirect descriptor patch, it happens
>> as soon as the number of chained descritors is above 2.
>>
>> But the bug may in theory also be faced with normal descriptors,
>
> In theory, yes, and only in one case, that there is a Tx has 256
> descriptors chained. If that happens, I doubt things work well.
> So I would say it just happens __in theory__.
Right.

>
>> so it might
>> be good to have it 16.07?
>
> Even though, it apparently fixes a bug, so I think we could have it
> for 16.07.
Good, but don't delay 16.07 for that! :)

>
> Acked-by: Yuanhan Liu <yuanhan.liu@linux.intel.com>
>
> 	--yliu
>

Thanks,
Maxime
Thomas Monjalon July 25, 2016, 3:47 p.m. UTC | #3 
2016-07-25 23:24, Yuanhan Liu:
> On Mon, Jul 25, 2016 at 04:09:58PM +0200, Maxime Coquelin wrote:
> > nr_desc is not an index but the number of descriptors,
> > so can be equal to the virtqueue size.
> > 
> > Fixes: a436f53ebfeb ("vhost: avoid dead loop chain")
> > 
> > Cc: Yuanhan Liu <yuanhan.liu@linux.intel.com>
> > Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
> 
> Thanks for catching it!
> 
> > ---
> > Hi Yuanhan,
> > 
> > I faced the bug while testing my indirect descriptor patch, it happens
> > as soon as the number of chained descritors is above 2.
> > 
> > But the bug may in theory also be faced with normal descriptors,
> 
> In theory, yes, and only in one case, that there is a Tx has 256
> descriptors chained. If that happens, I doubt things work well.
> So I would say it just happens __in theory__.
> 
> > so it might
> > be good to have it 16.07?
> 
> Even though, it apparently fixes a bug, so I think we could have it
> for 16.07.
> 
> Acked-by: Yuanhan Liu <yuanhan.liu@linux.intel.com>

Applied, thanks
diff mbox

Patch

diff --git a/lib/librte_vhost/vhost_rxtx.c b/lib/librte_vhost/vhost_rxtx.c
index bc00518..08a73fd 100644
--- a/lib/librte_vhost/vhost_rxtx.c
+++ b/lib/librte_vhost/vhost_rxtx.c
@@ -748,7 +748,7 @@  copy_desc_to_mbuf(struct virtio_net *dev, struct vhost_virtqueue *vq,
 				break;
 
 			if (unlikely(desc->next >= vq->size ||
-				     ++nr_desc >= vq->size))
+				     ++nr_desc > vq->size))
 				return -1;
 			desc = &vq->desc[desc->next];