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

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:   Tue, 5 Oct 2021 22:15:40 +0300
From:   Leon Romanovsky <leon@...nel.org>
To:     Jakub Kicinski <kuba@...nel.org>
Cc:     "David S . Miller" <davem@...emloft.net>,
        Ido Schimmel <idosch@...dia.com>,
        Ingo Molnar <mingo@...hat.com>, Jiri Pirko <jiri@...dia.com>,
        linux-kernel@...r.kernel.org, linux-rdma@...r.kernel.org,
        mlxsw@...dia.com, Moshe Shemesh <moshe@...dia.com>,
        netdev@...r.kernel.org, Saeed Mahameed <saeedm@...dia.com>,
        Salil Mehta <salil.mehta@...wei.com>,
        Shay Drory <shayd@...dia.com>,
        Steven Rostedt <rostedt@...dmis.org>,
        Tariq Toukan <tariqt@...dia.com>,
        Yisen Zhuang <yisen.zhuang@...wei.com>
Subject: 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:
> > On Mon, Oct 04, 2021 at 04:44:13PM -0700, Jakub Kicinski wrote:
> > > On Sun,  3 Oct 2021 21:12:04 +0300 Leon Romanovsky wrote:  
> > > > From: Leon Romanovsky <leonro@...dia.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

So IMHO, it is reality.

> 
> > 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().

> 
> > > > 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