netdev - RE: [PATCH net 1/2] devlink: Hold rtnl lock while reading netdev attributes

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:   Wed, 25 Nov 2020 18:17:15 +0000
From:   Parav Pandit <parav@...dia.com>
To:     Jakub Kicinski <kuba@...nel.org>
CC:     "davem@...emloft.net" <davem@...emloft.net>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        Jiri Pirko <jiri@...dia.com>
Subject: RE: [PATCH net 1/2] devlink: Hold rtnl lock while reading netdev
 attributes



> From: Jakub Kicinski <kuba@...nel.org>
> Sent: Wednesday, November 25, 2020 11:12 PM
> 
> On Wed, 25 Nov 2020 17:21:41 +0000 Parav Pandit wrote:
> > > From: Jakub Kicinski <kuba@...nel.org>
> > > Sent: Wednesday, November 25, 2020 10:00 PM
> > >
> > > On Wed, 25 Nov 2020 07:13:40 +0000 Parav Pandit wrote:
> > > > > Maybe even add a check that drivers which support reload set
> > > > > namespace local on their netdevs.
> > > > This will break the backward compatibility as orchestration for
> > > > VFs are not using devlink reload, which is supported very
> > > > recently. But yes, for SF who doesn't have backward compatibility
> > > > issue, as soon as initial series is merged, I will mark it as
> > > > local, so that orchestration doesn't start on wrong foot.
> > >
> > > Ah, right, that will not work because of the shenanigans you guys
> > > play with the uplink port. If all reprs are NETNS_LOCAL it'd not be an
> issue.
> > I am not sure what secret are you talking about with uplink.
> 
> I'm referring to Mellanox conflating PF with uplink. It's not a secret,
Ok.
> we argued about it in the past.
> 
> > I am taking about the SF netdevice to have the NETNS_LOCAL not the SF
> rep.
> > SF rep anyway has NETNS_LOCAL set.
> 
> All reps build by mlx5e_build_rep_netdev() have NETNS_LOCAL.
> 
Yes. this is clear to me and we are good here. 😊

> > I do not follow your comment - 'that will not work'. Can you please explain?
> 
> My half-baked suggestion was to basically add a:
> 
> 	WARN_ON(ops->reload_down && ops->reload_up &&
> 		!(netdev->priv & NETIF_F_NETNS_LOCAL));
> 
> to devlink_port_type_netdev_checks(). Given if device has a reload
> callback devlink is the way to change netns. But yeah, we can't break
> existing behavior so your uplink has to be movable and can't have
> NETNS_LOCAL. IOW adding the WARN_ON() won't work.
> 
Right.

> Hope this is crystal clear now.
Yes, its clear. Thanks.

I addressed your comments and cut down both the fixes to merely 7 lines change. Sent v2.