IB/mlx4: Build the port IBoE GID table properly under bonding

Hello Moni Shoua,

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

The patch ad4885d279b6: "IB/mlx4: Build the port IBoE GID table properly under bonding" from Feb 5, 2014, leads to the following Smatch complaint:

drivers/infiniband/hw/mlx4/main.c:1749 mlx4_ib_scan_netdevs()
error: we previously assumed 'curr_netdev' could be null (see line 1722)

drivers/infiniband/hw/mlx4/main.c
1721

   1722			if (curr_netdev) {
                             ^^^^^^^^^^^

Check.

   1723				port_state = (netif_running(curr_netdev) && netif_carrier_ok(curr_netdev)) ?
   1724							IB_PORT_ACTIVE : IB_PORT_DOWN;
   1725				mlx4_ib_set_default_gid(ibdev, curr_netdev, port);
   1726			} else {
   1727				reset_gid_table(ibdev, port);
   1728			}
   1729			/* if using bonding/team and a slave port is down, we don't the bond IP
   1730			 * based gids in the table since flows that select port by gid may get
   1731			 * the down port.
   1732			 */
   1733			if (curr_master && (port_state == IB_PORT_DOWN)) {
   1734				reset_gid_table(ibdev, port);
   1735				mlx4_ib_set_default_gid(ibdev, curr_netdev, port);
   1736			}
   1737			/* if bonding is used it is possible that we add it to masters
   1738			 * only after IP address is assigned to the net bonding
   1739			 * interface.
   1740			*/
   1741			if (curr_master && (old_master != curr_master)) {
   1742				reset_gid_table(ibdev, port);
   1743				mlx4_ib_set_default_gid(ibdev, curr_netdev, port);
   1744				mlx4_ib_get_dev_addr(curr_master, ibdev, port);
   1745			}
   1746	
   1747			if (!curr_master && (old_master != curr_master)) {
   1748				reset_gid_table(ibdev, port);
   1749				mlx4_ib_set_default_gid(ibdev, curr_netdev, port);
                                                                ^^^^^^^^^^^

Dereferenced without checking.

   1750				mlx4_ib_get_dev_addr(curr_netdev, ibdev, port);
   1751			}

regards,
dan carpenter

Quoted 47 lines · Dan Carpenter

Hello Moni Shoua,

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

The patch ad4885d279b6: "IB/mlx4: Build the port IBoE GID table properly under bonding" from Feb 5, 2014, leads to the following Smatch complaint:

drivers/infiniband/hw/mlx4/main.c:1749 mlx4_ib_scan_netdevs()
error: we previously assumed 'curr_netdev' could be null (see line 1722)

drivers/infiniband/hw/mlx4/main.c
1721

  1722			if (curr_netdev) {
                            ^^^^^^^^^^^

Check.

1723 port_state = (netif_running(curr_netdev) && netif_carrier_ok(curr_netdev)) ?
1724 IB_PORT_ACTIVE : IB_PORT_DOWN;
1725 mlx4_ib_set_default_gid(ibdev, curr_netdev, port);
1726 } else {
1727 reset_gid_table(ibdev, port);
1728 }
1729 /* if using bonding/team and a slave port is down, we don't the bond IP
1730 * based gids in the table since flows that select port by gid may get
1731 * the down port.
1732 */
1733 if (curr_master && (port_state == IB_PORT_DOWN)) {
1734 reset_gid_table(ibdev, port);
1735 mlx4_ib_set_default_gid(ibdev, curr_netdev, port);
1736 }
1737 /* if bonding is used it is possible that we add it to masters
1738 * only after IP address is assigned to the net bonding
1739 * interface.
1740 */
1741 if (curr_master && (old_master != curr_master)) {
1742 reset_gid_table(ibdev, port);
1743 mlx4_ib_set_default_gid(ibdev, curr_netdev, port);
1744 mlx4_ib_get_dev_addr(curr_master, ibdev, port);
1745 }
1746
1747 if (!curr_master && (old_master != curr_master)) {
1748 reset_gid_table(ibdev, port);

  1749				mlx4_ib_set_default_gid(ibdev, curr_netdev, port);
                                                               ^^^^^^^^^^^

Dereferenced without checking.

Quoted 5 lines

  1750				mlx4_ib_get_dev_addr(curr_netdev, ibdev, port);
  1751			}

regards,
dan carpenter

Not really a bug. (curr_master != NULL) implies (curr_netdev != NULL)

Quoted 6 lines

Ha ha. Take another look. That's what I was just explaining about! :) On line 1743 when curr_master is non-NULL then Smatch doesn't complain because it understands about the relationship between curr_master and curr_netdev. But here it is complaining about line 1749 where curr_master is NULL so the implication doesn't apply. Nice, huh? regards, dan carpenter

You're right :)
I'll write the fix.

Quoted 8 lines · Moni Shoua

Quoted 6 lines

Ha ha. Take another look. That's what I was just explaining about! :) On line 1743 when curr_master is non-NULL then Smatch doesn't complain because it understands about the relationship between curr_master and curr_netdev. But here it is complaining about line 1749 where curr_master is NULL so the implication doesn't apply. Nice, huh? regards, dan carpenter

You're right :)
I'll write the fix.

Hello Moni,

Have you already had a chance to look further into this issue ?

Thanks,

Bart.