Recursion in non-recursive mutex when using the grant table free callbacks

[Akshay Jaggi]

Roger would be a better person to answer this and I would wait for him to
respond, but here are my 2 cents.

First let's finalize and record the semantics of the callbacks. What
guarantee do we want to provide callbacks about locks held (no locks held
OR x,y,z locks held)? This is mostly a product decision. Usually callbacks
are called with no locks held.
Post this problem is simple. If the list_lock would be held during
callbacks, we need to provide a lock-free function (which assumes lock
would be held before function is called and use that) and if the lock would
not be held we should fix the locking to ensure the case you mentioned
should not happen.

WDYT?

Regards,
Akshay

On Sun, 29 Jul 2018 at 14:43, Pratyush Yadav <pratyush at freebsd.org> wrote:

> Hi,
>
> Currently, the grant table free callbacks can not work. This is
> because of a recursion on a non-recursive mutex that causes a kernel
> panic. The cause of the recursion is: check_free_callbacks() is always
> called with the lock gnttab_list_lock held. So, the callback function
> is called with the lock held. So, when the client uses any of the
> grant reference allocation methods get_free_entries() is called, which
> tries to acquire gnttab_list_lock(grant_table.c:77 [0]), causing a
> recursion on the lock.
>
> I'm not sure what the correct fix would be though. One way I can think
> of is that check_free_callback() should be called without the lock
> held. But with this fix, it is possible for the callback to be called
> even though the grant references it needs are not available. This
> would happen when another thread takes those references while the
> current thread has completed the check if(gnttab_free_count >=
> callback->count) but has not yet called the callback
> (grant_table,c:105 [1]).
>
> I think a better way to fix this would be to have a check in
> get_free_entries() whether the current thread holds the lock, so it
> does not try to acquire the lock if the current thread already holds
> it.
>
> If you agree, I will send a patch.
>
>
>
> [0]
> https://github.com/freebsd/freebsd/blob/master/sys/dev/xen/grant_table/grant_table.c#L77
> [1]
> https://github.com/freebsd/freebsd/blob/master/sys/dev/xen/grant_table/grant_table.c#L105
>
> --
> Regards,
> Pratyush Yadav
>