| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Fri, 9 Oct 2020 17:57:51 -0700
From: Jakub Kicinski <kuba@...nel.org>
To: Marc Kleine-Budde <mkl@...gutronix.de>
Cc: netdev@...r.kernel.org, davem@...emloft.net,
linux-can@...r.kernel.org, kernel@...gutronix.de,
Oliver Hartkopp <socketcan@...tkopp.net>
Subject: Re: [PATCH 08/17] can: add ISO 15765-2:2016 transport protocol
On Wed, 7 Oct 2020 23:31:50 +0200 Marc Kleine-Budde wrote:
> From: Oliver Hartkopp <socketcan@...tkopp.net>
>
> CAN Transport Protocols offer support for segmented Point-to-Point
> communication between CAN nodes via two defined CAN Identifiers.
> As CAN frames can only transport a small amount of data bytes
> (max. 8 bytes for 'classic' CAN and max. 64 bytes for CAN FD) this
> segmentation is needed to transport longer PDUs as needed e.g. for
> vehicle diagnosis (UDS, ISO 14229) or IP-over-CAN traffic.
> This protocol driver implements data transfers according to
> ISO 15765-2:2016 for 'classic' CAN and CAN FD frame types.
A few random things jump out here at a quick scan. Most of them are
not important enough to have to be addressed, but please follow up on
the 'default y' thing ASAP.
> +/*
> + * Remark on CAN_ISOTP_DEFAULT_RECV_* values:
> + *
> + * We can strongly assume, that the Linux Kernel implementation of
> + * CAN_ISOTP is capable to run with BS=0, STmin=0 and WFTmax=0.
> + * But as we like to be able to behave as a commonly available ECU,
> + * these default settings can be changed via sockopts.
> + * For that reason the STmin value is intentionally _not_ checked for
> + * consistency and copied directly into the flow control (FC) frame.
> + *
spurious empty comment line
> + */
> +
> +#endif /* !_UAPI_CAN_ISOTP_H */
> diff --git a/net/can/Kconfig b/net/can/Kconfig
> index 25436a715db3..021fe03a8ed6 100644
> --- a/net/can/Kconfig
> +++ b/net/can/Kconfig
> @@ -55,6 +55,19 @@ config CAN_GW
>
> source "net/can/j1939/Kconfig"
>
> +config CAN_ISOTP
> + tristate "ISO 15765-2:2016 CAN transport protocol"
> + default y
default should not be y unless there is a very good reason.
I don't see such reason here. This is new functionality, users
can enable it if they need it.
> + help
> + CAN Transport Protocols offer support for segmented Point-to-Point
> + communication between CAN nodes via two defined CAN Identifiers.
> + As CAN frames can only transport a small amount of data bytes
> + (max. 8 bytes for 'classic' CAN and max. 64 bytes for CAN FD) this
> + segmentation is needed to transport longer PDUs as needed e.g. for
> + vehicle diagnosis (UDS, ISO 14229) or IP-over-CAN traffic.
> + This protocol driver implements data transfers according to
> + ISO 15765-2:2016 for 'classic' CAN and CAN FD frame types.
> +
> source "drivers/net/can/Kconfig"
>
> endif
> +#define CAN_ISOTP_VERSION "20200928"
We've been removing such version strings throughout the drivers.
Kernel version should be sufficient for in-tree modules.
> +static enum hrtimer_restart isotp_tx_timer_handler(struct hrtimer *hrtimer)
> +{
> + struct isotp_sock *so = container_of(hrtimer, struct isotp_sock,
> + txtimer);
> + struct sock *sk = &so->sk;
> + struct sk_buff *skb;
> + struct net_device *dev;
> + struct canfd_frame *cf;
> + enum hrtimer_restart restart = HRTIMER_NORESTART;
> + int can_send_ret;
> + int ae = (so->opt.flags & CAN_ISOTP_EXTEND_ADDR) ? 1 : 0;
> +
> + switch (so->tx.state) {
> + case ISOTP_WAIT_FC:
> + case ISOTP_WAIT_FIRST_FC:
> +
> + /* we did not get any flow control frame in time */
> +
> + /* report 'communication error on send' */
> + sk->sk_err = ECOMM;
> + if (!sock_flag(sk, SOCK_DEAD))
> + sk->sk_error_report(sk);
> +
> + /* reset tx state */
> + so->tx.state = ISOTP_IDLE;
> + wake_up_interruptible(&so->wait);
> + break;
> +
> + case ISOTP_SENDING:
> +
> + /* push out the next segmented pdu */
> + dev = dev_get_by_index(sock_net(sk), so->ifindex);
> + if (!dev)
> + break;
> +
> +isotp_tx_burst:
> + skb = alloc_skb(so->ll.mtu + sizeof(struct can_skb_priv),
> + gfp_any());
This is always in a timer context, so no need for gfp_any(), right?
> + if (!skb) {
> + dev_put(dev);
> + break;
> + }
> +
> + can_skb_reserve(skb);
> + can_skb_prv(skb)->ifindex = dev->ifindex;
> + can_skb_prv(skb)->skbcnt = 0;
> +
> + cf = (struct canfd_frame *)skb->data;
> + skb_put(skb, so->ll.mtu);
> +
> + /* create consecutive frame */
> + isotp_fill_dataframe(cf, so, ae, 0);
> +
> + /* place consecutive frame N_PCI in appropriate index */
> + cf->data[ae] = N_PCI_CF | so->tx.sn++;
> + so->tx.sn %= 16;
> + so->tx.bs++;
> +
> + if (so->ll.mtu == CANFD_MTU)
> + cf->flags = so->ll.tx_flags;
> +
> + skb->dev = dev;
> + can_skb_set_owner(skb, sk);
> +
> + can_send_ret = can_send(skb, 1);
> + if (can_send_ret)
> + printk_once(KERN_NOTICE "can-isotp: %s: can_send_ret %d\n",
> + __func__, can_send_ret);
pr_notice_once()
> +
> + if (so->tx.idx >= so->tx.len) {
> + /* we are done */
> + so->tx.state = ISOTP_IDLE;
> + dev_put(dev);
> + wake_up_interruptible(&so->wait);
> + break;
> + }
> +
> + if (so->txfc.bs && so->tx.bs >= so->txfc.bs) {
> + /* stop and wait for FC */
> + so->tx.state = ISOTP_WAIT_FC;
> + dev_put(dev);
> + hrtimer_set_expires(&so->txtimer,
> + ktime_add(ktime_get(),
> + ktime_set(1, 0)));
> + restart = HRTIMER_RESTART;
> + break;
> + }
> +
> + /* no gap between data frames needed => use burst mode */
> + if (!so->tx_gap)
> + goto isotp_tx_burst;
> +
> + /* start timer to send next data frame with correct delay */
> + dev_put(dev);
> + hrtimer_set_expires(&so->txtimer,
> + ktime_add(ktime_get(), so->tx_gap));
> + restart = HRTIMER_RESTART;
> + break;
> +
> + default:
> + WARN_ON_ONCE(1);
> + }
> +
> + return restart;
> +}
Powered by blists - more mailing lists