[PATCH net 0/2] net: hsr: Use the seqnr lock for frames received via interlink port.

[PATCH net 0/2] net: hsr: Use the seqnr lock for frames received via interlink port.

[Sebastian Andrzej Siewior]

This is follow-up to the thread at
	https://lore.kernel.org/all/20240904133725.1073963-1-edumazet@google.com/

I hope the two patches in a series targeting different trees is okay.
Otherwise I will resend.

Sebastian

[PATCH net 1/2] net: hsr: Use the seqnr lock for frames received via interlink port.

[Sebastian Andrzej Siewior]

syzbot reported that the seqnr_lock is not acquire for frames received
over the interlink port. In the interlink case a new seqnr is generated
and assigned to the frame.
Frames, which are received over the slave port have already a sequence
number assigned so the lock is not required.

Acquire the hsr_priv::seqnr_lock during in the invocation of
hsr_forward_skb() if a packet has been received from the interlink port.

Reported-by: syzbot+3d602af7549af539274e@syzkaller.appspotmail.com
Closes: https://groups.google.com/g/syzkaller-bugs/c/KppVvGviGg4/m/EItSdCZdBAAJ
Fixes: 5055cccfc2d1c ("net: hsr: Provide RedBox support (HSR-SAN)")
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
 net/hsr/hsr_slave.c | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/net/hsr/hsr_slave.c b/net/hsr/hsr_slave.c
index af6cf64a00e08..464f683e016db 100644
--- a/net/hsr/hsr_slave.c
+++ b/net/hsr/hsr_slave.c
@@ -67,7 +67,16 @@ static rx_handler_result_t hsr_handle_frame(struct sk_buff **pskb)
 		skb_set_network_header(skb, ETH_HLEN + HSR_HLEN);
 	skb_reset_mac_len(skb);
 
-	hsr_forward_skb(skb, port);
+	/* Only the frames received over the interlink port will assign a
+	 * sequence number and require synchronisation vs other sender.
+	 */
+	if (port->type == HSR_PT_INTERLINK) {
+		spin_lock_bh(&hsr->seqnr_lock);
+		hsr_forward_skb(skb, port);
+		spin_unlock_bh(&hsr->seqnr_lock);
+	} else {
+		hsr_forward_skb(skb, port);
+	}
 
 finish_consume:
 	return RX_HANDLER_CONSUMED;
-- 
2.45.2

Re: [PATCH net 1/2] net: hsr: Use the seqnr lock for frames received via interlink port.

[Lukasz Majewski]

[-- Attachment #1: Type: text/plain, Size: 2350 bytes --]

Hi Sebastian,

> syzbot reported that the seqnr_lock is not acquire for frames received
> over the interlink port. In the interlink case a new seqnr is
> generated and assigned to the frame.

Yes, correct.

The seq number for frames incomming from HSR ring are extracted from
the HSR header.

For frames going from interlink (SAN) network to RedBox, the start seq
number is assigned when node is created (and the creation node code is
reused).

> Frames, which are received over the slave port have already a sequence
> number assigned so the lock is not required.
> 
> Acquire the hsr_priv::seqnr_lock during in the invocation of
> hsr_forward_skb() if a packet has been received from the interlink
> port.
> 
> Reported-by: syzbot+3d602af7549af539274e@syzkaller.appspotmail.com
> Closes:
> https://groups.google.com/g/syzkaller-bugs/c/KppVvGviGg4/m/EItSdCZdBAAJ
> Fixes: 5055cccfc2d1c ("net: hsr: Provide RedBox support (HSR-SAN)")
> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de> ---
>  net/hsr/hsr_slave.c | 11 ++++++++++-
>  1 file changed, 10 insertions(+), 1 deletion(-)
> 
> diff --git a/net/hsr/hsr_slave.c b/net/hsr/hsr_slave.c
> index af6cf64a00e08..464f683e016db 100644
> --- a/net/hsr/hsr_slave.c
> +++ b/net/hsr/hsr_slave.c
> @@ -67,7 +67,16 @@ static rx_handler_result_t hsr_handle_frame(struct
> sk_buff **pskb) skb_set_network_header(skb, ETH_HLEN + HSR_HLEN);
>  	skb_reset_mac_len(skb);
>  
> -	hsr_forward_skb(skb, port);
> +	/* Only the frames received over the interlink port will
> assign a
> +	 * sequence number and require synchronisation vs other
> sender.
> +	 */
> +	if (port->type == HSR_PT_INTERLINK) {
> +		spin_lock_bh(&hsr->seqnr_lock);

I'm just wondering if this could have impact on offloaded HSR operation.

I will try to run hsr_redbox.sh test on this patch (with QEMU) and
share results.

> +		hsr_forward_skb(skb, port);
> +		spin_unlock_bh(&hsr->seqnr_lock);
> +	} else {
> +		hsr_forward_skb(skb, port);
> +	}
>  
>  finish_consume:
>  	return RX_HANDLER_CONSUMED;




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

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH net 1/2] net: hsr: Use the seqnr lock for frames received via interlink port.

[Jakub Kicinski]

On Mon, 9 Sep 2024 11:49:48 +0200 Lukasz Majewski wrote:
> > +	if (port->type == HSR_PT_INTERLINK) {
> > +		spin_lock_bh(&hsr->seqnr_lock);  
> 
> I'm just wondering if this could have impact on offloaded HSR operation.
> 
> I will try to run hsr_redbox.sh test on this patch (with QEMU) and
> share results.

Hi Lukasz, any luck with the testing?

Re: [PATCH net 1/2] net: hsr: Use the seqnr lock for frames received via interlink port.

[Lukasz Majewski]

[-- Attachment #1: Type: text/plain, Size: 683 bytes --]

Hi Jakub,

> On Mon, 9 Sep 2024 11:49:48 +0200 Lukasz Majewski wrote:
> > > +	if (port->type == HSR_PT_INTERLINK) {
> > > +		spin_lock_bh(&hsr->seqnr_lock);    
> > 
> > I'm just wondering if this could have impact on offloaded HSR
> > operation.
> > 
> > I will try to run hsr_redbox.sh test on this patch (with QEMU) and
> > share results.  
> 
> Hi Lukasz, any luck with the testing?

I will reply by EOD


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

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH net 1/2] net: hsr: Use the seqnr lock for frames received via interlink port.

[Lukasz Majewski]

[-- Attachment #1: Type: text/plain, Size: 2169 bytes --]

Hi Sebastian Andrzej,

> syzbot reported that the seqnr_lock is not acquire for frames received
> over the interlink port. In the interlink case a new seqnr is
> generated and assigned to the frame.
> Frames, which are received over the slave port have already a sequence
> number assigned so the lock is not required.
> 
> Acquire the hsr_priv::seqnr_lock during in the invocation of
> hsr_forward_skb() if a packet has been received from the interlink
> port.
> 
> Reported-by: syzbot+3d602af7549af539274e@syzkaller.appspotmail.com
> Closes:
> https://groups.google.com/g/syzkaller-bugs/c/KppVvGviGg4/m/EItSdCZdBAAJ
> Fixes: 5055cccfc2d1c ("net: hsr: Provide RedBox support (HSR-SAN)")
> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de> ---
>  net/hsr/hsr_slave.c | 11 ++++++++++-
>  1 file changed, 10 insertions(+), 1 deletion(-)
> 
> diff --git a/net/hsr/hsr_slave.c b/net/hsr/hsr_slave.c
> index af6cf64a00e08..464f683e016db 100644
> --- a/net/hsr/hsr_slave.c
> +++ b/net/hsr/hsr_slave.c
> @@ -67,7 +67,16 @@ static rx_handler_result_t hsr_handle_frame(struct
> sk_buff **pskb) skb_set_network_header(skb, ETH_HLEN + HSR_HLEN);
>  	skb_reset_mac_len(skb);
>  
> -	hsr_forward_skb(skb, port);
> +	/* Only the frames received over the interlink port will
> assign a
> +	 * sequence number and require synchronisation vs other
> sender.
> +	 */
> +	if (port->type == HSR_PT_INTERLINK) {
> +		spin_lock_bh(&hsr->seqnr_lock);
> +		hsr_forward_skb(skb, port);
> +		spin_unlock_bh(&hsr->seqnr_lock);
> +	} else {
> +		hsr_forward_skb(skb, port);
> +	}
>  
>  finish_consume:
>  	return RX_HANDLER_CONSUMED;

I've run it through the QEMU + buildroot setup on net-next (SHA1:
bf73478b539b) and no regression was seen.

Thanks for preparing this patch :-)

Reviewed-by: Lukasz Majewski <lukma@denx.de>
Tested-by: Lukasz Majewski <lukma@denx.de>


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

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[PATCH net-next 2/2] net: hsr: Remove interlink_sequence_nr.

[Sebastian Andrzej Siewior]

From: Eric Dumazet <edumazet@google.com>

Remove interlink_sequence_nr which is unused.

[ bigeasy: split out from Eric's patch ].

Signed-off-by: Eric Dumazet <edumazet@google.com>
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
 net/hsr/hsr_device.c | 1 -
 net/hsr/hsr_main.h   | 1 -
 2 files changed, 2 deletions(-)

diff --git a/net/hsr/hsr_device.c b/net/hsr/hsr_device.c
index a06e790042e2e..10393836992df 100644
--- a/net/hsr/hsr_device.c
+++ b/net/hsr/hsr_device.c
@@ -625,7 +625,6 @@ int hsr_dev_finalize(struct net_device *hsr_dev, struct net_device *slave[2],
 	/* Overflow soon to find bugs easier: */
 	hsr->sequence_nr = HSR_SEQNR_START;
 	hsr->sup_sequence_nr = HSR_SUP_SEQNR_START;
-	hsr->interlink_sequence_nr = HSR_SEQNR_START;
 
 	timer_setup(&hsr->announce_timer, hsr_announce, 0);
 	timer_setup(&hsr->prune_timer, hsr_prune_nodes, 0);
diff --git a/net/hsr/hsr_main.h b/net/hsr/hsr_main.h
index ab1f8d35d9dcf..fcfeb79bb0401 100644
--- a/net/hsr/hsr_main.h
+++ b/net/hsr/hsr_main.h
@@ -203,7 +203,6 @@ struct hsr_priv {
 	struct timer_list	prune_proxy_timer;
 	int announce_count;
 	u16 sequence_nr;
-	u16 interlink_sequence_nr; /* Interlink port seq_nr */
 	u16 sup_sequence_nr;	/* For HSRv1 separate seq_nr for supervision */
 	enum hsr_version prot_version;	/* Indicate if HSRv0, HSRv1 or PRPv1 */
 	spinlock_t seqnr_lock;	/* locking for sequence_nr */
-- 
2.45.2

Re: [PATCH net-next 2/2] net: hsr: Remove interlink_sequence_nr.

[Lukasz Majewski]

[-- Attachment #1: Type: text/plain, Size: 2280 bytes --]

Hi Sebastian Andrzej,

> From: Eric Dumazet <edumazet@google.com>
> 
> Remove interlink_sequence_nr which is unused.
> 
> [ bigeasy: split out from Eric's patch ].
> 
> Signed-off-by: Eric Dumazet <edumazet@google.com>
> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
> ---
>  net/hsr/hsr_device.c | 1 -
>  net/hsr/hsr_main.h   | 1 -
>  2 files changed, 2 deletions(-)
> 
> diff --git a/net/hsr/hsr_device.c b/net/hsr/hsr_device.c
> index a06e790042e2e..10393836992df 100644
> --- a/net/hsr/hsr_device.c
> +++ b/net/hsr/hsr_device.c
> @@ -625,7 +625,6 @@ int hsr_dev_finalize(struct net_device *hsr_dev,
> struct net_device *slave[2], /* Overflow soon to find bugs easier: */
>  	hsr->sequence_nr = HSR_SEQNR_START;
>  	hsr->sup_sequence_nr = HSR_SUP_SEQNR_START;
> -	hsr->interlink_sequence_nr = HSR_SEQNR_START;
>  
>  	timer_setup(&hsr->announce_timer, hsr_announce, 0);
>  	timer_setup(&hsr->prune_timer, hsr_prune_nodes, 0);
> diff --git a/net/hsr/hsr_main.h b/net/hsr/hsr_main.h
> index ab1f8d35d9dcf..fcfeb79bb0401 100644
> --- a/net/hsr/hsr_main.h
> +++ b/net/hsr/hsr_main.h
> @@ -203,7 +203,6 @@ struct hsr_priv {
>  	struct timer_list	prune_proxy_timer;
>  	int announce_count;
>  	u16 sequence_nr;
> -	u16 interlink_sequence_nr; /* Interlink port seq_nr */

I think that this was an attempt to exactly follow standard (point
5.2.2.2 HSR-SAN RedBox for attachment to a single-thread LAN) which
states that proxy node table shall keep for each interlink the sequence
number [*]. Instead in code the sequence number for a new frame which
comes from interlink port is assigned int:

hsr_get_node() -> file line 271 

and then this starting sequence numer is used for this proxy node table
node.

Hence it shall be safe to remove it.

>  	u16 sup_sequence_nr;	/* For HSRv1 separate seq_nr for
> supervision */ enum hsr_version prot_version;	/* Indicate if
> HSRv0, HSRv1 or PRPv1 */ spinlock_t seqnr_lock;	/* locking for
> sequence_nr */




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

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH net 0/2] net: hsr: Use the seqnr lock for frames received via interlink port.

[Jakub Kicinski]

On Fri,  6 Sep 2024 15:25:30 +0200 Sebastian Andrzej Siewior wrote:
> I hope the two patches in a series targeting different trees is okay.

Not really. Out of curiosity did you expect them to be applied
immediately but separately; or that we would stash half of the
series somewhere until the trees converge?

> Otherwise I will resend.

The fix doesn't look super urgent and with a repost it won't have
time to get into tomorrow's PR with fixes. So I just pushed them
both into net-next.

Re: [PATCH net 0/2] net: hsr: Use the seqnr lock for frames received via interlink port.

[Sebastian Andrzej Siewior]

On 2024-09-11 15:53:24 [-0700], Jakub Kicinski wrote:
> On Fri,  6 Sep 2024 15:25:30 +0200 Sebastian Andrzej Siewior wrote:
> > I hope the two patches in a series targeting different trees is okay.
> 
> Not really. Out of curiosity did you expect them to be applied
> immediately but separately; or that we would stash half of the
> series somewhere until the trees converge?

1/2 should not clash with 2/2. So one could go to net and the other to
net-next. But now that I know, I won't do it again.

> > Otherwise I will resend.
> 
> The fix doesn't look super urgent and with a repost it won't have
> time to get into tomorrow's PR with fixes. So I just pushed them
> both into net-next.

I just noticed that you applied
   b3c9e65eb2272 ("net: hsr: remove seqnr_lock")

to net. Patch 1/2 should replace that one and clashes with this one now.
I tried to explain that removing the lock and making it atomic can break
things again.
Should I send a revert of b3c9e65eb2272 to net?

Sebastian

Re: [PATCH net 0/2] net: hsr: Use the seqnr lock for frames received via interlink port.

[Jakub Kicinski]

On Thu, 12 Sep 2024 08:51:55 +0200 Sebastian Andrzej Siewior wrote:
> > The fix doesn't look super urgent and with a repost it won't have
> > time to get into tomorrow's PR with fixes. So I just pushed them
> > both into net-next.  
> 
> I just noticed that you applied

Yeah, the plural "you", but still my bad for not putting two
and two together :S

>    b3c9e65eb2272 ("net: hsr: remove seqnr_lock")
> 
> to net. Patch 1/2 should replace that one and clashes with this one now.
> I tried to explain that removing the lock and making it atomic can break
> things again.
> Should I send a revert of b3c9e65eb2272 to net?

I have a potentially very stupid plan to squash the revert into 
the cross merge..

Re: [PATCH net 0/2] net: hsr: Use the seqnr lock for frames received via interlink port.

[Sebastian Andrzej Siewior]

On 2024-09-12 17:14:13 [-0700], Jakub Kicinski wrote:
> On Thu, 12 Sep 2024 08:51:55 +0200 Sebastian Andrzej Siewior wrote:
> > > The fix doesn't look super urgent and with a repost it won't have
> > > time to get into tomorrow's PR with fixes. So I just pushed them
> > > both into net-next.  
> > 
> > I just noticed that you applied
> 
> Yeah, the plural "you", but still my bad for not putting two
> and two together :S

Oh I'm sorry, I didn't pay attention ;)

> >    b3c9e65eb2272 ("net: hsr: remove seqnr_lock")
> > 
> > to net. Patch 1/2 should replace that one and clashes with this one now.
> > I tried to explain that removing the lock and making it atomic can break
> > things again.
> > Should I send a revert of b3c9e65eb2272 to net?
> 
> I have a potentially very stupid plan to squash the revert into 
> the cross merge..

Whatever works best for you. I will probably send the revert+patch to
Greg for stable once he asks for it.

Sebastian

Re: [PATCH net 0/2] net: hsr: Use the seqnr lock for frames received via interlink port.

[patchwork-bot+netdevbpf]

Hello:

This series was applied to netdev/net-next.git (main)
by Jakub Kicinski <kuba@kernel.org>:

On Fri,  6 Sep 2024 15:25:30 +0200 you wrote:
> This is follow-up to the thread at
> 	https://lore.kernel.org/all/20240904133725.1073963-1-edumazet@google.com/
> 
> I hope the two patches in a series targeting different trees is okay.
> Otherwise I will resend.
> 
> Sebastian

Here is the summary with links:
  - [net,1/2] net: hsr: Use the seqnr lock for frames received via interlink port.
    https://git.kernel.org/netdev/net-next/c/430d67bdcb04
  - [net-next,2/2] net: hsr: Remove interlink_sequence_nr.
    https://git.kernel.org/netdev/net-next/c/35e24f28c2e9

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html