linux-kernel - Re: [PATCH] RDMA/rxe: Use kzalloc() to alloc map

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <123f5c7e-c2eb-7703-ace9-bc5074a394dd@fujitsu.com>
Date:   Wed, 25 May 2022 01:31:44 +0000
From:   "lizhijian@...itsu.com" <lizhijian@...itsu.com>
To:     Haris Iqbal <haris.iqbal@...os.com>
CC:     Jason Gunthorpe <jgg@...dia.com>,
        Bob Pearson <rpearsonhpe@...il.com>,
        Zhu Yanjun <zyjzyj2000@...il.com>,
        "linux-rdma@...r.kernel.org" <linux-rdma@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        Guoqing Jiang <guoqing.jiang@...ux.dev>,
        Aleksei Marov <aleksei.marov@...os.com>,
        Jinpu Wang <jinpu.wang@...os.com>
Subject: Re: [PATCH] RDMA/rxe: Use kzalloc() to alloc map_set



On 24/05/2022 18:56, Haris Iqbal wrote:
> On Tue, May 24, 2022 at 6:00 AM lizhijian@...itsu.com
> <lizhijian@...itsu.com> wrote:
>> Hi Jason & Bob
>> CC Guoqing
>>
>> @Guoqing, It may correlate with your previous bug report: https://lore.kernel.org/all/20220210073655.42281-1-guoqing.jiang@linux.dev/T/
>>
>>
>> It's observed that a same MR in rnbd server will trigger below code
>> path:
>>    -> rxe_mr_init_fast()
>>    |-> alloc map_set() # map_set is uninitialized
>>    |...-> rxe_map_mr_sg() # build the map_set
>>        |-> rxe_mr_set_page()
>>    |...-> rxe_reg_fast_mr() # mr->state change to VALID from FREE that means
>>                             # we can access host memory(such rxe_mr_copy)
>>    |...-> rxe_invalidate_mr() # mr->state change to FREE from VALID
>>    |...-> rxe_reg_fast_mr() # mr->state change to VALID from FREE,
>>                             # but map_set was not built again
>>    |...-> rxe_mr_copy() # kernel crash due to access wild addresses
>>                         # that lookup from the map_set
>>
>> I draft a patch like below for it, but i wonder if it's rxe's responsibility to do such checking.
>> Any comments are very welcome.
>>
> Thanks for posting the description and the patch.
>
> We have been facing the exact same issue (only with rxe),

> and we also
> realized that to get around this we will have to call ib_map_mr_sg()
> before every IB_WR_REG_MR wr; even if we are reusing the MR and simply
> invalidating and re-validating them.
Yes, this workaround should work but expensive.
It seems Bob has started a new thread to discuss the FMRs in https://www.spinics.net/lists/linux-rdma/msg110836.html

Thanks
Zhijian

>
> In reference to this, we have 2 questions.
>
> 1) This change was made in the following commit.
>
> commit 647bf13ce944f20f7402f281578423a952274e4a
> Author: Bob Pearson <rpearsonhpe@...il.com>
> Date:   Tue Sep 14 11:42:06 2021 -0500
>
>      RDMA/rxe: Create duplicate mapping tables for FMRs
>
>      For fast memory regions create duplicate mapping tables so ib_map_mr_sg()
>      can build a new mapping table which is then swapped into place
>      synchronously with the execution of an IB_WR_REG_MR work request.
>
>      Currently the rxe driver uses the same table for receiving RDMA operations
>      and for building new tables in preparation for reusing the MR. This
>      exposes users to potentially incorrect results.
>
>      Link: https://lore.kernel.org/r/20210914164206.19768-5-rpearsonhpe@gmail.com
>      Signed-off-by: Bob Pearson <rpearsonhpe@...il.com>
>      Signed-off-by: Jason Gunthorpe <jgg@...dia.com>
>
> We tried to understand what potential incorrect result that commit
> message talks about, but were not able to. If someone can through
> light into this scenario where single mapping table can result into
> issues, it would be great.
>
> 2)
> We wanted to confirm that, with the above patch, it clearly means that
> the use-case where we reuse the MR, by simply invalidating and
> re-validating (IB_WR_REG_MR wr) is a correct one.
> And there is no requirement saying that ib_map_mr_sg() needs to be
> done everytime regardless.
>
> (We were planning to send this in the coming days, but wanted other
> discussions to get over. Since the patch got posted and discussion
> started, we thought better to put this out.)
>
> Regards
>
>>
>> On 23/05/2022 22:02, Li, Zhijian wrote:
>>> on 2022/5/20 22:45, Jason Gunthorpe wrote:
>>>> On Wed, May 18, 2022 at 12:37:25PM +0800, Li Zhijian wrote:
>>>>> Below call chains will alloc map_set without fully initializing map_set.
>>>>> rxe_mr_init_fast()
>>>>>    -> rxe_mr_alloc()
>>>>>       -> rxe_mr_alloc_map_set()
>>>>>
>>>>> Uninitialized values inside struct rxe_map_set are possible to cause
>>>>> kernel panic.
>>>> If the value is uninitialized then why is 0 an OK value?
>>>>
>>>> Would be happier to know the exact value that is not initialized
>>> Well, good question. After re-think of this issue, it seems this patch wasn't the root cause though it made the crash disappear in some extent.
>>>
>>> I'm still working on the root cause :)
>>>
>>> Thanks
>>>
>>> Zhijian
>>>
>>>
>>>> Jason
>>>