diff mbox

9pfs: fix migration_block leak

Message ID 1490876834-61392-1-git-send-email-liqiang6-s@360.cn (mailing list archive)
State New, archived
Headers show

Commit Message

Li Qiang March 30, 2017, 12:27 p.m. UTC
The guest can leave the pdu->s->migration_blocker exists by attach
but not remove a fid. Then if we hot unplug the 9pfs device, the
v9fs_reset() just free the fids, but not free the migration_blocker.
This will leak a memory leak. This patch avoid this.

Signed-off-by: Li Qiang <liqiang6-s@360.cn>
---
 hw/9pfs/9p.c | 6 ++++++
 1 file changed, 6 insertions(+)

Comments

Eric Blake March 30, 2017, 1:25 p.m. UTC | #1
On 03/30/2017 07:27 AM, Li Qiang wrote:
> The guest can leave the pdu->s->migration_blocker exists by attach

s/exists/in place/
s/attach/attaching/

> but not remove a fid. Then if we hot unplug the 9pfs device, the

s/remove/removing/

> v9fs_reset() just free the fids, but not free the migration_blocker.
> This will leak a memory leak. This patch avoid this.

s/leak a/cause a/
s/avoid/avoids/

> 
> Signed-off-by: Li Qiang <liqiang6-s@360.cn>
> ---
>  hw/9pfs/9p.c | 6 ++++++
>  1 file changed, 6 insertions(+)

Probably worth including in 2.9 as a bug fix.

Reviewed-by: Eric Blake <eblake@redhat.com>

> 
> diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c
> index 48babce..b55c02d 100644
> --- a/hw/9pfs/9p.c
> +++ b/hw/9pfs/9p.c
> @@ -548,6 +548,12 @@ static void coroutine_fn virtfs_reset(V9fsPDU *pdu)
>              free_fid(pdu, fidp);
>          }
>      }
> +
> +    if (pdu->s->migration_blocker) {
> +        migrate_del_blocker(pdu->s->migration_blocker);
> +        error_free(pdu->s->migration_blocker);
> +        pdu->s->migration_blocker = NULL;
> +    }
>  }
>  
>  #define P9_QID_TYPE_DIR         0x80
>
Greg Kurz March 30, 2017, 3:46 p.m. UTC | #2
On Thu, 30 Mar 2017 08:25:25 -0500
Eric Blake <eblake@redhat.com> wrote:

> On 03/30/2017 07:27 AM, Li Qiang wrote:
> > The guest can leave the pdu->s->migration_blocker exists by attach  
> 
> s/exists/in place/
> s/attach/attaching/
> 
> > but not remove a fid. Then if we hot unplug the 9pfs device, the  
> 

In theory you're right, but the current 9p client in linux won't let you hot
unplug the device unless you unmount the 9p share first, hence freeing the
blocker.

> s/remove/removing/
> 
> > v9fs_reset() just free the fids, but not free the migration_blocker.
> > This will leak a memory leak. This patch avoid this.  

I had a similar issue sitting my TODO list for quite a time: the blocker
survives a system_reset. It doesn't cause a memory leak but it prevents
migration until the guest mounts/unmounts the 9p share again.

This boils down to virtfs_reset() calling free_fid() instead of put_fid() IIRC.

> 
> s/leak a/cause a/
> s/avoid/avoids/
> 
> > 
> > Signed-off-by: Li Qiang <liqiang6-s@360.cn>
> > ---
> >  hw/9pfs/9p.c | 6 ++++++
> >  1 file changed, 6 insertions(+)  
> 
> Probably worth including in 2.9 as a bug fix.
> 
> Reviewed-by: Eric Blake <eblake@redhat.com>
> 
> > 
> > diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c
> > index 48babce..b55c02d 100644
> > --- a/hw/9pfs/9p.c
> > +++ b/hw/9pfs/9p.c
> > @@ -548,6 +548,12 @@ static void coroutine_fn virtfs_reset(V9fsPDU *pdu)
> >              free_fid(pdu, fidp);
> >          }
> >      }
> > +
> > +    if (pdu->s->migration_blocker) {
> > +        migrate_del_blocker(pdu->s->migration_blocker);
> > +        error_free(pdu->s->migration_blocker);
> > +        pdu->s->migration_blocker = NULL;
> > +    }

I'd prefer to drain all PDUs in virtfs_reset() and have the loop above
to call put_fid() instead of free_fid(). If this isn't doable for 2.9,
I'll apply this patch with a comment.

> >  }
> >  
> >  #define P9_QID_TYPE_DIR         0x80
> >   
>
diff mbox

Patch

diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c
index 48babce..b55c02d 100644
--- a/hw/9pfs/9p.c
+++ b/hw/9pfs/9p.c
@@ -548,6 +548,12 @@  static void coroutine_fn virtfs_reset(V9fsPDU *pdu)
             free_fid(pdu, fidp);
         }
     }
+
+    if (pdu->s->migration_blocker) {
+        migrate_del_blocker(pdu->s->migration_blocker);
+        error_free(pdu->s->migration_blocker);
+        pdu->s->migration_blocker = NULL;
+    }
 }
 
 #define P9_QID_TYPE_DIR         0x80