Re: [PATCH net-next v2 3/5] devlink: Allow set specific ops callbacks dynamically

On Tue, Oct 05, 2021 at 11:32:13AM -0700, Jakub Kicinski wrote:

On Tue, 5 Oct 2021 10:32:45 +0300 Leon Romanovsky wrote:

quoted

On Mon, Oct 04, 2021 at 04:44:13PM -0700, Jakub Kicinski wrote:

quoted

On Sun,  3 Oct 2021 21:12:04 +0300 Leon Romanovsky wrote:  

quoted

From: Leon Romanovsky <leonro@nvidia.com>

Introduce new devlink call to set specific ops callback during
device initialization phase after devlink_alloc() is already
called.

This allows us to set specific ops based on device property which
is not known at the beginning of driver initialization.

For the sake of simplicity, this API lacks any type of locking and
needs to be called before devlink_register() to make sure that no
parallel access to the ops is possible at this stage.  

The fact that it's not registered does not mean that the callbacks
won't be invoked. Look at uses of devlink_compat_flash_update().  

It is impossible, devlink_register() is part of .probe() flow and if it
wasn't called -> probe didn't success -> net_device doesn't exist.

Are you talking about reality or the bright future brought by auxbus?

I looked on all the drivers which called to devlink_alloc() which is
starting point before devlink_register(). All of them used it in the
probe. My annotation patch checks that too.

https://lore.kernel.org/linux-rdma/f65772d429d2c259bbc18cf5b1bbe61e39eb7081.1633284302.git.leonro@nvidia.com/T/#u (local)

So IMHO, it is reality.

quoted

We are not having net_device without "connected" device beneath, aren't we?

At least drivers that I checked are not prepared at all to handle call
to devlink->ops.flash_update() if they didn't probe successfully.

Last time I checked you moved the devlink_register() at the end of
probe which for all no-auxbus drivers means after register_netdev().

I need to add a check of if(devlink_register) inside devlink_compat_flash_update().

quoted

quoted

quoted

diff --git a/net/core/devlink.c b/net/core/devlink.c
index 4e484afeadea..25c2aa2b35cd 100644
--- a/net/core/devlink.c
+++ b/net/core/devlink.c

@@ -53,7 +53,7 @@ struct devlink {
 	struct list_head trap_list;
 	struct list_head trap_group_list;
 	struct list_head trap_policer_list;
-	const struct devlink_ops *ops;
+	struct devlink_ops ops;  

Security people like ops to live in read-only memory. You're making
them r/w for every devlink instance now.  

Yes, but we are explicitly copy every function pointer, which is safe.

The goal is for ops to live in pages which are mapped read-only,
so that heap overflows can overwrite the pointers.

<...>

I don't like it. If you're feeling strongly please gather support of
other developers. Right now it's my preference against yours. I don't
even see you making arguments that your approach is better, just that
mine is not perfect and requires some similar changes.

I have an idea of how to keep static ops and allow devlink_set_ops()
like functionality.

What about if I group ops by some sort of commonalities?

In my case, it will be devlink_reload_ops, which will include reload
relevant callbacks and provide devlink_set_reload_ops() wrapper to set
them?

It will ensure that all pointers are const without need to have feature
bits.

Thanks