On 11/10/2017 06:57 PM, Stefano Stabellini wrote:
On Tue, 7 Nov 2017, Juergen Gross wrote:
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.carpen...@oracle.com>
Signed-off-by: Stefano Stabellini <sstabell...@kernel.org>
CC: boris.ostrov...@oracle.com
CC: jgr...@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()?
Hi Juergen, sorry for the late reply.
Yes, you are right. Given the patch, it would be just the same to use
mutex_lock.
This is where I realized that actually we have a problem: no matter if
we use mutex_lock or mutex_trylock, there are no guarantees that we'll
be the last to take the in/out_mutex. Other waiters could be still
outstanding.
We solved the same problem using a refcount in pvcalls_front_remove. In
this case, I was thinking of reusing the mutex internal counter for
efficiency, instead of adding one more refcount.
For using the mutex as a refcount, there is really no need to call
mutex_trylock or mutex_lock. I suggest checking on the mutex counter
directly:
I think you want to say
while(mutex_locked(&map->active.in_mutex.owner) ||
mutex_locked(&map->active.out_mutex.owner))
cpu_relax();
since owner being NULL is not a documented value of a free mutex.
-boris
while (atomic_long_read(&map->active.in_mutex.owner) != 0UL ||
atomic_long_read(&map->active.out_mutex.owner) != 0UL)
cpu_relax();
Cheers,
Stefano
---
xen/pvcalls: fix potential endless loop in pvcalls-front.c
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 time
you call mutex_trylock() in_mutex is going to fail. It's an endless
loop.
Actually, we don't want to use mutex_trylock at all: we don't need to
take the mutex, we only need to wait until the last mutex waiter/holder
releases it.
Instead of calling mutex_trylock or mutex_lock, just check on the mutex
refcount instead.
Reported-by: Dan Carpenter <dan.carpen...@oracle.com>
Signed-off-by: Stefano Stabellini <sstabell...@kernel.org>
CC: boris.ostrov...@oracle.com
CC: jgr...@suse.com
diff --git a/drivers/xen/pvcalls-front.c b/drivers/xen/pvcalls-front.c
index 0c1ec68..9f33cb8 100644
--- a/drivers/xen/pvcalls-front.c
+++ b/drivers/xen/pvcalls-front.c
@@ -1048,8 +1048,8 @@ 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 (atomic_long_read(&map->active.in_mutex.owner) != 0UL ||
+ atomic_long_read(&map->active.out_mutex.owner) != 0UL)
cpu_relax();
pvcalls_front_free_map(bedata, map);
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel