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

From: Edwin Peer <edwin.peer@broadcom.com>
To: Leon Romanovsky <leon@kernel.org>
Cc: Ido Schimmel <idosch@idosch.org>,
	"David S . Miller" <davem@davemloft.net>,
	Jakub Kicinski <kuba@kernel.org>,
	Ido Schimmel <idosch@mellanox.com>,
	Jiri Pirko <jiri@mellanox.com>,
	Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
	netdev <netdev@vger.kernel.org>,
	syzbot+93d5accfaefceedf43c1@syzkaller.appspotmail.com,
	Michael Chan <michael.chan@broadcom.com>
Subject: Re: [PATCH net-next] netdevsim: Register and unregister devlink traps on probe/remove device
Date: Wed, 27 Oct 2021 01:46:44 -0700	[thread overview]
Message-ID: <CAKOOJTyrUUydu9aNJSB4S_5dfqjkc6Y-14up4-V+aNcQ7TWVdQ@mail.gmail.com> (raw)
In-Reply-To: <YXj1J/Z8HYvBWC6Y@unreal>

On Tue, Oct 26, 2021 at 11:43 PM Leon Romanovsky <leon@kernel.org> wrote:

> In our case, the eth driver is part of mlx5_core module, so at the
> device creation phase that module is already loaded and driver/core
> will try to autoprobe it.

> However, the last step is not always performed and controlled by the
> userspace. Users can disable driver autoprobe and bind manually. This
> is pretty standard practice in the SR-IOV or VFIO modes.

While you say the netdev will not necessarily be bound, that still
sounds like the netdev will indeed be presented to user space before
devlink_register() when it is auto-probed?

> This is why devlink has monitor mode where you can see devlink device
> addition and removal. It is user space job to check that device is
> ready.

Isn't it more a question of what existing user space _does_ rather
than what user space _should_ do?

> > This isn't about kernel API. This is precisely about existing user
> > space that expects devlink to work immediately after the netdev
> > appears.
>
> Can you please share open source project that has such assumption?

I'm no python expert, but it looks like
https://github.com/openstack-charmers/mlnx-switchdev-mode/ might.
We've certainly had implicit user space assumptions trip over
registration order before, hence the change we made in January last
year to move devlink registration earlier. Granted, upon deeper
analysis, that specific case pertained to phys port name via sysfs,
which technically only needs port attrs via ndo_get_devlink_port, not
devlink_register(). That said, I'm certainly not confident that there
are no other existing users that might expect to be able to invoke
devlink in ifup scripts.

> > What do you suggest instead?
>
> Fix your test respect devlink notifications and don't ignore them.

That's not very helpful. The test case does what the user in the field
did to break it. We can't assume users have always been using our APIs
the way we intended.

Regards,
Edwin Peer