| 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: Sat, 10 Oct 2020 16:29:06 +0200
From: Oliver Hartkopp <socketcan@...tkopp.net>
To: Jakub Kicinski <kuba@...nel.org>,
Marc Kleine-Budde <mkl@...gutronix.de>
Cc: netdev@...r.kernel.org, davem@...emloft.net,
linux-can@...r.kernel.org, kernel@...gutronix.de
Subject: Re: [PATCH 08/17] can: add ISO 15765-2:2016 transport protocol
On 10.10.20 02:57, Jakub Kicinski wrote:
> 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
>
Oh, yes - at the front and at the end. I will fix that.
>> + */
>> +
>> +#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.
>
Yes. I agree. But there is a good reason for it.
The ISO 15765-2 protocol is used for vehicle diagnosis and is a *very*
common CAN bus use case.
The config item only shows up when CONFIG_CAN is selected and then ISO
15765-2 should be enabled too. I have implemented and maintained the
out-of-tree driver for ~12 years now and the people have real problems
using e.g. Ubuntu with signed kernel modules when they need this protocol.
Therefore the option should default to 'y' to make sure the common
distros (that enable CONFIG_CAN) enable ISO-TP too.
>> + 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.
Yes. Good point.
I will send a separate patch which removes all the VERSION information
from the entire CAN bus subsystem (core, raw, bcm, gw).
>> +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?
>
Some code from the time where hrtimer was only in hard-irq context. Will
fix that too.
>> + 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()
Ok. Will change that at the several occurrences.
>> +
>> + 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;
>> +}
Thanks for the review!
Will send the patches for the net-next tree later this day.
Best regards,
Oliver
Powered by blists - more mailing lists