netdev - Re: [not-yet-signed PATCH] RDMA/core: flush gid_cache_wq WQ from disable

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: <ace1ebe4-4fdb-49f4-a3fa-bbf11e1b40ed@I-love.SAKURA.ne.jp>
Date: Tue, 16 Dec 2025 22:29:29 +0900
From: Tetsuo Handa <penguin-kernel@...ove.SAKURA.ne.jp>
To: Leon Romanovsky <leon@...nel.org>, Majd Dibbiny <majd@...lanox.com>,
        Doug Ledford <dledford@...hat.com>, Jason Gunthorpe <jgg@...dia.com>,
        Yuval Shaia <yshaia@...vell.com>, Matan Barak <matanb@...lanox.com>
Cc: Bernard Metzler <bernard.metzler@...ux.dev>,
        OFED mailing list <linux-rdma@...r.kernel.org>,
        Network Development <netdev@...r.kernel.org>
Subject: Re: [not-yet-signed PATCH] RDMA/core: flush gid_cache_wq WQ from
 disable_device()

Hmm, I misinterpreted that commit. Since ib_register_device() was doing

	down_write(&lists_rwsem);
	list_add_tail(&device->core_list, &device_list);
	up_write(&lists_rwsem);

, it was

	down_write(&lists_rwsem);
	list_del(&device->core_list);

in ib_unregister_device() that makes ib_enum_all_roce_netdevs() no longer
call ib_enum_roce_netdev().

Then, calling ib_enum_all_roce_netdevs() asynchronously was always racy
since commit 03db3a2d81e6 ("IB/core: Add RoCE GID table management") was added?

On 2025/12/16 20:28, Tetsuo Handa wrote:
> Hello, Jason Gunthorpe.
> 
> Commit 0df91bb67334 ("RDMA/devices: Use xarray to store the client_data")
> says
> 
>     Since xarray can store 'going_down' using a mark just entirely eliminate
>     the struct ib_client_data and directly store the client_data value in the
>     xarray. However this does require a special iterator as we must still
>     iterate over any NULL client_data values.
> 
> and started using DEVICE_REGISTERED mark. But It seems to me that that commit
> has missed that ib_enum_roce_netdev() from ib_enum_all_roce_netdevs() is called
> asynchronously from WQ context. Due to that commit, ib_enum_roce_netdev() became
> no longer being called as soon as DEVICE_REGISTERED is cleared in
> ib_unregister_device(); I guess that that commit needed to wait for pending
> work items in gid_cache_wq WQ to complete. What do you think?
> 
> On 2025/12/15 23:09, Tetsuo Handa wrote:
>> On 2025/12/11 22:24, Tetsuo Handa wrote:
>>> Since a reproducer for this bug is not available, I haven't verified
>>> whether this is a bug syzbot is currently reporting in
>>> https://syzkaller.appspot.com/bug?extid=881d65229ca4f9ae8c84 .
>>> But I'd like to add Reported-by: syzbot if netdevice_event_work_handler()
>>> is supposed to be called for releasing GID entry upon NETDEV_UNREGISTER
>>> event. Thus, please review this change.
>>
>> I can observe using simple atomic_t counters that there are sometimes pending
>> netdevice_event() works as of immediately before clearing DEVICE_REGISTERED flag.
>> That is, clearing DEVICE_REGISTERED flag without flushing pending netdevice_event()
>> works results in failing to process some of netdev events.
>>
>> I considered resolving DEVICE_REGISTERED flag inside netdevice_event() and then
>> flush pending netdevice_event() works after clearing DEVICE_REGISTERED flag (diff
>> is shown below). But I immediately got circular locking dependency problem by just
>> executing "rdma link add siw0 type siw netdev lo" command line. Therefore, I guess
>> that the reason RDMA code defers netdevice_event() handling to WQ context is to
>> avoid circular locking dependency problem. But I guess that due to lack of reliable
>> flushing mechanism when clearing DEVICE_REGISTERED flag, sometimes operations for
>> deleting GID entry are not invoked, and syzbot is reporting refcount leak...