* [PATCH net] can: j1939: j1939_xtp_rx_dat_one(): cancel session if receive TP.DT with error length @ 2021-09-30 3:33 Zhang Changzhong 2021-09-30 7:42 ` Kurt Van Dijck ` (2 more replies) 0 siblings, 3 replies; 14+ messages in thread From: Zhang Changzhong @ 2021-09-30 3:33 UTC (permalink / raw) To: Robin van der Gracht, Oleksij Rempel, kernel, Oliver Hartkopp, Marc Kleine-Budde, David S. Miller, Jakub Kicinski, Kurt Van Dijck, Maxime Jayat Cc: Zhang Changzhong, linux-can, netdev, linux-kernel According to SAE-J1939-21, the data length of TP.DT must be 8 bytes, so cancel session when receive unexpected TP.DT message. Fixes: 9d71dd0c7009 ("can: add support of SAE J1939 protocol") Signed-off-by: Zhang Changzhong <zhangchangzhong@huawei.com> --- net/can/j1939/transport.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/net/can/j1939/transport.c b/net/can/j1939/transport.c index bb5c4b8..eedaeaf 100644 --- a/net/can/j1939/transport.c +++ b/net/can/j1939/transport.c @@ -1789,6 +1789,7 @@ static void j1939_xtp_rx_dpo(struct j1939_priv *priv, struct sk_buff *skb, static void j1939_xtp_rx_dat_one(struct j1939_session *session, struct sk_buff *skb) { + enum j1939_xtp_abort abort = J1939_XTP_ABORT_FAULT; struct j1939_priv *priv = session->priv; struct j1939_sk_buff_cb *skcb, *se_skcb; struct sk_buff *se_skb = NULL; @@ -1803,9 +1804,11 @@ static void j1939_xtp_rx_dat_one(struct j1939_session *session, skcb = j1939_skb_to_cb(skb); dat = skb->data; - if (skb->len <= 1) + if (skb->len != 8) { /* makes no sense */ + abort = J1939_XTP_ABORT_UNEXPECTED_DATA; goto out_session_cancel; + } switch (session->last_cmd) { case 0xff: @@ -1904,7 +1907,7 @@ static void j1939_xtp_rx_dat_one(struct j1939_session *session, out_session_cancel: kfree_skb(se_skb); j1939_session_timers_cancel(session); - j1939_session_cancel(session, J1939_XTP_ABORT_FAULT); + j1939_session_cancel(session, abort); j1939_session_put(session); } -- 2.9.5 ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH net] can: j1939: j1939_xtp_rx_dat_one(): cancel session if receive TP.DT with error length 2021-09-30 3:33 [PATCH net] can: j1939: j1939_xtp_rx_dat_one(): cancel session if receive TP.DT with error length Zhang Changzhong @ 2021-09-30 7:42 ` Kurt Van Dijck 2021-10-08 9:22 ` Zhang Changzhong 2021-10-12 11:09 ` Oleksij Rempel 2021-10-17 10:32 ` Marc Kleine-Budde 2 siblings, 1 reply; 14+ messages in thread From: Kurt Van Dijck @ 2021-09-30 7:42 UTC (permalink / raw) To: Zhang Changzhong Cc: Robin van der Gracht, Oleksij Rempel, kernel, Oliver Hartkopp, Marc Kleine-Budde, David S. Miller, Jakub Kicinski, Maxime Jayat, linux-can, netdev, linux-kernel On Thu, 30 Sep 2021 11:33:20 +0800, Zhang Changzhong wrote: > According to SAE-J1939-21, the data length of TP.DT must be 8 bytes, so > cancel session when receive unexpected TP.DT message. SAE-j1939-21 indeed says that all TP.DT must be 8 bytes. However, the last TP.DT may contain up to 6 stuff bytes, which have no meaning. If I remember well, they are even not 'reserved'. > > Fixes: 9d71dd0c7009 ("can: add support of SAE J1939 protocol") > Signed-off-by: Zhang Changzhong <zhangchangzhong@huawei.com> > --- > net/can/j1939/transport.c | 7 +++++-- > 1 file changed, 5 insertions(+), 2 deletions(-) > > diff --git a/net/can/j1939/transport.c b/net/can/j1939/transport.c > index bb5c4b8..eedaeaf 100644 > --- a/net/can/j1939/transport.c > +++ b/net/can/j1939/transport.c > @@ -1789,6 +1789,7 @@ static void j1939_xtp_rx_dpo(struct j1939_priv *priv, struct sk_buff *skb, > static void j1939_xtp_rx_dat_one(struct j1939_session *session, > struct sk_buff *skb) > { > + enum j1939_xtp_abort abort = J1939_XTP_ABORT_FAULT; > struct j1939_priv *priv = session->priv; > struct j1939_sk_buff_cb *skcb, *se_skcb; > struct sk_buff *se_skb = NULL; > @@ -1803,9 +1804,11 @@ static void j1939_xtp_rx_dat_one(struct j1939_session *session, > > skcb = j1939_skb_to_cb(skb); > dat = skb->data; > - if (skb->len <= 1) > + if (skb->len != 8) { > /* makes no sense */ > + abort = J1939_XTP_ABORT_UNEXPECTED_DATA; > goto out_session_cancel; I think this is a situation of "be strict on what you send, be tolerant on what you receive". Did you find a technical reason to abort a session because the last frame didn't bring overhead that you don't use? Kind regards, Kurt > + } > > switch (session->last_cmd) { > case 0xff: > @@ -1904,7 +1907,7 @@ static void j1939_xtp_rx_dat_one(struct j1939_session *session, > out_session_cancel: > kfree_skb(se_skb); > j1939_session_timers_cancel(session); > - j1939_session_cancel(session, J1939_XTP_ABORT_FAULT); > + j1939_session_cancel(session, abort); > j1939_session_put(session); > } > > -- > 2.9.5 > ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH net] can: j1939: j1939_xtp_rx_dat_one(): cancel session if receive TP.DT with error length 2021-09-30 7:42 ` Kurt Van Dijck @ 2021-10-08 9:22 ` Zhang Changzhong 2021-10-08 11:00 ` Oleksij Rempel 0 siblings, 1 reply; 14+ messages in thread From: Zhang Changzhong @ 2021-10-08 9:22 UTC (permalink / raw) To: Robin van der Gracht, Oleksij Rempel, kernel, Oliver Hartkopp, Marc Kleine-Budde, David S. Miller, Jakub Kicinski, Maxime Jayat, linux-can, netdev, linux-kernel Hi Kurt, Sorry for the late reply. On 2021/9/30 15:42, Kurt Van Dijck wrote: > On Thu, 30 Sep 2021 11:33:20 +0800, Zhang Changzhong wrote: >> According to SAE-J1939-21, the data length of TP.DT must be 8 bytes, so >> cancel session when receive unexpected TP.DT message. > > SAE-j1939-21 indeed says that all TP.DT must be 8 bytes. > However, the last TP.DT may contain up to 6 stuff bytes, which have no meaning. > If I remember well, they are even not 'reserved'. Agree, these bytes are meaningless for last TP.DT. > >> >> Fixes: 9d71dd0c7009 ("can: add support of SAE J1939 protocol") >> Signed-off-by: Zhang Changzhong <zhangchangzhong@huawei.com> >> --- >> net/can/j1939/transport.c | 7 +++++-- >> 1 file changed, 5 insertions(+), 2 deletions(-) >> >> diff --git a/net/can/j1939/transport.c b/net/can/j1939/transport.c >> index bb5c4b8..eedaeaf 100644 >> --- a/net/can/j1939/transport.c >> +++ b/net/can/j1939/transport.c >> @@ -1789,6 +1789,7 @@ static void j1939_xtp_rx_dpo(struct j1939_priv *priv, struct sk_buff *skb, >> static void j1939_xtp_rx_dat_one(struct j1939_session *session, >> struct sk_buff *skb) >> { >> + enum j1939_xtp_abort abort = J1939_XTP_ABORT_FAULT; >> struct j1939_priv *priv = session->priv; >> struct j1939_sk_buff_cb *skcb, *se_skcb; >> struct sk_buff *se_skb = NULL; >> @@ -1803,9 +1804,11 @@ static void j1939_xtp_rx_dat_one(struct j1939_session *session, >> >> skcb = j1939_skb_to_cb(skb); >> dat = skb->data; >> - if (skb->len <= 1) >> + if (skb->len != 8) { >> /* makes no sense */ >> + abort = J1939_XTP_ABORT_UNEXPECTED_DATA; >> goto out_session_cancel; > > I think this is a situation of > "be strict on what you send, be tolerant on what you receive". > > Did you find a technical reason to abort a session because the last frame didn't > bring overhead that you don't use? No technical reason. The only reason is that SAE-J1939-82 requires responder to abort session if any TP.DT less than 8 bytes (section A.3.4, Row 7). Best regards, Changzhong > > Kind regards, > Kurt >> + } >> >> switch (session->last_cmd) { >> case 0xff: >> @@ -1904,7 +1907,7 @@ static void j1939_xtp_rx_dat_one(struct j1939_session *session, >> out_session_cancel: >> kfree_skb(se_skb); >> j1939_session_timers_cancel(session); >> - j1939_session_cancel(session, J1939_XTP_ABORT_FAULT); >> + j1939_session_cancel(session, abort); >> j1939_session_put(session); >> } >> >> -- >> 2.9.5 >> > . > ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH net] can: j1939: j1939_xtp_rx_dat_one(): cancel session if receive TP.DT with error length 2021-10-08 9:22 ` Zhang Changzhong @ 2021-10-08 11:00 ` Oleksij Rempel 2021-10-08 17:09 ` Kurt Van Dijck 2021-10-09 8:43 ` Zhang Changzhong 0 siblings, 2 replies; 14+ messages in thread From: Oleksij Rempel @ 2021-10-08 11:00 UTC (permalink / raw) To: Zhang Changzhong Cc: Robin van der Gracht, Oleksij Rempel, kernel, Oliver Hartkopp, Marc Kleine-Budde, David S. Miller, Jakub Kicinski, Maxime Jayat, linux-can, netdev, linux-kernel On Fri, Oct 08, 2021 at 05:22:12PM +0800, Zhang Changzhong wrote: > Hi Kurt, > Sorry for the late reply. > > On 2021/9/30 15:42, Kurt Van Dijck wrote: > > On Thu, 30 Sep 2021 11:33:20 +0800, Zhang Changzhong wrote: > >> According to SAE-J1939-21, the data length of TP.DT must be 8 bytes, so > >> cancel session when receive unexpected TP.DT message. > > > > SAE-j1939-21 indeed says that all TP.DT must be 8 bytes. > > However, the last TP.DT may contain up to 6 stuff bytes, which have no meaning. > > If I remember well, they are even not 'reserved'. > > Agree, these bytes are meaningless for last TP.DT. > > > > >> > >> Fixes: 9d71dd0c7009 ("can: add support of SAE J1939 protocol") > >> Signed-off-by: Zhang Changzhong <zhangchangzhong@huawei.com> > >> --- > >> net/can/j1939/transport.c | 7 +++++-- > >> 1 file changed, 5 insertions(+), 2 deletions(-) > >> > >> diff --git a/net/can/j1939/transport.c b/net/can/j1939/transport.c > >> index bb5c4b8..eedaeaf 100644 > >> --- a/net/can/j1939/transport.c > >> +++ b/net/can/j1939/transport.c > >> @@ -1789,6 +1789,7 @@ static void j1939_xtp_rx_dpo(struct j1939_priv *priv, struct sk_buff *skb, > >> static void j1939_xtp_rx_dat_one(struct j1939_session *session, > >> struct sk_buff *skb) > >> { > >> + enum j1939_xtp_abort abort = J1939_XTP_ABORT_FAULT; > >> struct j1939_priv *priv = session->priv; > >> struct j1939_sk_buff_cb *skcb, *se_skcb; > >> struct sk_buff *se_skb = NULL; > >> @@ -1803,9 +1804,11 @@ static void j1939_xtp_rx_dat_one(struct j1939_session *session, > >> > >> skcb = j1939_skb_to_cb(skb); > >> dat = skb->data; > >> - if (skb->len <= 1) > >> + if (skb->len != 8) { > >> /* makes no sense */ > >> + abort = J1939_XTP_ABORT_UNEXPECTED_DATA; > >> goto out_session_cancel; > > > > I think this is a situation of > > "be strict on what you send, be tolerant on what you receive". > > > > Did you find a technical reason to abort a session because the last frame didn't > > bring overhead that you don't use? > > No technical reason. The only reason is that SAE-J1939-82 requires responder > to abort session if any TP.DT less than 8 bytes (section A.3.4, Row 7). Do you mean: "BAM Transport: Ensure DUT discards BAM transport when TP.DT data packets are not correct size" ... "Verify DUT discards the BAM transport if any TP.DT data packet has less than 8 bytes"? Regards, Oleksij -- Pengutronix e.K. | | Steuerwalder Str. 21 | http://www.pengutronix.de/ | 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 | Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 | ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH net] can: j1939: j1939_xtp_rx_dat_one(): cancel session if receive TP.DT with error length 2021-10-08 11:00 ` Oleksij Rempel @ 2021-10-08 17:09 ` Kurt Van Dijck 2021-10-09 9:12 ` Zhang Changzhong 2021-10-09 8:43 ` Zhang Changzhong 1 sibling, 1 reply; 14+ messages in thread From: Kurt Van Dijck @ 2021-10-08 17:09 UTC (permalink / raw) To: Oleksij Rempel Cc: Zhang Changzhong, Robin van der Gracht, Oleksij Rempel, kernel, Oliver Hartkopp, Marc Kleine-Budde, David S. Miller, Jakub Kicinski, Maxime Jayat, linux-can, netdev, linux-kernel On Fri, 08 Oct 2021 13:00:07 +0200, Oleksij Rempel wrote: > On Fri, Oct 08, 2021 at 05:22:12PM +0800, Zhang Changzhong wrote: > > Hi Kurt, > > Sorry for the late reply. > > > > On 2021/9/30 15:42, Kurt Van Dijck wrote: > > > On Thu, 30 Sep 2021 11:33:20 +0800, Zhang Changzhong wrote: > > >> According to SAE-J1939-21, the data length of TP.DT must be 8 bytes, so > > >> cancel session when receive unexpected TP.DT message. > > > > > > SAE-j1939-21 indeed says that all TP.DT must be 8 bytes. > > > However, the last TP.DT may contain up to 6 stuff bytes, which have no meaning. > > > If I remember well, they are even not 'reserved'. > > > > Agree, these bytes are meaningless for last TP.DT. > > > > > > > >> > > >> Fixes: 9d71dd0c7009 ("can: add support of SAE J1939 protocol") > > >> Signed-off-by: Zhang Changzhong <zhangchangzhong@huawei.com> > > >> --- > > >> net/can/j1939/transport.c | 7 +++++-- > > >> 1 file changed, 5 insertions(+), 2 deletions(-) > > >> > > >> diff --git a/net/can/j1939/transport.c b/net/can/j1939/transport.c > > >> index bb5c4b8..eedaeaf 100644 > > >> --- a/net/can/j1939/transport.c > > >> +++ b/net/can/j1939/transport.c > > >> @@ -1789,6 +1789,7 @@ static void j1939_xtp_rx_dpo(struct j1939_priv *priv, struct sk_buff *skb, > > >> static void j1939_xtp_rx_dat_one(struct j1939_session *session, > > >> struct sk_buff *skb) > > >> { > > >> + enum j1939_xtp_abort abort = J1939_XTP_ABORT_FAULT; > > >> struct j1939_priv *priv = session->priv; > > >> struct j1939_sk_buff_cb *skcb, *se_skcb; > > >> struct sk_buff *se_skb = NULL; > > >> @@ -1803,9 +1804,11 @@ static void j1939_xtp_rx_dat_one(struct j1939_session *session, > > >> > > >> skcb = j1939_skb_to_cb(skb); > > >> dat = skb->data; > > >> - if (skb->len <= 1) > > >> + if (skb->len != 8) { > > >> /* makes no sense */ > > >> + abort = J1939_XTP_ABORT_UNEXPECTED_DATA; > > >> goto out_session_cancel; > > > > > > I think this is a situation of > > > "be strict on what you send, be tolerant on what you receive". > > > > > > Did you find a technical reason to abort a session because the last frame didn't > > > bring overhead that you don't use? > > > > No technical reason. The only reason is that SAE-J1939-82 requires responder > > to abort session if any TP.DT less than 8 bytes (section A.3.4, Row 7). IMHO, this is some kind of laziness to make the exception for the last TP.DT. I attended an ISOBUS certification (back in 2013) where the transmitting node effectively stripped the trailing bytes, and this 'deviation' was not even noticed. This change applies to the receiving side. Would a sender that leaves the trailing bytes want you to discard the session bacause of this? So the spirit of the SAE-J1939-82 is, in this case, different from the strict literal interpretation. > > Do you mean: "BAM Transport: Ensure DUT discards BAM transport when > TP.DT data packets are not correct size" ... "Verify DUT discards the > BAM transport if any TP.DT data packet has less than 8 bytes"? Kind regards, Kurt ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH net] can: j1939: j1939_xtp_rx_dat_one(): cancel session if receive TP.DT with error length 2021-10-08 17:09 ` Kurt Van Dijck @ 2021-10-09 9:12 ` Zhang Changzhong 0 siblings, 0 replies; 14+ messages in thread From: Zhang Changzhong @ 2021-10-09 9:12 UTC (permalink / raw) To: Kurt Van Dijck Cc: Oleksij Rempel, Robin van der Gracht, Oleksij Rempel, kernel, Oliver Hartkopp, Marc Kleine-Budde, David S. Miller, Jakub Kicinski, Maxime Jayat, linux-can, netdev, linux-kernel On 2021/10/9 1:09, Kurt Van Dijck wrote: > On Fri, 08 Oct 2021 13:00:07 +0200, Oleksij Rempel wrote: >> On Fri, Oct 08, 2021 at 05:22:12PM +0800, Zhang Changzhong wrote: >>> Hi Kurt, >>> Sorry for the late reply. >>> >>> On 2021/9/30 15:42, Kurt Van Dijck wrote: >>>> On Thu, 30 Sep 2021 11:33:20 +0800, Zhang Changzhong wrote: >>>>> According to SAE-J1939-21, the data length of TP.DT must be 8 bytes, so >>>>> cancel session when receive unexpected TP.DT message. >>>> >>>> SAE-j1939-21 indeed says that all TP.DT must be 8 bytes. >>>> However, the last TP.DT may contain up to 6 stuff bytes, which have no meaning. >>>> If I remember well, they are even not 'reserved'. >>> >>> Agree, these bytes are meaningless for last TP.DT. >>> >>>> >>>>> >>>>> Fixes: 9d71dd0c7009 ("can: add support of SAE J1939 protocol") >>>>> Signed-off-by: Zhang Changzhong <zhangchangzhong@huawei.com> >>>>> --- >>>>> net/can/j1939/transport.c | 7 +++++-- >>>>> 1 file changed, 5 insertions(+), 2 deletions(-) >>>>> >>>>> diff --git a/net/can/j1939/transport.c b/net/can/j1939/transport.c >>>>> index bb5c4b8..eedaeaf 100644 >>>>> --- a/net/can/j1939/transport.c >>>>> +++ b/net/can/j1939/transport.c >>>>> @@ -1789,6 +1789,7 @@ static void j1939_xtp_rx_dpo(struct j1939_priv *priv, struct sk_buff *skb, >>>>> static void j1939_xtp_rx_dat_one(struct j1939_session *session, >>>>> struct sk_buff *skb) >>>>> { >>>>> + enum j1939_xtp_abort abort = J1939_XTP_ABORT_FAULT; >>>>> struct j1939_priv *priv = session->priv; >>>>> struct j1939_sk_buff_cb *skcb, *se_skcb; >>>>> struct sk_buff *se_skb = NULL; >>>>> @@ -1803,9 +1804,11 @@ static void j1939_xtp_rx_dat_one(struct j1939_session *session, >>>>> >>>>> skcb = j1939_skb_to_cb(skb); >>>>> dat = skb->data; >>>>> - if (skb->len <= 1) >>>>> + if (skb->len != 8) { >>>>> /* makes no sense */ >>>>> + abort = J1939_XTP_ABORT_UNEXPECTED_DATA; >>>>> goto out_session_cancel; >>>> >>>> I think this is a situation of >>>> "be strict on what you send, be tolerant on what you receive". >>>> >>>> Did you find a technical reason to abort a session because the last frame didn't >>>> bring overhead that you don't use? >>> >>> No technical reason. The only reason is that SAE-J1939-82 requires responder >>> to abort session if any TP.DT less than 8 bytes (section A.3.4, Row 7). > > IMHO, this is some kind of laziness to make the exception for the last TP.DT. > > I attended an ISOBUS certification (back in 2013) where the transmitting > node effectively stripped the trailing bytes, and this 'deviation' was > not even noticed. I found that SAE-J1939-82 contains the following test: "BAM Transport: Ensure extra (unused) bytes of last Data Transfer data packet is/are filled-in correctly. (DUT as Originator)" ... "Verify last TP.DT data packet for a BAM transport is sent with an 8 byte data field and the unused bytes of this packet are filled with FF" (section A.3.3, Row 8). So the J1939 compliance test can detect this kind of 'deviation', perhaps ISOBUS certification does not do this check? > > This change applies to the receiving side. Would a sender that > leaves the trailing bytes want you to discard the session bacause of this? > So the spirit of the SAE-J1939-82 is, in this case, different from > the strict literal interpretation. Such packets should not be sent if the sender complies with SAE-J1939-82, but if the transmitting node you mentioned above exist on the network, this patch will casue their sessions to be aborted. From this point of view, I think it is reasonable to drop this patch. Regards, Changzhong > >> >> Do you mean: "BAM Transport: Ensure DUT discards BAM transport when >> TP.DT data packets are not correct size" ... "Verify DUT discards the >> BAM transport if any TP.DT data packet has less than 8 bytes"? > > Kind regards, > Kurt > . > ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH net] can: j1939: j1939_xtp_rx_dat_one(): cancel session if receive TP.DT with error length 2021-10-08 11:00 ` Oleksij Rempel 2021-10-08 17:09 ` Kurt Van Dijck @ 2021-10-09 8:43 ` Zhang Changzhong 2021-10-11 6:35 ` Oleksij Rempel 1 sibling, 1 reply; 14+ messages in thread From: Zhang Changzhong @ 2021-10-09 8:43 UTC (permalink / raw) To: Oleksij Rempel Cc: Robin van der Gracht, Oleksij Rempel, kernel, Oliver Hartkopp, Marc Kleine-Budde, David S. Miller, Jakub Kicinski, Maxime Jayat, linux-can, netdev, linux-kernel On 2021/10/8 19:00, Oleksij Rempel wrote: > On Fri, Oct 08, 2021 at 05:22:12PM +0800, Zhang Changzhong wrote: >> Hi Kurt, >> Sorry for the late reply. >> >> On 2021/9/30 15:42, Kurt Van Dijck wrote: >>> On Thu, 30 Sep 2021 11:33:20 +0800, Zhang Changzhong wrote: >>>> According to SAE-J1939-21, the data length of TP.DT must be 8 bytes, so >>>> cancel session when receive unexpected TP.DT message. >>> >>> SAE-j1939-21 indeed says that all TP.DT must be 8 bytes. >>> However, the last TP.DT may contain up to 6 stuff bytes, which have no meaning. >>> If I remember well, they are even not 'reserved'. >> >> Agree, these bytes are meaningless for last TP.DT. >> >>> >>>> >>>> Fixes: 9d71dd0c7009 ("can: add support of SAE J1939 protocol") >>>> Signed-off-by: Zhang Changzhong <zhangchangzhong@huawei.com> >>>> --- >>>> net/can/j1939/transport.c | 7 +++++-- >>>> 1 file changed, 5 insertions(+), 2 deletions(-) >>>> >>>> diff --git a/net/can/j1939/transport.c b/net/can/j1939/transport.c >>>> index bb5c4b8..eedaeaf 100644 >>>> --- a/net/can/j1939/transport.c >>>> +++ b/net/can/j1939/transport.c >>>> @@ -1789,6 +1789,7 @@ static void j1939_xtp_rx_dpo(struct j1939_priv *priv, struct sk_buff *skb, >>>> static void j1939_xtp_rx_dat_one(struct j1939_session *session, >>>> struct sk_buff *skb) >>>> { >>>> + enum j1939_xtp_abort abort = J1939_XTP_ABORT_FAULT; >>>> struct j1939_priv *priv = session->priv; >>>> struct j1939_sk_buff_cb *skcb, *se_skcb; >>>> struct sk_buff *se_skb = NULL; >>>> @@ -1803,9 +1804,11 @@ static void j1939_xtp_rx_dat_one(struct j1939_session *session, >>>> >>>> skcb = j1939_skb_to_cb(skb); >>>> dat = skb->data; >>>> - if (skb->len <= 1) >>>> + if (skb->len != 8) { >>>> /* makes no sense */ >>>> + abort = J1939_XTP_ABORT_UNEXPECTED_DATA; >>>> goto out_session_cancel; >>> >>> I think this is a situation of >>> "be strict on what you send, be tolerant on what you receive". >>> >>> Did you find a technical reason to abort a session because the last frame didn't >>> bring overhead that you don't use? >> >> No technical reason. The only reason is that SAE-J1939-82 requires responder >> to abort session if any TP.DT less than 8 bytes (section A.3.4, Row 7). > > Do you mean: "BAM Transport: Ensure DUT discards BAM transport when > TP.DT data packets are not correct size" ... "Verify DUT discards the > BAM transport if any TP.DT data packet has less than 8 bytes"? Yes. Regards, Changzhong > > Regards, > Oleksij > ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH net] can: j1939: j1939_xtp_rx_dat_one(): cancel session if receive TP.DT with error length 2021-10-09 8:43 ` Zhang Changzhong @ 2021-10-11 6:35 ` Oleksij Rempel 2021-10-11 7:18 ` Kurt Van Dijck 2021-10-11 10:40 ` Zhang Changzhong 0 siblings, 2 replies; 14+ messages in thread From: Oleksij Rempel @ 2021-10-11 6:35 UTC (permalink / raw) To: Zhang Changzhong Cc: Maxime Jayat, Robin van der Gracht, linux-kernel, Oleksij Rempel, netdev, Marc Kleine-Budde, kernel, Oliver Hartkopp, Jakub Kicinski, linux-can, David S. Miller On Sat, Oct 09, 2021 at 04:43:56PM +0800, Zhang Changzhong wrote: > On 2021/10/8 19:00, Oleksij Rempel wrote: > > On Fri, Oct 08, 2021 at 05:22:12PM +0800, Zhang Changzhong wrote: > >> Hi Kurt, > >> Sorry for the late reply. > >> > >> On 2021/9/30 15:42, Kurt Van Dijck wrote: > >>> On Thu, 30 Sep 2021 11:33:20 +0800, Zhang Changzhong wrote: > >>>> According to SAE-J1939-21, the data length of TP.DT must be 8 bytes, so > >>>> cancel session when receive unexpected TP.DT message. > >>> > >>> SAE-j1939-21 indeed says that all TP.DT must be 8 bytes. > >>> However, the last TP.DT may contain up to 6 stuff bytes, which have no meaning. > >>> If I remember well, they are even not 'reserved'. > >> > >> Agree, these bytes are meaningless for last TP.DT. > >> > >>> > >>>> > >>>> Fixes: 9d71dd0c7009 ("can: add support of SAE J1939 protocol") > >>>> Signed-off-by: Zhang Changzhong <zhangchangzhong@huawei.com> > >>>> --- > >>>> net/can/j1939/transport.c | 7 +++++-- > >>>> 1 file changed, 5 insertions(+), 2 deletions(-) > >>>> > >>>> diff --git a/net/can/j1939/transport.c b/net/can/j1939/transport.c > >>>> index bb5c4b8..eedaeaf 100644 > >>>> --- a/net/can/j1939/transport.c > >>>> +++ b/net/can/j1939/transport.c > >>>> @@ -1789,6 +1789,7 @@ static void j1939_xtp_rx_dpo(struct j1939_priv *priv, struct sk_buff *skb, > >>>> static void j1939_xtp_rx_dat_one(struct j1939_session *session, > >>>> struct sk_buff *skb) > >>>> { > >>>> + enum j1939_xtp_abort abort = J1939_XTP_ABORT_FAULT; > >>>> struct j1939_priv *priv = session->priv; > >>>> struct j1939_sk_buff_cb *skcb, *se_skcb; > >>>> struct sk_buff *se_skb = NULL; > >>>> @@ -1803,9 +1804,11 @@ static void j1939_xtp_rx_dat_one(struct j1939_session *session, > >>>> > >>>> skcb = j1939_skb_to_cb(skb); > >>>> dat = skb->data; > >>>> - if (skb->len <= 1) > >>>> + if (skb->len != 8) { > >>>> /* makes no sense */ > >>>> + abort = J1939_XTP_ABORT_UNEXPECTED_DATA; > >>>> goto out_session_cancel; > >>> > >>> I think this is a situation of > >>> "be strict on what you send, be tolerant on what you receive". > >>> > >>> Did you find a technical reason to abort a session because the last frame didn't > >>> bring overhead that you don't use? > >> > >> No technical reason. The only reason is that SAE-J1939-82 requires responder > >> to abort session if any TP.DT less than 8 bytes (section A.3.4, Row 7). > > > > Do you mean: "BAM Transport: Ensure DUT discards BAM transport when > > TP.DT data packets are not correct size" ... "Verify DUT discards the > > BAM transport if any TP.DT data packet has less than 8 bytes"? > > Yes. OK, then I have some problems to understand this part: - 5.10.2.4 Connection Closure The “connection abort” message is not allowed to be used by responders in the case of a global destination (i.e. BAM). My assumption would be: In case of broadcast transfer, multiple MCU are receivers. If one of MCU was not able to get complete TP.DT, it should not abort BAM for all. So, "DUT discards the BAM transport" sounds for me as local action. Complete TP would be dropped locally. Regards Oleksij -- Pengutronix e.K. | | Steuerwalder Str. 21 | http://www.pengutronix.de/ | 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 | Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 | ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH net] can: j1939: j1939_xtp_rx_dat_one(): cancel session if receive TP.DT with error length 2021-10-11 6:35 ` Oleksij Rempel @ 2021-10-11 7:18 ` Kurt Van Dijck 2021-10-11 10:40 ` Zhang Changzhong 1 sibling, 0 replies; 14+ messages in thread From: Kurt Van Dijck @ 2021-10-11 7:18 UTC (permalink / raw) To: Oleksij Rempel Cc: Zhang Changzhong, Maxime Jayat, Robin van der Gracht, linux-kernel, Oleksij Rempel, netdev, Marc Kleine-Budde, kernel, Oliver Hartkopp, Jakub Kicinski, linux-can, David S. Miller On Mon, 11 Oct 2021 08:35:07 +0200, Oleksij Rempel wrote: > On Sat, Oct 09, 2021 at 04:43:56PM +0800, Zhang Changzhong wrote: > > On 2021/10/8 19:00, Oleksij Rempel wrote: > > > On Fri, Oct 08, 2021 at 05:22:12PM +0800, Zhang Changzhong wrote: > > >> Hi Kurt, > > >> Sorry for the late reply. > > >> > > >> On 2021/9/30 15:42, Kurt Van Dijck wrote: > > >>> On Thu, 30 Sep 2021 11:33:20 +0800, Zhang Changzhong wrote: > > >>>> According to SAE-J1939-21, the data length of TP.DT must be 8 bytes, so > > >>>> cancel session when receive unexpected TP.DT message. > > >>> > > >>> SAE-j1939-21 indeed says that all TP.DT must be 8 bytes. > > >>> However, the last TP.DT may contain up to 6 stuff bytes, which have no meaning. > > >>> If I remember well, they are even not 'reserved'. > > >> > > >> Agree, these bytes are meaningless for last TP.DT. > > >> > > >>> > > >>>> [...] > > >>> > > >>> I think this is a situation of > > >>> "be strict on what you send, be tolerant on what you receive". > > >>> > > >>> Did you find a technical reason to abort a session because the last frame didn't > > >>> bring overhead that you don't use? > > >> > > >> No technical reason. The only reason is that SAE-J1939-82 requires responder > > >> to abort session if any TP.DT less than 8 bytes (section A.3.4, Row 7). > > > > > > Do you mean: "BAM Transport: Ensure DUT discards BAM transport when > > > TP.DT data packets are not correct size" ... "Verify DUT discards the > > > BAM transport if any TP.DT data packet has less than 8 bytes"? > > > > Yes. > > OK, then I have some problems to understand this part: > - 5.10.2.4 Connection Closure > The “connection abort” message is not allowed to be used by responders in the > case of a global destination (i.e. BAM). > > My assumption would be: In case of broadcast transfer, multiple MCU are > receivers. If one of MCU was not able to get complete TP.DT, it should > not abort BAM for all. There is indeed no action defined to abort at BAM. > > So, "DUT discards the BAM transport" sounds for me as local action. > Complete TP would be dropped locally. exact. Kurt ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH net] can: j1939: j1939_xtp_rx_dat_one(): cancel session if receive TP.DT with error length 2021-10-11 6:35 ` Oleksij Rempel 2021-10-11 7:18 ` Kurt Van Dijck @ 2021-10-11 10:40 ` Zhang Changzhong 2021-10-12 10:21 ` Oleksij Rempel 1 sibling, 1 reply; 14+ messages in thread From: Zhang Changzhong @ 2021-10-11 10:40 UTC (permalink / raw) To: Oleksij Rempel Cc: Maxime Jayat, Robin van der Gracht, linux-kernel, Oleksij Rempel, netdev, Marc Kleine-Budde, kernel, Oliver Hartkopp, Jakub Kicinski, linux-can, David S. Miller On 2021/10/11 14:35, Oleksij Rempel wrote: > On Sat, Oct 09, 2021 at 04:43:56PM +0800, Zhang Changzhong wrote: >> On 2021/10/8 19:00, Oleksij Rempel wrote: >>> On Fri, Oct 08, 2021 at 05:22:12PM +0800, Zhang Changzhong wrote: >>>> Hi Kurt, >>>> Sorry for the late reply. >>>> >>>> On 2021/9/30 15:42, Kurt Van Dijck wrote: >>>>> On Thu, 30 Sep 2021 11:33:20 +0800, Zhang Changzhong wrote: >>>>>> According to SAE-J1939-21, the data length of TP.DT must be 8 bytes, so >>>>>> cancel session when receive unexpected TP.DT message. >>>>> >>>>> SAE-j1939-21 indeed says that all TP.DT must be 8 bytes. >>>>> However, the last TP.DT may contain up to 6 stuff bytes, which have no meaning. >>>>> If I remember well, they are even not 'reserved'. >>>> >>>> Agree, these bytes are meaningless for last TP.DT. >>>> >>>>> >>>>>> >>>>>> Fixes: 9d71dd0c7009 ("can: add support of SAE J1939 protocol") >>>>>> Signed-off-by: Zhang Changzhong <zhangchangzhong@huawei.com> >>>>>> --- >>>>>> net/can/j1939/transport.c | 7 +++++-- >>>>>> 1 file changed, 5 insertions(+), 2 deletions(-) >>>>>> >>>>>> diff --git a/net/can/j1939/transport.c b/net/can/j1939/transport.c >>>>>> index bb5c4b8..eedaeaf 100644 >>>>>> --- a/net/can/j1939/transport.c >>>>>> +++ b/net/can/j1939/transport.c >>>>>> @@ -1789,6 +1789,7 @@ static void j1939_xtp_rx_dpo(struct j1939_priv *priv, struct sk_buff *skb, >>>>>> static void j1939_xtp_rx_dat_one(struct j1939_session *session, >>>>>> struct sk_buff *skb) >>>>>> { >>>>>> + enum j1939_xtp_abort abort = J1939_XTP_ABORT_FAULT; >>>>>> struct j1939_priv *priv = session->priv; >>>>>> struct j1939_sk_buff_cb *skcb, *se_skcb; >>>>>> struct sk_buff *se_skb = NULL; >>>>>> @@ -1803,9 +1804,11 @@ static void j1939_xtp_rx_dat_one(struct j1939_session *session, >>>>>> >>>>>> skcb = j1939_skb_to_cb(skb); >>>>>> dat = skb->data; >>>>>> - if (skb->len <= 1) >>>>>> + if (skb->len != 8) { >>>>>> /* makes no sense */ >>>>>> + abort = J1939_XTP_ABORT_UNEXPECTED_DATA; >>>>>> goto out_session_cancel; >>>>> >>>>> I think this is a situation of >>>>> "be strict on what you send, be tolerant on what you receive". >>>>> >>>>> Did you find a technical reason to abort a session because the last frame didn't >>>>> bring overhead that you don't use? >>>> >>>> No technical reason. The only reason is that SAE-J1939-82 requires responder >>>> to abort session if any TP.DT less than 8 bytes (section A.3.4, Row 7). >>> >>> Do you mean: "BAM Transport: Ensure DUT discards BAM transport when >>> TP.DT data packets are not correct size" ... "Verify DUT discards the >>> BAM transport if any TP.DT data packet has less than 8 bytes"? >> >> Yes. > > OK, then I have some problems to understand this part: > - 5.10.2.4 Connection Closure > The “connection abort” message is not allowed to be used by responders in the > case of a global destination (i.e. BAM). > > My assumption would be: In case of broadcast transfer, multiple MCU are > receivers. If one of MCU was not able to get complete TP.DT, it should > not abort BAM for all. > > So, "DUT discards the BAM transport" sounds for me as local action. > Complete TP would be dropped locally. Yeah, you are right. With this patch receivers drop BAM transport locally because j1939_session_cancel() only send abort message in RTS/CTS transport. For RTS/CTS transport, SAE-J1939-82 also has similar requirements: "RTS/CTS Transport: Data field size of Transport Data packets for RTS/CTS (DUT as Responder)"..."Verify DUT behavior, e.g., sends a TP.CM_CTS to have packets resent or sends a TP.Conn_Abort, when it receives TP.DT data packets with less than 8 bytes" (section A.3.6, Row 18) Regards, Changzhong . ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH net] can: j1939: j1939_xtp_rx_dat_one(): cancel session if receive TP.DT with error length 2021-10-11 10:40 ` Zhang Changzhong @ 2021-10-12 10:21 ` Oleksij Rempel 2021-10-12 10:41 ` Kurt Van Dijck 0 siblings, 1 reply; 14+ messages in thread From: Oleksij Rempel @ 2021-10-12 10:21 UTC (permalink / raw) To: Zhang Changzhong Cc: Oliver Hartkopp, Maxime Jayat, Robin van der Gracht, linux-kernel, Oleksij Rempel, Marc Kleine-Budde, kernel, netdev, Jakub Kicinski, linux-can, David S. Miller On Mon, Oct 11, 2021 at 06:40:15PM +0800, Zhang Changzhong wrote: > On 2021/10/11 14:35, Oleksij Rempel wrote: > > On Sat, Oct 09, 2021 at 04:43:56PM +0800, Zhang Changzhong wrote: > >> On 2021/10/8 19:00, Oleksij Rempel wrote: > >>> On Fri, Oct 08, 2021 at 05:22:12PM +0800, Zhang Changzhong wrote: > >>>> Hi Kurt, > >>>> Sorry for the late reply. > >>>> > >>>> On 2021/9/30 15:42, Kurt Van Dijck wrote: > >>>>> On Thu, 30 Sep 2021 11:33:20 +0800, Zhang Changzhong wrote: > >>>>>> According to SAE-J1939-21, the data length of TP.DT must be 8 bytes, so > >>>>>> cancel session when receive unexpected TP.DT message. > >>>>> > >>>>> SAE-j1939-21 indeed says that all TP.DT must be 8 bytes. > >>>>> However, the last TP.DT may contain up to 6 stuff bytes, which have no meaning. > >>>>> If I remember well, they are even not 'reserved'. > >>>> > >>>> Agree, these bytes are meaningless for last TP.DT. > >>>> > >>>>> > >>>>>> > >>>>>> Fixes: 9d71dd0c7009 ("can: add support of SAE J1939 protocol") > >>>>>> Signed-off-by: Zhang Changzhong <zhangchangzhong@huawei.com> > >>>>>> --- > >>>>>> net/can/j1939/transport.c | 7 +++++-- > >>>>>> 1 file changed, 5 insertions(+), 2 deletions(-) > >>>>>> > >>>>>> diff --git a/net/can/j1939/transport.c b/net/can/j1939/transport.c > >>>>>> index bb5c4b8..eedaeaf 100644 > >>>>>> --- a/net/can/j1939/transport.c > >>>>>> +++ b/net/can/j1939/transport.c > >>>>>> @@ -1789,6 +1789,7 @@ static void j1939_xtp_rx_dpo(struct j1939_priv *priv, struct sk_buff *skb, > >>>>>> static void j1939_xtp_rx_dat_one(struct j1939_session *session, > >>>>>> struct sk_buff *skb) > >>>>>> { > >>>>>> + enum j1939_xtp_abort abort = J1939_XTP_ABORT_FAULT; > >>>>>> struct j1939_priv *priv = session->priv; > >>>>>> struct j1939_sk_buff_cb *skcb, *se_skcb; > >>>>>> struct sk_buff *se_skb = NULL; > >>>>>> @@ -1803,9 +1804,11 @@ static void j1939_xtp_rx_dat_one(struct j1939_session *session, > >>>>>> > >>>>>> skcb = j1939_skb_to_cb(skb); > >>>>>> dat = skb->data; > >>>>>> - if (skb->len <= 1) > >>>>>> + if (skb->len != 8) { > >>>>>> /* makes no sense */ > >>>>>> + abort = J1939_XTP_ABORT_UNEXPECTED_DATA; > >>>>>> goto out_session_cancel; > >>>>> > >>>>> I think this is a situation of > >>>>> "be strict on what you send, be tolerant on what you receive". > >>>>> > >>>>> Did you find a technical reason to abort a session because the last frame didn't > >>>>> bring overhead that you don't use? > >>>> > >>>> No technical reason. The only reason is that SAE-J1939-82 requires responder > >>>> to abort session if any TP.DT less than 8 bytes (section A.3.4, Row 7). > >>> > >>> Do you mean: "BAM Transport: Ensure DUT discards BAM transport when > >>> TP.DT data packets are not correct size" ... "Verify DUT discards the > >>> BAM transport if any TP.DT data packet has less than 8 bytes"? > >> > >> Yes. > > > > OK, then I have some problems to understand this part: > > - 5.10.2.4 Connection Closure > > The “connection abort” message is not allowed to be used by responders in the > > case of a global destination (i.e. BAM). > > > > My assumption would be: In case of broadcast transfer, multiple MCU are > > receivers. If one of MCU was not able to get complete TP.DT, it should > > not abort BAM for all. > > > > So, "DUT discards the BAM transport" sounds for me as local action. > > Complete TP would be dropped locally. > > Yeah, you are right. With this patch receivers drop BAM transport locally > because j1939_session_cancel() only send abort message in RTS/CTS transport. > > For RTS/CTS transport, SAE-J1939-82 also has similar requirements: > "RTS/CTS Transport: Data field size of Transport Data packets for RTS/CTS > (DUT as Responder)"..."Verify DUT behavior, e.g., sends a TP.CM_CTS to have > packets resent or sends a TP.Conn_Abort, when it receives TP.DT data packets > with less than 8 bytes" (section A.3.6, Row 18) You are right. Sounds plausible. If we find some device in the field which will need a workaround to support less than 8byte, then we will need to add some UAPI to configure it. By default we should follow the spec. @Kurt, do you have anything against it? Regards, Oleksij -- Pengutronix e.K. | | Steuerwalder Str. 21 | http://www.pengutronix.de/ | 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 | Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 | ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH net] can: j1939: j1939_xtp_rx_dat_one(): cancel session if receive TP.DT with error length 2021-10-12 10:21 ` Oleksij Rempel @ 2021-10-12 10:41 ` Kurt Van Dijck 0 siblings, 0 replies; 14+ messages in thread From: Kurt Van Dijck @ 2021-10-12 10:41 UTC (permalink / raw) To: Oleksij Rempel Cc: Zhang Changzhong, Oliver Hartkopp, Maxime Jayat, Robin van der Gracht, linux-kernel, Oleksij Rempel, Marc Kleine-Budde, kernel, netdev, Jakub Kicinski, linux-can, David S. Miller On Tue, 12 Oct 2021 12:21:31 +0200, Oleksij Rempel wrote: > On Mon, Oct 11, 2021 at 06:40:15PM +0800, Zhang Changzhong wrote: > > On 2021/10/11 14:35, Oleksij Rempel wrote: > > > On Sat, Oct 09, 2021 at 04:43:56PM +0800, Zhang Changzhong wrote: > > >> On 2021/10/8 19:00, Oleksij Rempel wrote: > > >>> On Fri, Oct 08, 2021 at 05:22:12PM +0800, Zhang Changzhong wrote: > > >>>> Hi Kurt, > > >>>> Sorry for the late reply. > > >>>> > > >>>> On 2021/9/30 15:42, Kurt Van Dijck wrote: > > >>>>> On Thu, 30 Sep 2021 11:33:20 +0800, Zhang Changzhong wrote: > > >>>>>> According to SAE-J1939-21, the data length of TP.DT must be 8 bytes, so > > >>>>>> cancel session when receive unexpected TP.DT message. > > >>>>> > > >>>>> SAE-j1939-21 indeed says that all TP.DT must be 8 bytes. > > >>>>> However, the last TP.DT may contain up to 6 stuff bytes, which have no meaning. > > >>>>> If I remember well, they are even not 'reserved'. > > >>>> > > >>>> Agree, these bytes are meaningless for last TP.DT. > > >>>> > > >>>>> > > >>>>>> > > >>>>>> Fixes: 9d71dd0c7009 ("can: add support of SAE J1939 protocol") > > >>>>>> Signed-off-by: Zhang Changzhong <zhangchangzhong@huawei.com> > > >>>>>> --- > > >>>>>> net/can/j1939/transport.c | 7 +++++-- > > >>>>>> 1 file changed, 5 insertions(+), 2 deletions(-) > > >>>>>> > > >>>>>> diff --git a/net/can/j1939/transport.c b/net/can/j1939/transport.c > > >>>>>> index bb5c4b8..eedaeaf 100644 > > >>>>>> --- a/net/can/j1939/transport.c > > >>>>>> +++ b/net/can/j1939/transport.c > > >>>>>> @@ -1789,6 +1789,7 @@ static void j1939_xtp_rx_dpo(struct j1939_priv *priv, struct sk_buff *skb, > > >>>>>> static void j1939_xtp_rx_dat_one(struct j1939_session *session, > > >>>>>> struct sk_buff *skb) > > >>>>>> { > > >>>>>> + enum j1939_xtp_abort abort = J1939_XTP_ABORT_FAULT; > > >>>>>> struct j1939_priv *priv = session->priv; > > >>>>>> struct j1939_sk_buff_cb *skcb, *se_skcb; > > >>>>>> struct sk_buff *se_skb = NULL; > > >>>>>> @@ -1803,9 +1804,11 @@ static void j1939_xtp_rx_dat_one(struct j1939_session *session, > > >>>>>> > > >>>>>> skcb = j1939_skb_to_cb(skb); > > >>>>>> dat = skb->data; > > >>>>>> - if (skb->len <= 1) > > >>>>>> + if (skb->len != 8) { > > >>>>>> /* makes no sense */ > > >>>>>> + abort = J1939_XTP_ABORT_UNEXPECTED_DATA; > > >>>>>> goto out_session_cancel; > > >>>>> > > >>>>> I think this is a situation of > > >>>>> "be strict on what you send, be tolerant on what you receive". > > >>>>> > > >>>>> Did you find a technical reason to abort a session because the last frame didn't > > >>>>> bring overhead that you don't use? > > >>>> > > >>>> No technical reason. The only reason is that SAE-J1939-82 requires responder > > >>>> to abort session if any TP.DT less than 8 bytes (section A.3.4, Row 7). > > >>> > > >>> Do you mean: "BAM Transport: Ensure DUT discards BAM transport when > > >>> TP.DT data packets are not correct size" ... "Verify DUT discards the > > >>> BAM transport if any TP.DT data packet has less than 8 bytes"? > > >> > > >> Yes. > > > > > > OK, then I have some problems to understand this part: > > > - 5.10.2.4 Connection Closure > > > The “connection abort” message is not allowed to be used by responders in the > > > case of a global destination (i.e. BAM). > > > > > > My assumption would be: In case of broadcast transfer, multiple MCU are > > > receivers. If one of MCU was not able to get complete TP.DT, it should > > > not abort BAM for all. > > > > > > So, "DUT discards the BAM transport" sounds for me as local action. > > > Complete TP would be dropped locally. > > > > Yeah, you are right. With this patch receivers drop BAM transport locally > > because j1939_session_cancel() only send abort message in RTS/CTS transport. > > > > For RTS/CTS transport, SAE-J1939-82 also has similar requirements: > > "RTS/CTS Transport: Data field size of Transport Data packets for RTS/CTS > > (DUT as Responder)"..."Verify DUT behavior, e.g., sends a TP.CM_CTS to have > > packets resent or sends a TP.Conn_Abort, when it receives TP.DT data packets > > with less than 8 bytes" (section A.3.6, Row 18) > > You are right. Sounds plausible. If we find some device in the field > which will need a workaround to support less than 8byte, then we will > need to add some UAPI to configure it. By default we should follow the > spec. @Kurt, do you have anything against it? Zhang Changzhong suggested that this is part of compliance testing nowadays. That obsoletes all technical arguments, and you have no choice than to adapt. Kurt ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH net] can: j1939: j1939_xtp_rx_dat_one(): cancel session if receive TP.DT with error length 2021-09-30 3:33 [PATCH net] can: j1939: j1939_xtp_rx_dat_one(): cancel session if receive TP.DT with error length Zhang Changzhong 2021-09-30 7:42 ` Kurt Van Dijck @ 2021-10-12 11:09 ` Oleksij Rempel 2021-10-17 10:32 ` Marc Kleine-Budde 2 siblings, 0 replies; 14+ messages in thread From: Oleksij Rempel @ 2021-10-12 11:09 UTC (permalink / raw) To: Zhang Changzhong Cc: Robin van der Gracht, Oleksij Rempel, kernel, Oliver Hartkopp, Marc Kleine-Budde, David S. Miller, Jakub Kicinski, Kurt Van Dijck, Maxime Jayat, netdev, linux-kernel, linux-can On Thu, Sep 30, 2021 at 11:33:20AM +0800, Zhang Changzhong wrote: > According to SAE-J1939-21, the data length of TP.DT must be 8 bytes, so > cancel session when receive unexpected TP.DT message. > > Fixes: 9d71dd0c7009 ("can: add support of SAE J1939 protocol") > Signed-off-by: Zhang Changzhong <zhangchangzhong@huawei.com> Acked-by: Oleksij Rempel <o.rempel@pengutronix.de> Thank you! > --- > net/can/j1939/transport.c | 7 +++++-- > 1 file changed, 5 insertions(+), 2 deletions(-) > > diff --git a/net/can/j1939/transport.c b/net/can/j1939/transport.c > index bb5c4b8..eedaeaf 100644 > --- a/net/can/j1939/transport.c > +++ b/net/can/j1939/transport.c > @@ -1789,6 +1789,7 @@ static void j1939_xtp_rx_dpo(struct j1939_priv *priv, struct sk_buff *skb, > static void j1939_xtp_rx_dat_one(struct j1939_session *session, > struct sk_buff *skb) > { > + enum j1939_xtp_abort abort = J1939_XTP_ABORT_FAULT; > struct j1939_priv *priv = session->priv; > struct j1939_sk_buff_cb *skcb, *se_skcb; > struct sk_buff *se_skb = NULL; > @@ -1803,9 +1804,11 @@ static void j1939_xtp_rx_dat_one(struct j1939_session *session, > > skcb = j1939_skb_to_cb(skb); > dat = skb->data; > - if (skb->len <= 1) > + if (skb->len != 8) { > /* makes no sense */ > + abort = J1939_XTP_ABORT_UNEXPECTED_DATA; > goto out_session_cancel; > + } > > switch (session->last_cmd) { > case 0xff: > @@ -1904,7 +1907,7 @@ static void j1939_xtp_rx_dat_one(struct j1939_session *session, > out_session_cancel: > kfree_skb(se_skb); > j1939_session_timers_cancel(session); > - j1939_session_cancel(session, J1939_XTP_ABORT_FAULT); > + j1939_session_cancel(session, abort); > j1939_session_put(session); > } > > -- > 2.9.5 > > > -- Pengutronix e.K. | | Steuerwalder Str. 21 | http://www.pengutronix.de/ | 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 | Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 | ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH net] can: j1939: j1939_xtp_rx_dat_one(): cancel session if receive TP.DT with error length 2021-09-30 3:33 [PATCH net] can: j1939: j1939_xtp_rx_dat_one(): cancel session if receive TP.DT with error length Zhang Changzhong 2021-09-30 7:42 ` Kurt Van Dijck 2021-10-12 11:09 ` Oleksij Rempel @ 2021-10-17 10:32 ` Marc Kleine-Budde 2 siblings, 0 replies; 14+ messages in thread From: Marc Kleine-Budde @ 2021-10-17 10:32 UTC (permalink / raw) To: Zhang Changzhong Cc: Robin van der Gracht, Oleksij Rempel, kernel, Oliver Hartkopp, David S. Miller, Jakub Kicinski, Kurt Van Dijck, Maxime Jayat, linux-can, netdev, linux-kernel [-- Attachment #1: Type: text/plain, Size: 656 bytes --] On 30.09.2021 11:33:20, Zhang Changzhong wrote: > According to SAE-J1939-21, the data length of TP.DT must be 8 bytes, so > cancel session when receive unexpected TP.DT message. > > Fixes: 9d71dd0c7009 ("can: add support of SAE J1939 protocol") > Signed-off-by: Zhang Changzhong <zhangchangzhong@huawei.com> Applied to linux-can/testing, added stable on Cc. Thanks, Marc -- Pengutronix e.K. | Marc Kleine-Budde | Embedded Linux | https://www.pengutronix.de | Vertretung West/Dortmund | Phone: +49-231-2826-924 | Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 | [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 488 bytes --] ^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2021-10-17 10:34 UTC | newest] Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-09-30 3:33 [PATCH net] can: j1939: j1939_xtp_rx_dat_one(): cancel session if receive TP.DT with error length Zhang Changzhong 2021-09-30 7:42 ` Kurt Van Dijck 2021-10-08 9:22 ` Zhang Changzhong 2021-10-08 11:00 ` Oleksij Rempel 2021-10-08 17:09 ` Kurt Van Dijck 2021-10-09 9:12 ` Zhang Changzhong 2021-10-09 8:43 ` Zhang Changzhong 2021-10-11 6:35 ` Oleksij Rempel 2021-10-11 7:18 ` Kurt Van Dijck 2021-10-11 10:40 ` Zhang Changzhong 2021-10-12 10:21 ` Oleksij Rempel 2021-10-12 10:41 ` Kurt Van Dijck 2021-10-12 11:09 ` Oleksij Rempel 2021-10-17 10:32 ` Marc Kleine-Budde
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.