* [bug report] iwpm: crash fix for large connections test
@ 2022-11-15 13:17 Dan Carpenter
2022-11-17 9:24 ` Leon Romanovsky
0 siblings, 1 reply; 6+ messages in thread
From: Dan Carpenter @ 2022-11-15 13:17 UTC (permalink / raw)
To: faisal.latif; +Cc: linux-rdma
[ 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?
436 barrier();
--> 437 up(&nlmsg_request->sem);
^^^^^^^^^^^^^
Dereference.
438 return 0;
439 }
regards,
dan carpenter
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [bug report] iwpm: crash fix for large connections test
2022-11-15 13:17 [bug report] iwpm: crash fix for large connections test Dan Carpenter
@ 2022-11-17 9:24 ` Leon Romanovsky
2022-11-18 20:44 ` Ismail, Mustafa
0 siblings, 1 reply; 6+ messages in thread
From: Leon Romanovsky @ 2022-11-17 9:24 UTC (permalink / raw)
To: Dan Carpenter; +Cc: faisal.latif, linux-rdma
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* RE: [bug report] iwpm: crash fix for large connections test
2022-11-17 9:24 ` Leon Romanovsky
@ 2022-11-18 20:44 ` Ismail, Mustafa
2022-11-19 7:31 ` Dan Carpenter
2022-11-28 7:34 ` Dan Carpenter
0 siblings, 2 replies; 6+ messages in thread
From: Ismail, Mustafa @ 2022-11-18 20:44 UTC (permalink / raw)
To: Leon Romanovsky, Dan Carpenter; +Cc: Latif, Faisal, linux-rdma
> 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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [bug report] iwpm: crash fix for large connections test
2022-11-18 20:44 ` Ismail, Mustafa
@ 2022-11-19 7:31 ` Dan Carpenter
2022-11-28 7:34 ` Dan Carpenter
1 sibling, 0 replies; 6+ messages in thread
From: Dan Carpenter @ 2022-11-19 7:31 UTC (permalink / raw)
To: Ismail, Mustafa; +Cc: Leon Romanovsky, Latif, Faisal, linux-rdma
On Fri, Nov 18, 2022 at 08:44:11PM +0000, Ismail, Mustafa wrote:
> > > 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.
>
Let's not expend too much time trying to silence this warning. One way
to silence the warning would be to do:
kref_put(&nlmsg_request->kref, NULL);
I'm conficted about this approach, but no good can come from calling
iwpm_free_nlmsg_request() on this path.
A better way to silence the warning would be to do:
diff --git a/drivers/infiniband/core/iwpm_util.c b/drivers/infiniband/core/iwpm_util.c
index 358a2db38d23..4f819e6c1b09 100644
--- a/drivers/infiniband/core/iwpm_util.c
+++ b/drivers/infiniband/core/iwpm_util.c
@@ -357,7 +357,7 @@ struct iwpm_nlmsg_request *iwpm_find_nlmsg_request(__u32 echo_seq)
inprocess_list) {
if (nlmsg_request->nlmsg_seq == echo_seq) {
found_request = nlmsg_request;
- kref_get(&nlmsg_request->kref);
+ kref_get(&found_request->kref);
break;
}
}
But the best way would be to make Smatch parse iwpm_find_nlmsg_request()
correctly as-is.
regards,
dan carpenter
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [bug report] iwpm: crash fix for large connections test
2022-11-18 20:44 ` Ismail, Mustafa
2022-11-19 7:31 ` Dan Carpenter
@ 2022-11-28 7:34 ` Dan Carpenter
2023-01-20 11:13 ` Greg Kroah-Hartman
1 sibling, 1 reply; 6+ messages in thread
From: Dan Carpenter @ 2022-11-28 7:34 UTC (permalink / raw)
To: Ismail, Mustafa, Greg Kroah-Hartman
Cc: Leon Romanovsky, Latif, Faisal, linux-rdma
So the background here is that Smatch sees this:
kref_put(&nlmsg_request->kref, iwpm_free_nlmsg_request);
and correctly says "if we call iwpm_free_nlmsg_request() then
dereferencing nlmsg_request is a use after free". However, the code
is holding two references at this point so it will never call
iwpm_free_nlmsg_request().
Smatch already checks to see if we are holding two references, but it
doesn't parse this code correctly. Smatch could be fixed, but there are
other places with similar warnings that are more difficult to fix.
What we could do is create a kref_no_release() function that just calls
WARN(). This would silence the warning and, I think, this would make
the code more readable.
What do other people think?
regards,
dan carpenter
diff --git a/include/linux/kref.h b/include/linux/kref.h
index d32e21a2538c..f40089f61aa6 100644
--- a/include/linux/kref.h
+++ b/include/linux/kref.h
@@ -45,6 +45,11 @@ static inline void kref_get(struct kref *kref)
refcount_inc(&kref->refcount);
}
+static inline void kref_no_release(struct kref *kref)
+{
+ WARN(1, "Unexpected kref release");
+}
+
/**
* kref_put - decrement refcount for object.
* @kref: object.
diff --git a/drivers/infiniband/core/iwpm_msg.c b/drivers/infiniband/core/iwpm_msg.c
index 3c9a9869212b..1acb96bfaf9c 100644
--- a/drivers/infiniband/core/iwpm_msg.c
+++ b/drivers/infiniband/core/iwpm_msg.c
@@ -432,7 +432,7 @@ int iwpm_register_pid_cb(struct sk_buff *skb, struct netlink_callback *cb)
register_pid_response_exit:
nlmsg_request->request_done = 1;
/* always for found nlmsg_request */
- kref_put(&nlmsg_request->kref, iwpm_free_nlmsg_request);
+ kref_put(&nlmsg_request->kref, kref_no_release);
barrier();
up(&nlmsg_request->sem);
return 0;
@@ -504,7 +504,7 @@ int iwpm_add_mapping_cb(struct sk_buff *skb, struct netlink_callback *cb)
add_mapping_response_exit:
nlmsg_request->request_done = 1;
/* always for found request */
- kref_put(&nlmsg_request->kref, iwpm_free_nlmsg_request);
+ kref_put(&nlmsg_request->kref, kref_no_release);
barrier();
up(&nlmsg_request->sem);
return 0;
@@ -602,7 +602,7 @@ int iwpm_add_and_query_mapping_cb(struct sk_buff *skb,
query_mapping_response_exit:
nlmsg_request->request_done = 1;
/* always for found request */
- kref_put(&nlmsg_request->kref, iwpm_free_nlmsg_request);
+ kref_put(&nlmsg_request->kref, kref_no_release);
barrier();
up(&nlmsg_request->sem);
return 0;
@@ -801,7 +801,7 @@ int iwpm_mapping_error_cb(struct sk_buff *skb, struct netlink_callback *cb)
nlmsg_request->err_code = err_code;
nlmsg_request->request_done = 1;
/* always for found request */
- kref_put(&nlmsg_request->kref, iwpm_free_nlmsg_request);
+ kref_put(&nlmsg_request->kref, kref_no_release);
barrier();
up(&nlmsg_request->sem);
return 0;
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [bug report] iwpm: crash fix for large connections test
2022-11-28 7:34 ` Dan Carpenter
@ 2023-01-20 11:13 ` Greg Kroah-Hartman
0 siblings, 0 replies; 6+ messages in thread
From: Greg Kroah-Hartman @ 2023-01-20 11:13 UTC (permalink / raw)
To: Dan Carpenter; +Cc: Ismail, Mustafa, Leon Romanovsky, Latif, Faisal, linux-rdma
On Mon, Nov 28, 2022 at 10:34:17AM +0300, Dan Carpenter wrote:
> So the background here is that Smatch sees this:
>
> kref_put(&nlmsg_request->kref, iwpm_free_nlmsg_request);
>
> and correctly says "if we call iwpm_free_nlmsg_request() then
> dereferencing nlmsg_request is a use after free". However, the code
> is holding two references at this point so it will never call
> iwpm_free_nlmsg_request().
>
> Smatch already checks to see if we are holding two references, but it
> doesn't parse this code correctly. Smatch could be fixed, but there are
> other places with similar warnings that are more difficult to fix.
>
> What we could do is create a kref_no_release() function that just calls
> WARN(). This would silence the warning and, I think, this would make
> the code more readable.
>
> What do other people think?
Sure, that looks semi-decent if it helps out with the automated tools.
thanks
greg k-h
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2023-01-20 11:14 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-11-15 13:17 [bug report] iwpm: crash fix for large connections test Dan Carpenter
2022-11-17 9:24 ` Leon Romanovsky
2022-11-18 20:44 ` Ismail, Mustafa
2022-11-19 7:31 ` Dan Carpenter
2022-11-28 7:34 ` Dan Carpenter
2023-01-20 11:13 ` Greg Kroah-Hartman
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.