lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  linux-cve-announce  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID:
 <PAXPR04MB85104B9FCD3D74743E9B261488552@PAXPR04MB8510.eurprd04.prod.outlook.com>
Date: Thu, 31 Oct 2024 03:46:47 +0000
From: Wei Fang <wei.fang@....com>
To: Vladimir Oltean <vladimir.oltean@....com>
CC: "davem@...emloft.net" <davem@...emloft.net>, "edumazet@...gle.com"
	<edumazet@...gle.com>, "kuba@...nel.org" <kuba@...nel.org>,
	"pabeni@...hat.com" <pabeni@...hat.com>, "andrew+netdev@...n.ch"
	<andrew+netdev@...n.ch>, Claudiu Manoil <claudiu.manoil@....com>,
	"netdev@...r.kernel.org" <netdev@...r.kernel.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"imx@...ts.linux.dev" <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 :)
> >
> 
> My bad, I'll fix the typo
> 
> > 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:
> 
> Okay, thanks
> 
> >
> > 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))
> >

Actually, VFs of eno0 have ENETC_SI_F_QBU bit set. So we should use the
following check instead.

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

Or we only set ENETC_SI_F_QBU bit for PF in enetc_get_si_caps() if the PF
supports 802.1 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.
> >
> 
> Yeah, I agree with you, I will use your solution, thanks.
> 
> > >  	priv->preemptible_tcs = preemptible_tcs;
> > >  	enetc_mm_commit_preemptible_tcs(priv);
> > >  }
> > > --
> > > 2.34.1
> > >

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ