> Subject: Re: [bug report] iwpm: crash fix for large connections test
>
> 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.
>
I agree with Leon. The ref count should be 2 here.
However, I don't see why the kref_put() can't be moved after the up(&nlmsg_request->sem) to get rid of the warning.
Regards,
Mustafa
