[PATCH net 2/4] hv_netvsc: reset vf_inject on VF removal
We reset vf_inject on VF going down (netvsc_vf_down()) but we don't on
VF removal (netvsc_unregister_vf()) so vf_inject stays 'true' while
vf_netdev is already NULL and we're trying to inject packets into NULL
net device in netvsc_recv_callback() causing kernel to crash.
Signed-off-by: Vitaly Kuznetsov
---
drivers/net/hyperv/netvsc_drv.c | 26 --
1 file changed, 16 insertions(+), 10 deletions(-)
diff --git a/drivers/net/hyperv/netvsc_drv.c b/drivers/net/hyperv/netvsc_drv.c
index 794139b..b3c31e3 100644
--- a/drivers/net/hyperv/netvsc_drv.c
+++ b/drivers/net/hyperv/netvsc_drv.c
@@ -1216,6 +1216,19 @@ static int netvsc_register_vf(struct net_device
*vf_netdev)
return NOTIFY_OK;
}
+static void netvsc_inject_enable(struct net_device_context *net_device_ctx)
+{
+ net_device_ctx->vf_inject = true;
+}
+
+static void netvsc_inject_disable(struct net_device_context *net_device_ctx)
+{
+ net_device_ctx->vf_inject = false;
+
+ /* Wait for currently active users to drain out. */
+ while (atomic_read(&net_device_ctx->vf_use_cnt) != 0)
+ udelay(50);
+}
static int netvsc_vf_up(struct net_device *vf_netdev)
{
@@ -1238,7 +1251,7 @@ static int netvsc_vf_up(struct net_device *vf_netdev)
return NOTIFY_DONE;
netdev_info(ndev, "VF up: %s\n", vf_netdev->name);
- net_device_ctx->vf_inject = true;
+ netvsc_inject_enable(net_device_ctx);
/*
* Open the device before switching data path.
@@ -1288,14 +1301,7 @@ static int netvsc_vf_down(struct net_device *vf_netdev)
return NOTIFY_DONE;
netdev_info(ndev, "VF down: %s\n", vf_netdev->name);
- net_device_ctx->vf_inject = false;
- /*
-* Wait for currently active users to
-* drain out.
-*/
-
- while (atomic_read(&net_device_ctx->vf_use_cnt) != 0)
- udelay(50);
+ netvsc_inject_disable(net_device_ctx);
netvsc_switch_datapath(ndev, false);
netdev_info(ndev, "Data path switched from VF: %s\n", vf_netdev->name);
rndis_filter_close(netvsc_dev);
@@ -1331,7 +1337,7 @@ static int netvsc_unregister_vf(struct net_device
*vf_netdev)
if (netvsc_dev == NULL)
return NOTIFY_DONE;
netdev_info(ndev, "VF unregistering: %s\n", vf_netdev->name);
-
+ netvsc_inject_disable(net_device_ctx);
net_device_ctx->vf_netdev = NULL;
module_put(THIS_MODULE);
return NOTIFY_OK;
--
2.7.4
RE: [PATCH net 2/4] hv_netvsc: reset vf_inject on VF removal
> +static void netvsc_inject_enable(struct net_device_context
> +*net_device_ctx) {
> + net_device_ctx->vf_inject = true;
> +}
> +
> +static void netvsc_inject_disable(struct net_device_context
> +*net_device_ctx) {
> + net_device_ctx->vf_inject = false;
> +
> + /* Wait for currently active users to drain out. */
> + while (atomic_read(&net_device_ctx->vf_use_cnt) != 0)
> + udelay(50);
> +}
That was already the behavior before, but are you certain you
want to unconditionally block without any possible timeout?
Re: [PATCH net 2/4] hv_netvsc: reset vf_inject on VF removal
Yuval Mintz writes:
>> +static void netvsc_inject_enable(struct net_device_context
>> +*net_device_ctx) {
>> +net_device_ctx->vf_inject = true;
>> +}
>> +
>> +static void netvsc_inject_disable(struct net_device_context
>> +*net_device_ctx) {
>> +net_device_ctx->vf_inject = false;
>> +
>> +/* Wait for currently active users to drain out. */
>> +while (atomic_read(&net_device_ctx->vf_use_cnt) != 0)
>> +udelay(50);
>> +}
>
> That was already the behavior before, but are you certain you
> want to unconditionally block without any possible timeout?
Yes, this is OK. After PATCH4 of this series there is only one place
which takes the vf_use_cnt (netvsc_recv_callback()) and it is an
interrupt handler, there are no sleepable operations there.
--
Vitaly
RE: [PATCH net 2/4] hv_netvsc: reset vf_inject on VF removal
> -Original Message- > From: Vitaly Kuznetsov [mailto:vkuzn...@redhat.com] > Sent: Thursday, August 11, 2016 6:59 AM > To: netdev@vger.kernel.org > Cc: de...@linuxdriverproject.org; linux-ker...@vger.kernel.org; Haiyang > Zhang ; KY Srinivasan > Subject: [PATCH net 2/4] hv_netvsc: reset vf_inject on VF removal > > We reset vf_inject on VF going down (netvsc_vf_down()) but we don't on > VF removal (netvsc_unregister_vf()) so vf_inject stays 'true' while > vf_netdev is already NULL and we're trying to inject packets into NULL > net device in netvsc_recv_callback() causing kernel to crash. > > Signed-off-by: Vitaly Kuznetsov Acked-by: Haiyang Zhang
Re: [PATCH net 2/4] hv_netvsc: reset vf_inject on VF removal
On Thu, 11 Aug 2016 14:09:53 +0200
Vitaly Kuznetsov wrote:
> Yuval Mintz writes:
>
> >> +static void netvsc_inject_enable(struct net_device_context
> >> +*net_device_ctx) {
> >> + net_device_ctx->vf_inject = true;
> >> +}
> >> +
> >> +static void netvsc_inject_disable(struct net_device_context
> >> +*net_device_ctx) {
> >> + net_device_ctx->vf_inject = false;
> >> +
> >> + /* Wait for currently active users to drain out. */
> >> + while (atomic_read(&net_device_ctx->vf_use_cnt) != 0)
> >> + udelay(50);
> >> +}
> >
> > That was already the behavior before, but are you certain you
> > want to unconditionally block without any possible timeout?
>
> Yes, this is OK. After PATCH4 of this series there is only one place
> which takes the vf_use_cnt (netvsc_recv_callback()) and it is an
> interrupt handler, there are no sleepable operations there.
>
Since network devices are protected by RCU, it looks like the refcount
is not necessary. I think vf_inject flag and vf_use_cnt could just be replaced
by doing RCU on vf_netdev.
The callback is invoked from tasklet (softirq) context.
Re: [PATCH net 2/4] hv_netvsc: reset vf_inject on VF removal
From: Vitaly Kuznetsov Date: Thu, 11 Aug 2016 12:58:55 +0200 > We reset vf_inject on VF going down (netvsc_vf_down()) but we don't on > VF removal (netvsc_unregister_vf()) so vf_inject stays 'true' while > vf_netdev is already NULL and we're trying to inject packets into NULL > net device in netvsc_recv_callback() causing kernel to crash. > > Signed-off-by: Vitaly Kuznetsov You can't create a blocking operation problem knowingly in this patch just because you fix it in patch #4. You must order your patches such that the driver does not regress at any intermediate stage of your patch series.
