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

[Leon Romanovsky <leon-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>]

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