netdev - Re: [PATCH net v2] devlink: Delay health recover notification until devlink registered

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 for Android: free password hash cracker in your pocket

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

Message-ID: <ZNXrr7MspgDp8QfA@nanopsycho>
Date: Fri, 11 Aug 2023 10:05:03 +0200
From: Jiri Pirko <jiri@...nulli.us>
To: Jakub Kicinski <kuba@...nel.org>
Cc: Shay Drory <shayd@...dia.com>, netdev@...r.kernel.org,
	pabeni@...hat.com, davem@...emloft.net, edumazet@...gle.com,
	Jiri Pirko <jiri@...dia.com>
Subject: Re: [PATCH net v2] devlink: Delay health recover notification until
 devlink registered

Thu, Aug 10, 2023 at 07:33:00PM CEST, kuba@...nel.org wrote:
>On Wed, 9 Aug 2023 23:35:21 +0300 Shay Drory wrote:
>> From one side, devl_register() is done last in device initialization
>> phase, in order to expose devlink to the user only when device is
>> ready. From second side, it is valid to create health reporters
>> during device initialization, in order to recover and/or notify the
>> user.
>> As a result, a health recover can be invoked before devl_register().
>> However, invoking health recover before devl_register() triggers a
>> WARN_ON.
>
>My comment on v1 wasn't clear enough, I guess.
>
>What I was trying to get across is that because drivers can take
>devl_lock(), devl_register() does not have to be last.
>
>AFAIU your driver does:
>
>  devlink_port_health_reporter_create()
>  ...
>  devlink_register()
>
>why not change it to do:
>
>  devl_lock()
>  devl_register()
>  devl_port_health_reporter_create()
>  ...
>  devl_unlock() # until unlock user space can't access the instance

This patch is not about user accessing it, this is about
notification that would be tried to send before the instance is
registered. So the lock scheme you suggest is not necessary. What helps
is to move devl_port_health_reporter_create() call after register.

We have the same issue with mlxsw where this notification may be called
before register from mlxsw_core_health_event_work()

Limiting the creation of health reporter only when instance is
registered does not seem like a good solution to me.

As with any other object, we postpone the notifications only after
register is done, that sounds fine, doesn't it?

>
>?
>