Re: [bug report] iwpm: crash fix for large connections test

[Leon Romanovsky <leon@kernel.org>]

On Tue, Nov 15, 2022 at 04:17:32PM +0300, Dan Carpenter wrote:
> [ This isn't really the correct patch to blame.  Sorry! -dan ]
> 
> Hello Faisal Latif,
> 
> The patch dafb5587178a: "iwpm: crash fix for large connections test"
> from Feb 26, 2016, leads to the following Smatch static checker
> warning:
> 
> drivers/infiniband/core/iwpm_msg.c:437 iwpm_register_pid_cb() warn: 'nlmsg_request' was already freed.
> drivers/infiniband/core/iwpm_msg.c:509 iwpm_add_mapping_cb() warn: 'nlmsg_request' was already freed.
> drivers/infiniband/core/iwpm_msg.c:607 iwpm_add_and_query_mapping_cb() warn: 'nlmsg_request' was already freed.
> drivers/infiniband/core/iwpm_msg.c:806 iwpm_mapping_error_cb() warn: 'nlmsg_request' was already freed.
> 
> drivers/infiniband/core/iwpm_msg.c
>     385 int iwpm_register_pid_cb(struct sk_buff *skb, struct netlink_callback *cb)
>     386 {
>     387         struct iwpm_nlmsg_request *nlmsg_request = NULL;
>     388         struct nlattr *nltb[IWPM_NLA_RREG_PID_MAX];
>     389         struct iwpm_dev_data *pm_msg;
>     390         char *dev_name, *iwpm_name;
>     391         u32 msg_seq;
>     392         u8 nl_client;
>     393         u16 iwpm_version;
>     394         const char *msg_type = "Register Pid response";
>     395 
>     396         if (iwpm_parse_nlmsg(cb, IWPM_NLA_RREG_PID_MAX,
>     397                                 resp_reg_policy, nltb, msg_type))
>     398                 return -EINVAL;
>     399 
>     400         msg_seq = nla_get_u32(nltb[IWPM_NLA_RREG_PID_SEQ]);
>     401         nlmsg_request = iwpm_find_nlmsg_request(msg_seq);
>     402         if (!nlmsg_request) {
>     403                 pr_info("%s: Could not find a matching request (seq = %u)\n",
>     404                                  __func__, msg_seq);
>     405                 return -EINVAL;
>     406         }
>     407         pm_msg = nlmsg_request->req_buffer;
>     408         nl_client = nlmsg_request->nl_client;
>     409         dev_name = (char *)nla_data(nltb[IWPM_NLA_RREG_IBDEV_NAME]);
>     410         iwpm_name = (char *)nla_data(nltb[IWPM_NLA_RREG_ULIB_NAME]);
>     411         iwpm_version = nla_get_u16(nltb[IWPM_NLA_RREG_ULIB_VER]);
>     412 
>     413         /* check device name, ulib name and version */
>     414         if (strcmp(pm_msg->dev_name, dev_name) ||
>     415                         strcmp(iwpm_ulib_name, iwpm_name) ||
>     416                         iwpm_version < IWPM_UABI_VERSION_MIN) {
>     417 
>     418                 pr_info("%s: Incorrect info (dev = %s name = %s version = %u)\n",
>     419                                 __func__, dev_name, iwpm_name, iwpm_version);
>     420                 nlmsg_request->err_code = IWPM_USER_LIB_INFO_ERR;
>     421                 goto register_pid_response_exit;
>     422         }
>     423         iwpm_user_pid = cb->nlh->nlmsg_pid;
>     424         iwpm_ulib_version = iwpm_version;
>     425         if (iwpm_ulib_version < IWPM_UABI_VERSION)
>     426                 pr_warn_once("%s: Down level iwpmd/pid %d.  Continuing...",
>     427                         __func__, iwpm_user_pid);
>     428         atomic_set(&echo_nlmsg_seq, cb->nlh->nlmsg_seq);
>     429         pr_debug("%s: iWarp Port Mapper (pid = %d) is available!\n",
>     430                         __func__, iwpm_user_pid);
>     431         iwpm_set_registration(nl_client, IWPM_REG_VALID);
>     432 register_pid_response_exit:
>     433         nlmsg_request->request_done = 1;
>     434         /* always for found nlmsg_request */
>     435         kref_put(&nlmsg_request->kref, iwpm_free_nlmsg_request);
> 
> The iwpm_free_nlmsg_request() function will free "nlmsg_request"...
> It's not clear what the "/* always for found nlmsg_request */" comment
> means.  Maybe it means that the refcount won't drop to zero so the
> free function won't be called?

I think so. The nlmsg_request reference counter is elevated when it is
found in iwpm_find_nlmsg_request(). So I assume that it will be at least
2 before call to kref_put(). Most likely, nlmsg_request->sem prevents
from parallel threads to decrease that reference counter.

BTW, not IWPM expert.

Thanks