Re: [PATCH] 9pfs: Fix potential deadlock of QEMU mainloop

[Christian Schoenebeck <qemu_oss@crudebyte.com>]

On Donnerstag, 7. Mai 2020 16:33:28 CEST Greg Kurz wrote:
> > I also haven't reviewed QEMU's lock implementations in very detail, but
> > IIRC CoMutexes are completely handled in user space, while QemuMutex uses
> > regular OS mutexes and hence might cost context switches.
> 
> ... since the locking would only been exercised with an hypothetical
> client doing stupid things, this is beginning to look like bike-shedding
> to me. :)

Aha, keep that in mind when you're doing your next review. ;-)

No seriously, like I said, I don't really care too much about Mutex vs. 
CoMutex in you patch here. It was actually more about wide-picture thinking, 
i.e. other places of (co)mutexes being used or other potential changes that 
would make this or other uses more relevant one day.

> > > > > diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c
> > > > > index 9e046f7acb51..ac84ae804496 100644
> > > > > --- a/hw/9pfs/9p.c
> > > > > +++ b/hw/9pfs/9p.c
> > > > > @@ -2170,7 +2170,7 @@ static int coroutine_fn
> > > > > v9fs_do_readdir_with_stat(V9fsPDU *pdu, int32_t count = 0;
> > > > > 
> > > > >      struct stat stbuf;
> > > > >      off_t saved_dir_pos;
> > > > > 
> > > > > -    struct dirent *dent;
> > > > > +    struct dirent dent;
> > > > > 
> > > > >      /* save the directory position */
> > > > >      saved_dir_pos = v9fs_co_telldir(pdu, fidp);
> > > > > 
> > > > > @@ -2181,13 +2181,11 @@ static int coroutine_fn
> > > > > v9fs_do_readdir_with_stat(V9fsPDU *pdu, while (1) {
> > > > > 
> > > > >          v9fs_path_init(&path);
> > > > > 
> > > > > -        v9fs_readdir_lock(&fidp->fs.dir);
> > > > > -
> > > > 
> > > > That's the deadlock fix, but ...
> > > > 
> > > > >          err = v9fs_co_readdir(pdu, fidp, &dent);
> > > > > 
> > > > > -        if (err || !dent) {
> > > > > +        if (err <= 0) {
> > > > > 
> > > > >              break;
> > > > >          
> > > > >          }
> > > > 
> > > > ... even though this code simplification might make sense, I don't
> > > > think
> > > > it
> > > > should be mixed with the deadlock fix together in one patch. They are
> > > > not
> > > 
> > > I could possibly split this in two patches, one for returning a copy
> > > and one for moving the locking around, but...
> > > 
> > > > related with each other, nor is the code simplification you are aiming
> > > > trivial
> > > 
> > > ... this assertion is somewhat wrong: moving the locking to
> > > v9fs_co_readdir() really requires it returns a copy.
> > 
> > Yeah, I am also not sure whether a split would make it more trivial enough
> > in this case to be worth the hassle. If you find an acceptable solution,
> > good, if not then leave it one patch.
> 
> Another option would be to g_malloc() the dirent in v9fs_co_readdir() and
> g_free() in the callers. This would cause less churn since we could keep
> the same function signature.

I was actually just going to suggest the same. So yes, looks like a less 
invasive change to me.

Best regards,
Christian Schoenebeck