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]

Message-ID: <20201125094157.737d37aa@kicinski-fedora-pc1c0hjn.dhcp.thefacebook.com>
Date:   Wed, 25 Nov 2020 09:41:57 -0800
From:   Jakub Kicinski <kuba@...nel.org>
To:     Parav Pandit <parav@...dia.com>
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

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

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

Hope this is crystal clear now.

> Do you mean I should take care for SF's netdevice to have NETNS_LOCAL in first patchset or you mean setting NETNS_LOCAL for VF's Netdev will not work?
> If its later, sure it will break the backward compatibility, so will not do as default.