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

[Leon Romanovsky <leon@kernel.org>]

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