| Message ID | 20240906132816.657485-2-bigeasy@linutronix.de (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 430d67bdcb04ee8502c2b10dcbaced4253649189 |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: hsr: Use the seqnr lock for frames received via interlink port. | expand |
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
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?
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
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
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;
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(-)