Message-ID: <20241030151547.5t5f55hxwd33qysw@skbuf>
Date: Wed, 30 Oct 2024 17:15:47 +0200
From: Vladimir Oltean <vladimir.oltean@....com>
To: Wei Fang <wei.fang@....com>
Cc: davem@...emloft.net, edumazet@...gle.com, kuba@...nel.org,
	pabeni@...hat.com, andrew+netdev@...n.ch, claudiu.manoil@....com,
	netdev@...r.kernel.org, linux-kernel@...r.kernel.org,
	imx@...ts.linux.dev
Subject: Re: [PATCH net] net: enetc: prevent VF from configuring preemptiable
 TCs

802.1Q spells 'preemptible' using 'i', 802.3 spells it using 'a', you
decided to spell it using both :)

FWIW, the kernel uses 'preemptible' because 802.1Q is the authoritative
standard for this feature, 802.3 just references it.

On Wed, Oct 30, 2024 at 04:21:17PM +0800, Wei Fang wrote:
> Both ENETC PF and VF drivers share enetc_setup_tc_mqprio() to configure
> MQPRIO. And enetc_setup_tc_mqprio() calls enetc_change_preemptible_tcs()
> to configure preemptible TCs. However, only PF is able to configure
> preemptible TCs. Because only PF has related registers, while VF does not
> have these registers. So for VF, its hw->port pointer is NULL. Therefore,
> VF will access an invalid pointer when accessing a non-existent register,
> which will cause a call trace issue. The call trace log is shown below.
> 
> root@...028ardb:~# tc qdisc add dev eno0vf0 parent root handle 100: \
> mqprio num_tc 4 map 0 0 1 1 2 2 3 3 queues 1@0 1@1 1@2 1@3 hw 1
> [  187.290775] Unable to handle kernel paging request at virtual address 0000000000001f00
> [  187.298760] Mem abort info:
> [  187.301607]   ESR = 0x0000000096000004
> [  187.305373]   EC = 0x25: DABT (current EL), IL = 32 bits
> [  187.310714]   SET = 0, FnV = 0
> [  187.313778]   EA = 0, S1PTW = 0
> [  187.316923]   FSC = 0x04: level 0 translation fault
> [  187.321818] Data abort info:
> [  187.324701]   ISV = 0, ISS = 0x00000004, ISS2 = 0x00000000
> [  187.330207]   CM = 0, WnR = 0, TnD = 0, TagAccess = 0
> [  187.335278]   GCS = 0, Overlay = 0, DirtyBit = 0, Xs = 0
> [  187.340610] user pgtable: 4k pages, 48-bit VAs, pgdp=0000002084b71000
> [  187.347076] [0000000000001f00] pgd=0000000000000000, p4d=0000000000000000
> [  187.353900] Internal error: Oops: 0000000096000004 [#1] PREEMPT SMP
> [  187.360188] Modules linked in: xt_conntrack xt_addrtype cfg80211 rfkill snd_soc_hdmi_codec fsl_jr_uio caam_jr caamkeyblob_desc caamhash_desc caamalg_descp
> [  187.406320] CPU: 0 PID: 1117 Comm: tc Not tainted 6.6.52-gfbb48d8e2ddb #1
> [  187.413131] Hardware name: LS1028A RDB Board (DT)
> [  187.417846] pstate: 40000005 (nZcv daif -PAN -UAO -TCO -DIT -SSBS BTYPE=--)
> [  187.424831] pc : enetc_mm_commit_preemptible_tcs+0x1c4/0x400
> [  187.430518] lr : enetc_mm_commit_preemptible_tcs+0x30c/0x400
> [  187.436195] sp : ffff800084a4b400
> [  187.439515] x29: ffff800084a4b400 x28: 0000000000000004 x27: ffff6a58c5229808
> [  187.446679] x26: 0000000080000203 x25: ffff800085218600 x24: 0000000000000004
> [  187.453842] x23: ffff6a58c42e4a00 x22: ffff6a58c5229808 x21: ffff6a58c42e4980
> [  187.461004] x20: ffff6a58c5229800 x19: 0000000000001f00 x18: 0000000000000001
> [  187.468167] x17: 0000000000000000 x16: 0000000000000000 x15: 0000000000000000
> [  187.475328] x14: 00000000000003f8 x13: 0000000000000400 x12: 0000000000065feb
> [  187.482491] x11: 0000000000000000 x10: ffff6a593f6f9918 x9 : 0000000000000000
> [  187.489653] x8 : ffff800084a4b668 x7 : 0000000000000003 x6 : 0000000000000001
> [  187.496815] x5 : 0000000000001f00 x4 : 0000000000000000 x3 : 0000000000000000
> [  187.503977] x2 : 0000000000000000 x1 : 0000000000000200 x0 : ffffd2fbd8497460
> [  187.511140] Call trace:
> [  187.513588]  enetc_mm_commit_preemptible_tcs+0x1c4/0x400
> [  187.518918]  enetc_setup_tc_mqprio+0x180/0x214
> [  187.523374]  enetc_vf_setup_tc+0x1c/0x30
> [  187.527306]  mqprio_enable_offload+0x144/0x178
> [  187.531766]  mqprio_init+0x3ec/0x668
> [  187.535351]  qdisc_create+0x15c/0x488
> [  187.539023]  tc_modify_qdisc+0x398/0x73c
> [  187.542958]  rtnetlink_rcv_msg+0x128/0x378
> [  187.547064]  netlink_rcv_skb+0x60/0x130
> [  187.550910]  rtnetlink_rcv+0x18/0x24
> [  187.554492]  netlink_unicast+0x300/0x36c
> [  187.558425]  netlink_sendmsg+0x1a8/0x420
> [  187.562358]  ____sys_sendmsg+0x214/0x26c
> [  187.566292]  ___sys_sendmsg+0xb0/0x108
> [  187.570050]  __sys_sendmsg+0x88/0xe8
> [  187.573634]  __arm64_sys_sendmsg+0x24/0x30
> [  187.577742]  invoke_syscall+0x48/0x114
> [  187.581503]  el0_svc_common.constprop.0+0x40/0xe0
> [  187.586222]  do_el0_svc+0x1c/0x28
> [  187.589544]  el0_svc+0x40/0xe4
> [  187.592607]  el0t_64_sync_handler+0x120/0x12c
> [  187.596976]  el0t_64_sync+0x190/0x194
> [  187.600648] Code: d65f03c0 d283e005 8b050273 14000050 (b9400273)
> [  187.606759] ---[ end trace 0000000000000000 ]---

Please be more succint with the stack trace. Nobody cares about more
than this:

Unable to handle kernel paging request at virtual address 0000000000001f00
Internal error: Oops: 0000000096000004 [#1] PREEMPT SMP
Hardware name: LS1028A RDB Board (DT)
pc : enetc_mm_commit_preemptible_tcs+0x1c4/0x400
lr : enetc_mm_commit_preemptible_tcs+0x30c/0x400
Call trace:
 enetc_mm_commit_preemptible_tcs+0x1c4/0x400
 enetc_setup_tc_mqprio+0x180/0x214
 enetc_vf_setup_tc+0x1c/0x30
 mqprio_enable_offload+0x144/0x178
 mqprio_init+0x3ec/0x668
 qdisc_create+0x15c/0x488
 tc_modify_qdisc+0x398/0x73c
 rtnetlink_rcv_msg+0x128/0x378
 netlink_rcv_skb+0x60/0x130

> 
> Fixes: 827145392a4a ("net: enetc: only commit preemptible TCs to hardware when MM TX is active")
> Signed-off-by: Wei Fang <wei.fang@....com>
> ---
>  drivers/net/ethernet/freescale/enetc/enetc.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/net/ethernet/freescale/enetc/enetc.c b/drivers/net/ethernet/freescale/enetc/enetc.c
> index c09370eab319..c9f7b7b4445f 100644
> --- a/drivers/net/ethernet/freescale/enetc/enetc.c
> +++ b/drivers/net/ethernet/freescale/enetc/enetc.c
> @@ -28,6 +28,9 @@ EXPORT_SYMBOL_GPL(enetc_port_mac_wr);
>  static void enetc_change_preemptible_tcs(struct enetc_ndev_priv *priv,
>  					 u8 preemptible_tcs)
>  {
> +	if (!enetc_si_is_pf(priv->si))
> +		return;
> +

Actually please do this instead:

	if (!(si->hw_features & ENETC_SI_F_QBU))

We also shouldn't do anything here for those PFs which do not support frame
preemption (eno1, eno3 on LS1028A). It won't crash like it does for VFs,
but we should still avoid accessing registers which aren't implemented.
The ethtool mm ops are protected using this test, but I didn't realize
that tc has its own separate entry point which needs it too.

>  	priv->preemptible_tcs = preemptible_tcs;
>  	enetc_mm_commit_preemptible_tcs(priv);
>  }
> -- 
> 2.34.1
>

Powered by blists - more mailing lists