| Message ID | 20240226152447.3439219-1-lukma@denx.de (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: hsr: Use correct offset for HSR TLV values in supervisory HSR frames | expand |
Mon, Feb 26, 2024 at 04:24:47PM CET, lukma@denx.de wrote: >Current HSR implementation uses following supervisory frame (even for >HSRv1 the HSR tag is not is not present): > >00000000: 01 15 4e 00 01 2d XX YY ZZ 94 77 10 88 fb 00 01 >00000010: 7e 1c 17 06 XX YY ZZ 94 77 10 1e 06 XX YY ZZ 94 >00000020: 77 10 00 00 00 00 00 00 00 00 00 00 00 00 00 00 >00000030: 00 00 00 00 00 00 00 00 00 00 00 00 > >The current code adds extra two bytes (i.e. sizeof(struct hsr_sup_tlv)) >when offset for skb_pull() is calculated. >This is wrong, as both 'struct hsrv1_ethhdr_sp' and 'hsrv0_ethhdr_sp' >already have 'struct hsr_sup_tag' defined in them, so there is no need >for adding extra two bytes. > >This code was working correctly as with no RedBox support, the check for >HSR_TLV_EOT (0x00) was off by two bytes, which were corresponding to >zeroed padded bytes for minimal packet size. > >Fixes: f43200a2c98b ("net: hsr: Provide RedBox support") > No need for this empty line. >Signed-off-by: Lukasz Majewski <lukma@denx.de> Reviewed-by: Jiri Pirko <jiri@nvidia.com>
On Mon, Feb 26, 2024 at 04:24:47PM +0100, Lukasz Majewski wrote: > Current HSR implementation uses following supervisory frame (even for > HSRv1 the HSR tag is not is not present): > > 00000000: 01 15 4e 00 01 2d XX YY ZZ 94 77 10 88 fb 00 01 > 00000010: 7e 1c 17 06 XX YY ZZ 94 77 10 1e 06 XX YY ZZ 94 > 00000020: 77 10 00 00 00 00 00 00 00 00 00 00 00 00 00 00 > 00000030: 00 00 00 00 00 00 00 00 00 00 00 00 > > The current code adds extra two bytes (i.e. sizeof(struct hsr_sup_tlv)) > when offset for skb_pull() is calculated. > This is wrong, as both 'struct hsrv1_ethhdr_sp' and 'hsrv0_ethhdr_sp' > already have 'struct hsr_sup_tag' defined in them, so there is no need > for adding extra two bytes. > > This code was working correctly as with no RedBox support, the check for > HSR_TLV_EOT (0x00) was off by two bytes, which were corresponding to > zeroed padded bytes for minimal packet size. > > Fixes: f43200a2c98b ("net: hsr: Provide RedBox support") Hi Lukasz, The commit cited above does seem to be present in net or net-next. Perhaps the tag should be: Fixes: eafaa88b3eb7 ("net: hsr: Add support for redbox supervision frames") > > Signed-off-by: Lukasz Majewski <lukma@denx.de> ...
Hi Simon, > On Mon, Feb 26, 2024 at 04:24:47PM +0100, Lukasz Majewski wrote: > > Current HSR implementation uses following supervisory frame (even > > for HSRv1 the HSR tag is not is not present): > > > > 00000000: 01 15 4e 00 01 2d XX YY ZZ 94 77 10 88 fb 00 01 > > 00000010: 7e 1c 17 06 XX YY ZZ 94 77 10 1e 06 XX YY ZZ 94 > > 00000020: 77 10 00 00 00 00 00 00 00 00 00 00 00 00 00 00 > > 00000030: 00 00 00 00 00 00 00 00 00 00 00 00 > > > > The current code adds extra two bytes (i.e. sizeof(struct > > hsr_sup_tlv)) when offset for skb_pull() is calculated. > > This is wrong, as both 'struct hsrv1_ethhdr_sp' and > > 'hsrv0_ethhdr_sp' already have 'struct hsr_sup_tag' defined in > > them, so there is no need for adding extra two bytes. > > > > This code was working correctly as with no RedBox support, the > > check for HSR_TLV_EOT (0x00) was off by two bytes, which were > > corresponding to zeroed padded bytes for minimal packet size. > > > > Fixes: f43200a2c98b ("net: hsr: Provide RedBox support") > > Hi Lukasz, > > The commit cited above does seem to be present in net or net-next. > Perhaps the tag should be: > > Fixes: eafaa88b3eb7 ("net: hsr: Add support for redbox supervision > frames") Thanks for spotting it - I must have overlook it when preparing the commit message. > > > > > Signed-off-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_forward.c b/net/hsr/hsr_forward.c index 2afe28712a7a..5d68cb181695 100644 --- a/net/hsr/hsr_forward.c +++ b/net/hsr/hsr_forward.c @@ -83,7 +83,7 @@ static bool is_supervision_frame(struct hsr_priv *hsr, struct sk_buff *skb) return false; /* Get next tlv */ - total_length += sizeof(struct hsr_sup_tlv) + hsr_sup_tag->tlv.HSR_TLV_length; + total_length += hsr_sup_tag->tlv.HSR_TLV_length; if (!pskb_may_pull(skb, total_length)) return false; skb_pull(skb, total_length);
Current HSR implementation uses following supervisory frame (even for HSRv1 the HSR tag is not is not present): 00000000: 01 15 4e 00 01 2d XX YY ZZ 94 77 10 88 fb 00 01 00000010: 7e 1c 17 06 XX YY ZZ 94 77 10 1e 06 XX YY ZZ 94 00000020: 77 10 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00000030: 00 00 00 00 00 00 00 00 00 00 00 00 The current code adds extra two bytes (i.e. sizeof(struct hsr_sup_tlv)) when offset for skb_pull() is calculated. This is wrong, as both 'struct hsrv1_ethhdr_sp' and 'hsrv0_ethhdr_sp' already have 'struct hsr_sup_tag' defined in them, so there is no need for adding extra two bytes. This code was working correctly as with no RedBox support, the check for HSR_TLV_EOT (0x00) was off by two bytes, which were corresponding to zeroed padded bytes for minimal packet size. Fixes: f43200a2c98b ("net: hsr: Provide RedBox support") Signed-off-by: Lukasz Majewski <lukma@denx.de> --- net/hsr/hsr_forward.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)