[ofa-general] [PATCH] IB/core - ib_sa_remove_one() can panic due to NULL pointer dereference

[Ralph Campbell]

Yes, I have seen it when update_sm_ah() calls ib_create_ah() which
fails and leaves sm_ah == NULL. Granted that ib_create_ah() rarely
fails though.

On Wed, 2008-07-16 at 17:46 -0700, Roland Dreier wrote:
> > If an HCA does not get a SM LID, ib_sa_remove_one() can cause a NULL
>  > pointer bug when it calls kref_put().  This is easy to see since
>  > ib_sa_add_one() sets sm_ah to NULL.
> 
> Have you seen this in practice?  Because ib_sa_add_one() calls
> update_sm_ah() before returning, so ib_sa_remove_one() shouldn't see a
> NULL sm_ah because of that.  And ib_sa_event() schedules the update task
> after setting sm_ah to NULL, and ib_sa_remove_one() does
> flush_scheduled_work() so update_sm_ah() should be guaranteed to run in
> that case too.
> 
> I do see the possibility of hitting one of the failures in
> update_sm_ah() and ending up not setting sm_ah, so your patch is
> correct, but I'm just wondering how you hit this in practice.
> 
> I also see what I guess is techically a bug in ib_sa_add_one(), namely
> that it does
> 
> 	ib_set_client_data(device, &sa_client, sa_dev);
> 
> before
> 
> 	INIT_IB_EVENT_HANDLER(&sa_dev->event_handler, device, ib_sa_event);
> 	if (ib_register_event_handler(&sa_dev->event_handler))
> 		goto err;
> 
> and if that function returns an error, then sa_dev gets freed and
> ib_sa_remove_one() ends up doing a use-after-free.  However with the
> current code, ib_register_event_handler() can never return an error.
> 
> I wonder if the best fix is just to make ib_register_event_handler()
> a void function.
> 
>  - R.