Need a second set of eyeballs for a possible startup race condition in vc04_services/vchiq.

[Michael Zoran]

Resending to a larger e-mail list...

On Mon, 2017-01-30 at 04:57 -0800, Michael Zoran wrote:
> I'm looking at linux-next:
> drivers/staging/vc04_services/interface/vchiq_arm/vchiq_connected.c
> 
> First it looks this is some kind of startup notification mechanism
> and
> it is used by the custom RPI kernel on www.github.com. Client drivers
> call vchiq_add_connected_callback to register for a notification that
> the driver is good to go.
> 
> But I see a few interesting issues:
> 
> 1. vchiq_add_connected_callback can be called by multiple clients at
> the same time.  If this happens they will all find that g_once_init
> isn't set, so all the clients will call mutex_init at the same time. 
> That's sounds like a possible corruption issue. 
> 
> 2. On a multiprocessor machine, I didn't think it was OK to simply
> set
> g_once_init without any kind of protection either since it may not
> propagate to the other cores.  You know some kind of barrier
> instruction.
> 
> 3. A change was made in checkin
> 826d73b3024485677163253b59ef9bd187ff765.
> 
> This changed the function mutex_lock_interruptable which was at that
> time a macro to a custom function called
> mutex_lock_interruptable_killable to simply mutex_lock_killable. The
> old function does a dance with setting the process signal masks, but
> mutex_lock_killable doesn't.
> 
> Like I said, I'm new to trying to contribute to the linux kernel. But
> I'm not sure the two are completely a drop in replacement. I was
> wondering if the change is doing the correct thing?
> 
> It looks a fix would be easy to implement just by adding an atomic
> exchange instead of a simple test and set.
> 
> Thoughts?
> 
> 
> drivers/staging/vc04_services/interface/vchiq_arm/vchiq_connected.c
> 
> static void connected_init(void)
> {
>         if (!g_once_init) {
>                 mutex_init(&g_connected_mutex);
>                 g_once_init = 1;
>         }
> }
> 
> /********************************************************************
> **
> ******
> *
> * This function is used to defer initialization until the vchiq stack
> is
> * initialized. If the stack is already initialized, then the callback
> will
> * be made immediately, otherwise it will be deferred until
> * vchiq_call_connected_callbacks is called.
> *
> *********************************************************************
> **
> ****/
> 
> void vchiq_add_connected_callback(VCHIQ_CONNECTED_CALLBACK_T
> callback)
> {
>         connected_init();
> 
>         if (mutex_lock_killable(&g_connected_mutex) != 0)
>                 return;
> 
>         if (g_connected)
>                 /* We're already connected. Call the callback
> immediately. */
> 
>                 callback();
>         else {
>                 if (g_num_deferred_callbacks >= MAX_CALLBACKS)
>                         vchiq_log_error(vchiq_core_log_level,
>                                 "There already %d callback registered
> -
> "
>                                 "please increase MAX_CALLBACKS",
>                                 g_num_deferred_callbacks);
>                 else {
>                         g_deferred_callback[g_num_deferred_callbacks]
> =
>                                 callback;
>                         g_num_deferred_callbacks++;
>                 }
>         }
>         mutex_unlock(&g_connected_mutex);
> }
> 
> /********************************************************************
> **
> ******
> *
> * This function is called by the vchiq stack once it has been
> connected
> to
> * the videocore and clients can start to use the stack.
> *
> *********************************************************************
> **
> ****/
> 
> void vchiq_call_connected_callbacks(void)
> {
>         int i;
> 
>         connected_init();
> 
>         if (mutex_lock_killable(&g_connected_mutex) != 0)
>                 return;
> 
>         for (i = 0; i <  g_num_deferred_callbacks; i++)
>                 g_deferred_callback[i]();
> 
>         g_num_deferred_callbacks = 0;
>         g_connected = 1;
>         mutex_unlock(&g_connected_mutex);
> }
> EXPORT_SYMBOL(vchiq_add_connected_callback);