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);