[bug report] IB/mlx5: Respect mlx5_core reserved GIDs

[bug report] IB/mlx5: Respect mlx5_core reserved GIDs

[Dan Carpenter]

Hello Ilan Tayari,

This is a semi-automatic email about new static checker warnings.

The patch 095b0927f0ce: "IB/mlx5: Respect mlx5_core reserved GIDs"
from May 14, 2017, leads to the following Smatch complaint:

drivers/infiniband/hw/mlx5/main.c:327 set_roce_addr()
	 error: we previously assumed 'gid' could be null (see line 300)

drivers/infiniband/hw/mlx5/main.c
   299	
   300		if (gid) {
                    ^^^
Before we used to return early if gid was NULL.

   301			gid_type = attr->gid_type;
   302			ether_addr_copy(mac, attr->ndev->dev_addr);
   303	
   304			if (is_vlan_dev(attr->ndev)) {
   305				vlan = true;
   306				vlan_id = vlan_dev_vlan_id(attr->ndev);
   307			}
   308		}
   309	
   310		switch (gid_type) {
   311		case IB_GID_TYPE_IB:
   312			roce_version = MLX5_ROCE_VERSION_1;
   313			break;
   314		case IB_GID_TYPE_ROCE_UDP_ENCAP:
   315			roce_version = MLX5_ROCE_VERSION_2;
   316			if (ipv6_addr_v4mapped((void *)gid))
   317				roce_l3_type = MLX5_ROCE_L3_TYPE_IPV4;
   318			else
   319				roce_l3_type = MLX5_ROCE_L3_TYPE_IPV6;
   320			break;
   321	
   322		default:
   323			mlx5_ib_warn(dev, "Unexpected GID type %u\n", gid_type);
   324		}
   325	
   326		return mlx5_core_roce_gid_set(dev->mdev, index, roce_version,
   327					      roce_l3_type, gid->raw, mac, vlan,
                                                            ^^^^^^^^
Dereference.

   328					      vlan_id);
   329	}

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: [bug report] IB/mlx5: Respect mlx5_core reserved GIDs

[Leon Romanovsky]

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

On Thu, Jul 06, 2017 at 12:39:24PM +0300, Dan Carpenter wrote:
> Hello Ilan Tayari,
>
> This is a semi-automatic email about new static checker warnings.
>
> The patch 095b0927f0ce: "IB/mlx5: Respect mlx5_core reserved GIDs"
> from May 14, 2017, leads to the following Smatch complaint:
>
> drivers/infiniband/hw/mlx5/main.c:327 set_roce_addr()
> 	 error: we previously assumed 'gid' could be null (see line 300)
>
> drivers/infiniband/hw/mlx5/main.c
>    299
>    300		if (gid) {
>                     ^^^
> Before we used to return early if gid was NULL.

Thanks Dan,
It is bug.
mlx5_ib_del_gid calls this function with gid == NULL.

 363 static int mlx5_ib_del_gid(struct ib_device *device, u8 port_num,
 364                            unsigned int index, __always_unused void **context)
 365 {
 366         return set_roce_addr(device, port_num, index, NULL, NULL);
 367 }

>
>    301			gid_type = attr->gid_type;
>    302			ether_addr_copy(mac, attr->ndev->dev_addr);
>    303
>    304			if (is_vlan_dev(attr->ndev)) {
>    305				vlan = true;
>    306				vlan_id = vlan_dev_vlan_id(attr->ndev);
>    307			}
>    308		}
>    309
>    310		switch (gid_type) {
>    311		case IB_GID_TYPE_IB:
>    312			roce_version = MLX5_ROCE_VERSION_1;
>    313			break;
>    314		case IB_GID_TYPE_ROCE_UDP_ENCAP:
>    315			roce_version = MLX5_ROCE_VERSION_2;
>    316			if (ipv6_addr_v4mapped((void *)gid))
>    317				roce_l3_type = MLX5_ROCE_L3_TYPE_IPV4;
>    318			else
>    319				roce_l3_type = MLX5_ROCE_L3_TYPE_IPV6;
>    320			break;
>    321
>    322		default:
>    323			mlx5_ib_warn(dev, "Unexpected GID type %u\n", gid_type);
>    324		}
>    325
>    326		return mlx5_core_roce_gid_set(dev->mdev, index, roce_version,
>    327					      roce_l3_type, gid->raw, mac, vlan,
>                                                             ^^^^^^^^
> Dereference.
>
>    328					      vlan_id);
>    329	}
>
> 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

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

Re: [bug report] IB/mlx5: Respect mlx5_core reserved GIDs

[Dan Carpenter]

On Thu, Jul 06, 2017 at 01:38:11PM +0300, Leon Romanovsky wrote:
> On Thu, Jul 06, 2017 at 12:39:24PM +0300, Dan Carpenter wrote:
> > Hello Ilan Tayari,
> >
> > This is a semi-automatic email about new static checker warnings.
> >
> > The patch 095b0927f0ce: "IB/mlx5: Respect mlx5_core reserved GIDs"
> > from May 14, 2017, leads to the following Smatch complaint:
> >
> > drivers/infiniband/hw/mlx5/main.c:327 set_roce_addr()
> > 	 error: we previously assumed 'gid' could be null (see line 300)
> >
> > drivers/infiniband/hw/mlx5/main.c
> >    299
> >    300		if (gid) {
> >                     ^^^
> > Before we used to return early if gid was NULL.
> 
> Thanks Dan,
> It is bug.
> mlx5_ib_del_gid calls this function with gid == NULL.
> 
>  363 static int mlx5_ib_del_gid(struct ib_device *device, u8 port_num,
>  364                            unsigned int index, __always_unused void **context)
>  365 {
>  366         return set_roce_addr(device, port_num, index, NULL, NULL);
>  367 }
> 

Hm...  You're right.  Btw, that code also generates a static checker
warning, but I just hadn't got around to reporting it yet.

	drivers/infiniband/hw/mlx5/main.c:342 mlx5_ib_del_gid()
	error: NULL dereference inside function.

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: [bug report] IB/mlx5: Respect mlx5_core reserved GIDs

[Ilan Tayari]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 2251 bytes --]

> -----Original Message-----
> From: Dan Carpenter [mailto:dan.carpenter@oracle.com]
> Sent: Thursday, July 06, 2017 2:11 PM
> To: Leon Romanovsky <leon@kernel.org>
> Cc: Ilan Tayari <ilant@mellanox.com>; linux-rdma@vger.kernel.org
> Subject: Re: [bug report] IB/mlx5: Respect mlx5_core reserved GIDs
> 
> On Thu, Jul 06, 2017 at 01:38:11PM +0300, Leon Romanovsky wrote:
> > On Thu, Jul 06, 2017 at 12:39:24PM +0300, Dan Carpenter wrote:
> > > Hello Ilan Tayari,
> > >
> > > This is a semi-automatic email about new static checker warnings.
> > >
> > > The patch 095b0927f0ce: "IB/mlx5: Respect mlx5_core reserved GIDs"
> > > from May 14, 2017, leads to the following Smatch complaint:
> > >
> > > drivers/infiniband/hw/mlx5/main.c:327 set_roce_addr()
> > > 	 error: we previously assumed 'gid' could be null (see line 300)
> > >
> > > drivers/infiniband/hw/mlx5/main.c
> > >    299
> > >    300		if (gid) {
> > >                     ^^^
> > > Before we used to return early if gid was NULL.
> >
> > Thanks Dan,
> > It is bug.
> > mlx5_ib_del_gid calls this function with gid == NULL.
> >
> >  363 static int mlx5_ib_del_gid(struct ib_device *device, u8 port_num,
> >  364                            unsigned int index, __always_unused void
> **context)
> >  365 {
> >  366         return set_roce_addr(device, port_num, index, NULL, NULL);
> >  367 }
> >
> 
> Hm...  You're right.  Btw, that code also generates a static checker
> warning, but I just hadn't got around to reporting it yet.
> 
> 	drivers/infiniband/hw/mlx5/main.c:342 mlx5_ib_del_gid()
> 	error: NULL dereference inside function.

This is not a real NULL pointer dereference, because set_roce_addr() passes
parameter 4 to mlx5_core_roce_gid_set() as a pointer. So the -> operator
is not a dereference, it's pointer arithmetics.

It might have been a logic error, except that gid->raw is the first member,
so gid->raw == gid always.

So the logic in mlx5_core_roce_gid_set() also works as intended.

Nevertheless, this is semantically wrong, so I'll push a fix.

> 
> regards,
> dan carpenter

N‹§²æìr¸›yúèšØb²X¬¶Ç§vØ^–)Þº{.nÇ+‰·¥Š{±­ÙšŠ{ayº\x1dʇڙë,j\a­¢f£¢·hš‹»öì\x17/oSc¾™Ú³9˜uÀ¦æå‰È&jw¨®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿïêäz¹Þ–Šàþf£¢·hšˆ§~ˆmš

Re: [bug report] IB/mlx5: Respect mlx5_core reserved GIDs

[Dan Carpenter]

Ah...  You're right, of course.  I've been meaning to fix those false
positives for a while.

The only problem is that how would I still catch the bug if ->raw wasn't
the first member...  I need to think about that some more...

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: [bug report] IB/mlx5: Respect mlx5_core reserved GIDs

[Leon Romanovsky]

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

On Thu, Jul 06, 2017 at 11:24:13AM +0000, Ilan Tayari wrote:
> > -----Original Message-----
> > From: Dan Carpenter [mailto:dan.carpenter-QHcLZuEGTsvQT0dZR+AlfA@public.gmane.org]
> > Sent: Thursday, July 06, 2017 2:11 PM
> > To: Leon Romanovsky <leon-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
> > Cc: Ilan Tayari <ilant-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>; linux-rdma-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> > Subject: Re: [bug report] IB/mlx5: Respect mlx5_core reserved GIDs
> >
> > On Thu, Jul 06, 2017 at 01:38:11PM +0300, Leon Romanovsky wrote:
> > > On Thu, Jul 06, 2017 at 12:39:24PM +0300, Dan Carpenter wrote:
> > > > Hello Ilan Tayari,
> > > >
> > > > This is a semi-automatic email about new static checker warnings.
> > > >
> > > > The patch 095b0927f0ce: "IB/mlx5: Respect mlx5_core reserved GIDs"
> > > > from May 14, 2017, leads to the following Smatch complaint:
> > > >
> > > > drivers/infiniband/hw/mlx5/main.c:327 set_roce_addr()
> > > > 	 error: we previously assumed 'gid' could be null (see line 300)
> > > >
> > > > drivers/infiniband/hw/mlx5/main.c
> > > >    299
> > > >    300		if (gid) {
> > > >                     ^^^
> > > > Before we used to return early if gid was NULL.
> > >
> > > Thanks Dan,
> > > It is bug.
> > > mlx5_ib_del_gid calls this function with gid == NULL.
> > >
> > >  363 static int mlx5_ib_del_gid(struct ib_device *device, u8 port_num,
> > >  364                            unsigned int index, __always_unused void
> > **context)
> > >  365 {
> > >  366         return set_roce_addr(device, port_num, index, NULL, NULL);
> > >  367 }
> > >
> >
> > Hm...  You're right.  Btw, that code also generates a static checker
> > warning, but I just hadn't got around to reporting it yet.
> >
> > 	drivers/infiniband/hw/mlx5/main.c:342 mlx5_ib_del_gid()
> > 	error: NULL dereference inside function.
>
> This is not a real NULL pointer dereference, because set_roce_addr() passes
> parameter 4 to mlx5_core_roce_gid_set() as a pointer. So the -> operator
> is not a dereference, it's pointer arithmetics.
>
> It might have been a logic error, except that gid->raw is the first member,
> so gid->raw == gid always.
>
> So the logic in mlx5_core_roce_gid_set() also works as intended.

It works by chance, assuming that "->raw" will be always first member is
not a good strategy, so I agree with Dan. It is NULL dereferencing.

>
> Nevertheless, this is semantically wrong, so I'll push a fix.
>
> >
> > regards,
> > dan carpenter
>

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

Re: [bug report] IB/mlx5: Respect mlx5_core reserved GIDs

[Leon Romanovsky]

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

On Thu, Jul 06, 2017 at 02:44:59PM +0300, Dan Carpenter wrote:
> Ah...  You're right, of course.  I've been meaning to fix those false
> positives for a while.
>
> The only problem is that how would I still catch the bug if ->raw wasn't
> the first member...  I need to think about that some more...

I think that you should report all those cases without relation
to first/not first member.

>
> regards,
> dan carpenter
>

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

RE: [bug report] IB/mlx5: Respect mlx5_core reserved GIDs

[Ilan Tayari]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 937 bytes --]

> -----Original Message-----
> From: Leon Romanovsky [mailto:leon@kernel.org]
> Subject: Re: [bug report] IB/mlx5: Respect mlx5_core reserved GIDs
> 
> On Thu, Jul 06, 2017 at 02:44:59PM +0300, Dan Carpenter wrote:
> > Ah...  You're right, of course.  I've been meaning to fix those false
> > positives for a while.
> >
> > The only problem is that how would I still catch the bug if ->raw wasn't
> > the first member...  I need to think about that some more...
> 
> I think that you should report all those cases without relation
> to first/not first member.

I must agree with Leon.

The fact that it doesn't dereference a NULL now, doesn't mean it's not bug,
lurking to crash at the next patch, and no other review will ever find it.

> 
> >
> > regards,
> > dan carpenter
> >
N‹§²æìr¸›yúèšØb²X¬¶Ç§vØ^–)Þº{.nÇ+‰·¥Š{±­ÙšŠ{ayº\x1dʇڙë,j\a­¢f£¢·hš‹»öì\x17/oSc¾™Ú³9˜uÀ¦æå‰È&jw¨®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿïêäz¹Þ–Šàþf£¢·hšˆ§~ˆmš

Re: [bug report] IB/mlx5: Respect mlx5_core reserved GIDs

[Dan Carpenter]

On Thu, Jul 06, 2017 at 02:47:33PM +0300, Leon Romanovsky wrote:
> 
> It works by chance, assuming that "->raw" will be always first member is
> not a good strategy, so I agree with Dan. It is NULL dereferencing.
> 

How these false positives normally look is like this:

	struct whatever *p = foo->bar;

	if (!foo)
		return -EINVAL;

It looks like a dereference but isn't.  And in this case it doesn't
matter if it's the first member or not.

Anyway, just from a correctness point of view it's better to not print
false positives.

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: [bug report] IB/mlx5: Respect mlx5_core reserved GIDs

[Ilan Tayari]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 1300 bytes --]

> -----Original Message-----
> From: Dan Carpenter [mailto:dan.carpenter@oracle.com]
> Subject: Re: [bug report] IB/mlx5: Respect mlx5_core reserved GIDs
> 
> On Thu, Jul 06, 2017 at 02:47:33PM +0300, Leon Romanovsky wrote:
> >
> > It works by chance, assuming that "->raw" will be always first member is
> > not a good strategy, so I agree with Dan. It is NULL dereferencing.
> >
> 
> How these false positives normally look is like this:
> 
> 	struct whatever *p = foo->bar;
> 
> 	if (!foo)
> 		return -EINVAL;
> 
> It looks like a dereference but isn't.  And in this case it doesn't
> matter if it's the first member or not.
> 
> Anyway, just from a correctness point of view it's better to not print
> false positives.
> 

Dan,

After taking another look, I am now sure this is a false positive, and
the existing code is OK and should not change.

Do note that this is a pointer to a union, not to a struct!
So the -> operator performs a cast, not a dereference.

Also, it doesn't matter if it's a first member or not as all
members of a union are at the same pointer.

Do you think smatch static checker needs a fix?

Ilan.
N‹§²æìr¸›yúèšØb²X¬¶Ç§vØ^–)Þº{.nÇ+‰·¥Š{±­ÙšŠ{ayº\x1dʇڙë,j\a­¢f£¢·hš‹»öì\x17/oSc¾™Ú³9˜uÀ¦æå‰È&jw¨®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿïêäz¹Þ–Šàþf£¢·hšˆ§~ˆmš