Re: [Xen-devel] [PATCH] xen/pvcalls: fix potential endless loop in pvcalls-front.c

[Boris Ostrovsky <boris.ostrovsky@xxxxxxxxxx>]

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