| Message ID | 1698440390-13719-1-git-send-email-longli@linuxonhyperv.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [v2] hv_netvsc: Mark VF as slave before exposing it to user-mode | expand |
On Fri, 27 Oct 2023 13:59:50 -0700 longli@linuxonhyperv.com wrote: > When a VF is being exposed form the kernel, it should be marked as "slave" > before exposing to the user-mode. The VF is not usable without netvsc running > as master. The user-mode should never see a VF without the "slave" flag. > > This commit moves the code of setting the slave flag to the time before VF is > exposed to user-mode. Can you give a real example in the commit message of a flow in user space which would get confused by seeing the VF netdev without IFF_SLAVE? You're only moving setting IFF_SLAVE but not linking the master, is there no code which would assume that if SLAVE is set there is a master?
> Subject: Re: [Patch v2] hv_netvsc: Mark VF as slave before exposing it to user- > mode > > On Fri, 27 Oct 2023 13:59:50 -0700 longli@linuxonhyperv.com wrote: > > When a VF is being exposed form the kernel, it should be marked as "slave" > > before exposing to the user-mode. The VF is not usable without netvsc > > running as master. The user-mode should never see a VF without the "slave" > flag. > > > > This commit moves the code of setting the slave flag to the time > > before VF is exposed to user-mode. > > Can you give a real example in the commit message of a flow in user space > which would get confused by seeing the VF netdev without IFF_SLAVE? A user-mode program may see the VF netdev show up without SLAVE flag before seeing the NETVSC netdev. It may try to configure the VF before it will be bonded to a NETVSC. With the IFF_SLAVE correctly set at the time of VF showing up to the user-mode, it can rely on this flag to decide if this device should be ignored. (without implementing some timeout logic to detect a potential NETVSC device that may show up later) > > You're only moving setting IFF_SLAVE but not linking the master, is there no > code which would assume that if SLAVE is set there is a master? The same (taking IFF_SLAVE without linking to master) is done in the original code before VF is joined, but it was for another purpose. I think there is a gap between when the VF is acted upon by other parts of the system and when it's bonded.
diff --git a/drivers/net/hyperv/netvsc_drv.c b/drivers/net/hyperv/netvsc_drv.c index ec77fb9dcf89..fdad58dcc6a8 100644 --- a/drivers/net/hyperv/netvsc_drv.c +++ b/drivers/net/hyperv/netvsc_drv.c @@ -2206,9 +2206,6 @@ static int netvsc_vf_join(struct net_device *vf_netdev, goto upper_link_failed; } - /* set slave flag before open to prevent IPv6 addrconf */ - vf_netdev->flags |= IFF_SLAVE; - schedule_delayed_work(&ndev_ctx->vf_takeover, VF_TAKEOVER_INT); call_netdevice_notifiers(NETDEV_JOIN, vf_netdev); @@ -2320,11 +2317,9 @@ static struct net_device *get_netvsc_byslot(const struct net_device *vf_netdev) */ list_for_each_entry(ndev_ctx, &netvsc_dev_list, list) { ndev = hv_get_drvdata(ndev_ctx->device_ctx); - if (ether_addr_equal(vf_netdev->perm_addr, ndev->perm_addr)) { - netdev_notice(vf_netdev, - "falling back to mac addr based matching\n"); + if (ether_addr_equal(vf_netdev->perm_addr, ndev->perm_addr) || + ether_addr_equal(vf_netdev->dev_addr, ndev->perm_addr)) return ndev; - } } netdev_notice(vf_netdev, @@ -2332,6 +2327,19 @@ static struct net_device *get_netvsc_byslot(const struct net_device *vf_netdev) return NULL; } +static int netvsc_prepare_slave(struct net_device *vf_netdev) +{ + struct net_device *ndev; + + ndev = get_netvsc_byslot(vf_netdev); + if (!ndev) + return NOTIFY_DONE; + + /* set slave flag before open to prevent IPv6 addrconf */ + vf_netdev->flags |= IFF_SLAVE; + return NOTIFY_DONE; +} + static int netvsc_register_vf(struct net_device *vf_netdev) { struct net_device_context *net_device_ctx; @@ -2753,6 +2761,8 @@ static int netvsc_netdev_event(struct notifier_block *this, return NOTIFY_DONE; switch (event) { + case NETDEV_POST_INIT: + return netvsc_prepare_slave(event_dev); case NETDEV_REGISTER: return netvsc_register_vf(event_dev); case NETDEV_UNREGISTER: