| Message ID | 1510006679-6381-1-git-send-email-sstabellini@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 11/06/2017 05:17 PM, Stefano Stabellini wrote: > mutex_trylock() returns 1 if you take the lock and 0 if not. Assume you > take in_mutex on the first try, but you can't take out_mutex. Next times > you call mutex_trylock() in_mutex is going to fail. It's an endless > loop. > > Solve the problem by moving the two mutex_trylock calls to two separate > loops. > > Reported-by: Dan Carpenter <dan.carpenter@oracle.com> > Signed-off-by: Stefano Stabellini <sstabellini@kernel.org> > CC: boris.ostrovsky@oracle.com > CC: jgross@suse.com Reviewed-by: Boris Ostrovsky <boris.ostrovsky@oracle.com> BTW, I assume that when recvmsg or sendmsg is called no other locks are held, right? (otherwise we'd get into a deadlock.) -boris > --- > drivers/xen/pvcalls-front.c | 5 +++-- > 1 file changed, 3 insertions(+), 2 deletions(-) > > diff --git a/drivers/xen/pvcalls-front.c b/drivers/xen/pvcalls-front.c > index 0c1ec68..047dce7 100644 > --- a/drivers/xen/pvcalls-front.c > +++ b/drivers/xen/pvcalls-front.c > @@ -1048,8 +1048,9 @@ int pvcalls_front_release(struct socket *sock) > * is set to NULL -- we only need to wait for the existing > * waiters to return. > */ > - while (!mutex_trylock(&map->active.in_mutex) || > - !mutex_trylock(&map->active.out_mutex)) > + while (!mutex_trylock(&map->active.in_mutex)) > + cpu_relax(); > + while (!mutex_trylock(&map->active.out_mutex)) > cpu_relax(); > > pvcalls_front_free_map(bedata, map);
On Mon, 6 Nov 2017, Boris Ostrovsky wrote: > On 11/06/2017 05:17 PM, Stefano Stabellini wrote: > > mutex_trylock() returns 1 if you take the lock and 0 if not. Assume you > > take in_mutex on the first try, but you can't take out_mutex. Next times > > you call mutex_trylock() in_mutex is going to fail. It's an endless > > loop. > > > > Solve the problem by moving the two mutex_trylock calls to two separate > > loops. > > > > Reported-by: Dan Carpenter <dan.carpenter@oracle.com> > > Signed-off-by: Stefano Stabellini <sstabellini@kernel.org> > > CC: boris.ostrovsky@oracle.com > > CC: jgross@suse.com > > Reviewed-by: Boris Ostrovsky <boris.ostrovsky@oracle.com> > > BTW, I assume that when recvmsg or sendmsg is called no other locks are > held, right? (otherwise we'd get into a deadlock.) Yes, but most importantly the other assumption is that recvmsg and sendmsg are already running: the partially visible comment explains it: * Wait until there are no more waiters on the mutexes. * We know that no new waiters can be added because sk_send_head * is set to NULL -- we only need to wait for the existing * waiters to return. > > --- > > drivers/xen/pvcalls-front.c | 5 +++-- > > 1 file changed, 3 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/xen/pvcalls-front.c b/drivers/xen/pvcalls-front.c > > index 0c1ec68..047dce7 100644 > > --- a/drivers/xen/pvcalls-front.c > > +++ b/drivers/xen/pvcalls-front.c > > @@ -1048,8 +1048,9 @@ int pvcalls_front_release(struct socket *sock) > > * is set to NULL -- we only need to wait for the existing > > * waiters to return. > > */ > > - while (!mutex_trylock(&map->active.in_mutex) || > > - !mutex_trylock(&map->active.out_mutex)) > > + while (!mutex_trylock(&map->active.in_mutex)) > > + cpu_relax(); > > + while (!mutex_trylock(&map->active.out_mutex)) > > cpu_relax(); > > > > pvcalls_front_free_map(bedata, map); >
On 06/11/17 23:17, Stefano Stabellini wrote: > mutex_trylock() returns 1 if you take the lock and 0 if not. Assume you > take in_mutex on the first try, but you can't take out_mutex. Next times > you call mutex_trylock() in_mutex is going to fail. It's an endless > loop. > > Solve the problem by moving the two mutex_trylock calls to two separate > loops. > > Reported-by: Dan Carpenter <dan.carpenter@oracle.com> > Signed-off-by: Stefano Stabellini <sstabellini@kernel.org> > CC: boris.ostrovsky@oracle.com > CC: jgross@suse.com > --- > drivers/xen/pvcalls-front.c | 5 +++-- > 1 file changed, 3 insertions(+), 2 deletions(-) > > diff --git a/drivers/xen/pvcalls-front.c b/drivers/xen/pvcalls-front.c > index 0c1ec68..047dce7 100644 > --- a/drivers/xen/pvcalls-front.c > +++ b/drivers/xen/pvcalls-front.c > @@ -1048,8 +1048,9 @@ int pvcalls_front_release(struct socket *sock) > * is set to NULL -- we only need to wait for the existing > * waiters to return. > */ > - while (!mutex_trylock(&map->active.in_mutex) || > - !mutex_trylock(&map->active.out_mutex)) > + while (!mutex_trylock(&map->active.in_mutex)) > + cpu_relax(); > + while (!mutex_trylock(&map->active.out_mutex)) > cpu_relax(); Any reason you don't just use mutex_lock()? Juergen
diff --git a/drivers/xen/pvcalls-front.c b/drivers/xen/pvcalls-front.c index 0c1ec68..047dce7 100644 --- a/drivers/xen/pvcalls-front.c +++ b/drivers/xen/pvcalls-front.c @@ -1048,8 +1048,9 @@ int pvcalls_front_release(struct socket *sock) * is set to NULL -- we only need to wait for the existing * waiters to return. */ - while (!mutex_trylock(&map->active.in_mutex) || - !mutex_trylock(&map->active.out_mutex)) + while (!mutex_trylock(&map->active.in_mutex)) + cpu_relax(); + while (!mutex_trylock(&map->active.out_mutex)) cpu_relax(); pvcalls_front_free_map(bedata, map);
mutex_trylock() returns 1 if you take the lock and 0 if not. Assume you take in_mutex on the first try, but you can't take out_mutex. Next times you call mutex_trylock() in_mutex is going to fail. It's an endless loop. Solve the problem by moving the two mutex_trylock calls to two separate loops. Reported-by: Dan Carpenter <dan.carpenter@oracle.com> Signed-off-by: Stefano Stabellini <sstabellini@kernel.org> CC: boris.ostrovsky@oracle.com CC: jgross@suse.com --- drivers/xen/pvcalls-front.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)