netdev - Re: [PATCH net-next] netdevsim: Register and unregister devlink traps on probe/remove device

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]

Date:   Mon, 25 Oct 2021 22:12:42 +0300
From:   Leon Romanovsky <leon@...nel.org>
To:     Jakub Kicinski <kuba@...nel.org>
Cc:     Ido Schimmel <idosch@...sch.org>,
        "David S . Miller" <davem@...emloft.net>,
        Ido Schimmel <idosch@...lanox.com>,
        Jiri Pirko <jiri@...lanox.com>, linux-kernel@...r.kernel.org,
        netdev@...r.kernel.org,
        syzbot+93d5accfaefceedf43c1@...kaller.appspotmail.com
Subject: Re: [PATCH net-next] netdevsim: Register and unregister devlink
 traps on probe/remove device

On Mon, Oct 25, 2021 at 11:24:53AM -0700, Jakub Kicinski wrote:
> On Mon, 25 Oct 2021 11:08:00 +0300 Ido Schimmel wrote:
> > No, it's not correct. After your patch, trap properties like action are
> > not set back to the default. Regardless of what you think is the "right
> > design", you cannot introduce such regressions.
> > 
> > Calling devlink_*_unregister() in reload_down() and devlink_*_register()
> > in reload_up() is not new. It is done for multiple objects (e.g., ports,
> > regions, shared buffer, etc). After your patch, netdevsim is still doing
> > it.
> 
> If we want to push forward in the direction that Leon advocates we'd
> have to unregister the devlink instance before reload_down(), right?

Not really, we ensure that during "devlink reload", users can't send any
get/set commands. So you don't need to unregister anything, because
driver is safe to change any internal values it wants. The devlink instance
is locked for exclusive access.

The protection is done now by devlink_mutex, but will be slightly
different in the near future.

> 
> Otherwise it seems fundamentally incompatible with the idea of reload
> for reconfig. And we'd be trading one partially correct model for
> another partially correct model :/

I see devlink reload as command to reconfigure device and not devlink
itself. This is the difference between my direction vs. mlxsw way.

Thanks