Re: [PATCH v2 06/17] iommu: Add iommu_fwspec_alloc/dealloc()
On 2023/11/15 23:05, Jason Gunthorpe wrote:
> Allow fwspec to exist independently from the dev->iommu by providing
> functions to allow allocating and freeing the raw struct iommu_fwspec.
>
> Reflow the existing paths to call the new alloc/dealloc functions.
>
> Reviewed-by: Jerry Snitselaar
> Signed-off-by: Jason Gunthorpe
> ---
> drivers/iommu/iommu.c | 82 ---
> include/linux/iommu.h | 11 +-
> 2 files changed, 72 insertions(+), 21 deletions(-)
>
> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
> index 18a82a20934d53..86bbb9e75c7e03 100644
> --- a/drivers/iommu/iommu.c
> +++ b/drivers/iommu/iommu.c
> @@ -361,10 +361,8 @@ static void dev_iommu_free(struct device *dev)
> struct dev_iommu *param = dev->iommu;
>
> dev->iommu = NULL;
> - if (param->fwspec) {
> - fwnode_handle_put(param->fwspec->iommu_fwnode);
> - kfree(param->fwspec);
> - }
> + if (param->fwspec)
> + iommu_fwspec_dealloc(param->fwspec);
> kfree(param);
> }
>
> @@ -2920,10 +2918,61 @@ const struct iommu_ops *iommu_ops_from_fwnode(struct
> fwnode_handle *fwnode)
> return ops;
> }
>
> +static int iommu_fwspec_assign_iommu(struct iommu_fwspec *fwspec,
> + struct device *dev,
> + struct fwnode_handle *iommu_fwnode)
> +{
> + const struct iommu_ops *ops;
> +
> + if (fwspec->iommu_fwnode) {
> + /*
> + * fwspec->iommu_fwnode is the first iommu's fwnode. In the rare
> + * case of multiple iommus for one device they must point to the
> + * same driver, checked via same ops.
> + */
> + ops = iommu_ops_from_fwnode(iommu_fwnode);
This carries over a related bug from the original code: If a device has
two IOMMUs and the first one probes but the second one defers, ops will
be NULL here and the check will fail with EINVAL.
Adding a check for that case here fixes it:
if (!ops)
return driver_deferred_probe_check_state(dev);
With that, for the whole series:
Tested-by: Hector Martin
I can't specifically test for the probe races the series intends to fix
though, since that bug we only hit extremely rarely. I'm just testing
that nothing breaks.
> + if (fwspec->ops != ops)
> + return -EINVAL;
> + return 0;
> + }
> +
> + if (!fwspec->ops) {
> + ops = iommu_ops_from_fwnode(iommu_fwnode);
> + if (!ops)
> + return driver_deferred_probe_check_state(dev);
> + fwspec->ops = ops;
> + }
> +
> + of_node_get(to_of_node(iommu_fwnode));
> + fwspec->iommu_fwnode = iommu_fwnode;
> + return 0;
> +}
> +
> +struct iommu_fwspec *iommu_fwspec_alloc(void)
> +{
> + struct iommu_fwspec *fwspec;
> +
> + fwspec = kzalloc(sizeof(*fwspec), GFP_KERNEL);
> + if (!fwspec)
> + return ERR_PTR(-ENOMEM);
> + return fwspec;
> +}
> +
> +void iommu_fwspec_dealloc(struct iommu_fwspec *fwspec)
> +{
> + if (!fwspec)
> + return;
> +
> + if (fwspec->iommu_fwnode)
> + fwnode_handle_put(fwspec->iommu_fwnode);
> + kfree(fwspec);
> +}
> +
> int iommu_fwspec_init(struct device *dev, struct fwnode_handle *iommu_fwnode,
> const struct iommu_ops *ops)
> {
> struct iommu_fwspec *fwspec = dev_iommu_fwspec_get(dev);
> + int ret;
>
> if (fwspec)
> return ops == fwspec->ops ? 0 : -EINVAL;
> @@ -2931,29 +2980,22 @@ int iommu_fwspec_init(struct device *dev, struct
> fwnode_handle *iommu_fwnode,
> if (!dev_iommu_get(dev))
> return -ENOMEM;
>
> - fwspec = kzalloc(sizeof(*fwspec), GFP_KERNEL);
> - if (!fwspec)
> - return -ENOMEM;
> + fwspec = iommu_fwspec_alloc();
> + if (IS_ERR(fwspec))
> + return PTR_ERR(fwspec);
>
> - of_node_get(to_of_node(iommu_fwnode));
> - fwspec->iommu_fwnode = iommu_fwnode;
> fwspec->ops = ops;
> + ret = iommu_fwspec_assign_iommu(fwspec, dev, iommu_fwnode);
> + if (ret) {
> + iommu_fwspec_dealloc(fwspec);
> + return ret;
> + }
> +
> dev_iommu_fwspec_set(dev, fwspec);
> return 0;
> }
> EXPORT_SYMBOL_GPL(iommu_fwspec_init);
>
> -void iommu_fwspec_free(struct device *dev)
> -{
> - struct iommu_fwspec *fwspec = dev_iommu_fwspec_get(dev);
> -
> - if (fwspec) {
> - fwnode_handle_put(fwspec->iommu_fwnode);
> - kfree(fwspec);
> - dev_iommu_fwspec_set(dev, NULL);
> - }
> -}
> -EXPORT_SYMBOL_GPL(iommu_fwspec_free);
>
> int iommu_fwspec_add_ids(struct device *dev, u32 *ids, int num_ids)
> {
> diff --git a/include/linux/iommu.h b/include/linux/iommu.h
> index e98a4ca8f536b7..c7c68cb59aa4dc 100644
> --- a/include/linux/iommu.h
> +++ b/include/linux/iommu.h
> @
Re: [PATCH v2 06/17] iommu: Add iommu_fwspec_alloc/dealloc()
On 2023/11/19 17:10, Hector Martin wrote:
> On 2023/11/15 23:05, Jason Gunthorpe wrote:
>> Allow fwspec to exist independently from the dev->iommu by providing
>> functions to allow allocating and freeing the raw struct iommu_fwspec.
>>
>> Reflow the existing paths to call the new alloc/dealloc functions.
>>
>> Reviewed-by: Jerry Snitselaar
>> Signed-off-by: Jason Gunthorpe
>> ---
>> drivers/iommu/iommu.c | 82 ---
>> include/linux/iommu.h | 11 +-
>> 2 files changed, 72 insertions(+), 21 deletions(-)
>>
>> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
>> index 18a82a20934d53..86bbb9e75c7e03 100644
>> --- a/drivers/iommu/iommu.c
>> +++ b/drivers/iommu/iommu.c
>> @@ -361,10 +361,8 @@ static void dev_iommu_free(struct device *dev)
>> struct dev_iommu *param = dev->iommu;
>>
>> dev->iommu = NULL;
>> -if (param->fwspec) {
>> -fwnode_handle_put(param->fwspec->iommu_fwnode);
>> -kfree(param->fwspec);
>> -}
>> +if (param->fwspec)
>> +iommu_fwspec_dealloc(param->fwspec);
>> kfree(param);
>> }
>>
>> @@ -2920,10 +2918,61 @@ const struct iommu_ops *iommu_ops_from_fwnode(struct
>> fwnode_handle *fwnode)
>> return ops;
>> }
>>
>> +static int iommu_fwspec_assign_iommu(struct iommu_fwspec *fwspec,
>> + struct device *dev,
>> + struct fwnode_handle *iommu_fwnode)
>> +{
>> +const struct iommu_ops *ops;
>> +
>> +if (fwspec->iommu_fwnode) {
>> +/*
>> + * fwspec->iommu_fwnode is the first iommu's fwnode. In the rare
>> + * case of multiple iommus for one device they must point to the
>> + * same driver, checked via same ops.
>> + */
>> +ops = iommu_ops_from_fwnode(iommu_fwnode);
>
> This carries over a related bug from the original code: If a device has
> two IOMMUs and the first one probes but the second one defers, ops will
> be NULL here and the check will fail with EINVAL.
>
> Adding a check for that case here fixes it:
>
> if (!ops)
> return driver_deferred_probe_check_state(dev);
>
> With that, for the whole series:
>
> Tested-by: Hector Martin
>
> I can't specifically test for the probe races the series intends to fix
> though, since that bug we only hit extremely rarely. I'm just testing
> that nothing breaks.
Actually no, this fix is not sufficient. If the first IOMMU is ready
then the xlate path allocates dev->iommu, which then
__iommu_probe_device takes as a sign that all IOMMUs are ready and does
the device init. Then when the xlate comes along again after suceeding
with the second IOMMU, __iommu_probe_device sees the device is already
in a group and never initializes the second IOMMU, leaving the device
with only one IOMMU.
This patch fixes it, but honestly, at this point I have no idea how to
"properly" fix this. There is *way* too much subtlety in this whole
codepath.
diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index 2477dec29740..2e4baf0572e7 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -2935,6 +2935,12 @@ int iommu_fwspec_of_xlate(struct iommu_fwspec
*fwspec, struct device *dev,
int ret;
ret = iommu_fwspec_assign_iommu(fwspec, dev, iommu_fwnode);
+ if (ret == -EPROBE_DEFER) {
+ mutex_lock(&iommu_probe_device_lock);
+ if (dev->iommu)
+ dev_iommu_free(dev);
+ mutex_unlock(&iommu_probe_device_lock);
+ }
if (ret)
return ret;
>
>> +if (fwspec->ops != ops)
>> +return -EINVAL;
>> +return 0;
>> +}
>> +
>> +if (!fwspec->ops) {
>> +ops = iommu_ops_from_fwnode(iommu_fwnode);
>> +if (!ops)
>> +return driver_deferred_probe_check_state(dev);
>> +fwspec->ops = ops;
>> +}
>> +
>> +of_node_get(to_of_node(iommu_fwnode));
>> +fwspec->iommu_fwnode = iommu_fwnode;
>> +return 0;
>> +}
>> +
>> +struct iommu_fwspec *iommu_fwspec_alloc(void)
>> +{
>> +struct iommu_fwspec *fwspec;
>> +
>> +fwspec = kzalloc(sizeof(*fwspec), GFP_KERNEL);
>> +if (!fwspec)
>> +return ERR_PTR(-ENOMEM);
>> +return fwspec;
>> +}
>> +
>> +void iommu_fwspec_dealloc(struct iommu_fwspec *fwspec)
>> +{
>> +if (!fwspec)
>> +return;
>> +
>> +if (fwspec->iommu_fwnode)
>> +fwnode_handle_put(fwspec->iommu_fwnode);
>> +kfree(fwspec);
>> +}
>> +
>> int iommu_fwspec_init(struct device *dev, struct fwnode_handle
>> *iommu_fwnode,
>>const struct iommu_ops *ops)
>> {
>> struct iommu_fwspec *fwspec = dev_iommu_fwspec_get(dev);
>> +int ret;
>>
>> if (fwspec)
>> return ops == fwspec->ops ? 0 : -EINVAL;
>> @@ -2931,29 +2980,22 @@ int iommu_fwspec_init(struct device *
Re: [PATCH v2 06/17] iommu: Add iommu_fwspec_alloc/dealloc()
On Sun, Nov 19, 2023 at 06:19:43PM +0900, Hector Martin wrote:
> >> +static int iommu_fwspec_assign_iommu(struct iommu_fwspec *fwspec,
> >> + struct device *dev,
> >> + struct fwnode_handle *iommu_fwnode)
> >> +{
> >> + const struct iommu_ops *ops;
> >> +
> >> + if (fwspec->iommu_fwnode) {
> >> + /*
> >> + * fwspec->iommu_fwnode is the first iommu's fwnode. In the rare
> >> + * case of multiple iommus for one device they must point to the
> >> + * same driver, checked via same ops.
> >> + */
> >> + ops = iommu_ops_from_fwnode(iommu_fwnode);
> >
> > This carries over a related bug from the original code: If a device has
> > two IOMMUs and the first one probes but the second one defers, ops will
> > be NULL here and the check will fail with EINVAL.
> >
> > Adding a check for that case here fixes it:
> >
> > if (!ops)
> > return driver_deferred_probe_check_state(dev);
Yes!
> > With that, for the whole series:
> >
> > Tested-by: Hector Martin
> >
> > I can't specifically test for the probe races the series intends to fix
> > though, since that bug we only hit extremely rarely. I'm just testing
> > that nothing breaks.
>
> Actually no, this fix is not sufficient. If the first IOMMU is ready
> then the xlate path allocates dev->iommu, which then
> __iommu_probe_device takes as a sign that all IOMMUs are ready and does
> the device init.
It doesn't.. The code there is:
if (!fwspec && dev->iommu)
fwspec = dev->iommu->fwspec;
if (fwspec)
ops = fwspec->ops;
else
ops = dev->bus->iommu_ops;
if (!ops) {
ret = -ENODEV;
goto out_unlock;
}
Which is sensitive only to !NULL fwspec, and if EPROBE_DEFER is
returned fwspec will be freed and dev->iommu->fwspec will be NULL
here.
In the NULL case it does a 'bus probe' with a NULL fwspec and all the
fwspec drivers return immediately from their probe functions.
Did I miss something?
> Then when the xlate comes along again after suceeding
> with the second IOMMU, __iommu_probe_device sees the device is already
> in a group and never initializes the second IOMMU, leaving the device
> with only one IOMMU.
This should be fixed by the first hunk to check every iommu and fail?
BTW, do you have a systems with same device attached to multiple
iommus?
I've noticed another bug here, many drivers don't actually support
differing iommu instances and nothing seems to check it..
Thanks,
Jason
[PATCH net,v5, 0/3] hv_netvsc: fix race of netvsc, VF register, and slave bit
There are some races between netvsc probe, set notifier, VF register, and slave bit setting. This patch set fixes them. Haiyang Zhang (2): hv_netvsc: fix race of netvsc and VF register_netdevice hv_netvsc: Fix race of register_netdevice_notifier and VF register Long Li (1): hv_netvsc: Mark VF as slave before exposing it to user-mode drivers/net/hyperv/netvsc_drv.c | 66 ++--- 1 file changed, 45 insertions(+), 21 deletions(-) -- 2.34.1
[PATCH net,v5, 1/3] hv_netvsc: fix race of netvsc and VF register_netdevice
The rtnl lock also needs to be held before rndis_filter_device_add()
which advertises nvsp_2_vsc_capability / sriov bit, and triggers
VF NIC offering and registering. If VF NIC finished register_netdev()
earlier it may cause name based config failure.
To fix this issue, move the call to rtnl_lock() before
rndis_filter_device_add(), so VF will be registered later than netvsc
/ synthetic NIC, and gets a name numbered (ethX) after netvsc.
Cc: sta...@vger.kernel.org
Fixes: e04e7a7bbd4b ("hv_netvsc: Fix a deadlock by getting rtnl lock earlier in
netvsc_probe()")
Reported-by: Dexuan Cui
Signed-off-by: Haiyang Zhang
Reviewed-by: Wojciech Drewek
Reviewed-by: Simon Horman
Reviewed-by: Dexuan Cui
---
v3:
Divide it into two patches, suggested by Jakub Kicinski.
v2:
Fix rtnl_unlock() in error handling as found by Wojciech Drewek.
---
drivers/net/hyperv/netvsc_drv.c | 25 +++--
1 file changed, 15 insertions(+), 10 deletions(-)
diff --git a/drivers/net/hyperv/netvsc_drv.c b/drivers/net/hyperv/netvsc_drv.c
index 3ba3c8fb28a5..5e528a76f5f5 100644
--- a/drivers/net/hyperv/netvsc_drv.c
+++ b/drivers/net/hyperv/netvsc_drv.c
@@ -2531,15 +2531,6 @@ static int netvsc_probe(struct hv_device *dev,
goto devinfo_failed;
}
- nvdev = rndis_filter_device_add(dev, device_info);
- if (IS_ERR(nvdev)) {
- ret = PTR_ERR(nvdev);
- netdev_err(net, "unable to add netvsc device (ret %d)\n", ret);
- goto rndis_failed;
- }
-
- eth_hw_addr_set(net, device_info->mac_adr);
-
/* We must get rtnl lock before scheduling nvdev->subchan_work,
* otherwise netvsc_subchan_work() can get rtnl lock first and wait
* all subchannels to show up, but that may not happen because
@@ -2547,9 +2538,23 @@ static int netvsc_probe(struct hv_device *dev,
* -> ... -> device_add() -> ... -> __device_attach() can't get
* the device lock, so all the subchannels can't be processed --
* finally netvsc_subchan_work() hangs forever.
+*
+* The rtnl lock also needs to be held before rndis_filter_device_add()
+* which advertises nvsp_2_vsc_capability / sriov bit, and triggers
+* VF NIC offering and registering. If VF NIC finished register_netdev()
+* earlier it may cause name based config failure.
*/
rtnl_lock();
+ nvdev = rndis_filter_device_add(dev, device_info);
+ if (IS_ERR(nvdev)) {
+ ret = PTR_ERR(nvdev);
+ netdev_err(net, "unable to add netvsc device (ret %d)\n", ret);
+ goto rndis_failed;
+ }
+
+ eth_hw_addr_set(net, device_info->mac_adr);
+
if (nvdev->num_chn > 1)
schedule_work(&nvdev->subchan_work);
@@ -2586,9 +2591,9 @@ static int netvsc_probe(struct hv_device *dev,
return 0;
register_failed:
- rtnl_unlock();
rndis_filter_device_remove(dev, nvdev);
rndis_failed:
+ rtnl_unlock();
netvsc_devinfo_put(device_info);
devinfo_failed:
free_percpu(net_device_ctx->vf_stats);
--
2.34.1
[PATCH net,v5, 2/3] hv_netvsc: Fix race of register_netdevice_notifier and VF register
If VF NIC is registered earlier, NETDEV_REGISTER event is replayed,
but NETDEV_POST_INIT is not.
Move register_netdevice_notifier() earlier, so the call back
function is set before probing.
Cc: sta...@vger.kernel.org
Fixes: e04e7a7bbd4b ("hv_netvsc: Fix a deadlock by getting rtnl lock earlier in
netvsc_probe()")
Reported-by: Dexuan Cui
Signed-off-by: Haiyang Zhang
Reviewed-by: Wojciech Drewek
Reviewed-by: Dexuan Cui
---
v5:
Use a more idiomatic form for the error path, suggested by Simon Horman.
v3:
Divide it into two patches, suggested by Jakub Kicinski.
v2:
Fix rtnl_unlock() in error handling as found by Wojciech Drewek.
---
drivers/net/hyperv/netvsc_drv.c | 9 +++--
1 file changed, 7 insertions(+), 2 deletions(-)
diff --git a/drivers/net/hyperv/netvsc_drv.c b/drivers/net/hyperv/netvsc_drv.c
index 5e528a76f5f5..b7dfd51f09e6 100644
--- a/drivers/net/hyperv/netvsc_drv.c
+++ b/drivers/net/hyperv/netvsc_drv.c
@@ -2793,12 +2793,17 @@ static int __init netvsc_drv_init(void)
}
netvsc_ring_bytes = ring_size * PAGE_SIZE;
+ register_netdevice_notifier(&netvsc_netdev_notifier);
+
ret = vmbus_driver_register(&netvsc_drv);
if (ret)
- return ret;
+ goto err_vmbus_reg;
- register_netdevice_notifier(&netvsc_netdev_notifier);
return 0;
+
+err_vmbus_reg:
+ unregister_netdevice_notifier(&netvsc_netdev_notifier);
+ return ret;
}
MODULE_LICENSE("GPL");
--
2.34.1
[PATCH net,v5, 3/3] hv_netvsc: Mark VF as slave before exposing it to user-mode
From: Long Li
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.
Cc: sta...@vger.kernel.org
Fixes: 0c195567a8f6 ("netvsc: transparent VF management")
Signed-off-by: Long Li
Signed-off-by: Haiyang Zhang
---
v5:
Change function name netvsc_prepare_slave() to netvsc_prepare_bonding().
v4:
Add comments in get_netvsc_byslot() explaining the need to check dev_addr
v2:
Use a new function to handle NETDEV_POST_INIT.
---
drivers/net/hyperv/netvsc_drv.c | 32 +++-
1 file changed, 23 insertions(+), 9 deletions(-)
diff --git a/drivers/net/hyperv/netvsc_drv.c b/drivers/net/hyperv/netvsc_drv.c
index b7dfd51f09e6..706ea5263e87 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);
@@ -2315,16 +2312,18 @@ static struct net_device *get_netvsc_byslot(const
struct net_device *vf_netdev)
}
- /* Fallback path to check synthetic vf with
-* help of mac addr
+ /* Fallback path to check synthetic vf with help of mac addr.
+* Because this function can be called before vf_netdev is
+* initialized (NETDEV_POST_INIT) when its perm_addr has not been copied
+* from dev_addr, also try to match to its dev_addr.
+* Note: On Hyper-V and Azure, it's not possible to set a MAC address
+* on a VF that matches to the MAC of a unrelated NETVSC device.
*/
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 +2331,19 @@ static struct net_device *get_netvsc_byslot(const struct
net_device *vf_netdev)
return NULL;
}
+static int netvsc_prepare_bonding(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;
@@ -2758,6 +2770,8 @@ static int netvsc_netdev_event(struct notifier_block
*this,
return NOTIFY_DONE;
switch (event) {
+ case NETDEV_POST_INIT:
+ return netvsc_prepare_bonding(event_dev);
case NETDEV_REGISTER:
return netvsc_register_vf(event_dev);
case NETDEV_UNREGISTER:
--
2.34.1
Re: [PATCH net,v5, 3/3] hv_netvsc: Mark VF as slave before exposing it to user-mode
On Sun, 19 Nov 2023 08:23:43 -0800
Haiyang Zhang wrote:
> From: Long Li
>
> 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.
>
> Cc: sta...@vger.kernel.org
> Fixes: 0c195567a8f6 ("netvsc: transparent VF management")
> Signed-off-by: Long Li
> Signed-off-by: Haiyang Zhang
Acked-by: Stephen Hemminger
