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

[Roland Dreier]

 > 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.