Re: [dpdk-dev] [PATCH] vhost: fix crash if set vring num handling failed

[Tiwei Bie <tiwei.bie@intel.com>]

On Wed, Aug 29, 2018 at 05:00:47PM +0300, Ilya Maximets wrote:
> Any thoughts on this?

Hi Ilya,

Currently, the errors happened during messages handling
aren't handled properly. E.g. in vhost_user_set_mem_table(),
similar issues [1] may happen too. We might want to find
a way to fix this once and for all.

[1]
https://github.com/DPDK/dpdk/blob/3605968c2fa7/lib/librte_vhost/vhost_user.c#L826
https://github.com/DPDK/dpdk/blob/3605968c2fa7/lib/librte_vhost/vhost_user.c#L837
https://github.com/DPDK/dpdk/blob/3605968c2fa7/lib/librte_vhost/vhost_user.c#L887

Thanks

> 
> Best regards, Ilya Maximets.
> 
> On 17.08.2018 14:33, Ilya Maximets wrote:
> > Allocation failures of shadow used ring and batched copy array
> > are not recoverable and leads to the segmentation faults like
> > this on the receiving/transmission path:
> > 
> >   Program received signal SIGSEGV, Segmentation fault.
> >   [Switching to Thread 0x7f913fecf0 (LWP 43625)]
> >   in copy_desc_to_mbuf () at /lib/librte_vhost/virtio_net.c:760
> >   760       batch_copy[vq->batch_copy_nb_elems].dst =
> > 
> > This could be easily reproduced in case of low memory or big
> > number of vhost-user ports. Fix that by propagating error to
> > the upper layer which will end up with disconnection.
> > 
> > Fixes: f689586bc060 ("vhost: shadow used ring update")
> > Cc: stable@dpdk.org
> > 
> > Signed-off-by: Ilya Maximets <i.maximets@samsung.com>
> > ---
> >  lib/librte_vhost/vhost_user.c | 4 +++-
> >  1 file changed, 3 insertions(+), 1 deletion(-)
> > 
> > diff --git a/lib/librte_vhost/vhost_user.c b/lib/librte_vhost/vhost_user.c
> > index 9aa1ce118..4c7fd57fb 100644
> > --- a/lib/librte_vhost/vhost_user.c
> > +++ b/lib/librte_vhost/vhost_user.c
> > @@ -1693,7 +1693,9 @@ vhost_user_msg_handler(int vid, int fd)
> >  		break;
> >  
> >  	case VHOST_USER_SET_VRING_NUM:
> > -		vhost_user_set_vring_num(dev, &msg);
> > +		ret = vhost_user_set_vring_num(dev, &msg);
> > +		if (ret)
> > +			return -1;
> >  		break;
> >  	case VHOST_USER_SET_VRING_ADDR:
> >  		vhost_user_set_vring_addr(&dev, &msg);
> >