[patch] ib/rxe: double free on error

[patch] ib/rxe: double free on error

[Dan Carpenter]

"goto err1" could probably be remained "goto free_pkey_tbl" since
that's what it does.  This is a double free.

Fixes: 0784481b2f32 ('Add initialization for Soft RoCE driver, pools constants etc.')
Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>

diff --git a/drivers/infiniband/hw/rxe/rxe.c b/drivers/infiniband/hw/rxe/rxe.c
index 48c41e00..50e25b0 100644
--- a/drivers/infiniband/hw/rxe/rxe.c
+++ b/drivers/infiniband/hw/rxe/rxe.c
@@ -187,7 +187,6 @@ static int rxe_init_ports(struct rxe_dev *rxe)
 	port->pkey_tbl[0] = 0xffff;
 
 	if (!port->attr.gid_tbl_len) {
-		kfree(port->pkey_tbl);
 		err = -EINVAL;
 		goto err1;
 	}

Re: [patch] ib/rxe: double free on error

[Leon Romanovsky]

[-- Attachment #1.1: Type: text/plain, Size: 433 bytes --]

On Sat, Jun 18, 2016 at 11:40:21AM +0300, Dan Carpenter wrote:
> "goto err1" could probably be remained "goto free_pkey_tbl" since
> that's what it does.  This is a double free.
> 
> Fixes: 0784481b2f32 ('Add initialization for Soft RoCE driver, pools constants etc.')
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>

Hi Dan,

Thank you for pointing it out.

I rewrote your patch a little bit and applied it.


[-- Attachment #1.2: 0001-IB-rxe-Simplify-rxe_init_ports-logic.patch --]
[-- Type: text/x-diff, Size: 1805 bytes --]

From 6a320576c7304905df722afcf1b8d49242c8ae48 Mon Sep 17 00:00:00 2001
From: Dan Carpenter <dan.carpenter@oracle.com>
Date: Sat, 18 Jun 2016 11:40:21 +0300
Subject: [PATCH] IB/rxe: Simplify rxe_init_ports logic

Simplify rxe_init_ports and remove double free.

Fixes: 0784481b2f32 ('Add initialization for Soft RoCE driver, pools constants etc.')
Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
Signed-off-by: Leon Romanovsky <leon@kernel.org>
---
 drivers/infiniband/hw/rxe/rxe.c | 29 ++++++-----------------------
 1 file changed, 6 insertions(+), 23 deletions(-)

diff --git a/drivers/infiniband/hw/rxe/rxe.c b/drivers/infiniband/hw/rxe/rxe.c
index 48c41e00..156a1021 100644
--- a/drivers/infiniband/hw/rxe/rxe.c
+++ b/drivers/infiniband/hw/rxe/rxe.c
@@ -165,42 +165,25 @@ static int rxe_init_port_param(struct rxe_port *port)
  */
 static int rxe_init_ports(struct rxe_dev *rxe)
 {
-	int err;
-	struct rxe_port *port;
-
-	port = &rxe->port;
+	struct rxe_port *port =  rxe->port;
 
 	rxe_init_port_param(port);
 
-	if (!port->attr.pkey_tbl_len) {
-		err = -EINVAL;
-		goto err1;
-	}
+	if (!port->attr.pkey_tbl_len || !port->attr.gid_tbl_len)
+		return -EINVAL;
 
 	port->pkey_tbl = kcalloc(port->attr.pkey_tbl_len,
 			sizeof(*port->pkey_tbl), GFP_KERNEL);
-	if (!port->pkey_tbl) {
-		err = -ENOMEM;
-		goto err1;
-	}
-
-	port->pkey_tbl[0] = 0xffff;
 
-	if (!port->attr.gid_tbl_len) {
-		kfree(port->pkey_tbl);
-		err = -EINVAL;
-		goto err1;
-	}
+	if (!port->pkey_tbl)
+		return -ENOMEM;
 
+	port->pkey_tbl[0] = 0xffff;
 	port->port_guid = rxe->ifc_ops->port_guid(rxe);
 
 	spin_lock_init(&port->port_lock);
 
 	return 0;
-
-err1:
-	kfree(port->pkey_tbl);
-	return err;
 }
 
 /* init pools of managed objects */
-- 
2.1.4


[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [patch] ib/rxe: double free on error

[Dan Carpenter]

On Sat, Jun 18, 2016 at 01:34:30PM +0300, Leon Romanovsky wrote:
>  drivers/infiniband/hw/rxe/rxe.c | 29 ++++++-----------------------
>  1 file changed, 6 insertions(+), 23 deletions(-)
> 
> diff --git a/drivers/infiniband/hw/rxe/rxe.c b/drivers/infiniband/hw/rxe/rxe.c
> index 48c41e00..156a1021 100644
> --- a/drivers/infiniband/hw/rxe/rxe.c
> +++ b/drivers/infiniband/hw/rxe/rxe.c
> @@ -165,42 +165,25 @@ static int rxe_init_port_param(struct rxe_port *port)
>   */
>  static int rxe_init_ports(struct rxe_dev *rxe)
>  {
> -	int err;
> -	struct rxe_port *port;
> -
> -	port = &rxe->port;
> +	struct rxe_port *port =  rxe->port;

This won't compile.

Just give me Reported-by credit not authorship.

regards,
dan carpenter

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

Re: [patch] ib/rxe: double free on error

[Leon Romanovsky]

[-- Attachment #1: Type: text/plain, Size: 924 bytes --]

On Sat, Jun 18, 2016 at 02:00:03PM +0300, Dan Carpenter wrote:
> On Sat, Jun 18, 2016 at 01:34:30PM +0300, Leon Romanovsky wrote:
> >  drivers/infiniband/hw/rxe/rxe.c | 29 ++++++-----------------------
> >  1 file changed, 6 insertions(+), 23 deletions(-)
> > 
> > diff --git a/drivers/infiniband/hw/rxe/rxe.c b/drivers/infiniband/hw/rxe/rxe.c
> > index 48c41e00..156a1021 100644
> > --- a/drivers/infiniband/hw/rxe/rxe.c
> > +++ b/drivers/infiniband/hw/rxe/rxe.c
> > @@ -165,42 +165,25 @@ static int rxe_init_port_param(struct rxe_port *port)
> >   */
> >  static int rxe_init_ports(struct rxe_dev *rxe)
> >  {
> > -	int err;
> > -	struct rxe_port *port;
> > -
> > -	port = &rxe->port;
> > +	struct rxe_port *port =  rxe->port;
> 
> This won't compile.

Interesting, it compiled locally. I'll check it now.

> 
> Just give me Reported-by credit not authorship.
> 
> regards,
> dan carpenter
> 

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [patch] ib/rxe: double free on error

[Leon Romanovsky]

[-- Attachment #1: Type: text/plain, Size: 1102 bytes --]

On Sat, Jun 18, 2016 at 02:03:30PM +0300, Leon Romanovsky wrote:
> On Sat, Jun 18, 2016 at 02:00:03PM +0300, Dan Carpenter wrote:
> > On Sat, Jun 18, 2016 at 01:34:30PM +0300, Leon Romanovsky wrote:
> > >  drivers/infiniband/hw/rxe/rxe.c | 29 ++++++-----------------------
> > >  1 file changed, 6 insertions(+), 23 deletions(-)
> > > 
> > > diff --git a/drivers/infiniband/hw/rxe/rxe.c b/drivers/infiniband/hw/rxe/rxe.c
> > > index 48c41e00..156a1021 100644
> > > --- a/drivers/infiniband/hw/rxe/rxe.c
> > > +++ b/drivers/infiniband/hw/rxe/rxe.c
> > > @@ -165,42 +165,25 @@ static int rxe_init_port_param(struct rxe_port *port)
> > >   */
> > >  static int rxe_init_ports(struct rxe_dev *rxe)
> > >  {
> > > -	int err;
> > > -	struct rxe_port *port;
> > > -
> > > -	port = &rxe->port;
> > > +	struct rxe_port *port =  rxe->port;
> > 
> > This won't compile.
> 
> Interesting, it compiled locally. I'll check it now.

Thanks, I accidentally pushed wrong version.

> 
> > 
> > Just give me Reported-by credit not authorship.
> > 
> > regards,
> > dan carpenter
> > 



[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH] IB/rxe: Simplify rxe_init_ports logic

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 1422 bytes --]

Hi,

[auto build test ERROR on next-20160617]
[cannot apply to rdma/master v4.7-rc3 v4.7-rc2 v4.7-rc1 v4.7-rc3]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Leon-Romanovsky/IB-rxe-Simplify-rxe_init_ports-logic/20160618-183558
config: x86_64-allmodconfig (attached as .config)
compiler: gcc-6 (Debian 6.1.1-1) 6.1.1 20160430
reproduce:
        # save the attached .config to linux build tree
        make ARCH=x86_64 

All errors (new ones prefixed by >>):

   include/linux/compiler.h:232:8: sparse: attribute 'no_sanitize_address': unknown attribute
   drivers/infiniband/hw/rxe/rxe.c: In function 'rxe_init_ports':
>> drivers/infiniband/hw/rxe/rxe.c:168:27: error: incompatible types when initializing type 'struct rxe_port *' using type 'struct rxe_port'
     struct rxe_port *port =  rxe->port;
                              ^~~

vim +168 drivers/infiniband/hw/rxe/rxe.c

   162	
   163	/* initialize port state, note IB convention that HCA ports are always
   164	 * numbered from 1
   165	 */
   166	static int rxe_init_ports(struct rxe_dev *rxe)
   167	{
 > 168		struct rxe_port *port =  rxe->port;
   169	
   170		rxe_init_port_param(port);
   171	

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/octet-stream, Size: 55009 bytes --]

[PATCH] IB/rxe: double free on error

[Dan Carpenter]

"goto err;" has it's own kfree_skb() call so it's a double free.  We
only need to free on the "goto exit;" path.

Fixes: 8700e3e7c485 ("Soft RoCE driver")
Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
---
Static analysis.  Not tested.  Please review carefully.

diff --git a/drivers/infiniband/sw/rxe/rxe_req.c b/drivers/infiniband/sw/rxe/rxe_req.c
index dbfde0dc6ff7..9f95f50b2909 100644
--- a/drivers/infiniband/sw/rxe/rxe_req.c
+++ b/drivers/infiniband/sw/rxe/rxe_req.c
@@ -729,11 +729,11 @@ int rxe_requester(void *arg)
 	ret = rxe_xmit_packet(to_rdev(qp->ibqp.device), qp, &pkt, skb);
 	if (ret) {
 		qp->need_req_skb = 1;
-		kfree_skb(skb);
 
 		rollback_state(wqe, qp, &rollback_wqe, rollback_psn);
 
 		if (ret == -EAGAIN) {
+			kfree_skb(skb);
 			rxe_run_task(&qp->req.task, 1);
 			goto exit;
 		}

Re: [PATCH] IB/rxe: double free on error

[Leon Romanovsky]

[-- Attachment #1: Type: text/plain, Size: 1544 bytes --]

On Wed, Mar 08, 2017 at 08:21:52AM +0300, Dan Carpenter wrote:
> "goto err;" has it's own kfree_skb() call so it's a double free.  We
> only need to free on the "goto exit;" path.
>
> Fixes: 8700e3e7c485 ("Soft RoCE driver")
> Signed-off-by: Dan Carpenter <dan.carpenter-QHcLZuEGTsvQT0dZR+AlfA@public.gmane.org>
> ---
> Static analysis.  Not tested.  Please review carefully.
>
> diff --git a/drivers/infiniband/sw/rxe/rxe_req.c b/drivers/infiniband/sw/rxe/rxe_req.c
> index dbfde0dc6ff7..9f95f50b2909 100644
> --- a/drivers/infiniband/sw/rxe/rxe_req.c
> +++ b/drivers/infiniband/sw/rxe/rxe_req.c
> @@ -729,11 +729,11 @@ int rxe_requester(void *arg)
>  	ret = rxe_xmit_packet(to_rdev(qp->ibqp.device), qp, &pkt, skb);

This rxe_xmit_packet() looks a little bit awkward. It calls to
kfree_skb and returns ret = 0 after drop decision. It doesn't
free on error (ret != 0), but this rxe_requester does.

However in case of not error, the skb won't be released and goto
next_cqe will be called, which has a lot of exit paths without freeing
skb.

Moni, Yonatan
Is it done on purpose?


>  	if (ret) {
>  		qp->need_req_skb = 1;
> -		kfree_skb(skb);
>
>  		rollback_state(wqe, qp, &rollback_wqe, rollback_psn);
>
>  		if (ret == -EAGAIN) {
> +			kfree_skb(skb);
>  			rxe_run_task(&qp->req.task, 1);
>  			goto exit;
>  		}
> --
> 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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH] IB/rxe: double free on error

[Moni Shoua]

> This rxe_xmit_packet() looks a little bit awkward. It calls to
> kfree_skb and returns ret = 0 after drop decision. It doesn't
> free on error (ret != 0), but this rxe_requester does.
>
> However in case of not error, the skb won't be released and goto
> next_cqe will be called, which has a lot of exit paths without freeing
> skb.
>
> Moni, Yonatan
> Is it done on purpose?
>
>
In good flow skb will be freed when going down the chain call from
rxe_xmit_packet(). This is the design.
In bad flow we can immediately free the skb

>>       if (ret) {
>>               qp->need_req_skb = 1;
>> -             kfree_skb(skb);
>>
>>               rollback_state(wqe, qp, &rollback_wqe, rollback_psn);
>>
>>               if (ret == -EAGAIN) {
>> +                     kfree_skb(skb);
>>                       rxe_run_task(&qp->req.task, 1);
>>                       goto exit;
>>               }
>> --
>> 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
--
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

Re: [PATCH] IB/rxe: double free on error

[Moni Shoua]

On Wed, Mar 8, 2017 at 7:21 AM, Dan Carpenter <dan.carpenter@oracle.com> wrote:
> "goto err;" has it's own kfree_skb() call so it's a double free.  We
> only need to free on the "goto exit;" path.
>
> Fixes: 8700e3e7c485 ("Soft RoCE driver")
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> ---
> Static analysis.  Not tested.  Please review carefully.

Ack-by: Moni Shoua <monis@mellanox.com>

Re: [PATCH] IB/rxe: double free on error

[Doug Ledford]

On Wed, 2017-03-08 at 08:21 +0300, Dan Carpenter wrote:
> "goto err;" has it's own kfree_skb() call so it's a double free.  We
> only need to free on the "goto exit;" path.
> 
> Fixes: 8700e3e7c485 ("Soft RoCE driver")
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>

Thanks, applied to -rc.

-- 
Doug Ledford <dledford@redhat.com>
    GPG KeyID: B826A3330E572FDD
   
Key fingerprint = AE6B 1BDA 122B 23B4 265B  1274 B826 A333 0E57 2FDD