diff mbox

[v3,4/6] rpmsg: Guard against null endpoint ops in destroy

Message ID 1524783545-21951-5-git-send-email-clew@codeaurora.org (mailing list archive)
State Not Applicable, archived
Delegated to: Andy Gross
Headers show

Commit Message

Chris Lew April 26, 2018, 10:59 p.m. UTC
In RPMSG GLINK the chrdev device will allocate an ept as part of the
rpdev creation. This device will not register endpoint ops even though
it has an allocated ept. Protect against the case where the device is
being destroyed.

Signed-off-by: Chris Lew <clew@codeaurora.org>
---

Changes since v1:
- New change

 drivers/rpmsg/rpmsg_core.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Arnaud POULIQUEN April 30, 2018, 8:36 a.m. UTC | #1
Hello Chris,

On 04/27/2018 12:59 AM, Chris Lew wrote:
> In RPMSG GLINK the chrdev device will allocate an ept as part of the
> rpdev creation. This device will not register endpoint ops even though
> it has an allocated ept. Protect against the case where the device is
> being destroyed.
> 
> Signed-off-by: Chris Lew <clew@codeaurora.org>
> ---
> 
> Changes since v1:
> - New change
> 
>  drivers/rpmsg/rpmsg_core.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/rpmsg/rpmsg_core.c b/drivers/rpmsg/rpmsg_core.c
> index 920a02f0462c..7bfe36afccc5 100644
> --- a/drivers/rpmsg/rpmsg_core.c
> +++ b/drivers/rpmsg/rpmsg_core.c
> @@ -88,7 +88,7 @@ struct rpmsg_endpoint *rpmsg_create_ept(struct rpmsg_device *rpdev,
>   */
>  void rpmsg_destroy_ept(struct rpmsg_endpoint *ept)
>  {
> -	if (ept)
> +	if (ept && ept->ops)
>  		ept->ops->destroy_ept(ept);
>  }
>  EXPORT_SYMBOL(rpmsg_destroy_ept);
> 

Would make sense that you also add test on ept->ops->destroy_ept. I
guess that ops may not be null while destroy_ept pointer is.

Regards
Arnaud
--
To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Arnaud POULIQUEN April 30, 2018, 11:53 a.m. UTC | #2
On 04/30/2018 10:36 AM, Arnaud Pouliquen wrote:
> Hello Chris,
> 
> On 04/27/2018 12:59 AM, Chris Lew wrote:
>> In RPMSG GLINK the chrdev device will allocate an ept as part of the
>> rpdev creation. This device will not register endpoint ops even though
>> it has an allocated ept. Protect against the case where the device is
>> being destroyed.
>>
>> Signed-off-by: Chris Lew <clew@codeaurora.org>
>> ---
>>
>> Changes since v1:
>> - New change
>>
>>  drivers/rpmsg/rpmsg_core.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/rpmsg/rpmsg_core.c b/drivers/rpmsg/rpmsg_core.c
>> index 920a02f0462c..7bfe36afccc5 100644
>> --- a/drivers/rpmsg/rpmsg_core.c
>> +++ b/drivers/rpmsg/rpmsg_core.c
>> @@ -88,7 +88,7 @@ struct rpmsg_endpoint *rpmsg_create_ept(struct rpmsg_device *rpdev,
>>   */
>>  void rpmsg_destroy_ept(struct rpmsg_endpoint *ept)
>>  {
>> -	if (ept)
>> +	if (ept && ept->ops)
>>  		ept->ops->destroy_ept(ept);
>>  }
>>  EXPORT_SYMBOL(rpmsg_destroy_ept);
>>
> 
> Would make sense that you also add test on ept->ops->destroy_ept. I
> guess that ops may not be null while destroy_ept pointer is.

Sorry i cross checked in rpmsg_endpoint_ops. destroy_ept is required. So
my comment is not relevant.

Nevertheless do it make sense to have an endpoint without associated ops?
I don't use rpmsg_char but I tried to figure out how rpmsg_create_ept is
called in rpmsg_char without rpmsg_device ops...
Seems that qcom_glink_create_ept should be called, so  ept->ops should
point to  glink_endpoint_ops struct, right?

Another point concerns the send and try send ops that are also required.
Do you test rpmsg_trysend and/or rpmsg_send function call?
seems that you should face the same issue...

> 
> Regards
> Arnaud
> --
> To unsubscribe from this list: send the line "unsubscribe linux-remoteproc" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/drivers/rpmsg/rpmsg_core.c b/drivers/rpmsg/rpmsg_core.c
index 920a02f0462c..7bfe36afccc5 100644
--- a/drivers/rpmsg/rpmsg_core.c
+++ b/drivers/rpmsg/rpmsg_core.c
@@ -88,7 +88,7 @@  struct rpmsg_endpoint *rpmsg_create_ept(struct rpmsg_device *rpdev,
  */
 void rpmsg_destroy_ept(struct rpmsg_endpoint *ept)
 {
-	if (ept)
+	if (ept && ept->ops)
 		ept->ops->destroy_ept(ept);
 }
 EXPORT_SYMBOL(rpmsg_destroy_ept);