[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH] xen/pvcalls: fix potential endless loop in pvcalls-front.c
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.carpenter@xxxxxxxxxx> Signed-off-by: Stefano Stabellini <sstabellini@xxxxxxxxxx> CC: boris.ostrovsky@xxxxxxxxxx CC: jgross@xxxxxxxx --- 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.carpenter@xxxxxxxxxx> Signed-off-by: Stefano Stabellini <sstabellini@xxxxxxxxxx> CC: boris.ostrovsky@xxxxxxxxxx CC: jgross@xxxxxxxx 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@xxxxxxxxxxxxx https://lists.xen.org/xen-devel
|
![]() |
Lists.xenproject.org is hosted with RackSpace, monitoring our |