Re: [PATCH RE-RESEND V2 for-next 3/5] IB/core: Call ib_unregister_mad_agent() only for valid agents

[Majd Dibbiny <majd-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>]

On 17/2/2015 3:59 PM, Yann Droneaud wrote:
> Hi,
>
> Le lundi 16 février 2015 à 22:28 +0100, Yann Droneaud a écrit :
>> Le jeudi 05 février 2015 à 13:53 +0200, Or Gerlitz a écrit :
>>> From: Majd Dibbiny <majd-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
>>>
>>> In some error flows, ib_mad_unregister_agent is being invoked also in cases where
>>                       ^^^^^^^^^^^^^^^^^^^^^^^
>>                       ib_unregister_mad_agent()
>>
>>> the ib_mad_register_agent call failed (resulting in an illegal pointer in the
>>      ^^^^^^^^^^^^^^^^^^^^^
>>      ib_register_mad_agent()
>>
>>> agent field).  This causes a kernel crash in the error flow.
>>>
>>> Fix this by calling ib_unregister_mad_agent only for cases where
>>> ib_register_mad_agent succeeded.
>>>
>> The code was checking for struct ib_mad_agent *agent not being NULL, 
>> while ib_register_mad_agent() returns a ERR_PTR() in case of error
>> ... bad :(
>>
> After reading Jason's comments [1], I don't understand the purpose of
> this patch. How can an ERR_PTR() value be present in the arrays ?
>
> A quick review makes me think that mthca and mlx4 error paths lacks NULL
> assignment after calling ib_unregister_mad_agent(). In other words, qib
> might be correct while the others should be fixed.
You are right. Seems like I missed things here.
I'll fix it and send a new version soon.
>
> [1] http://mid.gmane.org/20150205174337.GB31711-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org
>
>>> Signed-off-by: Majd Dibbiny <majd-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
>>> Signed-off-by: Or Gerlitz <ogerlitz-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
>>> ---
>>>  drivers/infiniband/hw/mlx4/mad.c        |    2 +-
>>>  drivers/infiniband/hw/mthca/mthca_mad.c |    2 +-
>>>  drivers/infiniband/hw/qib/qib_mad.c     |    2 +-
>>>  3 files changed, 3 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/drivers/infiniband/hw/mlx4/mad.c b/drivers/infiniband/hw/mlx4/mad.c
>>> index 82a7dd8..6be0d2c 100644
>>> --- a/drivers/infiniband/hw/mlx4/mad.c
>>> +++ b/drivers/infiniband/hw/mlx4/mad.c
>>> @@ -907,7 +907,7 @@ int mlx4_ib_mad_init(struct mlx4_ib_dev *dev)
>>>  err:
>>>  	for (p = 0; p < dev->num_ports; ++p)
>>>  		for (q = 0; q <= 1; ++q)
>>> -			if (dev->send_agent[p][q])
>>> +			if (!IS_ERR(dev->send_agent[p][q]))
>>>  				ib_unregister_mad_agent(dev->send_agent[p][q]);
>>>  
>>>  	return ret;
>>> diff --git a/drivers/infiniband/hw/mthca/mthca_mad.c b/drivers/infiniband/hw/mthca/mthca_mad.c
>>> index 8881fa3..5f1a7ce 100644
>>> --- a/drivers/infiniband/hw/mthca/mthca_mad.c
>>> +++ b/drivers/infiniband/hw/mthca/mthca_mad.c
>>> @@ -317,7 +317,7 @@ int mthca_create_agents(struct mthca_dev *dev)
>>>  err:
>>>  	for (p = 0; p < dev->limits.num_ports; ++p)
>>>  		for (q = 0; q <= 1; ++q)
>>> -			if (dev->send_agent[p][q])
>>> +			if (!IS_ERR(dev->send_agent[p][q]))
>>>  				ib_unregister_mad_agent(dev->send_agent[p][q]);
>>>  
>>>  	return ret;
>>> diff --git a/drivers/infiniband/hw/qib/qib_mad.c b/drivers/infiniband/hw/qib/qib_mad.c
>>> index 636be11..6c7cc80 100644
>>> --- a/drivers/infiniband/hw/qib/qib_mad.c
>>> +++ b/drivers/infiniband/hw/qib/qib_mad.c
>>> @@ -2499,7 +2499,7 @@ int qib_create_agents(struct qib_ibdev *dev)
>>>  err:
>>>  	for (p = 0; p < dd->num_pports; p++) {
>>>  		ibp = &dd->pport[p].ibport_data;
>>> -		if (ibp->send_agent) {
>>> +		if (!IS_ERR(ibp->send_agent)) {
>>>  			agent = ibp->send_agent;
>>>  			ibp->send_agent = NULL;
>>>  			ib_unregister_mad_agent(agent);
> Regards.
>

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html