Re: [PATCH v4] net: hsr: Provide RedBox support (HSR-SAN)

[Lukasz Majewski]

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
> > 
> > The corresponding patch to modify iptable2 sources has already been
> > sent:
> > https://lore.kernel.org/netdev/20240308145729.490863-1-lukma@xxxxxxx/T/
> > 
> > Testing procedure:
> > ------------------
> > The EVB-KSZ9477 has been used for testing on net-next branch
> > (SHA1: 5fc68320c1fb3c7d456ddcae0b4757326a043e6f).
> > 
> > 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).
> > 
> > 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
> > 
> > Configuration - DAN-H (EVB-KSZ9477):
> > 
> > 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
> > 
> > This approach uses only SW based HSR devices (hsr1).
> > 
> > --------------          -----------------       ------------
> > DAN-H  Port5 | <------> | Port5         |       |
> >        Port4 | <------> | Port4   Port3 | <---> | PC
> >              |          | (RedBox)      |       | (USB-ETH)
> > EVB-KSZ9477  |          | EVB-KSZ9477   |       |
> > --------------          -----------------       ------------
> > 
> > Signed-off-by: Lukasz Majewski <lukma@xxxxxxx>  
> 
> This is 'net-next' patch, you must insert the target tree in the subj
> prefix.

Ok. I will add it in v5.

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

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

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. 

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).
> 

Ok.

> > @@ -561,6 +582,37 @@ void hsr_prune_nodes(struct timer_list *t)
> >  		  jiffies + msecs_to_jiffies(PRUNE_PERIOD));
> >  }
> >  
> > +void hsr_prune_proxy_nodes(struct timer_list *t)
> > +{
> > +	struct hsr_priv *hsr = 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 = 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 = 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));
> >  
> 
> AFAICS this timer not explicitly cancelled at hsr port tear-down time.
> 
> What prevent it from expiring after a port has been deleted and
> causing UaF?
> 

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,
> 
> Paolo
> 


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@xxxxxxx

Attachment: pgp9N_QI8s2cj.pgp
Description: OpenPGP digital signature