Linux Virtualization - Mar 2019 - [PATCH] vsock/virtio: fix kernel panic from virtio_transport_reset_no_sock

On Wed, 6 Mar 2019 08:41:04 +0000, Stefan Hajnoczi <stefanha at gmail.com>
wrote:
> On Tue, Mar 05, 2019 at 08:01:45PM +0200, Adalbert Laz?r wrote:
> 
> Thanks for the patch, Adalbert!  Please add a Signed-off-by tag so your
> patch can be merged (see Documentation/process/submitting-patches.rst
> Chapter 11 for details on the Developer's Certificate of Origin).
> 
> >  static int virtio_transport_reset_no_sock(struct virtio_vsock_pkt
*pkt)
> >  {
> > +	const struct virtio_transport *t;
> >  	struct virtio_vsock_pkt_info info = {
> >  		.op = VIRTIO_VSOCK_OP_RST,
> >  		.type = le16_to_cpu(pkt->hdr.type),
> > @@ -680,7 +681,11 @@ static int virtio_transport_reset_no_sock(struct
virtio_vsock_pkt *pkt)
> >  	if (!pkt)
> >  		return -ENOMEM;
> >  
> > -	return virtio_transport_get_ops()->send_pkt(pkt);
> > +	t = virtio_transport_get_ops();
> > +	if (!t)
> > +		return -ENOTCONN;
> 
> pkt is leaked here.  This is an easy mistake to make because the code is
> unclear. 
Thank you for your kind words :)
> The pkt argument is the received packet that we must reply to.
> The reply packet is allocated just before line 680 and must be free
> explicitly for return -ENOTCONN.
> 
> You can avoid the leak and make the code easier to read like this:
> 
>   struct virtio_vsock_pkt *reply;
> 
>   ...
> 
>      ------ avoid reusing 'pkt'
>     v
>   reply = virtio_transport_alloc_pkt(&info, 0, ...);
>   if (!reply)
>       return -ENOMEM;
> 
>   t = virtio_transport_get_ops();
>   if (!t) {
>       virtio_transport_free_pkt(reply); <-- prevent memory leak
>       return -ENOTCONN;
>   }
>   return t->send_pkt(reply);
What do you think about Stefano's suggestion, to move the check above
the line were the reply is allocated?

On Wed, Mar 06, 2019 at 11:10:41AM +0200, Adalbert Laz?r
wrote:
> On Wed, 6 Mar 2019 08:41:04 +0000, Stefan Hajnoczi <stefanha at
gmail.com> wrote:
> > On Tue, Mar 05, 2019 at 08:01:45PM +0200, Adalbert Laz?r wrote:
> > The pkt argument is the received packet that we must reply to.
> > The reply packet is allocated just before line 680 and must be free
> > explicitly for return -ENOTCONN.
> > 
> > You can avoid the leak and make the code easier to read like this:
> > 
> >   struct virtio_vsock_pkt *reply;
> > 
> >   ...
> > 
> >      ------ avoid reusing 'pkt'
> >     v
> >   reply = virtio_transport_alloc_pkt(&info, 0, ...);
> >   if (!reply)
> >       return -ENOMEM;
> > 
> >   t = virtio_transport_get_ops();
> >   if (!t) {
> >       virtio_transport_free_pkt(reply); <-- prevent memory leak
> >       return -ENOTCONN;
> >   }
> >   return t->send_pkt(reply);
> 
> What do you think about Stefano's suggestion, to move the check above
> the line were the reply is allocated?
That's fine too.

However a follow up patch to eliminate the confusing way that 'pkt' is
reused is still warranted.  If you are busy I'd be happy to send that
cleanup.

Stefan
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 455 bytes
Desc: not available
URL:
<http://lists.linuxfoundation.org/pipermail/virtualization/attachments/20190306/4fe21aaf/attachment.sig>

On Wed, 6 Mar 2019 17:02:16 +0000, Stefan Hajnoczi <stefanha at
redhat.com> wrote:
> On Wed, Mar 06, 2019 at 11:10:41AM +0200, Adalbert Laz?r wrote:
> > On Wed, 6 Mar 2019 08:41:04 +0000, Stefan Hajnoczi <stefanha at
gmail.com> wrote:
> > > On Tue, Mar 05, 2019 at 08:01:45PM +0200, Adalbert Laz?r wrote:
> > > The pkt argument is the received packet that we must reply to.
> > > The reply packet is allocated just before line 680 and must be
free
> > > explicitly for return -ENOTCONN.
> > > 
> > > You can avoid the leak and make the code easier to read like
this:
> > > 
> > >   struct virtio_vsock_pkt *reply;
> > > 
> > >   ...
> > > 
> > >      ------ avoid reusing 'pkt'
> > >     v
> > >   reply = virtio_transport_alloc_pkt(&info, 0, ...);
> > >   if (!reply)
> > >       return -ENOMEM;
> > > 
> > >   t = virtio_transport_get_ops();
> > >   if (!t) {
> > >       virtio_transport_free_pkt(reply); <-- prevent memory
leak
> > >       return -ENOTCONN;
> > >   }
> > >   return t->send_pkt(reply);
> > 
> > What do you think about Stefano's suggestion, to move the check
above
> > the line were the reply is allocated?
> 
> That's fine too.
> 
> However a follow up patch to eliminate the confusing way that 'pkt'
is
> reused is still warranted.  If you are busy I'd be happy to send that
> cleanup.
> 
> Stefan
I've got it, a couple of minutes after I've replied :)
The second version[1] should be in your mailbox.

Thank you,
Adalbert

[1]: https://patchwork.kernel.org/patch/10840787/