netdev - Re: [PATCH rdma-next 1/1] RDMA/mana_ib: Set correct device into ib

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: <20240626121118.GP29266@unreal>
Date: Wed, 26 Jun 2024 15:11:18 +0300
From: Leon Romanovsky <leon@...nel.org>
To: Konstantin Taranov <kotaranov@...rosoft.com>
Cc: Konstantin Taranov <kotaranov@...ux.microsoft.com>,
	Wei Hu <weh@...rosoft.com>,
	"sharmaajay@...rosoft.com" <sharmaajay@...rosoft.com>,
	Long Li <longli@...rosoft.com>, "jgg@...pe.ca" <jgg@...pe.ca>,
	"linux-rdma@...r.kernel.org" <linux-rdma@...r.kernel.org>,
	linux-netdev <netdev@...r.kernel.org>
Subject: Re: [PATCH rdma-next 1/1] RDMA/mana_ib: Set correct device into ib

On Wed, Jun 26, 2024 at 09:05:05AM +0000, Konstantin Taranov wrote:
> > > When mc->ports[0] is not slave, use it in the set_netdev.
> > > When mana is used in netvsc, the stored net devices in mana are slaves
> > > and GIDs should be taken from their master devices.
> > > In the baremetal case, the mc->ports devices will not be slaves.
> > 
> > I wonder, why do you have "... | IFF_SLAVE" in __netvsc_vf_setup() in a first
> > place? Isn't IFF_SLAVE is supposed to be set by bond driver?
> > 
> 
> I guess it is just a valid use of the IFF_SLAVE bit. In the bond case it is also set
> as a BOND netdev. The IFF_SLAVE helps to show users that another master
> netdev should be used for networking. But I am not an expert in netvsc.

The thing is that netvsc is virtual device like many others, but it is
the only one who uses IFF_SLAVE bit. The comment around that bit says
"slave of a load balancer.", which is not the case according to the
Hyper-V documentation.
https://learn.microsoft.com/en-us/windows-hardware/drivers/network/overview-of-hyper-v

You will need to get Ack from netdev maintainers to rely on IFF_SLAVE
bit in the way you are relying on it now.

> 
> Actually, another alternative solution for mana_ib is always set the slave device,
> but in the GID mgmt code we need the following patch. The problem is that it may require 
> testing/confirmation from other ib providers as in the worst case some GIDs will not be listed.

is_eth_active_slave_of_bonding_rcu() is for bonding.

> 
> diff --git a/drivers/infiniband/core/roce_gid_mgmt.c b/drivers/infiniband/core/roce_gid_mgmt.c
> index d5131b3ba8ab..0f20b4e2d1c2 100644
> --- a/drivers/infiniband/core/roce_gid_mgmt.c
> +++ b/drivers/infiniband/core/roce_gid_mgmt.c
> @@ -141,6 +141,8 @@ static enum bonding_slave_state is_eth_active_slave_of_bonding_rcu(struct net_de
>         return BONDING_SLAVE_STATE_NA;
>  }
> 
> +#define netdev_is_slave(dev)   (((dev)->flags & IFF_SLAVE) == IFF_SLAVE)
> +
>  #define REQUIRED_BOND_STATES           (BONDING_SLAVE_STATE_ACTIVE |   \
>                                          BONDING_SLAVE_STATE_NA)
>  static bool
> @@ -157,11 +159,14 @@ is_eth_port_of_netdev_filter(struct ib_device *ib_dev, u32 port,
>         real_dev = rdma_vlan_dev_real_dev(cookie);
>         if (!real_dev)
>                 real_dev = cookie;
> -
> +       /*
> +        * When rdma netdevice is used in netvsc, the master netdevice should
> +        * be considered for GIDs. Therefore, ignore slave rdma netdevices.
> +        */
>         res = ((rdma_is_upper_dev_rcu(rdma_ndev, cookie) &&
>                (is_eth_active_slave_of_bonding_rcu(rdma_ndev, real_dev) &
>                 REQUIRED_BOND_STATES)) ||
> -              real_dev == rdma_ndev);
> +              (real_dev == rdma_ndev && !netdev_is_slave(real_dev)));
> 
>         rcu_read_unlock();
>         return res;
> @@ -211,12 +216,14 @@ is_ndev_for_default_gid_filter(struct ib_device *ib_dev, u32 port,
> 
>         /*
>          * When rdma netdevice is used in bonding, bonding master netdevice
> -        * should be considered for default GIDs. Therefore, ignore slave rdma
> -        * netdevices when bonding is considered.
> +        * should be considered for default GIDs.
> +        * When rdma netdevice is used in netvsc, the master netdevice should
> +        * be considered for defauld GIDs. Therefore, ignore slave rdma
> +        * netdevices.
>          * Additionally when event(cookie) netdevice is bond master device,
>          * make sure that it the upper netdevice of rdma netdevice.
>          */
> -       res = ((cookie_ndev == rdma_ndev && !netif_is_bond_slave(rdma_ndev)) ||
> +       res = ((cookie_ndev == rdma_ndev && !netdev_is_slave(rdma_ndev)) ||
>                (netif_is_bond_master(cookie_ndev) &&
>                 rdma_is_upper_dev_rcu(rdma_ndev, cookie_ndev)));
> 
> > > +#define mana_ndev_is_slave(dev)   (((dev)->flags & IFF_SLAVE) ==
> > IFF_SLAVE)
> > 
> > There is no need in macro for one line of code and there is no need in "==",
> > as the result will be boolean.
> > 
> 
> Sure, can address in v2. I just saw a similar macro in another kernel file.

I grepped too and this is why it caused me to wonder why it is not used
except small number of places.

Thanks

> 
>