linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] hv_netvsc: Fix netvsc_start_xmit's return type
@ 2020-04-28  3:30 Nathan Chancellor
  2020-04-28 10:08 ` Wei Liu
  0 siblings, 1 reply; 9+ messages in thread
From: Nathan Chancellor @ 2020-04-28  3:30 UTC (permalink / raw)
  To: K. Y. Srinivasan, Haiyang Zhang, Stephen Hemminger, Wei Liu
  Cc: linux-hyperv, netdev, linux-kernel, clang-built-linux,
	Sami Tolvanen, Nathan Chancellor

netvsc_start_xmit is used as a callback function for the ndo_start_xmit
function pointer. ndo_start_xmit's return type is netdev_tx_t but
netvsc_start_xmit's return type is int.

This causes a failure with Control Flow Integrity (CFI), which requires
function pointer prototypes and callback function definitions to match
exactly. When CFI is in enforcing, the kernel panics. When booting a
CFI kernel with WSL 2, the VM is immediately terminated because of this:

$ wsl.exe -d ubuntu
The Windows Subsystem for Linux instance has terminated.

Avoid this by using the right return type for netvsc_start_xmit.

Fixes: fceaf24a943d8 ("Staging: hv: add the Hyper-V virtual network driver")
Link: https://github.com/ClangBuiltLinux/linux/issues/1009
Signed-off-by: Nathan Chancellor <natechancellor@gmail.com>
---

Do note that netvsc_xmit still returns int because netvsc_xmit has a
potential return from netvsc_vf_xmit, which does not return netdev_tx_t
because of the call to dev_queue_xmit.

I am not sure if that is an oversight that was introduced by
commit 0c195567a8f6e ("netvsc: transparent VF management") or if
everything works properly as it is now.

My patch is purely concerned with making the definition match the
prototype so it should be NFC aside from avoiding the CFI panic.

 drivers/net/hyperv/netvsc_drv.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/net/hyperv/netvsc_drv.c b/drivers/net/hyperv/netvsc_drv.c
index d8e86bdbfba1e..ebcfbae056900 100644
--- a/drivers/net/hyperv/netvsc_drv.c
+++ b/drivers/net/hyperv/netvsc_drv.c
@@ -707,7 +707,8 @@ static int netvsc_xmit(struct sk_buff *skb, struct net_device *net, bool xdp_tx)
 	goto drop;
 }
 
-static int netvsc_start_xmit(struct sk_buff *skb, struct net_device *ndev)
+static netdev_tx_t netvsc_start_xmit(struct sk_buff *skb,
+				     struct net_device *ndev)
 {
 	return netvsc_xmit(skb, ndev, false);
 }

base-commit: 51184ae37e0518fd90cb437a2fbc953ae558cd0d
-- 
2.26.2


^ permalink raw reply related	[flat|nested] 9+ messages in thread

* Re: [PATCH] hv_netvsc: Fix netvsc_start_xmit's return type
  2020-04-28  3:30 [PATCH] hv_netvsc: Fix netvsc_start_xmit's return type Nathan Chancellor
@ 2020-04-28 10:08 ` Wei Liu
  2020-04-28 17:54   ` [PATCH v2] " Nathan Chancellor
  0 siblings, 1 reply; 9+ messages in thread
From: Wei Liu @ 2020-04-28 10:08 UTC (permalink / raw)
  To: Nathan Chancellor
  Cc: K. Y. Srinivasan, Haiyang Zhang, Stephen Hemminger, Wei Liu,
	linux-hyperv, netdev, linux-kernel, clang-built-linux,
	Sami Tolvanen

On Mon, Apr 27, 2020 at 08:30:43PM -0700, Nathan Chancellor wrote:
> netvsc_start_xmit is used as a callback function for the ndo_start_xmit
> function pointer. ndo_start_xmit's return type is netdev_tx_t but
> netvsc_start_xmit's return type is int.
> 
> This causes a failure with Control Flow Integrity (CFI), which requires
> function pointer prototypes and callback function definitions to match
> exactly. When CFI is in enforcing, the kernel panics. When booting a
> CFI kernel with WSL 2, the VM is immediately terminated because of this:
> 
> $ wsl.exe -d ubuntu
> The Windows Subsystem for Linux instance has terminated.
> 
> Avoid this by using the right return type for netvsc_start_xmit.
> 
> Fixes: fceaf24a943d8 ("Staging: hv: add the Hyper-V virtual network driver")
> Link: https://github.com/ClangBuiltLinux/linux/issues/1009

Please consider pulling in the panic log from #1009 to the commit
message. It is much better than the one line message above.

> Signed-off-by: Nathan Chancellor <natechancellor@gmail.com>
> ---
> 
> Do note that netvsc_xmit still returns int because netvsc_xmit has a
> potential return from netvsc_vf_xmit, which does not return netdev_tx_t
> because of the call to dev_queue_xmit.
> 
> I am not sure if that is an oversight that was introduced by
> commit 0c195567a8f6e ("netvsc: transparent VF management") or if
> everything works properly as it is now.
> 
> My patch is purely concerned with making the definition match the
> prototype so it should be NFC aside from avoiding the CFI panic.
> 
>  drivers/net/hyperv/netvsc_drv.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/hyperv/netvsc_drv.c b/drivers/net/hyperv/netvsc_drv.c
> index d8e86bdbfba1e..ebcfbae056900 100644
> --- a/drivers/net/hyperv/netvsc_drv.c
> +++ b/drivers/net/hyperv/netvsc_drv.c
> @@ -707,7 +707,8 @@ static int netvsc_xmit(struct sk_buff *skb, struct net_device *net, bool xdp_tx)
>  	goto drop;
>  }
>  
> -static int netvsc_start_xmit(struct sk_buff *skb, struct net_device *ndev)
> +static netdev_tx_t netvsc_start_xmit(struct sk_buff *skb,
> +				     struct net_device *ndev)
>  {
>  	return netvsc_xmit(skb, ndev, false);
>  }
> 
> base-commit: 51184ae37e0518fd90cb437a2fbc953ae558cd0d
> -- 
> 2.26.2
> 

^ permalink raw reply	[flat|nested] 9+ messages in thread

* [PATCH v2] hv_netvsc: Fix netvsc_start_xmit's return type
  2020-04-28 10:08 ` Wei Liu
@ 2020-04-28 17:54   ` Nathan Chancellor
  2020-04-29 10:10     ` Wei Liu
                       ` (2 more replies)
  0 siblings, 3 replies; 9+ messages in thread
From: Nathan Chancellor @ 2020-04-28 17:54 UTC (permalink / raw)
  To: K. Y. Srinivasan, Haiyang Zhang, Stephen Hemminger, Wei Liu
  Cc: linux-hyperv, netdev, linux-kernel, clang-built-linux,
	Sami Tolvanen, Nathan Chancellor

netvsc_start_xmit is used as a callback function for the ndo_start_xmit
function pointer. ndo_start_xmit's return type is netdev_tx_t but
netvsc_start_xmit's return type is int.

This causes a failure with Control Flow Integrity (CFI), which requires
function pointer prototypes and callback function definitions to match
exactly. When CFI is in enforcing, the kernel panics. When booting a
CFI kernel with WSL 2, the VM is immediately terminated because of this.

The splat when CONFIG_CFI_PERMISSIVE is used:

[    5.916765] CFI failure (target: netvsc_start_xmit+0x0/0x10):
[    5.916771] WARNING: CPU: 8 PID: 0 at kernel/cfi.c:29 __cfi_check_fail+0x2e/0x40
[    5.916772] Modules linked in:
[    5.916774] CPU: 8 PID: 0 Comm: swapper/8 Not tainted 5.7.0-rc3-next-20200424-microsoft-cbl-00001-ged4eb37d2c69-dirty #1
[    5.916776] RIP: 0010:__cfi_check_fail+0x2e/0x40
[    5.916777] Code: 48 c7 c7 70 98 63 a9 48 c7 c6 11 db 47 a9 e8 69 55 59 00 85 c0 75 02 5b c3 48 c7 c7 73 c6 43 a9 48 89 de 31 c0 e8 12 2d f0 ff <0f> 0b 5b c3 00 00 cc cc 00 00 cc cc 00 00 cc cc 00 00 85 f6 74 25
[    5.916778] RSP: 0018:ffffa803c0260b78 EFLAGS: 00010246
[    5.916779] RAX: 712a1af25779e900 RBX: ffffffffa8cf7950 RCX: ffffffffa962cf08
[    5.916779] RDX: ffffffffa9c36b60 RSI: 0000000000000082 RDI: ffffffffa9c36b5c
[    5.916780] RBP: ffff8ffc4779c2c0 R08: 0000000000000001 R09: ffffffffa9c3c300
[    5.916781] R10: 0000000000000151 R11: ffffffffa9c36b60 R12: ffff8ffe39084000
[    5.916782] R13: ffffffffa8cf7950 R14: ffffffffa8d12cb0 R15: ffff8ffe39320140
[    5.916784] FS:  0000000000000000(0000) GS:ffff8ffe3bc00000(0000) knlGS:0000000000000000
[    5.916785] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[    5.916786] CR2: 00007ffef5749408 CR3: 00000002f4f5e000 CR4: 0000000000340ea0
[    5.916787] Call Trace:
[    5.916788]  <IRQ>
[    5.916790]  __cfi_check+0x3ab58/0x450e0
[    5.916793]  ? dev_hard_start_xmit+0x11f/0x160
[    5.916795]  ? sch_direct_xmit+0xf2/0x230
[    5.916796]  ? __dev_queue_xmit.llvm.11471227737707190958+0x69d/0x8e0
[    5.916797]  ? neigh_resolve_output+0xdf/0x220
[    5.916799]  ? neigh_connected_output.cfi_jt+0x8/0x8
[    5.916801]  ? ip6_finish_output2+0x398/0x4c0
[    5.916803]  ? nf_nat_ipv6_out+0x10/0xa0
[    5.916804]  ? nf_hook_slow+0x84/0x100
[    5.916807]  ? ip6_input_finish+0x8/0x8
[    5.916807]  ? ip6_output+0x6f/0x110
[    5.916808]  ? __ip6_local_out.cfi_jt+0x8/0x8
[    5.916810]  ? mld_sendpack+0x28e/0x330
[    5.916811]  ? ip_rt_bug+0x8/0x8
[    5.916813]  ? mld_ifc_timer_expire+0x2db/0x400
[    5.916814]  ? neigh_proxy_process+0x8/0x8
[    5.916816]  ? call_timer_fn+0x3d/0xd0
[    5.916817]  ? __run_timers+0x2a9/0x300
[    5.916819]  ? rcu_core_si+0x8/0x8
[    5.916820]  ? run_timer_softirq+0x14/0x30
[    5.916821]  ? __do_softirq+0x154/0x262
[    5.916822]  ? native_x2apic_icr_write+0x8/0x8
[    5.916824]  ? irq_exit+0xba/0xc0
[    5.916825]  ? hv_stimer0_vector_handler+0x99/0xe0
[    5.916826]  ? hv_stimer0_callback_vector+0xf/0x20
[    5.916826]  </IRQ>
[    5.916828]  ? hv_stimer_global_cleanup.cfi_jt+0x8/0x8
[    5.916829]  ? raw_setsockopt+0x8/0x8
[    5.916830]  ? default_idle+0xe/0x10
[    5.916832]  ? do_idle.llvm.10446269078108580492+0xb7/0x130
[    5.916833]  ? raw_setsockopt+0x8/0x8
[    5.916833]  ? cpu_startup_entry+0x15/0x20
[    5.916835]  ? cpu_hotplug_enable.cfi_jt+0x8/0x8
[    5.916836]  ? start_secondary+0x188/0x190
[    5.916837]  ? secondary_startup_64+0xa5/0xb0
[    5.916838] ---[ end trace f2683fa869597ba5 ]---

Avoid this by using the right return type for netvsc_start_xmit.

Fixes: fceaf24a943d8 ("Staging: hv: add the Hyper-V virtual network driver")
Link: https://github.com/ClangBuiltLinux/linux/issues/1009
Signed-off-by: Nathan Chancellor <natechancellor@gmail.com>
---

v1 -> v2:

* Move splat into commit message rather than issue.

Comment from previous version:

Do note that netvsc_xmit still returns int because netvsc_xmit has a
potential return from netvsc_vf_xmit, which does not return netdev_tx_t
because of the call to dev_queue_xmit.

I am not sure if that is an oversight that was introduced by
commit 0c195567a8f6e ("netvsc: transparent VF management") or if
everything works properly as it is now.

My patch is purely concerned with making the definition match the
prototype so it should be NFC aside from avoiding the CFI panic.

 drivers/net/hyperv/netvsc_drv.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/net/hyperv/netvsc_drv.c b/drivers/net/hyperv/netvsc_drv.c
index d8e86bdbfba1e..ebcfbae056900 100644
--- a/drivers/net/hyperv/netvsc_drv.c
+++ b/drivers/net/hyperv/netvsc_drv.c
@@ -707,7 +707,8 @@ static int netvsc_xmit(struct sk_buff *skb, struct net_device *net, bool xdp_tx)
 	goto drop;
 }
 
-static int netvsc_start_xmit(struct sk_buff *skb, struct net_device *ndev)
+static netdev_tx_t netvsc_start_xmit(struct sk_buff *skb,
+				     struct net_device *ndev)
 {
 	return netvsc_xmit(skb, ndev, false);
 }

base-commit: 51184ae37e0518fd90cb437a2fbc953ae558cd0d
-- 
2.26.2


^ permalink raw reply related	[flat|nested] 9+ messages in thread

* Re: [PATCH v2] hv_netvsc: Fix netvsc_start_xmit's return type
  2020-04-28 17:54   ` [PATCH v2] " Nathan Chancellor
@ 2020-04-29 10:10     ` Wei Liu
  2020-04-29 18:11       ` David Miller
  2020-04-30  0:06     ` Michael Kelley
  2020-05-01 22:25     ` David Miller
  2 siblings, 1 reply; 9+ messages in thread
From: Wei Liu @ 2020-04-29 10:10 UTC (permalink / raw)
  To: Nathan Chancellor, davem
  Cc: K. Y. Srinivasan, Haiyang Zhang, Stephen Hemminger, Wei Liu,
	linux-hyperv, netdev, linux-kernel, clang-built-linux,
	Sami Tolvanen

David

Do you want this to go through net tree? I can submit it via hyperv tree
if that's preferred.

Wei.

On Tue, Apr 28, 2020 at 10:54:56AM -0700, Nathan Chancellor wrote:
> netvsc_start_xmit is used as a callback function for the ndo_start_xmit
> function pointer. ndo_start_xmit's return type is netdev_tx_t but
> netvsc_start_xmit's return type is int.
> 
> This causes a failure with Control Flow Integrity (CFI), which requires
> function pointer prototypes and callback function definitions to match
> exactly. When CFI is in enforcing, the kernel panics. When booting a
> CFI kernel with WSL 2, the VM is immediately terminated because of this.
> 
> The splat when CONFIG_CFI_PERMISSIVE is used:
> 
> [    5.916765] CFI failure (target: netvsc_start_xmit+0x0/0x10):
> [    5.916771] WARNING: CPU: 8 PID: 0 at kernel/cfi.c:29 __cfi_check_fail+0x2e/0x40
> [    5.916772] Modules linked in:
> [    5.916774] CPU: 8 PID: 0 Comm: swapper/8 Not tainted 5.7.0-rc3-next-20200424-microsoft-cbl-00001-ged4eb37d2c69-dirty #1
> [    5.916776] RIP: 0010:__cfi_check_fail+0x2e/0x40
> [    5.916777] Code: 48 c7 c7 70 98 63 a9 48 c7 c6 11 db 47 a9 e8 69 55 59 00 85 c0 75 02 5b c3 48 c7 c7 73 c6 43 a9 48 89 de 31 c0 e8 12 2d f0 ff <0f> 0b 5b c3 00 00 cc cc 00 00 cc cc 00 00 cc cc 00 00 85 f6 74 25
> [    5.916778] RSP: 0018:ffffa803c0260b78 EFLAGS: 00010246
> [    5.916779] RAX: 712a1af25779e900 RBX: ffffffffa8cf7950 RCX: ffffffffa962cf08
> [    5.916779] RDX: ffffffffa9c36b60 RSI: 0000000000000082 RDI: ffffffffa9c36b5c
> [    5.916780] RBP: ffff8ffc4779c2c0 R08: 0000000000000001 R09: ffffffffa9c3c300
> [    5.916781] R10: 0000000000000151 R11: ffffffffa9c36b60 R12: ffff8ffe39084000
> [    5.916782] R13: ffffffffa8cf7950 R14: ffffffffa8d12cb0 R15: ffff8ffe39320140
> [    5.916784] FS:  0000000000000000(0000) GS:ffff8ffe3bc00000(0000) knlGS:0000000000000000
> [    5.916785] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> [    5.916786] CR2: 00007ffef5749408 CR3: 00000002f4f5e000 CR4: 0000000000340ea0
> [    5.916787] Call Trace:
> [    5.916788]  <IRQ>
> [    5.916790]  __cfi_check+0x3ab58/0x450e0
> [    5.916793]  ? dev_hard_start_xmit+0x11f/0x160
> [    5.916795]  ? sch_direct_xmit+0xf2/0x230
> [    5.916796]  ? __dev_queue_xmit.llvm.11471227737707190958+0x69d/0x8e0
> [    5.916797]  ? neigh_resolve_output+0xdf/0x220
> [    5.916799]  ? neigh_connected_output.cfi_jt+0x8/0x8
> [    5.916801]  ? ip6_finish_output2+0x398/0x4c0
> [    5.916803]  ? nf_nat_ipv6_out+0x10/0xa0
> [    5.916804]  ? nf_hook_slow+0x84/0x100
> [    5.916807]  ? ip6_input_finish+0x8/0x8
> [    5.916807]  ? ip6_output+0x6f/0x110
> [    5.916808]  ? __ip6_local_out.cfi_jt+0x8/0x8
> [    5.916810]  ? mld_sendpack+0x28e/0x330
> [    5.916811]  ? ip_rt_bug+0x8/0x8
> [    5.916813]  ? mld_ifc_timer_expire+0x2db/0x400
> [    5.916814]  ? neigh_proxy_process+0x8/0x8
> [    5.916816]  ? call_timer_fn+0x3d/0xd0
> [    5.916817]  ? __run_timers+0x2a9/0x300
> [    5.916819]  ? rcu_core_si+0x8/0x8
> [    5.916820]  ? run_timer_softirq+0x14/0x30
> [    5.916821]  ? __do_softirq+0x154/0x262
> [    5.916822]  ? native_x2apic_icr_write+0x8/0x8
> [    5.916824]  ? irq_exit+0xba/0xc0
> [    5.916825]  ? hv_stimer0_vector_handler+0x99/0xe0
> [    5.916826]  ? hv_stimer0_callback_vector+0xf/0x20
> [    5.916826]  </IRQ>
> [    5.916828]  ? hv_stimer_global_cleanup.cfi_jt+0x8/0x8
> [    5.916829]  ? raw_setsockopt+0x8/0x8
> [    5.916830]  ? default_idle+0xe/0x10
> [    5.916832]  ? do_idle.llvm.10446269078108580492+0xb7/0x130
> [    5.916833]  ? raw_setsockopt+0x8/0x8
> [    5.916833]  ? cpu_startup_entry+0x15/0x20
> [    5.916835]  ? cpu_hotplug_enable.cfi_jt+0x8/0x8
> [    5.916836]  ? start_secondary+0x188/0x190
> [    5.916837]  ? secondary_startup_64+0xa5/0xb0
> [    5.916838] ---[ end trace f2683fa869597ba5 ]---
> 
> Avoid this by using the right return type for netvsc_start_xmit.
> 
> Fixes: fceaf24a943d8 ("Staging: hv: add the Hyper-V virtual network driver")
> Link: https://github.com/ClangBuiltLinux/linux/issues/1009
> Signed-off-by: Nathan Chancellor <natechancellor@gmail.com>
> ---
> 
> v1 -> v2:
> 
> * Move splat into commit message rather than issue.
> 
> Comment from previous version:
> 
> Do note that netvsc_xmit still returns int because netvsc_xmit has a
> potential return from netvsc_vf_xmit, which does not return netdev_tx_t
> because of the call to dev_queue_xmit.
> 
> I am not sure if that is an oversight that was introduced by
> commit 0c195567a8f6e ("netvsc: transparent VF management") or if
> everything works properly as it is now.
> 
> My patch is purely concerned with making the definition match the
> prototype so it should be NFC aside from avoiding the CFI panic.
> 
>  drivers/net/hyperv/netvsc_drv.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/hyperv/netvsc_drv.c b/drivers/net/hyperv/netvsc_drv.c
> index d8e86bdbfba1e..ebcfbae056900 100644
> --- a/drivers/net/hyperv/netvsc_drv.c
> +++ b/drivers/net/hyperv/netvsc_drv.c
> @@ -707,7 +707,8 @@ static int netvsc_xmit(struct sk_buff *skb, struct net_device *net, bool xdp_tx)
>  	goto drop;
>  }
>  
> -static int netvsc_start_xmit(struct sk_buff *skb, struct net_device *ndev)
> +static netdev_tx_t netvsc_start_xmit(struct sk_buff *skb,
> +				     struct net_device *ndev)
>  {
>  	return netvsc_xmit(skb, ndev, false);
>  }
> 
> base-commit: 51184ae37e0518fd90cb437a2fbc953ae558cd0d
> -- 
> 2.26.2
> 

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v2] hv_netvsc: Fix netvsc_start_xmit's return type
  2020-04-29 10:10     ` Wei Liu
@ 2020-04-29 18:11       ` David Miller
  0 siblings, 0 replies; 9+ messages in thread
From: David Miller @ 2020-04-29 18:11 UTC (permalink / raw)
  To: wei.liu
  Cc: natechancellor, kys, haiyangz, sthemmin, linux-hyperv, netdev,
	linux-kernel, clang-built-linux, samitolvanen

From: Wei Liu <wei.liu@kernel.org>
Date: Wed, 29 Apr 2020 11:10:55 +0100

> Do you want this to go through net tree? I can submit it via hyperv tree
> if that's preferred.

I'll be taking this, thanks.

^ permalink raw reply	[flat|nested] 9+ messages in thread

* RE: [PATCH v2] hv_netvsc: Fix netvsc_start_xmit's return type
  2020-04-28 17:54   ` [PATCH v2] " Nathan Chancellor
  2020-04-29 10:10     ` Wei Liu
@ 2020-04-30  0:06     ` Michael Kelley
  2020-04-30  6:01       ` Nathan Chancellor
  2020-05-01 22:25     ` David Miller
  2 siblings, 1 reply; 9+ messages in thread
From: Michael Kelley @ 2020-04-30  0:06 UTC (permalink / raw)
  To: Nathan Chancellor, KY Srinivasan, Haiyang Zhang,
	Stephen Hemminger, Wei Liu
  Cc: linux-hyperv, netdev, linux-kernel, clang-built-linux, Sami Tolvanen

From: Nathan Chancellor <natechancellor@gmail.com> Sent: Tuesday, April 28, 2020 10:55 AM
> 
> Do note that netvsc_xmit still returns int because netvsc_xmit has a
> potential return from netvsc_vf_xmit, which does not return netdev_tx_t
> because of the call to dev_queue_xmit.
> 
> I am not sure if that is an oversight that was introduced by
> commit 0c195567a8f6e ("netvsc: transparent VF management") or if
> everything works properly as it is now.
> 
> My patch is purely concerned with making the definition match the
> prototype so it should be NFC aside from avoiding the CFI panic.
> 

While it probably works correctly now, I'm not too keen on just
changing the return type for netvsc_start_xmit() and assuming the
'int' that is returned from netvsc_xmit() will be correctly mapped to
the netdev_tx_t enum type.  While that mapping probably happens
correctly at the moment, this really should have a more holistic fix.

Nathan -- are you willing to look at doing the more holistic fix?  Or
should we see about asking Haiyang Zhang to do it?

Michael

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v2] hv_netvsc: Fix netvsc_start_xmit's return type
  2020-04-30  0:06     ` Michael Kelley
@ 2020-04-30  6:01       ` Nathan Chancellor
  2020-04-30 15:42         ` Haiyang Zhang
  0 siblings, 1 reply; 9+ messages in thread
From: Nathan Chancellor @ 2020-04-30  6:01 UTC (permalink / raw)
  To: Michael Kelley
  Cc: KY Srinivasan, Haiyang Zhang, Stephen Hemminger, Wei Liu,
	linux-hyperv, netdev, linux-kernel, clang-built-linux,
	Sami Tolvanen

Hi Michael,

On Thu, Apr 30, 2020 at 12:06:09AM +0000, Michael Kelley wrote:
> From: Nathan Chancellor <natechancellor@gmail.com> Sent: Tuesday, April 28, 2020 10:55 AM
> > 
> > Do note that netvsc_xmit still returns int because netvsc_xmit has a
> > potential return from netvsc_vf_xmit, which does not return netdev_tx_t
> > because of the call to dev_queue_xmit.
> > 
> > I am not sure if that is an oversight that was introduced by
> > commit 0c195567a8f6e ("netvsc: transparent VF management") or if
> > everything works properly as it is now.
> > 
> > My patch is purely concerned with making the definition match the
> > prototype so it should be NFC aside from avoiding the CFI panic.
> > 
> 
> While it probably works correctly now, I'm not too keen on just
> changing the return type for netvsc_start_xmit() and assuming the
> 'int' that is returned from netvsc_xmit() will be correctly mapped to
> the netdev_tx_t enum type.  While that mapping probably happens
> correctly at the moment, this really should have a more holistic fix.

While it might work correctly, I am not sure that the mapping is
correct, hence that comment.

netdev_tx_t is an enum with two acceptable types, 0x00 and 0x10. Up
until commit 0c195567a8f6e ("netvsc: transparent VF management"),
netvsc_xmit was guaranteed to return something of type netdev_tx_t.

However, after said commit, we could return anything from
netvsc_vf_xmit, which in turn calls dev_queue_xmit then
__dev_queue_xmit which will return either an error code (-ENOMEM or
-ENETDOWN) or something from __dev_xmit_skb, which appears to be
NET_XMIT_SUCCESS, NET_XMIT_DROP, or NET_XMIT_CN.

It does not look like netvsc_xmit or netvsc_vf_xmit try to convert those
returns to netdev_tx_t in some way; netvsc_vf_xmit just passes the
return value up to netvsc_xmit, which is the part that I am unsure
about...

> Nathan -- are you willing to look at doing the more holistic fix?  Or
> should we see about asking Haiyang Zhang to do it?

I would be fine trying to look at a more holistic fix but I know
basically nothing about this subsystem. I am unsure if something like
this would be acceptable or if something else needs to happen.
Otherwise, I'd be fine with you guys taking a look and just giving me
reported-by credit.

Cheers,
Nathan

diff --git a/drivers/net/hyperv/netvsc_drv.c b/drivers/net/hyperv/netvsc_drv.c
index d8e86bdbfba1e..a39480cfb8fa7 100644
--- a/drivers/net/hyperv/netvsc_drv.c
+++ b/drivers/net/hyperv/netvsc_drv.c
@@ -520,7 +520,8 @@ static int netvsc_vf_xmit(struct net_device *net, struct net_device *vf_netdev,
 	return rc;
 }
 
-static int netvsc_xmit(struct sk_buff *skb, struct net_device *net, bool xdp_tx)
+static netdev_tx_t netvsc_xmit(struct sk_buff *skb, struct net_device *net,
+			       bool xdp_tx)
 {
 	struct net_device_context *net_device_ctx = netdev_priv(net);
 	struct hv_netvsc_packet *packet = NULL;
@@ -537,8 +538,11 @@ static int netvsc_xmit(struct sk_buff *skb, struct net_device *net, bool xdp_tx)
 	 */
 	vf_netdev = rcu_dereference_bh(net_device_ctx->vf_netdev);
 	if (vf_netdev && netif_running(vf_netdev) &&
-	    !netpoll_tx_running(net))
-		return netvsc_vf_xmit(net, vf_netdev, skb);
+	    !netpoll_tx_running(net)) {
+		if (!netvsc_vf_xmit(net, vf_netdev, skb))
+			return NETDEV_TX_OK;
+		goto drop;
+	}
 
 	/* We will atmost need two pages to describe the rndis
 	 * header. We can only transmit MAX_PAGE_BUFFER_COUNT number
@@ -707,7 +711,8 @@ static int netvsc_xmit(struct sk_buff *skb, struct net_device *net, bool xdp_tx)
 	goto drop;
 }
 
-static int netvsc_start_xmit(struct sk_buff *skb, struct net_device *ndev)
+static netdev_tx_t netvsc_start_xmit(struct sk_buff *skb,
+				     struct net_device *ndev)
 {
 	return netvsc_xmit(skb, ndev, false);
 }

^ permalink raw reply related	[flat|nested] 9+ messages in thread

* RE: [PATCH v2] hv_netvsc: Fix netvsc_start_xmit's return type
  2020-04-30  6:01       ` Nathan Chancellor
@ 2020-04-30 15:42         ` Haiyang Zhang
  0 siblings, 0 replies; 9+ messages in thread
From: Haiyang Zhang @ 2020-04-30 15:42 UTC (permalink / raw)
  To: Nathan Chancellor, Michael Kelley
  Cc: KY Srinivasan, Stephen Hemminger, Wei Liu, linux-hyperv, netdev,
	linux-kernel, clang-built-linux, Sami Tolvanen



> -----Original Message-----
> From: Nathan Chancellor <natechancellor@gmail.com>
> Sent: Thursday, April 30, 2020 2:02 AM
> To: Michael Kelley <mikelley@microsoft.com>
> Cc: KY Srinivasan <kys@microsoft.com>; Haiyang Zhang
> <haiyangz@microsoft.com>; Stephen Hemminger
> <sthemmin@microsoft.com>; Wei Liu <wei.liu@kernel.org>; linux-
> hyperv@vger.kernel.org; netdev@vger.kernel.org; linux-
> kernel@vger.kernel.org; clang-built-linux@googlegroups.com; Sami
> Tolvanen <samitolvanen@google.com>
> Subject: Re: [PATCH v2] hv_netvsc: Fix netvsc_start_xmit's return type
> 
> Hi Michael,
> 
> On Thu, Apr 30, 2020 at 12:06:09AM +0000, Michael Kelley wrote:
> > From: Nathan Chancellor <natechancellor@gmail.com> Sent: Tuesday,
> > April 28, 2020 10:55 AM
> > >
> > > Do note that netvsc_xmit still returns int because netvsc_xmit has a
> > > potential return from netvsc_vf_xmit, which does not return
> > > netdev_tx_t because of the call to dev_queue_xmit.
> > >
> > > I am not sure if that is an oversight that was introduced by commit
> > > 0c195567a8f6e ("netvsc: transparent VF management") or if everything
> > > works properly as it is now.
> > >
> > > My patch is purely concerned with making the definition match the
> > > prototype so it should be NFC aside from avoiding the CFI panic.
> > >
> >
> > While it probably works correctly now, I'm not too keen on just
> > changing the return type for netvsc_start_xmit() and assuming the
> > 'int' that is returned from netvsc_xmit() will be correctly mapped to
> > the netdev_tx_t enum type.  While that mapping probably happens
> > correctly at the moment, this really should have a more holistic fix.
> 
> While it might work correctly, I am not sure that the mapping is correct,
> hence that comment.
> 
> netdev_tx_t is an enum with two acceptable types, 0x00 and 0x10. Up until
> commit 0c195567a8f6e ("netvsc: transparent VF management"), netvsc_xmit
> was guaranteed to return something of type netdev_tx_t.
> 
> However, after said commit, we could return anything from netvsc_vf_xmit,
> which in turn calls dev_queue_xmit then __dev_queue_xmit which will
> return either an error code (-ENOMEM or
> -ENETDOWN) or something from __dev_xmit_skb, which appears to be
> NET_XMIT_SUCCESS, NET_XMIT_DROP, or NET_XMIT_CN.
> 
> It does not look like netvsc_xmit or netvsc_vf_xmit try to convert those
> returns to netdev_tx_t in some way; netvsc_vf_xmit just passes the return
> value up to netvsc_xmit, which is the part that I am unsure about...
> 
> > Nathan -- are you willing to look at doing the more holistic fix?  Or
> > should we see about asking Haiyang Zhang to do it?
> 
> I would be fine trying to look at a more holistic fix but I know basically nothing
> about this subsystem. I am unsure if something like this would be acceptable
> or if something else needs to happen.
> Otherwise, I'd be fine with you guys taking a look and just giving me
> reported-by credit.

Here is more info regarding Linux network subsystem:

As said in "include/linux/netdevice.h", drivers are allowed to return any codes 
from the three different namespaces.
And hv_netvsc needs to support "transparent VF", and calls netvsc_vf_xmit >> 
dev_queue_xmit which returns qdisc return codes, and errnos like -ENOMEM, 
etc. These are compliant with the guideline below:

  79 /*
  80  * Transmit return codes: transmit return codes originate from three different
  81  * namespaces:
  82  *
  83  * - qdisc return codes
  84  * - driver transmit return codes
  85  * - errno values
  86  *
  87  * Drivers are allowed to return any one of those in their hard_start_xmit()

Also, ndo_start_xmit function pointer is used by upper layer functions which can 
handles three types of the return codes. 
For example, in the calling stack: ndo_start_xmit << netdev_start_xmit << 
xmit_one << dev_hard_start_xmit():
The function dev_hard_start_xmit() uses dev_xmit_complete() to handle the 
return codes. It handles three types of the return codes correctly.

 3483 struct sk_buff *dev_hard_start_xmit(struct sk_buff *first, struct net_device *dev,
 3484                                     struct netdev_queue *txq, int *ret)
 3485 {
 3486         struct sk_buff *skb = first;
 3487         int rc = NETDEV_TX_OK;
 3488
 3489         while (skb) {
 3490                 struct sk_buff *next = skb->next;
 3491
 3492                 skb_mark_not_on_list(skb);
 3493                 rc = xmit_one(skb, dev, txq, next != NULL);
 3494                 if (unlikely(!dev_xmit_complete(rc))) {
 3495                         skb->next = next;
 3496                         goto out;
 3497                 }
 3498
 3499                 skb = next;
 3500                 if (netif_tx_queue_stopped(txq) && skb) {
 3501                         rc = NETDEV_TX_BUSY;
 3502                         break;
 3503                 }
 3504         }
 3505
 3506 out:
 3507         *ret = rc;
 3508         return skb;
 3509 }


 118 /*
 119  * Current order: NETDEV_TX_MASK > NET_XMIT_MASK >= 0 is significant;
 120  * hard_start_xmit() return < NET_XMIT_MASK means skb was consumed.
 121  */
 122 static inline bool dev_xmit_complete(int rc)
 123 {
 124         /*
 125          * Positive cases with an skb consumed by a driver:
 126          * - successful transmission (rc == NETDEV_TX_OK)
 127          * - error while transmitting (rc < 0)
 128          * - error while queueing to a different device (rc & NET_XMIT_MASK)
 129          */
 130         if (likely(rc < NET_XMIT_MASK))
 131                 return true;
 132
 133         return false;
 134 }

Regarding "a more holistic fix", I believe the return type of ndo_start_xmit should be 
int, because of three namespaces of the return codes. This means to change all network 
drivers. I'm not proposing to do this big change right now.

So I have no objection of your patch.

Thanks,
- Haiyang

Reviewed-by: Haiyang Zhang <haiyangz@microsoft.com>


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v2] hv_netvsc: Fix netvsc_start_xmit's return type
  2020-04-28 17:54   ` [PATCH v2] " Nathan Chancellor
  2020-04-29 10:10     ` Wei Liu
  2020-04-30  0:06     ` Michael Kelley
@ 2020-05-01 22:25     ` David Miller
  2 siblings, 0 replies; 9+ messages in thread
From: David Miller @ 2020-05-01 22:25 UTC (permalink / raw)
  To: natechancellor
  Cc: kys, haiyangz, sthemmin, wei.liu, linux-hyperv, netdev,
	linux-kernel, clang-built-linux, samitolvanen

From: Nathan Chancellor <natechancellor@gmail.com>
Date: Tue, 28 Apr 2020 10:54:56 -0700

> netvsc_start_xmit is used as a callback function for the ndo_start_xmit
> function pointer. ndo_start_xmit's return type is netdev_tx_t but
> netvsc_start_xmit's return type is int.
> 
> This causes a failure with Control Flow Integrity (CFI), which requires
> function pointer prototypes and callback function definitions to match
> exactly. When CFI is in enforcing, the kernel panics. When booting a
> CFI kernel with WSL 2, the VM is immediately terminated because of this.
> 
> The splat when CONFIG_CFI_PERMISSIVE is used:
 ...
> Avoid this by using the right return type for netvsc_start_xmit.
> 
> Fixes: fceaf24a943d8 ("Staging: hv: add the Hyper-V virtual network driver")
> Link: https://github.com/ClangBuiltLinux/linux/issues/1009
> Signed-off-by: Nathan Chancellor <natechancellor@gmail.com>

Applied.

^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2020-05-01 22:25 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-04-28  3:30 [PATCH] hv_netvsc: Fix netvsc_start_xmit's return type Nathan Chancellor
2020-04-28 10:08 ` Wei Liu
2020-04-28 17:54   ` [PATCH v2] " Nathan Chancellor
2020-04-29 10:10     ` Wei Liu
2020-04-29 18:11       ` David Miller
2020-04-30  0:06     ` Michael Kelley
2020-04-30  6:01       ` Nathan Chancellor
2020-04-30 15:42         ` Haiyang Zhang
2020-05-01 22:25     ` David Miller

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).