[PATCH iproute2-rc 0/2] Bugfixes for rdmatool

[PATCH iproute2-rc 0/2] Bugfixes for rdmatool

[Junxian Huang]

Here are 2 bugfixes for rdmatool.

Chengchang Tang (1):
  rdma: Fix core dump when pretty is used

wenglianfa (1):
  rdma: Fix the error of accessing string variable outside the lifecycle

 rdma/res-cmid.c | 3 +--
 rdma/res-cq.c   | 3 +--
 rdma/res-ctx.c  | 3 +--
 rdma/res-mr.c   | 3 +--
 rdma/res-pd.c   | 3 +--
 rdma/res-qp.c   | 3 +--
 rdma/res-srq.c  | 3 +--
 rdma/stat.c     | 3 +--
 rdma/utils.c    | 6 +++++-
 9 files changed, 13 insertions(+), 17 deletions(-)

--
2.30.0

[PATCH iproute2-rc 1/2] rdma: Fix core dump when pretty is used

[Junxian Huang]

From: Chengchang Tang <tangchengchang@huawei.com>

There will be a core dump when pretty is used as the JSON object
hasn't been opened and closed properly.

Before:
$ rdma res show qp -jp -dd
[ {
    "ifindex": 1,
    "ifname": "hns_1",
    "port": 1,
    "lqpn": 1,
    "type": "GSI",
    "state": "RTS",
    "sq-psn": 0,
    "comm": "ib_core"
},
"drv_sq_wqe_cnt": 128,
"drv_sq_max_gs": 2,
"drv_rq_wqe_cnt": 512,
"drv_rq_max_gs": 1,
rdma: json_writer.c:130: jsonw_end: Assertion `self->depth > 0' failed.
Aborted (core dumped)

After:
$ rdma res show qp -jp -dd
[ {
        "ifindex": 2,
        "ifname": "hns_2",
        "port": 1,
        "lqpn": 1,
        "type": "GSI",
        "state": "RTS",
        "sq-psn": 0,
        "comm": "ib_core",{
            "drv_sq_wqe_cnt": 128,
            "drv_sq_max_gs": 2,
            "drv_rq_wqe_cnt": 512,
            "drv_rq_max_gs": 1,
            "drv_ext_sge_sge_cnt": 256
        }
    } ]

Fixes: 331152752a97 ("rdma: print driver resource attributes")
Signed-off-by: Chengchang Tang <tangchengchang@huawei.com>
Signed-off-by: Junxian Huang <huangjunxian6@hisilicon.com>
---
 rdma/utils.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/rdma/utils.c b/rdma/utils.c
index 09985069..dcf24337 100644
--- a/rdma/utils.c
+++ b/rdma/utils.c
@@ -926,7 +926,7 @@ void print_driver_table(struct rd *rd, struct nlattr *tb)
 		return;
 
 	if (rd->pretty_output)
-		newline_indent(rd);
+		open_json_object(NULL);
 
 	/*
 	 * Driver attrs are tuples of {key, [print-type], value}.
@@ -960,5 +960,9 @@ void print_driver_table(struct rd *rd, struct nlattr *tb)
 			key = NULL;
 		}
 	}
+
+	if (rd->pretty_output)
+		newline_indent(rd);
+
 	return;
 }
-- 
2.30.0

[PATCH iproute2-rc 2/2] rdma: Fix the error of accessing string variable outside the lifecycle

[Junxian Huang]

From: wenglianfa <wenglianfa@huawei.com>

All these SPRINT_BUF(b) definitions are inside the 'if' block, but
accessed outside the 'if' block through the pointers 'comm'. This
leads to empty 'comm' attribute when querying resource information.
So move the definitions to the beginning of the functions to extend
their life cycle.

Before:
$ rdma res show srq
dev hns_0 srqn 0 type BASIC lqpn 18 pdn 5 pid 7775 comm

After:
$ rdma res show srq
dev hns_0 srqn 0 type BASIC lqpn 18 pdn 5 pid 7775 comm ib_send_bw

Fixes: 1808f002dfdd ("lib/fs: fix memory leak in get_task_name()")
Signed-off-by: wenglianfa <wenglianfa@huawei.com>
Signed-off-by: Junxian Huang <huangjunxian6@hisilicon.com>
---
 rdma/res-cmid.c | 3 +--
 rdma/res-cq.c   | 3 +--
 rdma/res-ctx.c  | 3 +--
 rdma/res-mr.c   | 3 +--
 rdma/res-pd.c   | 3 +--
 rdma/res-qp.c   | 3 +--
 rdma/res-srq.c  | 3 +--
 rdma/stat.c     | 3 +--
 8 files changed, 8 insertions(+), 16 deletions(-)

diff --git a/rdma/res-cmid.c b/rdma/res-cmid.c
index 7371c3a6..595af848 100644
--- a/rdma/res-cmid.c
+++ b/rdma/res-cmid.c
@@ -102,6 +102,7 @@ static int res_cm_id_line(struct rd *rd, const char *name, int idx,
 	uint32_t lqpn = 0, ps;
 	uint32_t cm_idn = 0;
 	char *comm = NULL;
+	SPRINT_BUF(b);
 
 	if (!nla_line[RDMA_NLDEV_ATTR_RES_STATE] ||
 	    !nla_line[RDMA_NLDEV_ATTR_RES_PS])
@@ -159,8 +160,6 @@ static int res_cm_id_line(struct rd *rd, const char *name, int idx,
 		goto out;
 
 	if (nla_line[RDMA_NLDEV_ATTR_RES_PID]) {
-		SPRINT_BUF(b);
-
 		pid = mnl_attr_get_u32(nla_line[RDMA_NLDEV_ATTR_RES_PID]);
 		if (!get_task_name(pid, b, sizeof(b)))
 			comm = b;
diff --git a/rdma/res-cq.c b/rdma/res-cq.c
index 2cfa4994..80994a03 100644
--- a/rdma/res-cq.c
+++ b/rdma/res-cq.c
@@ -63,6 +63,7 @@ static int res_cq_line(struct rd *rd, const char *name, int idx,
 	uint32_t cqn = 0;
 	uint64_t users;
 	uint32_t cqe;
+	SPRINT_BUF(b);
 
 	if (!nla_line[RDMA_NLDEV_ATTR_RES_CQE] ||
 	    !nla_line[RDMA_NLDEV_ATTR_RES_USECNT])
@@ -84,8 +85,6 @@ static int res_cq_line(struct rd *rd, const char *name, int idx,
 		goto out;
 
 	if (nla_line[RDMA_NLDEV_ATTR_RES_PID]) {
-		SPRINT_BUF(b);
-
 		pid = mnl_attr_get_u32(nla_line[RDMA_NLDEV_ATTR_RES_PID]);
 		if (!get_task_name(pid, b, sizeof(b)))
 			comm = b;
diff --git a/rdma/res-ctx.c b/rdma/res-ctx.c
index 500186d9..99736ea0 100644
--- a/rdma/res-ctx.c
+++ b/rdma/res-ctx.c
@@ -13,13 +13,12 @@ static int res_ctx_line(struct rd *rd, const char *name, int idx,
 	char *comm = NULL;
 	uint32_t ctxn = 0;
 	uint32_t pid = 0;
+	SPRINT_BUF(b);
 
 	if (!nla_line[RDMA_NLDEV_ATTR_RES_CTXN])
 		return MNL_CB_ERROR;
 
 	if (nla_line[RDMA_NLDEV_ATTR_RES_PID]) {
-		SPRINT_BUF(b);
-
 		pid = mnl_attr_get_u32(nla_line[RDMA_NLDEV_ATTR_RES_PID]);
 		if (!get_task_name(pid, b, sizeof(b)))
 			comm = b;
diff --git a/rdma/res-mr.c b/rdma/res-mr.c
index fb48d5df..e6c81d11 100644
--- a/rdma/res-mr.c
+++ b/rdma/res-mr.c
@@ -30,6 +30,7 @@ static int res_mr_line(struct rd *rd, const char *name, int idx,
 	uint32_t pdn = 0;
 	uint32_t mrn = 0;
 	uint32_t pid = 0;
+	SPRINT_BUF(b);
 
 	if (!nla_line[RDMA_NLDEV_ATTR_RES_MRLEN])
 		return MNL_CB_ERROR;
@@ -47,8 +48,6 @@ static int res_mr_line(struct rd *rd, const char *name, int idx,
 		goto out;
 
 	if (nla_line[RDMA_NLDEV_ATTR_RES_PID]) {
-		SPRINT_BUF(b);
-
 		pid = mnl_attr_get_u32(nla_line[RDMA_NLDEV_ATTR_RES_PID]);
 		if (!get_task_name(pid, b, sizeof(b)))
 			comm = b;
diff --git a/rdma/res-pd.c b/rdma/res-pd.c
index 66f91f42..0dbb310a 100644
--- a/rdma/res-pd.c
+++ b/rdma/res-pd.c
@@ -16,6 +16,7 @@ static int res_pd_line(struct rd *rd, const char *name, int idx,
 	uint32_t pid = 0;
 	uint32_t pdn = 0;
 	uint64_t users;
+	SPRINT_BUF(b);
 
 	if (!nla_line[RDMA_NLDEV_ATTR_RES_USECNT])
 		return MNL_CB_ERROR;
@@ -34,8 +35,6 @@ static int res_pd_line(struct rd *rd, const char *name, int idx,
 			nla_line[RDMA_NLDEV_ATTR_RES_UNSAFE_GLOBAL_RKEY]);
 
 	if (nla_line[RDMA_NLDEV_ATTR_RES_PID]) {
-		SPRINT_BUF(b);
-
 		pid = mnl_attr_get_u32(nla_line[RDMA_NLDEV_ATTR_RES_PID]);
 		if (!get_task_name(pid, b, sizeof(b)))
 			comm = b;
diff --git a/rdma/res-qp.c b/rdma/res-qp.c
index c180a97e..cd2f4aa2 100644
--- a/rdma/res-qp.c
+++ b/rdma/res-qp.c
@@ -86,6 +86,7 @@ static int res_qp_line(struct rd *rd, const char *name, int idx,
 	uint32_t port = 0, pid = 0;
 	uint32_t pdn = 0;
 	char *comm = NULL;
+	SPRINT_BUF(b);
 
 	if (!nla_line[RDMA_NLDEV_ATTR_RES_LQPN] ||
 	    !nla_line[RDMA_NLDEV_ATTR_RES_SQ_PSN] ||
@@ -146,8 +147,6 @@ static int res_qp_line(struct rd *rd, const char *name, int idx,
 		goto out;
 
 	if (nla_line[RDMA_NLDEV_ATTR_RES_PID]) {
-		SPRINT_BUF(b);
-
 		pid = mnl_attr_get_u32(nla_line[RDMA_NLDEV_ATTR_RES_PID]);
 		if (!get_task_name(pid, b, sizeof(b)))
 			comm = b;
diff --git a/rdma/res-srq.c b/rdma/res-srq.c
index cf9209d7..758bb193 100644
--- a/rdma/res-srq.c
+++ b/rdma/res-srq.c
@@ -183,13 +183,12 @@ static int res_srq_line(struct rd *rd, const char *name, int idx,
 	char qp_str[MAX_QP_STR_LEN] = {};
 	char *comm = NULL;
 	uint8_t type = 0;
+	SPRINT_BUF(b);
 
 	if (!nla_line[RDMA_NLDEV_ATTR_RES_SRQN])
 		return MNL_CB_ERROR;
 
 	if (nla_line[RDMA_NLDEV_ATTR_RES_PID]) {
-		SPRINT_BUF(b);
-
 		pid = mnl_attr_get_u32(nla_line[RDMA_NLDEV_ATTR_RES_PID]);
 		if (!get_task_name(pid, b, sizeof(b)))
 			comm = b;
diff --git a/rdma/stat.c b/rdma/stat.c
index 3df2c98f..c7dde680 100644
--- a/rdma/stat.c
+++ b/rdma/stat.c
@@ -223,6 +223,7 @@ static int res_counter_line(struct rd *rd, const char *name, int index,
 	struct nlattr *hwc_table, *qp_table;
 	struct nlattr *nla_entry;
 	const char *comm = NULL;
+	SPRINT_BUF(b);
 	bool isfirst;
 	int err;
 
@@ -248,8 +249,6 @@ static int res_counter_line(struct rd *rd, const char *name, int index,
 		return MNL_CB_OK;
 
 	if (nla_line[RDMA_NLDEV_ATTR_RES_PID]) {
-		SPRINT_BUF(b);
-
 		pid = mnl_attr_get_u32(nla_line[RDMA_NLDEV_ATTR_RES_PID]);
 		if (!get_task_name(pid, b, sizeof(b)))
 			comm = b;
-- 
2.30.0

Re: [PATCH iproute2-rc 1/2] rdma: Fix core dump when pretty is used

[Stephen Hemminger]

On Fri, 29 Dec 2023 14:52:40 +0800
Junxian Huang <huangjunxian6@hisilicon.com> wrote:

> From: Chengchang Tang <tangchengchang@huawei.com>
> 
> There will be a core dump when pretty is used as the JSON object
> hasn't been opened and closed properly.
> 
> Before:
> $ rdma res show qp -jp -dd
> [ {
>     "ifindex": 1,
>     "ifname": "hns_1",
>     "port": 1,
>     "lqpn": 1,
>     "type": "GSI",
>     "state": "RTS",
>     "sq-psn": 0,
>     "comm": "ib_core"
> },
> "drv_sq_wqe_cnt": 128,
> "drv_sq_max_gs": 2,
> "drv_rq_wqe_cnt": 512,
> "drv_rq_max_gs": 1,
> rdma: json_writer.c:130: jsonw_end: Assertion `self->depth > 0' failed.
> Aborted (core dumped)
> 
> After:
> $ rdma res show qp -jp -dd
> [ {
>         "ifindex": 2,
>         "ifname": "hns_2",
>         "port": 1,
>         "lqpn": 1,
>         "type": "GSI",
>         "state": "RTS",
>         "sq-psn": 0,
>         "comm": "ib_core",{
>             "drv_sq_wqe_cnt": 128,
>             "drv_sq_max_gs": 2,
>             "drv_rq_wqe_cnt": 512,
>             "drv_rq_max_gs": 1,
>             "drv_ext_sge_sge_cnt": 256
>         }
>     } ]
> 
> Fixes: 331152752a97 ("rdma: print driver resource attributes")
> Signed-off-by: Chengchang Tang <tangchengchang@huawei.com>
> Signed-off-by: Junxian Huang <huangjunxian6@hisilicon.com>

This code in rdma seems to be miking json and newline functionality
which creates bug traps.

Also the json should have same effective output in pretty and non-pretty mode.
It looks like since pretty mode add extra object layer, the nesting of {} would be
different.

The conversion to json_print() was done but it isn't using same conventions
as ip or tc.

The correct fix needs to go deeper and hit other things.

Re: [PATCH iproute2-rc 1/2] rdma: Fix core dump when pretty is used

[Chengchang Tang]

On 2023/12/30 1:21, Stephen Hemminger wrote:
> On Fri, 29 Dec 2023 14:52:40 +0800
> Junxian Huang <huangjunxian6@hisilicon.com> wrote:
>
>> From: Chengchang Tang <tangchengchang@huawei.com>
>>
>> There will be a core dump when pretty is used as the JSON object
>> hasn't been opened and closed properly.
>>
>> Before:
>> $ rdma res show qp -jp -dd
>> [ {
>>      "ifindex": 1,
>>      "ifname": "hns_1",
>>      "port": 1,
>>      "lqpn": 1,
>>      "type": "GSI",
>>      "state": "RTS",
>>      "sq-psn": 0,
>>      "comm": "ib_core"
>> },
>> "drv_sq_wqe_cnt": 128,
>> "drv_sq_max_gs": 2,
>> "drv_rq_wqe_cnt": 512,
>> "drv_rq_max_gs": 1,
>> rdma: json_writer.c:130: jsonw_end: Assertion `self->depth > 0' failed.
>> Aborted (core dumped)
>>
>> After:
>> $ rdma res show qp -jp -dd
>> [ {
>>          "ifindex": 2,
>>          "ifname": "hns_2",
>>          "port": 1,
>>          "lqpn": 1,
>>          "type": "GSI",
>>          "state": "RTS",
>>          "sq-psn": 0,
>>          "comm": "ib_core",{
>>              "drv_sq_wqe_cnt": 128,
>>              "drv_sq_max_gs": 2,
>>              "drv_rq_wqe_cnt": 512,
>>              "drv_rq_max_gs": 1,
>>              "drv_ext_sge_sge_cnt": 256
>>          }
>>      } ]
>>
>> Fixes: 331152752a97 ("rdma: print driver resource attributes")
>> Signed-off-by: Chengchang Tang <tangchengchang@huawei.com>
>> Signed-off-by: Junxian Huang <huangjunxian6@hisilicon.com>
> This code in rdma seems to be miking json and newline functionality
> which creates bug traps.
>
> Also the json should have same effective output in pretty and non-pretty mode.
> It looks like since pretty mode add extra object layer, the nesting of {} would be
> different.
>
> The conversion to json_print() was done but it isn't using same conventions
> as ip or tc.
>
> The correct fix needs to go deeper and hit other things.
>

Hi, Stephen,

The root cause of this issue is that close_json_object() is being called 
in newline_indent(), resulting in a mismatch
of {}.

When fixing this problem, I was unsure why a newline() is needed in 
pretty mode, so I simply kept this logic and
solved the issue of open_json_object() and close_json_object() not 
matching. However, If the output of pretty mode
and not-pretty mode should be the same, then this problem can be 
resolved by deleting this newline_indent().

I believe the original developer may not have realized that 
close_json_object() is being called in newline(), which leads
to this problem. To improve the code's readability, I would try to strip 
out close_json_obejct() from newline().

Thanks,
Chengchang Tang

Re: [PATCH iproute2-rc 1/2] rdma: Fix core dump when pretty is used

[Leon Romanovsky]

On Tue, Jan 02, 2024 at 03:44:29PM +0800, Chengchang Tang wrote:
> 
> 
> On 2023/12/30 1:21, Stephen Hemminger wrote:
> > On Fri, 29 Dec 2023 14:52:40 +0800
> > Junxian Huang <huangjunxian6@hisilicon.com> wrote:
> > 
> > > From: Chengchang Tang <tangchengchang@huawei.com>
> > > 
> > > There will be a core dump when pretty is used as the JSON object
> > > hasn't been opened and closed properly.
> > > 
> > > Before:
> > > $ rdma res show qp -jp -dd
> > > [ {
> > >      "ifindex": 1,
> > >      "ifname": "hns_1",
> > >      "port": 1,
> > >      "lqpn": 1,
> > >      "type": "GSI",
> > >      "state": "RTS",
> > >      "sq-psn": 0,
> > >      "comm": "ib_core"
> > > },
> > > "drv_sq_wqe_cnt": 128,
> > > "drv_sq_max_gs": 2,
> > > "drv_rq_wqe_cnt": 512,
> > > "drv_rq_max_gs": 1,
> > > rdma: json_writer.c:130: jsonw_end: Assertion `self->depth > 0' failed.
> > > Aborted (core dumped)
> > > 
> > > After:
> > > $ rdma res show qp -jp -dd
> > > [ {
> > >          "ifindex": 2,
> > >          "ifname": "hns_2",
> > >          "port": 1,
> > >          "lqpn": 1,
> > >          "type": "GSI",
> > >          "state": "RTS",
> > >          "sq-psn": 0,
> > >          "comm": "ib_core",{
> > >              "drv_sq_wqe_cnt": 128,
> > >              "drv_sq_max_gs": 2,
> > >              "drv_rq_wqe_cnt": 512,
> > >              "drv_rq_max_gs": 1,
> > >              "drv_ext_sge_sge_cnt": 256
> > >          }
> > >      } ]
> > > 
> > > Fixes: 331152752a97 ("rdma: print driver resource attributes")
> > > Signed-off-by: Chengchang Tang <tangchengchang@huawei.com>
> > > Signed-off-by: Junxian Huang <huangjunxian6@hisilicon.com>
> > This code in rdma seems to be miking json and newline functionality
> > which creates bug traps.
> > 
> > Also the json should have same effective output in pretty and non-pretty mode.
> > It looks like since pretty mode add extra object layer, the nesting of {} would be
> > different.
> > 
> > The conversion to json_print() was done but it isn't using same conventions
> > as ip or tc.
> > 
> > The correct fix needs to go deeper and hit other things.
> > 
> 
> Hi, Stephen,
> 
> The root cause of this issue is that close_json_object() is being called in
> newline_indent(), resulting in a mismatch
> of {}.
> 
> When fixing this problem, I was unsure why a newline() is needed in pretty
> mode, so I simply kept this logic and
> solved the issue of open_json_object() and close_json_object() not matching.
> However, If the output of pretty mode
> and not-pretty mode should be the same, then this problem can be resolved by
> deleting this newline_indent().

Stephen didn't say that output of pretty and not-pretty should be the
same, but he said that JSON logic should be the same.

Thanks

> 
> I believe the original developer may not have realized that
> close_json_object() is being called in newline(), which leads
> to this problem. To improve the code's readability, I would try to strip out
> close_json_obejct() from newline().
> 
> Thanks,
> Chengchang Tang
> 

Re: [PATCH iproute2-rc 1/2] rdma: Fix core dump when pretty is used

[Chengchang Tang]

On 2024/1/2 16:32, Leon Romanovsky wrote:
> On Tue, Jan 02, 2024 at 03:44:29PM +0800, Chengchang Tang wrote:
>>
>> On 2023/12/30 1:21, Stephen Hemminger wrote:
>>> On Fri, 29 Dec 2023 14:52:40 +0800
>>> Junxian Huang <huangjunxian6@hisilicon.com> wrote:
>>>
>>>> From: Chengchang Tang <tangchengchang@huawei.com>
>>>>
>>>> There will be a core dump when pretty is used as the JSON object
>>>> hasn't been opened and closed properly.
>>>>
>>>> Before:
>>>> $ rdma res show qp -jp -dd
>>>> [ {
>>>>       "ifindex": 1,
>>>>       "ifname": "hns_1",
>>>>       "port": 1,
>>>>       "lqpn": 1,
>>>>       "type": "GSI",
>>>>       "state": "RTS",
>>>>       "sq-psn": 0,
>>>>       "comm": "ib_core"
>>>> },
>>>> "drv_sq_wqe_cnt": 128,
>>>> "drv_sq_max_gs": 2,
>>>> "drv_rq_wqe_cnt": 512,
>>>> "drv_rq_max_gs": 1,
>>>> rdma: json_writer.c:130: jsonw_end: Assertion `self->depth > 0' failed.
>>>> Aborted (core dumped)
>>>>
>>>> After:
>>>> $ rdma res show qp -jp -dd
>>>> [ {
>>>>           "ifindex": 2,
>>>>           "ifname": "hns_2",
>>>>           "port": 1,
>>>>           "lqpn": 1,
>>>>           "type": "GSI",
>>>>           "state": "RTS",
>>>>           "sq-psn": 0,
>>>>           "comm": "ib_core",{
>>>>               "drv_sq_wqe_cnt": 128,
>>>>               "drv_sq_max_gs": 2,
>>>>               "drv_rq_wqe_cnt": 512,
>>>>               "drv_rq_max_gs": 1,
>>>>               "drv_ext_sge_sge_cnt": 256
>>>>           }
>>>>       } ]
>>>>
>>>> Fixes: 331152752a97 ("rdma: print driver resource attributes")
>>>> Signed-off-by: Chengchang Tang <tangchengchang@huawei.com>
>>>> Signed-off-by: Junxian Huang <huangjunxian6@hisilicon.com>
>>> This code in rdma seems to be miking json and newline functionality
>>> which creates bug traps.
>>>
>>> Also the json should have same effective output in pretty and non-pretty mode.
>>> It looks like since pretty mode add extra object layer, the nesting of {} would be
>>> different.
>>>
>>> The conversion to json_print() was done but it isn't using same conventions
>>> as ip or tc.
>>>
>>> The correct fix needs to go deeper and hit other things.
>>>
>> Hi, Stephen,
>>
>> The root cause of this issue is that close_json_object() is being called in
>> newline_indent(), resulting in a mismatch
>> of {}.
>>
>> When fixing this problem, I was unsure why a newline() is needed in pretty
>> mode, so I simply kept this logic and
>> solved the issue of open_json_object() and close_json_object() not matching.
>> However, If the output of pretty mode
>> and not-pretty mode should be the same, then this problem can be resolved by
>> deleting this newline_indent().
> Stephen didn't say that output of pretty and not-pretty should be the
> same, but he said that JSON logic should be the same.
>
> Thanks

Hi, Leon,

Thank you for your reply. But I'm not sure what you mean by JSON logic? 
I understand that
pretty and not-pretty JSON should have the same content, but just 
difference display effects.
Do you mean that they only need to have the same structure?

Or, let's get back to this question. In the JSON format output, the 
newline() here seems
unnecessary, because json_print() can solve the line break problems 
during printing.
So I think the newline() here can be removed at least when outputting in 
JSON format.

Thanks,
Chengchang Tang
>
>> I believe the original developer may not have realized that
>> close_json_object() is being called in newline(), which leads
>> to this problem. To improve the code's readability, I would try to strip out
>> close_json_obejct() from newline().
>>
>> Thanks,
>> Chengchang Tang
>>
> .
>

Re: [PATCH iproute2-rc 1/2] rdma: Fix core dump when pretty is used

[Leon Romanovsky]

On Tue, Jan 02, 2024 at 08:06:19PM +0800, Chengchang Tang wrote:
> 
> 
> On 2024/1/2 16:32, Leon Romanovsky wrote:
> > On Tue, Jan 02, 2024 at 03:44:29PM +0800, Chengchang Tang wrote:
> > > 
> > > On 2023/12/30 1:21, Stephen Hemminger wrote:
> > > > On Fri, 29 Dec 2023 14:52:40 +0800
> > > > Junxian Huang <huangjunxian6@hisilicon.com> wrote:
> > > > 
> > > > > From: Chengchang Tang <tangchengchang@huawei.com>
> > > > > 
> > > > > There will be a core dump when pretty is used as the JSON object
> > > > > hasn't been opened and closed properly.
> > > > > 
> > > > > Before:
> > > > > $ rdma res show qp -jp -dd
> > > > > [ {
> > > > >       "ifindex": 1,
> > > > >       "ifname": "hns_1",
> > > > >       "port": 1,
> > > > >       "lqpn": 1,
> > > > >       "type": "GSI",
> > > > >       "state": "RTS",
> > > > >       "sq-psn": 0,
> > > > >       "comm": "ib_core"
> > > > > },
> > > > > "drv_sq_wqe_cnt": 128,
> > > > > "drv_sq_max_gs": 2,
> > > > > "drv_rq_wqe_cnt": 512,
> > > > > "drv_rq_max_gs": 1,
> > > > > rdma: json_writer.c:130: jsonw_end: Assertion `self->depth > 0' failed.
> > > > > Aborted (core dumped)
> > > > > 
> > > > > After:
> > > > > $ rdma res show qp -jp -dd
> > > > > [ {
> > > > >           "ifindex": 2,
> > > > >           "ifname": "hns_2",
> > > > >           "port": 1,
> > > > >           "lqpn": 1,
> > > > >           "type": "GSI",
> > > > >           "state": "RTS",
> > > > >           "sq-psn": 0,
> > > > >           "comm": "ib_core",{
> > > > >               "drv_sq_wqe_cnt": 128,
> > > > >               "drv_sq_max_gs": 2,
> > > > >               "drv_rq_wqe_cnt": 512,
> > > > >               "drv_rq_max_gs": 1,
> > > > >               "drv_ext_sge_sge_cnt": 256
> > > > >           }
> > > > >       } ]
> > > > > 
> > > > > Fixes: 331152752a97 ("rdma: print driver resource attributes")
> > > > > Signed-off-by: Chengchang Tang <tangchengchang@huawei.com>
> > > > > Signed-off-by: Junxian Huang <huangjunxian6@hisilicon.com>
> > > > This code in rdma seems to be miking json and newline functionality
> > > > which creates bug traps.
> > > > 
> > > > Also the json should have same effective output in pretty and non-pretty mode.
> > > > It looks like since pretty mode add extra object layer, the nesting of {} would be
> > > > different.
> > > > 
> > > > The conversion to json_print() was done but it isn't using same conventions
> > > > as ip or tc.
> > > > 
> > > > The correct fix needs to go deeper and hit other things.
> > > > 
> > > Hi, Stephen,
> > > 
> > > The root cause of this issue is that close_json_object() is being called in
> > > newline_indent(), resulting in a mismatch
> > > of {}.
> > > 
> > > When fixing this problem, I was unsure why a newline() is needed in pretty
> > > mode, so I simply kept this logic and
> > > solved the issue of open_json_object() and close_json_object() not matching.
> > > However, If the output of pretty mode
> > > and not-pretty mode should be the same, then this problem can be resolved by
> > > deleting this newline_indent().
> > Stephen didn't say that output of pretty and not-pretty should be the
> > same, but he said that JSON logic should be the same.
> > 
> > Thanks
> 
> Hi, Leon,
> 
> Thank you for your reply. But I'm not sure what you mean by JSON logic? I
> understand that
> pretty and not-pretty JSON should have the same content, but just difference
> display effects.
> Do you mean that they only need to have the same structure?
> 
> Or, let's get back to this question. In the JSON format output, the
> newline() here seems
> unnecessary, because json_print() can solve the line break problems during
> printing.
> So I think the newline() here can be removed at least when outputting in
> JSON format.

I think that your original patch is correct way to fix the mismatch as
it is not related to pretty/non-pretty.

Thanks

> 
> Thanks,
> Chengchang Tang
> > 
> > > I believe the original developer may not have realized that
> > > close_json_object() is being called in newline(), which leads
> > > to this problem. To improve the code's readability, I would try to strip out
> > > close_json_obejct() from newline().
> > > 
> > > Thanks,
> > > Chengchang Tang
> > > 
> > .
> > 
> 
> 

Re: [PATCH iproute2-rc 1/2] rdma: Fix core dump when pretty is used

[Stephen Hemminger]

On Tue, 2 Jan 2024 14:21:06 +0200
Leon Romanovsky <leon@kernel.org> wrote:

> On Tue, Jan 02, 2024 at 08:06:19PM +0800, Chengchang Tang wrote:
> > 
> > 
> > On 2024/1/2 16:32, Leon Romanovsky wrote:  
> > > On Tue, Jan 02, 2024 at 03:44:29PM +0800, Chengchang Tang wrote:  
> > > > 
> > > > On 2023/12/30 1:21, Stephen Hemminger wrote:  
> > > > > On Fri, 29 Dec 2023 14:52:40 +0800
> > > > > Junxian Huang <huangjunxian6@hisilicon.com> wrote:
> > > > >   
> > > > > > From: Chengchang Tang <tangchengchang@huawei.com>
> > > > > > 
> > > > > > There will be a core dump when pretty is used as the JSON object
> > > > > > hasn't been opened and closed properly.
> > > > > > 
> > > > > > Before:
> > > > > > $ rdma res show qp -jp -dd
> > > > > > [ {
> > > > > >       "ifindex": 1,
> > > > > >       "ifname": "hns_1",
> > > > > >       "port": 1,
> > > > > >       "lqpn": 1,
> > > > > >       "type": "GSI",
> > > > > >       "state": "RTS",
> > > > > >       "sq-psn": 0,
> > > > > >       "comm": "ib_core"
> > > > > > },
> > > > > > "drv_sq_wqe_cnt": 128,
> > > > > > "drv_sq_max_gs": 2,
> > > > > > "drv_rq_wqe_cnt": 512,
> > > > > > "drv_rq_max_gs": 1,
> > > > > > rdma: json_writer.c:130: jsonw_end: Assertion `self->depth > 0' failed.
> > > > > > Aborted (core dumped)
> > > > > > 
> > > > > > After:
> > > > > > $ rdma res show qp -jp -dd
> > > > > > [ {
> > > > > >           "ifindex": 2,
> > > > > >           "ifname": "hns_2",
> > > > > >           "port": 1,
> > > > > >           "lqpn": 1,
> > > > > >           "type": "GSI",
> > > > > >           "state": "RTS",
> > > > > >           "sq-psn": 0,
> > > > > >           "comm": "ib_core",{
> > > > > >               "drv_sq_wqe_cnt": 128,
> > > > > >               "drv_sq_max_gs": 2,
> > > > > >               "drv_rq_wqe_cnt": 512,
> > > > > >               "drv_rq_max_gs": 1,
> > > > > >               "drv_ext_sge_sge_cnt": 256
> > > > > >           }
> > > > > >       } ]
> > > > > > 
> > > > > > Fixes: 331152752a97 ("rdma: print driver resource attributes")
> > > > > > Signed-off-by: Chengchang Tang <tangchengchang@huawei.com>
> > > > > > Signed-off-by: Junxian Huang <huangjunxian6@hisilicon.com>  
> > > > > This code in rdma seems to be miking json and newline functionality
> > > > > which creates bug traps.
> > > > > 
> > > > > Also the json should have same effective output in pretty and non-pretty mode.
> > > > > It looks like since pretty mode add extra object layer, the nesting of {} would be
> > > > > different.
> > > > > 
> > > > > The conversion to json_print() was done but it isn't using same conventions
> > > > > as ip or tc.
> > > > > 
> > > > > The correct fix needs to go deeper and hit other things.
> > > > >   
> > > > Hi, Stephen,
> > > > 
> > > > The root cause of this issue is that close_json_object() is being called in
> > > > newline_indent(), resulting in a mismatch
> > > > of {}.
> > > > 
> > > > When fixing this problem, I was unsure why a newline() is needed in pretty
> > > > mode, so I simply kept this logic and
> > > > solved the issue of open_json_object() and close_json_object() not matching.
> > > > However, If the output of pretty mode
> > > > and not-pretty mode should be the same, then this problem can be resolved by
> > > > deleting this newline_indent().  
> > > Stephen didn't say that output of pretty and not-pretty should be the
> > > same, but he said that JSON logic should be the same.
> > > 
> > > Thanks  
> > 
> > Hi, Leon,
> > 
> > Thank you for your reply. But I'm not sure what you mean by JSON logic? I
> > understand that
> > pretty and not-pretty JSON should have the same content, but just difference
> > display effects.
> > Do you mean that they only need to have the same structure?
> > 
> > Or, let's get back to this question. In the JSON format output, the
> > newline() here seems
> > unnecessary, because json_print() can solve the line break problems during
> > printing.
> > So I think the newline() here can be removed at least when outputting in
> > JSON format.  
> 
> I think that your original patch is correct way to fix the mismatch as
> it is not related to pretty/non-pretty.
> 
> Thanks

Part of the problem is the meaning of pretty mode is different in rdma
than all of the other commands. The meaning of the flags should be the
same across ip, devlink, tc, and rdma; therefore pretty should mean
nothing unless json is enabled.

I can do some of the rework here, but don't have any rdma hardware
to test on.

Re: [PATCH iproute2-rc 1/2] rdma: Fix core dump when pretty is used

[Leon Romanovsky]

On Tue, Jan 02, 2024 at 08:27:46AM -0800, Stephen Hemminger wrote:
> On Tue, 2 Jan 2024 14:21:06 +0200
> Leon Romanovsky <leon@kernel.org> wrote:
> 
> > On Tue, Jan 02, 2024 at 08:06:19PM +0800, Chengchang Tang wrote:
> > > 
> > > 
> > > On 2024/1/2 16:32, Leon Romanovsky wrote:  
> > > > On Tue, Jan 02, 2024 at 03:44:29PM +0800, Chengchang Tang wrote:  
> > > > > 
> > > > > On 2023/12/30 1:21, Stephen Hemminger wrote:  
> > > > > > On Fri, 29 Dec 2023 14:52:40 +0800
> > > > > > Junxian Huang <huangjunxian6@hisilicon.com> wrote:
> > > > > >   
> > > > > > > From: Chengchang Tang <tangchengchang@huawei.com>
> > > > > > > 
> > > > > > > There will be a core dump when pretty is used as the JSON object
> > > > > > > hasn't been opened and closed properly.
> > > > > > > 
> > > > > > > Before:
> > > > > > > $ rdma res show qp -jp -dd
> > > > > > > [ {
> > > > > > >       "ifindex": 1,
> > > > > > >       "ifname": "hns_1",
> > > > > > >       "port": 1,
> > > > > > >       "lqpn": 1,
> > > > > > >       "type": "GSI",
> > > > > > >       "state": "RTS",
> > > > > > >       "sq-psn": 0,
> > > > > > >       "comm": "ib_core"
> > > > > > > },
> > > > > > > "drv_sq_wqe_cnt": 128,
> > > > > > > "drv_sq_max_gs": 2,
> > > > > > > "drv_rq_wqe_cnt": 512,
> > > > > > > "drv_rq_max_gs": 1,
> > > > > > > rdma: json_writer.c:130: jsonw_end: Assertion `self->depth > 0' failed.
> > > > > > > Aborted (core dumped)
> > > > > > > 
> > > > > > > After:
> > > > > > > $ rdma res show qp -jp -dd
> > > > > > > [ {
> > > > > > >           "ifindex": 2,
> > > > > > >           "ifname": "hns_2",
> > > > > > >           "port": 1,
> > > > > > >           "lqpn": 1,
> > > > > > >           "type": "GSI",
> > > > > > >           "state": "RTS",
> > > > > > >           "sq-psn": 0,
> > > > > > >           "comm": "ib_core",{
> > > > > > >               "drv_sq_wqe_cnt": 128,
> > > > > > >               "drv_sq_max_gs": 2,
> > > > > > >               "drv_rq_wqe_cnt": 512,
> > > > > > >               "drv_rq_max_gs": 1,
> > > > > > >               "drv_ext_sge_sge_cnt": 256
> > > > > > >           }
> > > > > > >       } ]
> > > > > > > 
> > > > > > > Fixes: 331152752a97 ("rdma: print driver resource attributes")
> > > > > > > Signed-off-by: Chengchang Tang <tangchengchang@huawei.com>
> > > > > > > Signed-off-by: Junxian Huang <huangjunxian6@hisilicon.com>  
> > > > > > This code in rdma seems to be miking json and newline functionality
> > > > > > which creates bug traps.
> > > > > > 
> > > > > > Also the json should have same effective output in pretty and non-pretty mode.
> > > > > > It looks like since pretty mode add extra object layer, the nesting of {} would be
> > > > > > different.
> > > > > > 
> > > > > > The conversion to json_print() was done but it isn't using same conventions
> > > > > > as ip or tc.
> > > > > > 
> > > > > > The correct fix needs to go deeper and hit other things.
> > > > > >   
> > > > > Hi, Stephen,
> > > > > 
> > > > > The root cause of this issue is that close_json_object() is being called in
> > > > > newline_indent(), resulting in a mismatch
> > > > > of {}.
> > > > > 
> > > > > When fixing this problem, I was unsure why a newline() is needed in pretty
> > > > > mode, so I simply kept this logic and
> > > > > solved the issue of open_json_object() and close_json_object() not matching.
> > > > > However, If the output of pretty mode
> > > > > and not-pretty mode should be the same, then this problem can be resolved by
> > > > > deleting this newline_indent().  
> > > > Stephen didn't say that output of pretty and not-pretty should be the
> > > > same, but he said that JSON logic should be the same.
> > > > 
> > > > Thanks  
> > > 
> > > Hi, Leon,
> > > 
> > > Thank you for your reply. But I'm not sure what you mean by JSON logic? I
> > > understand that
> > > pretty and not-pretty JSON should have the same content, but just difference
> > > display effects.
> > > Do you mean that they only need to have the same structure?
> > > 
> > > Or, let's get back to this question. In the JSON format output, the
> > > newline() here seems
> > > unnecessary, because json_print() can solve the line break problems during
> > > printing.
> > > So I think the newline() here can be removed at least when outputting in
> > > JSON format.  
> > 
> > I think that your original patch is correct way to fix the mismatch as
> > it is not related to pretty/non-pretty.
> > 
> > Thanks
> 
> Part of the problem is the meaning of pretty mode is different in rdma
> than all of the other commands. The meaning of the flags should be the
> same across ip, devlink, tc, and rdma; therefore pretty should mean
> nothing unless json is enabled.

I was very inspired by devlink when wrote rdmatool. It is supposed to
behave the same. :)

> 
> I can do some of the rework here, but don't have any rdma hardware
> to test on.

We will test it for you.

Thanks

Re: [PATCH iproute2-rc 1/2] rdma: Fix core dump when pretty is used

[David Ahern]

On 1/2/24 12:17 PM, Leon Romanovsky wrote:
>>
>> Part of the problem is the meaning of pretty mode is different in rdma
>> than all of the other commands. The meaning of the flags should be the
>> same across ip, devlink, tc, and rdma; therefore pretty should mean
>> nothing unless json is enabled.
> 
> I was very inspired by devlink when wrote rdmatool. It is supposed to
> behave the same. :)

You need better inspirations :-)

It was a mistake to merge devlink source code into iproute2 without a
commitment to bring it inline with other iproute2 commands.

Re: [PATCH iproute2-rc 2/2] rdma: Fix the error of accessing string variable outside the lifecycle

[Junxian Huang]

Hi all,

the first patch is replaced by Stephen's latest patches. Are there any
comments to this patch?

Thanks,
Junxian

On 2023/12/29 14:52, Junxian Huang wrote:
> From: wenglianfa <wenglianfa@huawei.com>
> 
> All these SPRINT_BUF(b) definitions are inside the 'if' block, but
> accessed outside the 'if' block through the pointers 'comm'. This
> leads to empty 'comm' attribute when querying resource information.
> So move the definitions to the beginning of the functions to extend
> their life cycle.
> 
> Before:
> $ rdma res show srq
> dev hns_0 srqn 0 type BASIC lqpn 18 pdn 5 pid 7775 comm
> 
> After:
> $ rdma res show srq
> dev hns_0 srqn 0 type BASIC lqpn 18 pdn 5 pid 7775 comm ib_send_bw
> 
> Fixes: 1808f002dfdd ("lib/fs: fix memory leak in get_task_name()")
> Signed-off-by: wenglianfa <wenglianfa@huawei.com>
> Signed-off-by: Junxian Huang <huangjunxian6@hisilicon.com>
> ---
>  rdma/res-cmid.c | 3 +--
>  rdma/res-cq.c   | 3 +--
>  rdma/res-ctx.c  | 3 +--
>  rdma/res-mr.c   | 3 +--
>  rdma/res-pd.c   | 3 +--
>  rdma/res-qp.c   | 3 +--
>  rdma/res-srq.c  | 3 +--
>  rdma/stat.c     | 3 +--
>  8 files changed, 8 insertions(+), 16 deletions(-)
> 
> diff --git a/rdma/res-cmid.c b/rdma/res-cmid.c
> index 7371c3a6..595af848 100644
> --- a/rdma/res-cmid.c
> +++ b/rdma/res-cmid.c
> @@ -102,6 +102,7 @@ static int res_cm_id_line(struct rd *rd, const char *name, int idx,
>  	uint32_t lqpn = 0, ps;
>  	uint32_t cm_idn = 0;
>  	char *comm = NULL;
> +	SPRINT_BUF(b);
>  
>  	if (!nla_line[RDMA_NLDEV_ATTR_RES_STATE] ||
>  	    !nla_line[RDMA_NLDEV_ATTR_RES_PS])
> @@ -159,8 +160,6 @@ static int res_cm_id_line(struct rd *rd, const char *name, int idx,
>  		goto out;
>  
>  	if (nla_line[RDMA_NLDEV_ATTR_RES_PID]) {
> -		SPRINT_BUF(b);
> -
>  		pid = mnl_attr_get_u32(nla_line[RDMA_NLDEV_ATTR_RES_PID]);
>  		if (!get_task_name(pid, b, sizeof(b)))
>  			comm = b;
> diff --git a/rdma/res-cq.c b/rdma/res-cq.c
> index 2cfa4994..80994a03 100644
> --- a/rdma/res-cq.c
> +++ b/rdma/res-cq.c
> @@ -63,6 +63,7 @@ static int res_cq_line(struct rd *rd, const char *name, int idx,
>  	uint32_t cqn = 0;
>  	uint64_t users;
>  	uint32_t cqe;
> +	SPRINT_BUF(b);
>  
>  	if (!nla_line[RDMA_NLDEV_ATTR_RES_CQE] ||
>  	    !nla_line[RDMA_NLDEV_ATTR_RES_USECNT])
> @@ -84,8 +85,6 @@ static int res_cq_line(struct rd *rd, const char *name, int idx,
>  		goto out;
>  
>  	if (nla_line[RDMA_NLDEV_ATTR_RES_PID]) {
> -		SPRINT_BUF(b);
> -
>  		pid = mnl_attr_get_u32(nla_line[RDMA_NLDEV_ATTR_RES_PID]);
>  		if (!get_task_name(pid, b, sizeof(b)))
>  			comm = b;
> diff --git a/rdma/res-ctx.c b/rdma/res-ctx.c
> index 500186d9..99736ea0 100644
> --- a/rdma/res-ctx.c
> +++ b/rdma/res-ctx.c
> @@ -13,13 +13,12 @@ static int res_ctx_line(struct rd *rd, const char *name, int idx,
>  	char *comm = NULL;
>  	uint32_t ctxn = 0;
>  	uint32_t pid = 0;
> +	SPRINT_BUF(b);
>  
>  	if (!nla_line[RDMA_NLDEV_ATTR_RES_CTXN])
>  		return MNL_CB_ERROR;
>  
>  	if (nla_line[RDMA_NLDEV_ATTR_RES_PID]) {
> -		SPRINT_BUF(b);
> -
>  		pid = mnl_attr_get_u32(nla_line[RDMA_NLDEV_ATTR_RES_PID]);
>  		if (!get_task_name(pid, b, sizeof(b)))
>  			comm = b;
> diff --git a/rdma/res-mr.c b/rdma/res-mr.c
> index fb48d5df..e6c81d11 100644
> --- a/rdma/res-mr.c
> +++ b/rdma/res-mr.c
> @@ -30,6 +30,7 @@ static int res_mr_line(struct rd *rd, const char *name, int idx,
>  	uint32_t pdn = 0;
>  	uint32_t mrn = 0;
>  	uint32_t pid = 0;
> +	SPRINT_BUF(b);
>  
>  	if (!nla_line[RDMA_NLDEV_ATTR_RES_MRLEN])
>  		return MNL_CB_ERROR;
> @@ -47,8 +48,6 @@ static int res_mr_line(struct rd *rd, const char *name, int idx,
>  		goto out;
>  
>  	if (nla_line[RDMA_NLDEV_ATTR_RES_PID]) {
> -		SPRINT_BUF(b);
> -
>  		pid = mnl_attr_get_u32(nla_line[RDMA_NLDEV_ATTR_RES_PID]);
>  		if (!get_task_name(pid, b, sizeof(b)))
>  			comm = b;
> diff --git a/rdma/res-pd.c b/rdma/res-pd.c
> index 66f91f42..0dbb310a 100644
> --- a/rdma/res-pd.c
> +++ b/rdma/res-pd.c
> @@ -16,6 +16,7 @@ static int res_pd_line(struct rd *rd, const char *name, int idx,
>  	uint32_t pid = 0;
>  	uint32_t pdn = 0;
>  	uint64_t users;
> +	SPRINT_BUF(b);
>  
>  	if (!nla_line[RDMA_NLDEV_ATTR_RES_USECNT])
>  		return MNL_CB_ERROR;
> @@ -34,8 +35,6 @@ static int res_pd_line(struct rd *rd, const char *name, int idx,
>  			nla_line[RDMA_NLDEV_ATTR_RES_UNSAFE_GLOBAL_RKEY]);
>  
>  	if (nla_line[RDMA_NLDEV_ATTR_RES_PID]) {
> -		SPRINT_BUF(b);
> -
>  		pid = mnl_attr_get_u32(nla_line[RDMA_NLDEV_ATTR_RES_PID]);
>  		if (!get_task_name(pid, b, sizeof(b)))
>  			comm = b;
> diff --git a/rdma/res-qp.c b/rdma/res-qp.c
> index c180a97e..cd2f4aa2 100644
> --- a/rdma/res-qp.c
> +++ b/rdma/res-qp.c
> @@ -86,6 +86,7 @@ static int res_qp_line(struct rd *rd, const char *name, int idx,
>  	uint32_t port = 0, pid = 0;
>  	uint32_t pdn = 0;
>  	char *comm = NULL;
> +	SPRINT_BUF(b);
>  
>  	if (!nla_line[RDMA_NLDEV_ATTR_RES_LQPN] ||
>  	    !nla_line[RDMA_NLDEV_ATTR_RES_SQ_PSN] ||
> @@ -146,8 +147,6 @@ static int res_qp_line(struct rd *rd, const char *name, int idx,
>  		goto out;
>  
>  	if (nla_line[RDMA_NLDEV_ATTR_RES_PID]) {
> -		SPRINT_BUF(b);
> -
>  		pid = mnl_attr_get_u32(nla_line[RDMA_NLDEV_ATTR_RES_PID]);
>  		if (!get_task_name(pid, b, sizeof(b)))
>  			comm = b;
> diff --git a/rdma/res-srq.c b/rdma/res-srq.c
> index cf9209d7..758bb193 100644
> --- a/rdma/res-srq.c
> +++ b/rdma/res-srq.c
> @@ -183,13 +183,12 @@ static int res_srq_line(struct rd *rd, const char *name, int idx,
>  	char qp_str[MAX_QP_STR_LEN] = {};
>  	char *comm = NULL;
>  	uint8_t type = 0;
> +	SPRINT_BUF(b);
>  
>  	if (!nla_line[RDMA_NLDEV_ATTR_RES_SRQN])
>  		return MNL_CB_ERROR;
>  
>  	if (nla_line[RDMA_NLDEV_ATTR_RES_PID]) {
> -		SPRINT_BUF(b);
> -
>  		pid = mnl_attr_get_u32(nla_line[RDMA_NLDEV_ATTR_RES_PID]);
>  		if (!get_task_name(pid, b, sizeof(b)))
>  			comm = b;
> diff --git a/rdma/stat.c b/rdma/stat.c
> index 3df2c98f..c7dde680 100644
> --- a/rdma/stat.c
> +++ b/rdma/stat.c
> @@ -223,6 +223,7 @@ static int res_counter_line(struct rd *rd, const char *name, int index,
>  	struct nlattr *hwc_table, *qp_table;
>  	struct nlattr *nla_entry;
>  	const char *comm = NULL;
> +	SPRINT_BUF(b);
>  	bool isfirst;
>  	int err;
>  
> @@ -248,8 +249,6 @@ static int res_counter_line(struct rd *rd, const char *name, int index,
>  		return MNL_CB_OK;
>  
>  	if (nla_line[RDMA_NLDEV_ATTR_RES_PID]) {
> -		SPRINT_BUF(b);
> -
>  		pid = mnl_attr_get_u32(nla_line[RDMA_NLDEV_ATTR_RES_PID]);
>  		if (!get_task_name(pid, b, sizeof(b)))
>  			comm = b;

Re: [PATCH iproute2-rc 2/2] rdma: Fix the error of accessing string variable outside the lifecycle

[Petr Machata]

Junxian Huang <huangjunxian6@hisilicon.com> writes:

> the first patch is replaced by Stephen's latest patches. Are there any
> comments to this patch?

Yeah, what the code is currently doing is invalid.

Reviewed-by: Petr Machata <petrm@nvidia.com>

> Thanks,
> Junxian
>
> On 2023/12/29 14:52, Junxian Huang wrote:
>> From: wenglianfa <wenglianfa@huawei.com>
>> 
>> All these SPRINT_BUF(b) definitions are inside the 'if' block, but
>> accessed outside the 'if' block through the pointers 'comm'. This
>> leads to empty 'comm' attribute when querying resource information.
>> So move the definitions to the beginning of the functions to extend
>> their life cycle.
>> 
>> Before:
>> $ rdma res show srq
>> dev hns_0 srqn 0 type BASIC lqpn 18 pdn 5 pid 7775 comm
>> 
>> After:
>> $ rdma res show srq
>> dev hns_0 srqn 0 type BASIC lqpn 18 pdn 5 pid 7775 comm ib_send_bw
>> 
>> Fixes: 1808f002dfdd ("lib/fs: fix memory leak in get_task_name()")
>> Signed-off-by: wenglianfa <wenglianfa@huawei.com>
>> Signed-off-by: Junxian Huang <huangjunxian6@hisilicon.com>
>> ---
>>  rdma/res-cmid.c | 3 +--
>>  rdma/res-cq.c   | 3 +--
>>  rdma/res-ctx.c  | 3 +--
>>  rdma/res-mr.c   | 3 +--
>>  rdma/res-pd.c   | 3 +--
>>  rdma/res-qp.c   | 3 +--
>>  rdma/res-srq.c  | 3 +--
>>  rdma/stat.c     | 3 +--
>>  8 files changed, 8 insertions(+), 16 deletions(-)
>> 
>> diff --git a/rdma/res-cmid.c b/rdma/res-cmid.c
>> index 7371c3a6..595af848 100644
>> --- a/rdma/res-cmid.c
>> +++ b/rdma/res-cmid.c
>> @@ -102,6 +102,7 @@ static int res_cm_id_line(struct rd *rd, const char *name, int idx,
>>  	uint32_t lqpn = 0, ps;
>>  	uint32_t cm_idn = 0;
>>  	char *comm = NULL;
>> +	SPRINT_BUF(b);
>>  
>>  	if (!nla_line[RDMA_NLDEV_ATTR_RES_STATE] ||
>>  	    !nla_line[RDMA_NLDEV_ATTR_RES_PS])
>> @@ -159,8 +160,6 @@ static int res_cm_id_line(struct rd *rd, const char *name, int idx,
>>  		goto out;
>>  
>>  	if (nla_line[RDMA_NLDEV_ATTR_RES_PID]) {
>> -		SPRINT_BUF(b);
>> -
>>  		pid = mnl_attr_get_u32(nla_line[RDMA_NLDEV_ATTR_RES_PID]);
>>  		if (!get_task_name(pid, b, sizeof(b)))
>>  			comm = b;
>> diff --git a/rdma/res-cq.c b/rdma/res-cq.c
>> index 2cfa4994..80994a03 100644
>> --- a/rdma/res-cq.c
>> +++ b/rdma/res-cq.c
>> @@ -63,6 +63,7 @@ static int res_cq_line(struct rd *rd, const char *name, int idx,
>>  	uint32_t cqn = 0;
>>  	uint64_t users;
>>  	uint32_t cqe;
>> +	SPRINT_BUF(b);
>>  
>>  	if (!nla_line[RDMA_NLDEV_ATTR_RES_CQE] ||
>>  	    !nla_line[RDMA_NLDEV_ATTR_RES_USECNT])
>> @@ -84,8 +85,6 @@ static int res_cq_line(struct rd *rd, const char *name, int idx,
>>  		goto out;
>>  
>>  	if (nla_line[RDMA_NLDEV_ATTR_RES_PID]) {
>> -		SPRINT_BUF(b);
>> -
>>  		pid = mnl_attr_get_u32(nla_line[RDMA_NLDEV_ATTR_RES_PID]);
>>  		if (!get_task_name(pid, b, sizeof(b)))
>>  			comm = b;
>> diff --git a/rdma/res-ctx.c b/rdma/res-ctx.c
>> index 500186d9..99736ea0 100644
>> --- a/rdma/res-ctx.c
>> +++ b/rdma/res-ctx.c
>> @@ -13,13 +13,12 @@ static int res_ctx_line(struct rd *rd, const char *name, int idx,
>>  	char *comm = NULL;
>>  	uint32_t ctxn = 0;
>>  	uint32_t pid = 0;
>> +	SPRINT_BUF(b);
>>  
>>  	if (!nla_line[RDMA_NLDEV_ATTR_RES_CTXN])
>>  		return MNL_CB_ERROR;
>>  
>>  	if (nla_line[RDMA_NLDEV_ATTR_RES_PID]) {
>> -		SPRINT_BUF(b);
>> -
>>  		pid = mnl_attr_get_u32(nla_line[RDMA_NLDEV_ATTR_RES_PID]);
>>  		if (!get_task_name(pid, b, sizeof(b)))
>>  			comm = b;
>> diff --git a/rdma/res-mr.c b/rdma/res-mr.c
>> index fb48d5df..e6c81d11 100644
>> --- a/rdma/res-mr.c
>> +++ b/rdma/res-mr.c
>> @@ -30,6 +30,7 @@ static int res_mr_line(struct rd *rd, const char *name, int idx,
>>  	uint32_t pdn = 0;
>>  	uint32_t mrn = 0;
>>  	uint32_t pid = 0;
>> +	SPRINT_BUF(b);
>>  
>>  	if (!nla_line[RDMA_NLDEV_ATTR_RES_MRLEN])
>>  		return MNL_CB_ERROR;
>> @@ -47,8 +48,6 @@ static int res_mr_line(struct rd *rd, const char *name, int idx,
>>  		goto out;
>>  
>>  	if (nla_line[RDMA_NLDEV_ATTR_RES_PID]) {
>> -		SPRINT_BUF(b);
>> -
>>  		pid = mnl_attr_get_u32(nla_line[RDMA_NLDEV_ATTR_RES_PID]);
>>  		if (!get_task_name(pid, b, sizeof(b)))
>>  			comm = b;
>> diff --git a/rdma/res-pd.c b/rdma/res-pd.c
>> index 66f91f42..0dbb310a 100644
>> --- a/rdma/res-pd.c
>> +++ b/rdma/res-pd.c
>> @@ -16,6 +16,7 @@ static int res_pd_line(struct rd *rd, const char *name, int idx,
>>  	uint32_t pid = 0;
>>  	uint32_t pdn = 0;
>>  	uint64_t users;
>> +	SPRINT_BUF(b);
>>  
>>  	if (!nla_line[RDMA_NLDEV_ATTR_RES_USECNT])
>>  		return MNL_CB_ERROR;
>> @@ -34,8 +35,6 @@ static int res_pd_line(struct rd *rd, const char *name, int idx,
>>  			nla_line[RDMA_NLDEV_ATTR_RES_UNSAFE_GLOBAL_RKEY]);
>>  
>>  	if (nla_line[RDMA_NLDEV_ATTR_RES_PID]) {
>> -		SPRINT_BUF(b);
>> -
>>  		pid = mnl_attr_get_u32(nla_line[RDMA_NLDEV_ATTR_RES_PID]);
>>  		if (!get_task_name(pid, b, sizeof(b)))
>>  			comm = b;
>> diff --git a/rdma/res-qp.c b/rdma/res-qp.c
>> index c180a97e..cd2f4aa2 100644
>> --- a/rdma/res-qp.c
>> +++ b/rdma/res-qp.c
>> @@ -86,6 +86,7 @@ static int res_qp_line(struct rd *rd, const char *name, int idx,
>>  	uint32_t port = 0, pid = 0;
>>  	uint32_t pdn = 0;
>>  	char *comm = NULL;
>> +	SPRINT_BUF(b);
>>  
>>  	if (!nla_line[RDMA_NLDEV_ATTR_RES_LQPN] ||
>>  	    !nla_line[RDMA_NLDEV_ATTR_RES_SQ_PSN] ||
>> @@ -146,8 +147,6 @@ static int res_qp_line(struct rd *rd, const char *name, int idx,
>>  		goto out;
>>  
>>  	if (nla_line[RDMA_NLDEV_ATTR_RES_PID]) {
>> -		SPRINT_BUF(b);
>> -
>>  		pid = mnl_attr_get_u32(nla_line[RDMA_NLDEV_ATTR_RES_PID]);
>>  		if (!get_task_name(pid, b, sizeof(b)))
>>  			comm = b;
>> diff --git a/rdma/res-srq.c b/rdma/res-srq.c
>> index cf9209d7..758bb193 100644
>> --- a/rdma/res-srq.c
>> +++ b/rdma/res-srq.c
>> @@ -183,13 +183,12 @@ static int res_srq_line(struct rd *rd, const char *name, int idx,
>>  	char qp_str[MAX_QP_STR_LEN] = {};
>>  	char *comm = NULL;
>>  	uint8_t type = 0;
>> +	SPRINT_BUF(b);
>>  
>>  	if (!nla_line[RDMA_NLDEV_ATTR_RES_SRQN])
>>  		return MNL_CB_ERROR;
>>  
>>  	if (nla_line[RDMA_NLDEV_ATTR_RES_PID]) {
>> -		SPRINT_BUF(b);
>> -
>>  		pid = mnl_attr_get_u32(nla_line[RDMA_NLDEV_ATTR_RES_PID]);
>>  		if (!get_task_name(pid, b, sizeof(b)))
>>  			comm = b;
>> diff --git a/rdma/stat.c b/rdma/stat.c
>> index 3df2c98f..c7dde680 100644
>> --- a/rdma/stat.c
>> +++ b/rdma/stat.c
>> @@ -223,6 +223,7 @@ static int res_counter_line(struct rd *rd, const char *name, int index,
>>  	struct nlattr *hwc_table, *qp_table;
>>  	struct nlattr *nla_entry;
>>  	const char *comm = NULL;
>> +	SPRINT_BUF(b);
>>  	bool isfirst;
>>  	int err;
>>  
>> @@ -248,8 +249,6 @@ static int res_counter_line(struct rd *rd, const char *name, int index,
>>  		return MNL_CB_OK;
>>  
>>  	if (nla_line[RDMA_NLDEV_ATTR_RES_PID]) {
>> -		SPRINT_BUF(b);
>> -
>>  		pid = mnl_attr_get_u32(nla_line[RDMA_NLDEV_ATTR_RES_PID]);
>>  		if (!get_task_name(pid, b, sizeof(b)))
>>  			comm = b;

Re: [PATCH iproute2-rc 2/2] rdma: Fix the error of accessing string variable outside the lifecycle

[Andrea Claudi]

On Mon, Jan 08, 2024 at 09:28:52AM +0800, Junxian Huang wrote:
> 
> Hi all,
> 
> the first patch is replaced by Stephen's latest patches. Are there any
> comments to this patch?
> 
> Thanks,
> Junxian
>
> On 2023/12/29 14:52, Junxian Huang wrote:
> > From: wenglianfa <wenglianfa@huawei.com>
> > 
> > All these SPRINT_BUF(b) definitions are inside the 'if' block, but
> > accessed outside the 'if' block through the pointers 'comm'. This
> > leads to empty 'comm' attribute when querying resource information.
> > So move the definitions to the beginning of the functions to extend
> > their life cycle.
> > 
> > Before:
> > $ rdma res show srq
> > dev hns_0 srqn 0 type BASIC lqpn 18 pdn 5 pid 7775 comm
> > 
> > After:
> > $ rdma res show srq
> > dev hns_0 srqn 0 type BASIC lqpn 18 pdn 5 pid 7775 comm ib_send_bw
> > 
> > Fixes: 1808f002dfdd ("lib/fs: fix memory leak in get_task_name()")
> > Signed-off-by: wenglianfa <wenglianfa@huawei.com>
> > Signed-off-by: Junxian Huang <huangjunxian6@hisilicon.com>
> > ---

Hi Junxian,
For future patches, you can have a faster feedback adding to cc the
author of the original patch. In this case it's me, so here's my

Acked-by: Andrea Claudi <aclaudi@redhat.com>

Re: [PATCH iproute2-rc 2/2] rdma: Fix the error of accessing string variable outside the lifecycle

[Stephen Hemminger]

On Mon, 8 Jan 2024 17:10:04 +0100
Andrea Claudi <aclaudi@redhat.com> wrote:

> On Mon, Jan 08, 2024 at 09:28:52AM +0800, Junxian Huang wrote:
> > 
> > Hi all,
> > 
> > the first patch is replaced by Stephen's latest patches. Are there any
> > comments to this patch?
> > 
> > Thanks,
> > Junxian
> >
> > On 2023/12/29 14:52, Junxian Huang wrote:  
> > > From: wenglianfa <wenglianfa@huawei.com>
> > > 
> > > All these SPRINT_BUF(b) definitions are inside the 'if' block, but
> > > accessed outside the 'if' block through the pointers 'comm'. This
> > > leads to empty 'comm' attribute when querying resource information.
> > > So move the definitions to the beginning of the functions to extend
> > > their life cycle.
> > > 
> > > Before:
> > > $ rdma res show srq
> > > dev hns_0 srqn 0 type BASIC lqpn 18 pdn 5 pid 7775 comm
> > > 
> > > After:
> > > $ rdma res show srq
> > > dev hns_0 srqn 0 type BASIC lqpn 18 pdn 5 pid 7775 comm ib_send_bw
> > > 
> > > Fixes: 1808f002dfdd ("lib/fs: fix memory leak in get_task_name()")
> > > Signed-off-by: wenglianfa <wenglianfa@huawei.com>
> > > Signed-off-by: Junxian Huang <huangjunxian6@hisilicon.com>
> > > ---  
> 
> Hi Junxian,
> For future patches, you can have a faster feedback adding to cc the
> author of the original patch. In this case it's me, so here's my
> 
> Acked-by: Andrea Claudi <aclaudi@redhat.com>
> 

I just merged this one

Re: [PATCH iproute2-rc 2/2] rdma: Fix the error of accessing string variable outside the lifecycle

[Junxian Huang]

On 2024/1/9 0:10, Andrea Claudi wrote:
> On Mon, Jan 08, 2024 at 09:28:52AM +0800, Junxian Huang wrote:
>>
>> Hi all,
>>
>> the first patch is replaced by Stephen's latest patches. Are there any
>> comments to this patch?
>>
>> Thanks,
>> Junxian
>>
>> On 2023/12/29 14:52, Junxian Huang wrote:
>>> From: wenglianfa <wenglianfa@huawei.com>
>>>
>>> All these SPRINT_BUF(b) definitions are inside the 'if' block, but
>>> accessed outside the 'if' block through the pointers 'comm'. This
>>> leads to empty 'comm' attribute when querying resource information.
>>> So move the definitions to the beginning of the functions to extend
>>> their life cycle.
>>>
>>> Before:
>>> $ rdma res show srq
>>> dev hns_0 srqn 0 type BASIC lqpn 18 pdn 5 pid 7775 comm
>>>
>>> After:
>>> $ rdma res show srq
>>> dev hns_0 srqn 0 type BASIC lqpn 18 pdn 5 pid 7775 comm ib_send_bw
>>>
>>> Fixes: 1808f002dfdd ("lib/fs: fix memory leak in get_task_name()")
>>> Signed-off-by: wenglianfa <wenglianfa@huawei.com>
>>> Signed-off-by: Junxian Huang <huangjunxian6@hisilicon.com>
>>> ---
> 
> Hi Junxian,
> For future patches, you can have a faster feedback adding to cc the
> author of the original patch. In this case it's me, so here's my
> 
> Acked-by: Andrea Claudi <aclaudi@redhat.com>
> 

Thanks for the advice!

Junxian