On Thu, Jul 06, 2017 at 11:24:13AM +0000, Ilan Tayari wrote: > > -----Original Message----- > > From: Dan Carpenter [mailto:dan.carpenter@xxxxxxxxxx] > > Sent: Thursday, July 06, 2017 2:11 PM > > To: Leon Romanovsky <leon@xxxxxxxxxx> > > Cc: Ilan Tayari <ilant@xxxxxxxxxxxx>; linux-rdma@xxxxxxxxxxxxxxx > > 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:
signature.asc
Description: PGP signature
