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

classic Classic list List threaded Threaded
4 messages Options
Reply | Threaded
Open this post in threaded view
|

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

Pratyush Yadav-2
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
_______________________________________________
[hidden email] mailing list
https://lists.freebsd.org/mailman/listinfo/freebsd-xen
To unsubscribe, send any mail to "[hidden email]"
Reply | Threaded
Open this post in threaded view
|

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

Akshay Jaggi-2
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 <[hidden email]> 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
>
_______________________________________________
[hidden email] mailing list
https://lists.freebsd.org/mailman/listinfo/freebsd-xen
To unsubscribe, send any mail to "[hidden email]"
Reply | Threaded
Open this post in threaded view
|

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

Roger Pau Monné-2
In reply to this post by Pratyush Yadav-2
On Sun, Jul 29, 2018 at 06:08:56PM +0530, Pratyush Yadav 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.

I agree in the analysis, however I think the proper solution is to use
a recursive lock.

Roger.
_______________________________________________
[hidden email] mailing list
https://lists.freebsd.org/mailman/listinfo/freebsd-xen
To unsubscribe, send any mail to "[hidden email]"
Reply | Threaded
Open this post in threaded view
|

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

Pratyush Yadav-2
In reply to this post by Akshay Jaggi-2
On Mon, Jul 30, 2018 at 4:00 AM Akshay Jaggi <[hidden email]> wrote:
>
> 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?

The guarantee we want to provide the callback is that the allocation
of grant references can not fail inside the callback (unless they try
to allocate more refs than initially specified). This means that we
can't call the callback without the lock held otherwise during the
execution of the callback it is possible for some other thread to get
the grant references, and the grant refs the callbacks needs would not
be available. So a change in the locking has to be done.

On Mon, Jul 30, 2018 at 3:02 PM Roger Pau Monné <[hidden email]> wrote:
> I agree in the analysis, however I think the proper solution is to use
> a recursive lock.

All right. I'll send in the patch.

--
Regards,
Pratyush Yadav
_______________________________________________
[hidden email] mailing list
https://lists.freebsd.org/mailman/listinfo/freebsd-xen
To unsubscribe, send any mail to "[hidden email]"