From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 5A742C433F5 for ; Mon, 11 Oct 2021 06:35:28 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id 4031F604D2 for ; Mon, 11 Oct 2021 06:35:28 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S234220AbhJKGhZ (ORCPT ); Mon, 11 Oct 2021 02:37:25 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:47542 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S234216AbhJKGhY (ORCPT ); Mon, 11 Oct 2021 02:37:24 -0400 Received: from metis.ext.pengutronix.de (metis.ext.pengutronix.de [IPv6:2001:67c:670:201:290:27ff:fe1d:cc33]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 4B02CC061570 for ; Sun, 10 Oct 2021 23:35:25 -0700 (PDT) Received: from ptx.hi.pengutronix.de ([2001:67c:670:100:1d::c0]) by metis.ext.pengutronix.de with esmtps (TLS1.3:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.92) (envelope-from ) id 1mZou1-0002vp-0J; Mon, 11 Oct 2021 08:35:13 +0200 Received: from ore by ptx.hi.pengutronix.de with local (Exim 4.92) (envelope-from ) id 1mZotv-0005WI-NG; Mon, 11 Oct 2021 08:35:07 +0200 Date: Mon, 11 Oct 2021 08:35:07 +0200 From: Oleksij Rempel To: Zhang Changzhong Cc: Maxime Jayat , Robin van der Gracht , linux-kernel@vger.kernel.org, Oleksij Rempel , netdev@vger.kernel.org, Marc Kleine-Budde , kernel@pengutronix.de, Oliver Hartkopp , Jakub Kicinski , linux-can@vger.kernel.org, "David S. Miller" Subject: Re: [PATCH net] can: j1939: j1939_xtp_rx_dat_one(): cancel session if receive TP.DT with error length Message-ID: <20211011063507.GI29653@pengutronix.de> References: <1632972800-45091-1-git-send-email-zhangchangzhong@huawei.com> <20210930074206.GB7502@x1.vandijck-laurijssen.be> <1cab07f2-593a-1d1c-3a29-43ee9df4b29e@huawei.com> <20211008110007.GE29653@pengutronix.de> <556a04ed-c350-7b2b-5bbe-98c03846630b@huawei.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <556a04ed-c350-7b2b-5bbe-98c03846630b@huawei.com> X-Sent-From: Pengutronix Hildesheim X-URL: http://www.pengutronix.de/ X-IRC: #ptxdist @freenode X-Accept-Language: de,en X-Accept-Content-Type: text/plain X-Uptime: 08:19:30 up 235 days, 9:43, 116 users, load average: 0.08, 0.12, 0.17 User-Agent: Mutt/1.10.1 (2018-07-13) X-SA-Exim-Connect-IP: 2001:67c:670:100:1d::c0 X-SA-Exim-Mail-From: ore@pengutronix.de X-SA-Exim-Scanned: No (on metis.ext.pengutronix.de); SAEximRunCond expanded to false X-PTX-Original-Recipient: netdev@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: netdev@vger.kernel.org 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 > >>>> --- > >>>> 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 |