Received-SPF: pass (google.com: domain of linux-kernel+bounces-131275-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
Date: Thu, 4 Apr 2024 12:51:59 +0200
From: Lukasz Majewski <lukma@denx.de>
To: Paolo Abeni <pabeni@redhat.com>
Cc: netdev@vger.kernel.org, Andrew Lunn <andrew@lunn.ch>, Eric Dumazet
 <edumazet@google.com>, Vladimir Oltean <olteanv@gmail.com>, "David S.
 Miller" <davem@davemloft.net>, Jakub Kicinski <kuba@kernel.org>, Oleksij
 Rempel <o.rempel@pengutronix.de>, Tristram.Ha@microchip.com, Sebastian
 Andrzej Siewior <bigeasy@linutronix.de>, Ravi Gunasekaran
 <r-gunasekaran@ti.com>, Simon Horman <horms@kernel.org>, Nikita
 Zhandarovich <n.zhandarovich@fintech.ru>, Murali Karicheri
 <m-karicheri2@ti.com>, Jiri Pirko <jiri@resnulli.us>, Dan Carpenter
 <dan.carpenter@linaro.org>, Ziyang Xuan <william.xuanziyang@huawei.com>,
 Shigeru Yoshida <syoshida@redhat.com>, linux-kernel@vger.kernel.org
Subject: Re: [PATCH v4] net: hsr: Provide RedBox support (HSR-SAN)
Message-ID: <20240404125159.721fbc19@wsk>
In-Reply-To: <168c9226d2cac6a5a53ce33ef1f76b99a02bedcb.camel@redhat.com>
References: <20240402085850.229058-1-lukma@denx.de>
	<168c9226d2cac6a5a53ce33ef1f76b99a02bedcb.camel@redhat.com>
Organization: denx.de
Precedence: bulk
MIME-Version: 1.0
Content-Type: multipart/signed; boundary="Sig_/2tuYs_oCKZ5lyc/yz2.Ps0l";
 protocol="application/pgp-signature"; micalg=pgp-sha512

--Sig_/2tuYs_oCKZ5lyc/yz2.Ps0l
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: quoted-printable

Hi Paolo,

> On Tue, 2024-04-02 at 10:58 +0200, Lukasz Majewski wrote:
> > Introduce RedBox support (HSR-SAN to be more precise) for HSR
> > networks. Following traffic reduction optimizations have been
> > implemented:
> > - Do not send HSR supervisory frames to Port C (interlink)
> > - Do not forward to HSR ring frames addressed to Port C
> > - Do not forward to Port C frames from HSR ring
> > - Do not send duplicate HSR frame to HSR ring when destination is
> > Port C
> >=20
> > The corresponding patch to modify iptable2 sources has already been
> > sent:
> > https://lore.kernel.org/netdev/20240308145729.490863-1-lukma@denx.de/T/
> >=20
> > Testing procedure:
> > ------------------
> > The EVB-KSZ9477 has been used for testing on net-next branch
> > (SHA1: 5fc68320c1fb3c7d456ddcae0b4757326a043e6f).
> >=20
> > Ports 4/5 were used for SW managed HSR (hsr1) as first hsr0 for
> > ports 1/2 (with HW offloading for ksz9477) was created. Port 3 has
> > been used as interlink port (single USB-ETH dongle).
> >=20
> > Configuration - RedBox (EVB-KSZ9477):
> > if link set lan1 down;ip link set lan2 down
> > ip link add name hsr0 type hsr slave1 lan1 slave2 lan2 supervision
> > 45 version 1 ip link add name hsr1 type hsr slave1 lan4 slave2 lan5
> > interlink lan3 supervision 45 version 1 ip link set lan4 up;ip link
> > set lan5 up ip link set lan3 up
> > ip addr add 192.168.0.11/24 dev hsr1
> > ip link set hsr1 up
> >=20
> > Configuration - DAN-H (EVB-KSZ9477):
> >=20
> > ip link set lan1 down;ip link set lan2 down
> > ip link add name hsr0 type hsr slave1 lan1 slave2 lan2 supervision
> > 45 version 1 ip link add name hsr1 type hsr slave1 lan4 slave2 lan5
> > supervision 45 version 1 ip link set lan4 up;ip link set lan5 up
> > ip addr add 192.168.0.12/24 dev hsr1
> > ip link set hsr1 up
> >=20
> > This approach uses only SW based HSR devices (hsr1).
> >=20
> > --------------          -----------------       ------------
> > DAN-H  Port5 | <------> | Port5         |       |
> >        Port4 | <------> | Port4   Port3 | <---> | PC
> >              |          | (RedBox)      |       | (USB-ETH)
> > EVB-KSZ9477  |          | EVB-KSZ9477   |       |
> > --------------          -----------------       ------------
> >=20
> > Signed-off-by: Lukasz Majewski <lukma@denx.de> =20
>=20
> This is 'net-next' patch, you must insert the target tree in the subj
> prefix.

Ok. I will add it in v5.

>=20
> Does not apply cleanly to 'net-next', please rebase.
>=20

This patch depends on:
https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net.git/commit/?id=
=3D10e52ad5ced2

which already has been pulled to net-vanila/main
(git://git.kernel.org/pub/scm/linux/kernel/git/netdev/net)

Apparently, I need to wait until net-next would is rebased on top of the
above tree.

> Introducing a new functionality, this deserve some paired self-tests.=20

I see. I can try to add something similar to
/tools/testing/selftests/net/hsr/hsr_ping.sh

to show the RedBox functionality.

> Does this have specific H/W requirement or can it run e.g. on top of
> veths?

I'm testing this code on two EVB-KSZ9477 boards. It _shall_ work also on
veths.

> If the latter applies, please bundle some basic test with the
> next revision (separate patch, same series).
>=20

Ok.

> > @@ -561,6 +582,37 @@ void hsr_prune_nodes(struct timer_list *t)
> >  		  jiffies + msecs_to_jiffies(PRUNE_PERIOD));
> >  }
> > =20
> > +void hsr_prune_proxy_nodes(struct timer_list *t)
> > +{
> > +	struct hsr_priv *hsr =3D from_timer(hsr, t,
> > prune_proxy_timer);
> > +	unsigned long timestamp;
> > +	struct hsr_node *node;
> > +	struct hsr_node *tmp;
> > +
> > +	spin_lock_bh(&hsr->list_lock);
> > +	list_for_each_entry_safe(node, tmp, &hsr->proxy_node_db,
> > mac_list) {
> > +		timestamp =3D node->time_in[HSR_PT_INTERLINK];
> > +
> > +		/* Prune old entries */
> > +		if (time_is_before_jiffies(timestamp +
> > +
> > msecs_to_jiffies(HSR_PROXY_NODE_FORGET_TIME))) {
> > +			hsr_nl_nodedown(hsr, node->macaddress_A);
> > +			if (!node->removed) {
> > +				list_del_rcu(&node->mac_list);
> > +				node->removed =3D true;
> > +				/* Note that we need to free this
> > entry later: */
> > +				kfree_rcu(node, rcu_head);
> > +			}
> > +		}
> > +	}
> > +
> > +	spin_unlock_bh(&hsr->list_lock);
> > +
> > +	/* Restart timer */
> > +	mod_timer(&hsr->prune_proxy_timer,
> > +		  jiffies + msecs_to_jiffies(PRUNE_PROXY_PERIOD));
> > =20
>=20
> AFAICS this timer not explicitly cancelled at hsr port tear-down time.
>=20
> What prevent it from expiring after a port has been deleted and
> causing UaF?
>=20

The purpose of this timer is to periodically call
hsr_prune_proxy_node() function (presented above your reply), which
internally uses list_for_each_entry_safe() to iterate over available
RedBox nodes.

If there are none available, the function just re-sets the timer to
the next PRUNE_PROXY_PERIOD.

In the same way works the timer for hsr_prune_node() function, which
removes stalled HSR nodes.

> Cheers,
>=20
> Paolo
>=20


Best regards,

Lukasz Majewski

--

DENX Software Engineering GmbH,      Managing Director: Erika Unter
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email: lukma@denx.de

--Sig_/2tuYs_oCKZ5lyc/yz2.Ps0l
Content-Type: application/pgp-signature
Content-Description: OpenPGP digital signature

-----BEGIN PGP SIGNATURE-----

iQEzBAEBCgAdFiEEgAyFJ+N6uu6+XupJAR8vZIA0zr0FAmYOhk8ACgkQAR8vZIA0
zr3xTAf/bb5rdgPpVrlVqowjRP6eM+4Haptg/NBK+ZLilY9NsJTsjNLMCorbZlKe
3ICvyAa4SoQyA8DZ2Q1826BnytlAGr4UgplAMyTWRMJpoJpDgl0WniiSJFRvPUAK
oAXVadGHx7JOcSooiiv5FQDFVwyxDkw6WNWK+AZnn+IpGym2O/eL5BnDMjQwJjjS
YUsPxXi5tKXGXQUslHj0Sor9zfJXvL4/oXcFiiVXLINf9nZbuVZ6KoznCPUOKbNk
4b+vwevnV/8UFqn+ztKVfVA+BhfsbyTfA1P8dUgnJ5cqECLIiCfpkr5fSz0L8voF
asODAcmz3JuZgSJrtBKhkAIfDyWKuQ==
=fm2p
-----END PGP SIGNATURE-----

--Sig_/2tuYs_oCKZ5lyc/yz2.Ps0l--