Re: [bug report] net/smc: CLC accept / confirm V2

[Ursula Braun <ubraun@linux.ibm.com>]

On 10/28/20 12:25 PM, Dan Carpenter wrote:
> Hello Ursula Braun,
> 
> The patch a7c9c5f4af7f: "net/smc: CLC accept / confirm V2" from Sep
> 26, 2020, leads to the following static checker warning:
> 
> 	net/smc/af_smc.c:1771 smc_listen_work()
> 	error: we previously assumed 'ini' could be null (see line 1715)
> 

Thanks for reporting this problem!
Karsten Graul provided already this fix on 10/23/20 (accepted by
Jakub Kicinski 10/27/20) for the net-tree:

   net/smc: fix null pointer dereference in smc_listen_decline()

   smc_listen_work() calls smc_listen_decline() on label out_decl,
   providing the ini pointer variable. But this pointer can still be null
   when the label out_decl is reached.
   Fix this by checking the ini variable in smc_listen_work() and call
   smc_listen_decline() with the result directly.

   Fixes: a7c9c5f4af7f ("net/smc: CLC accept / confirm V2")
   Signed-off-by: Karsten Graul <kgraul@linux.ibm.com>

This fix is supposed to solve your reported problem.

Regards, Ursula Braun

> net/smc/af_smc.c
>   1665  static void smc_listen_work(struct work_struct *work)
>   1666  {
>   1667          struct smc_sock *new_smc = container_of(work, struct smc_sock,
>   1668                                                  smc_listen_work);
>   1669          u8 version = smc_ism_v2_capable ? SMC_V2 : SMC_V1;
>   1670          struct socket *newclcsock = new_smc->clcsock;
>   1671          struct smc_clc_msg_accept_confirm *cclc;
>   1672          struct smc_clc_msg_proposal_area *buf;
>   1673          struct smc_clc_msg_proposal *pclc;
>   1674          struct smc_init_info *ini = NULL;
>                 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> 
>   1675          int rc = 0;
>   1676  
>   1677          if (new_smc->listen_smc->sk.sk_state != SMC_LISTEN)
>   1678                  return smc_listen_out_err(new_smc);
>   1679  
>   1680          if (new_smc->use_fallback) {
>   1681                  smc_listen_out_connected(new_smc);
>   1682                  return;
>   1683          }
>   1684  
>   1685          /* check if peer is smc capable */
>   1686          if (!tcp_sk(newclcsock->sk)->syn_smc) {
>   1687                  smc_switch_to_fallback(new_smc);
>   1688                  new_smc->fallback_rsn = SMC_CLC_DECL_PEERNOSMC;
>   1689                  smc_listen_out_connected(new_smc);
>   1690                  return;
>   1691          }
>   1692  
>   1693          /* do inband token exchange -
>   1694           * wait for and receive SMC Proposal CLC message
>   1695           */
>   1696          buf = kzalloc(sizeof(*buf), GFP_KERNEL);
>   1697          if (!buf) {
>   1698                  rc = SMC_CLC_DECL_MEM;
>   1699                  goto out_decl;
>                         ^^^^^^^^^^^^^^
> There are several paths where "ini" is NULL leading to an Oops in the
> error handling.
> 
>   1700          }
>   1701          pclc = (struct smc_clc_msg_proposal *)buf;
>   1702          rc = smc_clc_wait_msg(new_smc, pclc, sizeof(*buf),
>   1703                                SMC_CLC_PROPOSAL, CLC_WAIT_TIME);
>   1704          if (rc)
>   1705                  goto out_decl;
>                         ^^^^^^^^^^^^^
> 
>   1706          version = pclc->hdr.version == SMC_V1 ? SMC_V1 : version;
>   1707  
>   1708          /* IPSec connections opt out of SMC optimizations */
>   1709          if (using_ipsec(new_smc)) {
>   1710                  rc = SMC_CLC_DECL_IPSEC;
>   1711                  goto out_decl;
>                         ^^^^^^^^^^^^^
> 
>   1712          }
>   1713  
>   1714          ini = kzalloc(sizeof(*ini), GFP_KERNEL);
>   1715          if (!ini) {
>   1716                  rc = SMC_CLC_DECL_MEM;
>   1717                  goto out_decl;
>                         ^^^^^^^^^^^^^
> 
>   1718          }
>   1719  
>   1720          /* initial version checking */
>   1721          rc = smc_listen_v2_check(new_smc, pclc, ini);
>   1722          if (rc)
>   1723                  goto out_decl;
>   1724  
>   1725          mutex_lock(&smc_server_lgr_pending);
>   1726          smc_close_init(new_smc);
>   1727          smc_rx_init(new_smc);
>   1728          smc_tx_init(new_smc);
>   1729  
>   1730          /* determine ISM or RoCE device used for connection */
>   1731          rc = smc_listen_find_device(new_smc, pclc, ini);
>   1732          if (rc)
>   1733                  goto out_unlock;
>   1734  
>   1735          /* send SMC Accept CLC message */
>   1736          rc = smc_clc_send_accept(new_smc, ini->first_contact_local,
>   1737                                   ini->smcd_version == SMC_V2 ? SMC_V2 : SMC_V1);
>   1738          if (rc)
>   1739                  goto out_unlock;
>   1740  
>   1741          /* SMC-D does not need this lock any more */
>   1742          if (ini->is_smcd)
>   1743                  mutex_unlock(&smc_server_lgr_pending);
>   1744  
>   1745          /* receive SMC Confirm CLC message */
>   1746          memset(buf, 0, sizeof(*buf));
>   1747          cclc = (struct smc_clc_msg_accept_confirm *)buf;
>   1748          rc = smc_clc_wait_msg(new_smc, cclc, sizeof(*buf),
>   1749                                SMC_CLC_CONFIRM, CLC_WAIT_TIME);
>   1750          if (rc) {
>   1751                  if (!ini->is_smcd)
>   1752                          goto out_unlock;
>   1753                  goto out_decl;
>   1754          }
>   1755  
>   1756          /* finish worker */
>   1757          if (!ini->is_smcd) {
>   1758                  rc = smc_listen_rdma_finish(new_smc, cclc,
>   1759                                              ini->first_contact_local);
>   1760                  if (rc)
>   1761                          goto out_unlock;
>   1762                  mutex_unlock(&smc_server_lgr_pending);
>   1763          }
>   1764          smc_conn_save_peer_info(new_smc, cclc);
>   1765          smc_listen_out_connected(new_smc);
>   1766          goto out_free;
>   1767  
>   1768  out_unlock:
>   1769          mutex_unlock(&smc_server_lgr_pending);
>   1770  out_decl:
>   1771          smc_listen_decline(new_smc, rc, ini, version);
>                                                 ^^^
> If "ini" is NULL then this will Oops.
> 
>   1772  out_free:
>   1773          kfree(ini);
>   1774          kfree(buf);
>   1775  }
> 
> regards,
> dan carpenter
>