can: isotp: add module parameter for maximum pdu size

RFC PATCH Superseded Oliver Hartkopp 2 participants 7 messages Mar 13, 2023 – Mar 26, 2023

Subsystems (2)

CAN network drivers CAN network layer

lore

Share & tools

lore

Oliver Hartkopp

author maintainer

2023-03-13 17:27 UTC #0 lore raw

RFC PATCH v2 1/1 can: isotp: add module parameter for maximum pdu size

With ISO 15765-2:2016 the PDU size is not limited to 2^12 - 1 (4095) bytes but can be represented as a 32 bit unsigned integer value which allows 2^32 - 1 bytes (~4GB). The use-cases like automotive unified diagnostic services (UDS) and flashing of ECUs still use the small static buffers which are provided at socket creation time.

When a use-case requires to transfer PDUs up to 1025 kByte the maximum PDU size can now be extended by setting the module parameter max_pdu_size. The extended size buffers are only allocated on a per-socket/connection base when needed at run-time.

Link https://github.com/raspberrypi/linux/issues/5371

Signed-off-by Oliver Hartkopp <[email protected]>

---

v2: limit the minimum 'max_pdu_size' to 4095 to maintain the classic behavior

    before ISO 15765-2:2016

net/can/isotp.c | 65 ++++++++++++++++++++++++++++++++++++++++++-------

 1 file changed, 56 insertions(+), 9 deletions(-)

net/can/isotp.c +56 -9

index 9bc344851704..8a5d278c8bf1 100644
@@ -83,14 +83,25 @@ MODULE_ALIAS("can-proto-6");
 			 (CAN_EFF_MASK | CAN_EFF_FLAG | CAN_RTR_FLAG) : \
 			 (CAN_SFF_MASK | CAN_EFF_FLAG | CAN_RTR_FLAG))
 
 /* ISO 15765-2:2016 supports more than 4095 byte per ISO PDU as the FF_DL can
  * take full 32 bit values (4 Gbyte). We would need some good concept to handle
- * this between user space and kernel space. For now increase the static buffer
- * to something about 64 kbyte to be able to test this new functionality.
+ * this between user space and kernel space. For now set the static buffer to
+ * something about 8 kbyte to be able to test this new functionality.
  */
-#define MAX_MSG_LENGTH 66000
+#define DEFAULT_MAX_PDU_SIZE 8300
+
+/* maximum PDU size before ISO 15765-2:2016 extension was 4095 */
+#define MAX_12BIT_PDU_SIZE 4095
+
+/* limit the isotp pdu size from the optional module parameter to 1MByte */
+#define MAX_PDU_SIZE (1025 * 1024U)
+
+static unsigned int max_pdu_size __read_mostly = DEFAULT_MAX_PDU_SIZE;
+module_param(max_pdu_size, uint, 0444);
+MODULE_PARM_DESC(max_pdu_size, "maximum isotp pdu size (default "
+		 __stringify(DEFAULT_MAX_PDU_SIZE) ")");
 
 /* N_PCI type values in bits 7-4 of N_PCI bytes */
 #define N_PCI_SF 0x00	/* single frame */
 #define N_PCI_FF 0x10	/* first frame */
 #define N_PCI_CF 0x20	/* consecutive frame */

@@ -121,17 +132,19 @@ enum {
 	ISOTP_WAIT_DATA,
 	ISOTP_SENDING
 };
 
 struct tpcon {
-	unsigned int idx;
+	u8 *buf;
+	unsigned int buflen;
 	unsigned int len;
+	unsigned int idx;
 	u32 state;
 	u8 bs;
 	u8 sn;
 	u8 ll_dl;
-	u8 buf[MAX_MSG_LENGTH + 1];
+	u8 sbuf[DEFAULT_MAX_PDU_SIZE];
 };
 
 struct isotp_sock {
 	struct sock sk;
 	int bound;

@@ -501,11 +514,21 @@ static int isotp_rcv_ff(struct sock *sk, struct canfd_frame *cf, int ae)
 	off = (so->rx.ll_dl > CAN_MAX_DLEN) ? 1 : 0;
 
 	if (so->rx.len + ae + off + ff_pci_sz < so->rx.ll_dl)
 		return 1;
 
-	if (so->rx.len > MAX_MSG_LENGTH) {
+	/* PDU size > default => try max_pdu_size */
+	if (so->rx.len > so->rx.buflen && so->rx.buflen < max_pdu_size) {
+		u8 *newbuf = kmalloc(max_pdu_size, GFP_ATOMIC);
+
+		if (newbuf) {
+			so->rx.buf = newbuf;
+			so->rx.buflen = max_pdu_size;
+		}
+	}
+
+	if (so->rx.len > so->rx.buflen) {
 		/* send FC frame with overflow status */
 		isotp_send_fc(sk, ae, ISOTP_FC_OVFLW);
 		return 1;
 	}

@@ -805,11 +828,11 @@ static void isotp_create_fframe(struct canfd_frame *cf, struct isotp_sock *so,
 	cf->len = so->tx.ll_dl;
 	if (ae)
 		cf->data[0] = so->opt.ext_address;
 
 	/* create N_PCI bytes with 12/32 bit FF_DL data length */
-	if (so->tx.len > 4095) {
+	if (so->tx.len > MAX_12BIT_PDU_SIZE) {
 		/* use 32 bit FF_DL notation */
 		cf->data[ae] = N_PCI_FF;
 		cf->data[ae + 1] = 0;
 		cf->data[ae + 2] = (u8)(so->tx.len >> 24) & 0xFFU;
 		cf->data[ae + 3] = (u8)(so->tx.len >> 16) & 0xFFU;

@@ -945,11 +968,21 @@ static int isotp_sendmsg(struct socket *sock, struct msghdr *msg, size_t size)
 			goto err_out;
 
 		so->tx.state = ISOTP_SENDING;
 	}
 
-	if (!size || size > MAX_MSG_LENGTH) {
+	/* PDU size > default => try max_pdu_size */
+	if (size > so->tx.buflen && so->tx.buflen < max_pdu_size) {
+		u8 *newbuf = kmalloc(max_pdu_size, GFP_KERNEL);
+
+		if (newbuf) {
+			so->tx.buf = newbuf;
+			so->tx.buflen = max_pdu_size;
+		}
+	}
+
+	if (!size || size > so->tx.buflen) {
 		err = -EINVAL;
 		goto err_out_drop;
 	}
 
 	/* take care of a potential SF_DL ESC offset for TX_DL > 8 */

@@ -1193,10 +1226,16 @@ static int isotp_release(struct socket *sock)
 	hrtimer_cancel(&so->rxtimer);
 
 	so->ifindex = 0;
 	so->bound = 0;
 
+	if (so->rx.buf != so->rx.sbuf)
+		kfree(so->rx.buf);
+
+	if (so->tx.buf != so->tx.sbuf)
+		kfree(so->tx.buf);
+
 	sock_orphan(sk);
 	sock->sk = NULL;
 
 	release_sock(sk);
 	sock_put(sk);

@@ -1589,10 +1628,15 @@ static int isotp_init(struct sock *sk)
 	so->tx.ll_dl = so->ll.tx_dl;
 
 	so->rx.state = ISOTP_IDLE;
 	so->tx.state = ISOTP_IDLE;
 
+	so->rx.buf = so->rx.sbuf;
+	so->tx.buf = so->tx.sbuf;
+	so->rx.buflen = DEFAULT_MAX_PDU_SIZE;
+	so->tx.buflen = DEFAULT_MAX_PDU_SIZE;
+
 	hrtimer_init(&so->rxtimer, CLOCK_MONOTONIC, HRTIMER_MODE_REL_SOFT);
 	so->rxtimer.function = isotp_rx_timer_handler;
 	hrtimer_init(&so->txtimer, CLOCK_MONOTONIC, HRTIMER_MODE_REL_SOFT);
 	so->txtimer.function = isotp_tx_timer_handler;
 	hrtimer_init(&so->txfrtimer, CLOCK_MONOTONIC, HRTIMER_MODE_REL_SOFT);

@@ -1656,11 +1700,14 @@ static struct notifier_block canisotp_notifier = {
 
 static __init int isotp_module_init(void)
 {
 	int err;
 
-	pr_info("can: isotp protocol\n");
+	max_pdu_size = max_t(unsigned int, max_pdu_size, MAX_12BIT_PDU_SIZE);
+	max_pdu_size = min_t(unsigned int, max_pdu_size, MAX_PDU_SIZE);
+
+	pr_info("can: isotp protocol (max_pdu_size %d)\n", max_pdu_size);
 
 	err = can_proto_register(&isotp_can_proto);
 	if (err < 0)
 		pr_err("can: registration of isotp protocol failed %pe\n", ERR_PTR(err));
 	else

Signature · 1 line

2.30.2

Marc Kleine-Budde

maintainer

2023-03-20 16:31 UTC #1 lore raw

Quoted 6 lines · Oliver Hartkopp

With ISO 15765-2:2016 the PDU size is not limited to 2^12 - 1 (4095) bytes but can be represented as a 32 bit unsigned integer value which allows 2^32 - 1 bytes (~4GB). The use-cases like automotive unified diagnostic services (UDS) and flashing of ECUs still use the small static buffers which are provided at socket creation time.

Quoted 7 lines

When a use-case requires to transfer PDUs up to 1025 kByte the maximum PDU size can now be extended by setting the module parameter max_pdu_size. The extended size buffers are only allocated on a per-socket/connection base when needed at run-time.
Link https://github.com/raspberrypi/linux/issues/5371
Signed-off-by Oliver Hartkopp <[email protected]>

See comments inline

Quoted 64 lines

---

v2: limit the minimum 'max_pdu_size' to 4095 to maintain the classic behavior

    before ISO 15765-2:2016

net/can/isotp.c | 65 ++++++++++++++++++++++++++++++++++++++++++-------

 1 file changed, 56 insertions(+), 9 deletions(-)

net/can/isotp.c +18 -5

index 9bc344851704..8a5d278c8bf1 100644
@@ -83,14 +83,25 @@ MODULE_ALIAS("can-proto-6");
 			 (CAN_EFF_MASK | CAN_EFF_FLAG | CAN_RTR_FLAG) : \
 			 (CAN_SFF_MASK | CAN_EFF_FLAG | CAN_RTR_FLAG))
 
 /* ISO 15765-2:2016 supports more than 4095 byte per ISO PDU as the FF_DL can
  * take full 32 bit values (4 Gbyte). We would need some good concept to handle
- * this between user space and kernel space. For now increase the static buffer
- * to something about 64 kbyte to be able to test this new functionality.
+ * this between user space and kernel space. For now set the static buffer to
+ * something about 8 kbyte to be able to test this new functionality.
  */
-#define MAX_MSG_LENGTH 66000
+#define DEFAULT_MAX_PDU_SIZE 8300
+
+/* maximum PDU size before ISO 15765-2:2016 extension was 4095 */
+#define MAX_12BIT_PDU_SIZE 4095
+
+/* limit the isotp pdu size from the optional module parameter to 1MByte */
+#define MAX_PDU_SIZE (1025 * 1024U)
+
+static unsigned int max_pdu_size __read_mostly = DEFAULT_MAX_PDU_SIZE;
+module_param(max_pdu_size, uint, 0444);
+MODULE_PARM_DESC(max_pdu_size, "maximum isotp pdu size (default "
+		 __stringify(DEFAULT_MAX_PDU_SIZE) ")");
 
 /* N_PCI type values in bits 7-4 of N_PCI bytes */
 #define N_PCI_SF 0x00	/* single frame */
 #define N_PCI_FF 0x10	/* first frame */
 #define N_PCI_CF 0x20	/* consecutive frame */

@@ -121,17 +132,19 @@ enum {
 	ISOTP_WAIT_DATA,
 	ISOTP_SENDING
 };
 
 struct tpcon {
-	unsigned int idx;
+	u8 *buf;
+	unsigned int buflen;
 	unsigned int len;
+	unsigned int idx;
 	u32 state;
 	u8 bs;
 	u8 sn;
 	u8 ll_dl;
-	u8 buf[MAX_MSG_LENGTH + 1];
+	u8 sbuf[DEFAULT_MAX_PDU_SIZE];
 };
 
 struct isotp_sock {
 	struct sock sk;
 	int bound;

net/can/isotp.c static int isotp_rcv_ff(struct sock *sk, struct canfd_frame *cf, int ae) · line 519

Quoted 7 lines

net/can/isotp.c +1 -1

@@ -501,11 +514,21 @@ static int isotp_rcv_ff(struct sock *sk, struct canfd_frame *cf, int ae)
 	off = (so->rx.ll_dl > CAN_MAX_DLEN) ? 1 : 0;
 
 	if (so->rx.len + ae + off + ff_pci_sz < so->rx.ll_dl)
 		return 1;
 
-	if (so->rx.len > MAX_MSG_LENGTH) {
+	/* PDU size > default => try max_pdu_size */

What exactly is "so->rx.len"?

net/can/isotp.c static int isotp_rcv_ff(struct sock *sk, struct canfd_frame *cf, int ae) · line 520

Quoted 1 line

net/can/isotp.c +1 -0

+	if (so->rx.len > so->rx.buflen && so->rx.buflen < max_pdu_size) {

                                          ^^^^^^^^^^^^^

Why are you checking so->rx.buflen against max_pdu_size? Should you check rx.len instead?

net/can/isotp.c static int isotp_rcv_ff(struct sock *sk, struct canfd_frame *cf, int ae) · line 521

Quoted 1 line

net/can/isotp.c +1 -0

+		u8 *newbuf = kmalloc(max_pdu_size, GFP_ATOMIC);

In what context is this code executed? Is it really atomic? Why are you allocating the max_pdu_size, not rx.len?

net/can/isotp.c static int isotp_rcv_ff(struct sock *sk, struct canfd_frame *cf, int ae) · line 529

Quoted 8 lines

net/can/isotp.c +8 -0

+
+		if (newbuf) {
+			so->rx.buf = newbuf;
+			so->rx.buflen = max_pdu_size;
+		}
+	}
+
+	if (so->rx.len > so->rx.buflen) {

This patch is also taken if the kmalloc() fails, right?

Quoted 19 lines

net/can/isotp.c +1 -1

 		/* send FC frame with overflow status */
 		isotp_send_fc(sk, ae, ISOTP_FC_OVFLW);
 		return 1;
 	}

@@ -805,11 +828,11 @@ static void isotp_create_fframe(struct canfd_frame *cf, struct isotp_sock *so,
 	cf->len = so->tx.ll_dl;
 	if (ae)
 		cf->data[0] = so->opt.ext_address;
 
 	/* create N_PCI bytes with 12/32 bit FF_DL data length */
-	if (so->tx.len > 4095) {
+	if (so->tx.len > MAX_12BIT_PDU_SIZE) {
 		/* use 32 bit FF_DL notation */
 		cf->data[ae] = N_PCI_FF;
 		cf->data[ae + 1] = 0;
 		cf->data[ae + 2] = (u8)(so->tx.len >> 24) & 0xFFU;
 		cf->data[ae + 3] = (u8)(so->tx.len >> 16) & 0xFFU;

net/can/isotp.c static int isotp_sendmsg(struct socket *sock, struct msghdr *msg, size_t size) · line 975

Quoted 9 lines

net/can/isotp.c +3 -1

@@ -945,11 +968,21 @@ static int isotp_sendmsg(struct socket *sock, struct msghdr *msg, size_t size)
 			goto err_out;
 
 		so->tx.state = ISOTP_SENDING;
 	}
 
-	if (!size || size > MAX_MSG_LENGTH) {
+	/* PDU size > default => try max_pdu_size */
+	if (size > so->tx.buflen && so->tx.buflen < max_pdu_size) {
+		u8 *newbuf = kmalloc(max_pdu_size, GFP_KERNEL);

Same questions as in the RX-path. BTW: for the TX-path you can implement something like in the j1939 protocol. There we don't allocate the full TX buffer anymore, but handle it in chunks. Talk to Oleksij for details.

Quoted 31 lines

net/can/isotp.c +14 -0

+
+		if (newbuf) {
+			so->tx.buf = newbuf;
+			so->tx.buflen = max_pdu_size;
+		}
+	}
+
+	if (!size || size > so->tx.buflen) {
 		err = -EINVAL;
 		goto err_out_drop;
 	}
 
 	/* take care of a potential SF_DL ESC offset for TX_DL > 8 */

@@ -1193,10 +1226,16 @@ static int isotp_release(struct socket *sock)
 	hrtimer_cancel(&so->rxtimer);
 
 	so->ifindex = 0;
 	so->bound = 0;
 
+	if (so->rx.buf != so->rx.sbuf)
+		kfree(so->rx.buf);
+
+	if (so->tx.buf != so->tx.sbuf)
+		kfree(so->tx.buf);
+
 	sock_orphan(sk);
 	sock->sk = NULL;
 
 	release_sock(sk);
 	sock_put(sk);

net/can/isotp.c static int isotp_init(struct sock *sk) · line 1635

Quoted 8 lines

net/can/isotp.c +3 -0

@@ -1589,10 +1628,15 @@ static int isotp_init(struct sock *sk)
 	so->tx.ll_dl = so->ll.tx_dl;
 
 	so->rx.state = ISOTP_IDLE;
 	so->tx.state = ISOTP_IDLE;
 
+	so->rx.buf = so->rx.sbuf;
+	so->tx.buf = so->tx.sbuf;
+	so->rx.buflen = DEFAULT_MAX_PDU_SIZE;

what about: ARRAY_SIZE(so->rx.sbuf)

Quoted 27 lines

net/can/isotp.c +6 -1

+	so->tx.buflen = DEFAULT_MAX_PDU_SIZE;
+
 	hrtimer_init(&so->rxtimer, CLOCK_MONOTONIC, HRTIMER_MODE_REL_SOFT);
 	so->rxtimer.function = isotp_rx_timer_handler;
 	hrtimer_init(&so->txtimer, CLOCK_MONOTONIC, HRTIMER_MODE_REL_SOFT);
 	so->txtimer.function = isotp_tx_timer_handler;
 	hrtimer_init(&so->txfrtimer, CLOCK_MONOTONIC, HRTIMER_MODE_REL_SOFT);

@@ -1656,11 +1700,14 @@ static struct notifier_block canisotp_notifier = {
 
 static __init int isotp_module_init(void)
 {
 	int err;
 
-	pr_info("can: isotp protocol\n");
+	max_pdu_size = max_t(unsigned int, max_pdu_size, MAX_12BIT_PDU_SIZE);
+	max_pdu_size = min_t(unsigned int, max_pdu_size, MAX_PDU_SIZE);
+
+	pr_info("can: isotp protocol (max_pdu_size %d)\n", max_pdu_size);
 
 	err = can_proto_register(&isotp_can_proto);
 	if (err < 0)
 		pr_err("can: registration of isotp protocol failed %pe\n", ERR_PTR(err));
 	else

Signature · 1 line

2.30.2

Marc

Signature · 3 lines

Pengutronix e.K.                 | Marc Kleine-Budde           |
Embedded Linux                   | https://www.pengutronix.de  |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-5555 |

Oliver Hartkopp

author maintainer

2023-03-20 21:08 UTC #2 lore raw

Quoted 83 lines · Marc Kleine-Budde

Quoted 6 lines · Oliver Hartkopp

With ISO 15765-2:2016 the PDU size is not limited to 2^12 - 1 (4095) bytes but can be represented as a 32 bit unsigned integer value which allows 2^32 - 1 bytes (~4GB). The use-cases like automotive unified diagnostic services (UDS) and flashing of ECUs still use the small static buffers which are provided at socket creation time.

Quoted 7 lines

When a use-case requires to transfer PDUs up to 1025 kByte the maximum PDU size can now be extended by setting the module parameter max_pdu_size. The extended size buffers are only allocated on a per-socket/connection base when needed at run-time.
Link https://github.com/raspberrypi/linux/issues/5371
Signed-off-by Oliver Hartkopp <[email protected]>

See comments inline

Quoted 66 lines

---

v2: limit the minimum 'max_pdu_size' to 4095 to maintain the classic behavior

     before ISO 15765-2:2016

net/can/isotp.c | 65 ++++++++++++++++++++++++++++++++++++++++++-------

  1 file changed, 56 insertions(+), 9 deletions(-)

net/can/isotp.c +18 -5

index 9bc344851704..8a5d278c8bf1 100644
@@ -83,14 +83,25 @@ MODULE_ALIAS("can-proto-6");
  			 (CAN_EFF_MASK | CAN_EFF_FLAG | CAN_RTR_FLAG) : \
  			 (CAN_SFF_MASK | CAN_EFF_FLAG | CAN_RTR_FLAG))
 
  /* ISO 15765-2:2016 supports more than 4095 byte per ISO PDU as the FF_DL can
   * take full 32 bit values (4 Gbyte). We would need some good concept to handle
- * this between user space and kernel space. For now increase the static buffer
- * to something about 64 kbyte to be able to test this new functionality.
+ * this between user space and kernel space. For now set the static buffer to
+ * something about 8 kbyte to be able to test this new functionality.
   */
-#define MAX_MSG_LENGTH 66000
+#define DEFAULT_MAX_PDU_SIZE 8300
+
+/* maximum PDU size before ISO 15765-2:2016 extension was 4095 */
+#define MAX_12BIT_PDU_SIZE 4095
+
+/* limit the isotp pdu size from the optional module parameter to 1MByte */
+#define MAX_PDU_SIZE (1025 * 1024U)
+
+static unsigned int max_pdu_size __read_mostly = DEFAULT_MAX_PDU_SIZE;
+module_param(max_pdu_size, uint, 0444);
+MODULE_PARM_DESC(max_pdu_size, "maximum isotp pdu size (default "
+		 __stringify(DEFAULT_MAX_PDU_SIZE) ")");
 
  /* N_PCI type values in bits 7-4 of N_PCI bytes */
  #define N_PCI_SF 0x00	/* single frame */
  #define N_PCI_FF 0x10	/* first frame */
  #define N_PCI_CF 0x20	/* consecutive frame */

@@ -121,17 +132,19 @@ enum {
  	ISOTP_WAIT_DATA,
  	ISOTP_SENDING
  };
 
  struct tpcon {
-	unsigned int idx;
+	u8 *buf;
+	unsigned int buflen;
  	unsigned int len;
+	unsigned int idx;
  	u32 state;
  	u8 bs;
  	u8 sn;
  	u8 ll_dl;
-	u8 buf[MAX_MSG_LENGTH + 1];
+	u8 sbuf[DEFAULT_MAX_PDU_SIZE];
  };
 
  struct isotp_sock {
  	struct sock sk;
  	int bound;

@@ -501,11 +514,21 @@ static int isotp_rcv_ff(struct sock *sk, struct canfd_frame *cf, int ae)
  	off = (so->rx.ll_dl > CAN_MAX_DLEN) ? 1 : 0;

Quoted 8 lines

net/can/isotp.c static int isotp_rcv_ff(struct sock *sk, struct canfd_frame *cf, int ae) · line 519
Quoted 5 lines
net/can/isotp.c +1 -1
  	if (so->rx.len + ae + off + ff_pci_sz < so->rx.ll_dl)
  		return 1;
 
-	if (so->rx.len > MAX_MSG_LENGTH) {
+	/* PDU size > default => try max_pdu_size */
What exactly is "so->rx.len"?

This is the PDU size that is sent to this local host.

Quoted 4 lines

net/can/isotp.c static int isotp_rcv_ff(struct sock *sk, struct canfd_frame *cf, int ae) · line 520
Quoted 1 line
net/can/isotp.c +1 -0
+	if (so->rx.len > so->rx.buflen && so->rx.buflen < max_pdu_size) {
                                           ^^^^^^^^^^^^^
Why are you checking so->rx.buflen against max_pdu_size? Should you check rx.len instead?

if (to be received PDU does not fit into the rx-buffer AND the rx-buffer has not be extended to max_pdu_size so far) {
Try to increase the rx-buffer to max_pdu_size
}

Quoted 4 lines

net/can/isotp.c static int isotp_rcv_ff(struct sock *sk, struct canfd_frame *cf, int ae) · line 521
Quoted 2 lines
net/can/isotp.c +1 -0
+		u8 *newbuf = kmalloc(max_pdu_size, GFP_ATOMIC);
In what context is this code executed? Is it really atomic?

Yes. In NET_RX softirq.

Quoted 1 line

Why are you allocating the max_pdu_size, not rx.len?

There is one upper limit which is selected when the 8300 bytes (99,9% of isotp use-cases) are not enough.

I intentionally did not want to handle re-allocations for every increase of received PDU size on this socket.

Just for simplicity reasons.

Quoted 11 lines

net/can/isotp.c static int isotp_rcv_ff(struct sock *sk, struct canfd_frame *cf, int ae) · line 529
Quoted 9 lines
net/can/isotp.c +8 -0
+
+		if (newbuf) {
+			so->rx.buf = newbuf;
+			so->rx.buflen = max_pdu_size;
+		}
+	}
+
+	if (so->rx.len > so->rx.buflen) {
This patch is also taken if the kmalloc() fails, right?

s/patch/path/ ?!

Yes. At the end even the extended buffer might be too small. And when we don't have enough space - either with our without kmalloc() - it throws and error.

For that reason a failed kmalloc() does not create any stress. We just stay on the default buffer.

Quoted 25 lines

net/can/isotp.c +1 -1

  		/* send FC frame with overflow status */
  		isotp_send_fc(sk, ae, ISOTP_FC_OVFLW);
  		return 1;
  	}

@@ -805,11 +828,11 @@ static void isotp_create_fframe(struct canfd_frame *cf, struct isotp_sock *so,
  	cf->len = so->tx.ll_dl;
  	if (ae)
  		cf->data[0] = so->opt.ext_address;
 
  	/* create N_PCI bytes with 12/32 bit FF_DL data length */
-	if (so->tx.len > 4095) {
+	if (so->tx.len > MAX_12BIT_PDU_SIZE) {
  		/* use 32 bit FF_DL notation */
  		cf->data[ae] = N_PCI_FF;
  		cf->data[ae + 1] = 0;
  		cf->data[ae + 2] = (u8)(so->tx.len >> 24) & 0xFFU;
  		cf->data[ae + 3] = (u8)(so->tx.len >> 16) & 0xFFU;

@@ -945,11 +968,21 @@ static int isotp_sendmsg(struct socket *sock, struct msghdr *msg, size_t size)
  			goto err_out;
 
  		so->tx.state = ISOTP_SENDING;
  	}

Quoted 8 lines

net/can/isotp.c static int isotp_sendmsg(struct socket *sock, struct msghdr *msg, size_t size) · line 975
Quoted 4 lines
net/can/isotp.c +3 -1
-	if (!size || size > MAX_MSG_LENGTH) {
+	/* PDU size > default => try max_pdu_size */
+	if (size > so->tx.buflen && so->tx.buflen < max_pdu_size) {
+		u8 *newbuf = kmalloc(max_pdu_size, GFP_KERNEL);
Same questions as in the RX-path. BTW: for the TX-path you can implement something like in the j1939 protocol. There we don't allocate the full TX buffer anymore, but handle it in chunks. Talk to Oleksij for details.

As explained above the reason for this buffer extension is a rare use-case that two people have been asking for in nine years.

I've been thinking about some sendfile() implementation too. But this again would bloat the code and would not solve the rx side.

Therefore this KISS approach seemed the right decision to me to provide such a feature for people who know what they are doing and who only tweak the module parameter on demand.

Quoted 34 lines

net/can/isotp.c +14 -0

+
+		if (newbuf) {
+			so->tx.buf = newbuf;
+			so->tx.buflen = max_pdu_size;
+		}
+	}
+
+	if (!size || size > so->tx.buflen) {
  		err = -EINVAL;
  		goto err_out_drop;
  	}
 
  	/* take care of a potential SF_DL ESC offset for TX_DL > 8 */

@@ -1193,10 +1226,16 @@ static int isotp_release(struct socket *sock)
  	hrtimer_cancel(&so->rxtimer);
 
  	so->ifindex = 0;
  	so->bound = 0;
 
+	if (so->rx.buf != so->rx.sbuf)
+		kfree(so->rx.buf);
+
+	if (so->tx.buf != so->tx.sbuf)
+		kfree(so->tx.buf);
+
  	sock_orphan(sk);
  	sock->sk = NULL;
 
  	release_sock(sk);
  	sock_put(sk);

@@ -1589,10 +1628,15 @@ static int isotp_init(struct sock *sk)
  	so->tx.ll_dl = so->ll.tx_dl;

Quoted 9 lines

net/can/isotp.c static int isotp_init(struct sock *sk) · line 1635
Quoted 6 lines
net/can/isotp.c +3 -0
  	so->rx.state = ISOTP_IDLE;
  	so->tx.state = ISOTP_IDLE;
 
+	so->rx.buf = so->rx.sbuf;
+	so->tx.buf = so->tx.sbuf;
+	so->rx.buflen = DEFAULT_MAX_PDU_SIZE;
what about: ARRAY_SIZE(so->rx.sbuf)

Looks good. I was just unsure which macro to use ;-)

Quoted 2 lines

net/can/isotp.c static int isotp_init(struct sock *sk) · line 1637
Quoted 2 lines
net/can/isotp.c +2 -0
+	so->tx.buflen = DEFAULT_MAX_PDU_SIZE;
+

here too. This would use the DEFAULT_MAX_PDU_SIZE at one single point. No chance to get these values out of sync.

Best regards,
Oliver

Quoted 28 lines

Quoted 25 lines

net/can/isotp.c +4 -1

  	hrtimer_init(&so->rxtimer, CLOCK_MONOTONIC, HRTIMER_MODE_REL_SOFT);
  	so->rxtimer.function = isotp_rx_timer_handler;
  	hrtimer_init(&so->txtimer, CLOCK_MONOTONIC, HRTIMER_MODE_REL_SOFT);
  	so->txtimer.function = isotp_tx_timer_handler;
  	hrtimer_init(&so->txfrtimer, CLOCK_MONOTONIC, HRTIMER_MODE_REL_SOFT);

@@ -1656,11 +1700,14 @@ static struct notifier_block canisotp_notifier = {
 
  static __init int isotp_module_init(void)
  {
  	int err;
 
-	pr_info("can: isotp protocol\n");
+	max_pdu_size = max_t(unsigned int, max_pdu_size, MAX_12BIT_PDU_SIZE);
+	max_pdu_size = min_t(unsigned int, max_pdu_size, MAX_PDU_SIZE);
+
+	pr_info("can: isotp protocol (max_pdu_size %d)\n", max_pdu_size);
 
  	err = can_proto_register(&isotp_can_proto);
  	if (err < 0)
  		pr_err("can: registration of isotp protocol failed %pe\n", ERR_PTR(err));
  	else

Signature · 1 line

2.30.2

Marc

Marc Kleine-Budde

maintainer

2023-03-22 08:56 UTC #3 lore raw

Quoted 103 lines · Oliver Hartkopp

Quoted 76 lines · Marc Kleine-Budde
Quoted 6 lines · Oliver Hartkopp
With ISO 15765-2:2016 the PDU size is not limited to 2^12 - 1 (4095) bytes but can be represented as a 32 bit unsigned integer value which allows 2^32 - 1 bytes (~4GB). The use-cases like automotive unified diagnostic services (UDS) and flashing of ECUs still use the small static buffers which are provided at socket creation time.
Quoted 7 lines
When a use-case requires to transfer PDUs up to 1025 kByte the maximum PDU size can now be extended by setting the module parameter max_pdu_size. The extended size buffers are only allocated on a per-socket/connection base when needed at run-time.
Link https://github.com/raspberrypi/linux/issues/5371
Signed-off-by Oliver Hartkopp <[email protected]>
See comments inline
Quoted 59 lines
---
v2: limit the minimum 'max_pdu_size' to 4095 to maintain the classic behavior
     before ISO 15765-2:2016
net/can/isotp.c | 65 ++++++++++++++++++++++++++++++++++++++++++-------
  1 file changed, 56 insertions(+), 9 deletions(-)
net/can/isotp.c +18 -5
index 9bc344851704..8a5d278c8bf1 100644
@@ -83,14 +83,25 @@ MODULE_ALIAS("can-proto-6");
  			 (CAN_EFF_MASK | CAN_EFF_FLAG | CAN_RTR_FLAG) : \
  			 (CAN_SFF_MASK | CAN_EFF_FLAG | CAN_RTR_FLAG))
  /* ISO 15765-2:2016 supports more than 4095 byte per ISO PDU as the FF_DL can
   * take full 32 bit values (4 Gbyte). We would need some good concept to handle
- * this between user space and kernel space. For now increase the static buffer
- * to something about 64 kbyte to be able to test this new functionality.
+ * this between user space and kernel space. For now set the static buffer to
+ * something about 8 kbyte to be able to test this new functionality.
   */
-#define MAX_MSG_LENGTH 66000
+#define DEFAULT_MAX_PDU_SIZE 8300
+
+/* maximum PDU size before ISO 15765-2:2016 extension was 4095 */
+#define MAX_12BIT_PDU_SIZE 4095
+
+/* limit the isotp pdu size from the optional module parameter to 1MByte */
+#define MAX_PDU_SIZE (1025 * 1024U)
+
+static unsigned int max_pdu_size __read_mostly = DEFAULT_MAX_PDU_SIZE;
+module_param(max_pdu_size, uint, 0444);
+MODULE_PARM_DESC(max_pdu_size, "maximum isotp pdu size (default "
+		 __stringify(DEFAULT_MAX_PDU_SIZE) ")");
  /* N_PCI type values in bits 7-4 of N_PCI bytes */
  #define N_PCI_SF 0x00	/* single frame */
  #define N_PCI_FF 0x10	/* first frame */
  #define N_PCI_CF 0x20	/* consecutive frame */
@@ -121,17 +132,19 @@ enum {
  	ISOTP_WAIT_DATA,
  	ISOTP_SENDING
  };
  struct tpcon {
-	unsigned int idx;
+	u8 *buf;
+	unsigned int buflen;
  	unsigned int len;
+	unsigned int idx;
  	u32 state;
  	u8 bs;
  	u8 sn;
  	u8 ll_dl;
-	u8 buf[MAX_MSG_LENGTH + 1];
+	u8 sbuf[DEFAULT_MAX_PDU_SIZE];
  };
  struct isotp_sock {
  	struct sock sk;
  	int bound;
Quoted 9 lines
net/can/isotp.c static int isotp_rcv_ff(struct sock *sk, struct canfd_frame *cf, int ae) · line 517
Quoted 6 lines
net/can/isotp.c +1 -1
@@ -501,11 +514,21 @@ static int isotp_rcv_ff(struct sock *sk, struct canfd_frame *cf, int ae)
  	off = (so->rx.ll_dl > CAN_MAX_DLEN) ? 1 : 0;
  	if (so->rx.len + ae + off + ff_pci_sz < so->rx.ll_dl)
  		return 1;
-	if (so->rx.len > MAX_MSG_LENGTH) {
+	/* PDU size > default => try max_pdu_size */
What exactly is "so->rx.len"?
This is the PDU size that is sent to this local host.
Quoted 4 lines
net/can/isotp.c static int isotp_rcv_ff(struct sock *sk, struct canfd_frame *cf, int ae) · line 520
Quoted 1 line
net/can/isotp.c +1 -0
+	if (so->rx.len > so->rx.buflen && so->rx.buflen < max_pdu_size) {
                                           ^^^^^^^^^^^^^
Why are you checking so->rx.buflen against max_pdu_size? Should you check rx.len instead?
if (to be received PDU does not fit into the rx-buffer AND the rx-buffer has not be extended to max_pdu_size so far)
{
Try to increase the rx-buffer to max_pdu_size
}
Quoted 1 line

Quoted 5 lines

Quoted 3 lines
net/can/isotp.c static int isotp_rcv_ff(struct sock *sk, struct canfd_frame *cf, int ae) · line 521
Quoted 1 line
net/can/isotp.c +1 -0
+		u8 *newbuf = kmalloc(max_pdu_size, GFP_ATOMIC);
In what context is this code executed? Is it really atomic?
Yes. In NET_RX softirq.

Quoted 10 lines

Quoted 2 lines
Why are you allocating the max_pdu_size, not rx.len?
There is one upper limit which is selected when the 8300 bytes (99,9% of isotp use-cases) are not enough.
I intentionally did not want to handle re-allocations for every increase of received PDU size on this socket.
Just for simplicity reasons.

Hmmm. The worst case would be ~1MiB of contiguous kernel memory used, if a 8301 bytes message would be send. That puts a lot of pressure on the kernel memory for IMHO no good reason.

Quoted 11 lines

Quoted 10 lines
net/can/isotp.c static int isotp_rcv_ff(struct sock *sk, struct canfd_frame *cf, int ae) · line 529
Quoted 8 lines
net/can/isotp.c +8 -0
+
+		if (newbuf) {
+			so->rx.buf = newbuf;
+			so->rx.buflen = max_pdu_size;
+		}
+	}
+
+	if (so->rx.len > so->rx.buflen) {
This patch is also taken if the kmalloc() fails, right?

Quoted 1 line

s/patch/path/ ?!

doh!

Quoted 6 lines

Yes. At the end even the extended buffer might be too small. And when we don't have enough space - either with our without kmalloc() - it throws and error.
For that reason a failed kmalloc() does not create any stress. We just stay on the default buffer.

Just out of interest: How does ISOTP handle this situation? Is an error message forwarded to the sender?

Quoted 29 lines

Quoted 20 lines

net/can/isotp.c +1 -1

  		/* send FC frame with overflow status */
  		isotp_send_fc(sk, ae, ISOTP_FC_OVFLW);
  		return 1;
  	}

@@ -805,11 +828,11 @@ static void isotp_create_fframe(struct canfd_frame *cf, struct isotp_sock *so,
  	cf->len = so->tx.ll_dl;
  	if (ae)
  		cf->data[0] = so->opt.ext_address;
  	/* create N_PCI bytes with 12/32 bit FF_DL data length */
-	if (so->tx.len > 4095) {
+	if (so->tx.len > MAX_12BIT_PDU_SIZE) {
  		/* use 32 bit FF_DL notation */
  		cf->data[ae] = N_PCI_FF;
  		cf->data[ae + 1] = 0;
  		cf->data[ae + 2] = (u8)(so->tx.len >> 24) & 0xFFU;
  		cf->data[ae + 3] = (u8)(so->tx.len >> 16) & 0xFFU;

@@ -945,11 +968,21 @@ static int isotp_sendmsg(struct socket *sock, struct msghdr *msg, size_t size)
  			goto err_out;
  		so->tx.state = ISOTP_SENDING;
  	}

Quoted 8 lines

net/can/isotp.c static int isotp_sendmsg(struct socket *sock, struct msghdr *msg, size_t size) · line 975
Quoted 4 lines
net/can/isotp.c +3 -1
-	if (!size || size > MAX_MSG_LENGTH) {
+	/* PDU size > default => try max_pdu_size */
+	if (size > so->tx.buflen && so->tx.buflen < max_pdu_size) {
+		u8 *newbuf = kmalloc(max_pdu_size, GFP_KERNEL);
Same questions as in the RX-path. BTW: for the TX-path you can implement something like in the j1939 protocol. There we don't allocate the full TX buffer anymore, but handle it in chunks. Talk to Oleksij for details.

Quoted 5 lines

As explained above the reason for this buffer extension is a rare use-case that two people have been asking for in nine years.
I've been thinking about some sendfile() implementation too. But this again would bloat the code and would not solve the rx side.

I'm not talking about sendfile. Have a look at j1939's j1939_sk_send_loop();

| https://elixir.bootlin.com/linux/v6.2/source/net/can/j1939/socket.c#L1114

Quoted 42 lines

Therefore this KISS approach seemed the right decision to me to provide such a feature for people who know what they are doing and who only tweak the module parameter on demand.

Quoted 28 lines

net/can/isotp.c +14 -0

+
+		if (newbuf) {
+			so->tx.buf = newbuf;
+			so->tx.buflen = max_pdu_size;
+		}
+	}
+
+	if (!size || size > so->tx.buflen) {
  		err = -EINVAL;
  		goto err_out_drop;
  	}
  	/* take care of a potential SF_DL ESC offset for TX_DL > 8 */

@@ -1193,10 +1226,16 @@ static int isotp_release(struct socket *sock)
  	hrtimer_cancel(&so->rxtimer);
  	so->ifindex = 0;
  	so->bound = 0;
+	if (so->rx.buf != so->rx.sbuf)
+		kfree(so->rx.buf);
+
+	if (so->tx.buf != so->tx.sbuf)
+		kfree(so->tx.buf);
+
  	sock_orphan(sk);
  	sock->sk = NULL;
  	release_sock(sk);
  	sock_put(sk);

@@ -1589,10 +1628,15 @@ static int isotp_init(struct sock *sk)

Quoted 9 lines

net/can/isotp.c static int isotp_init(struct sock *sk) · line 1635
Quoted 6 lines
net/can/isotp.c +3 -0
  	so->tx.ll_dl = so->ll.tx_dl;
  	so->rx.state = ISOTP_IDLE;
  	so->tx.state = ISOTP_IDLE;
+	so->rx.buf = so->rx.sbuf;
+	so->tx.buf = so->tx.sbuf;
+	so->rx.buflen = DEFAULT_MAX_PDU_SIZE;
what about: ARRAY_SIZE(so->rx.sbuf)

Quoted 1 line

Looks good. I was just unsure which macro to use ;-)

You can also use sizeof(so->rx.sbuf).

ARRAY_SIZE would cause a compile error if you convert so->rx.sbuf to a pointer to dynamically allocated mem, while sizeof() still compiles.

Quoted 6 lines

Quoted 3 lines
net/can/isotp.c static int isotp_init(struct sock *sk) · line 1637
Quoted 3 lines
net/can/isotp.c +2 -0
+	so->tx.buflen = DEFAULT_MAX_PDU_SIZE;
+
here too. This would use the DEFAULT_MAX_PDU_SIZE at one single point. No chance to get these values out of sync.

ACK

Marc

Signature · 4 lines

Pengutronix e.K.                 | Marc Kleine-Budde           |
Embedded Linux                   | https://www.pengutronix.de  |
Vertretung Nürnberg              | Phone: +49-5121-206917-129  |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-5555 |

Oliver Hartkopp

author maintainer

2023-03-22 17:59 UTC #4 lore raw

Quoted 8 lines · Marc Kleine-Budde

Quoted 6 lines · Oliver Hartkopp
Quoted 2 lines
Why are you allocating the max_pdu_size, not rx.len?
There is one upper limit which is selected when the 8300 bytes (99,9% of isotp use-cases) are not enough.

Quoted 8 lines

Quoted 4 lines
I intentionally did not want to handle re-allocations for every increase of received PDU size on this socket.
Just for simplicity reasons.
Hmmm. The worst case would be ~1MiB of contiguous kernel memory used, if a 8301 bytes message would be send. That puts a lot of pressure on the kernel memory for IMHO no good reason.

No, that's not the plan.

The max_pdu_size is a module parameter that simplifies the process, when someone needs unusually long PDUs without changing the static buffer size in the kernel code.

When you have the use-case to transfer PDUs with 128 kByte you would set the max_pdu_size to 128 kByte.

If you need 8301 bytes you set it to 8301.

It is very likely that only one or two isotp socket instances would ever allocate this extended buffer on one system at one time.

All other isotp sockets stay with the static buffer of 8300 bytes.

Quoted 10 lines

Quoted 3 lines
Quoted 1 line
This patch is also taken if the kmalloc() fails, right?
s/patch/path/ ?!
doh!
Quoted 4 lines
Yes. At the end even the extended buffer might be too small. And when we don't have enough space - either with our without kmalloc() - it throws and error.

Quoted 8 lines

Quoted 2 lines
For that reason a failed kmalloc() does not create any stress. We just stay on the default buffer.
Just out of interest: How does ISOTP handle this situation? Is an error message forwarded to the sender?
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
   		/* send FC frame with overflow status */
   		isotp_send_fc(sk, ae, ISOTP_FC_OVFLW);

Yes. As you can see here the receiver sends and "overflow" error message to the sender, when the receive buffer can not handle the PDU size announced in the "first frame" of the PDU transmission.

The PDU transmission failure is therefore detected by the sender.

This is also implemented on the sender side where the flow control with ISOTP_FC_OVFLW leads to -EMSGSIZE as return value

https://elixir.bootlin.com/linux/v6.2/source/net/can/isotp.c#L419

Quoted 8 lines

Quoted 2 lines
I've been thinking about some sendfile() implementation too. But this again would bloat the code and would not solve the rx side.
I'm not talking about sendfile. Have a look at j1939's j1939_sk_send_loop();
| https://elixir.bootlin.com/linux/v6.2/source/net/can/j1939/socket.c#L1114

This does not work for isotp like this as you have to handle different block sizes in the flow control message.

Quoted 9 lines

Quoted 4 lines
Quoted 2 lines
what about: ARRAY_SIZE(so->rx.sbuf)
Looks good. I was just unsure which macro to use ;-)
You can also use sizeof(so->rx.sbuf).
ARRAY_SIZE would cause a compile error if you convert so->rx.sbuf to a pointer to dynamically allocated mem, while sizeof() still compiles.

so->rx.sbuf is always a static buffer.

Only so->rx.buf can point to either so->rx.sbuf or to a dynamically allocated memory.

But when sizeof() is always safe it would take this for the v3 patch.

Best regards,
Oliver

Quoted 12 lines

Quoted 7 lines
Quoted 4 lines
net/can/isotp.c static int isotp_init(struct sock *sk) · line 1637
Quoted 4 lines
net/can/isotp.c +2 -0
+	so->tx.buflen = DEFAULT_MAX_PDU_SIZE;
+
here too. This would use the DEFAULT_MAX_PDU_SIZE at one single point. No chance to get these values out of sync.
ACK
Marc

Marc Kleine-Budde

maintainer

2023-03-24 17:29 UTC #5 lore raw

On 22.03.2023 18:59:04, Oliver Hartkopp wrote:
.> > > I've been thinking about some sendfile() implementation too. But this again

Quoted 10 lines

Quoted 7 lines
Quoted 1 line
would bloat the code and would not solve the rx side.
I'm not talking about sendfile. Have a look at j1939's j1939_sk_send_loop();
| https://elixir.bootlin.com/linux/v6.2/source/net/can/j1939/socket.c#L1114
This does not work for isotp like this as you have to handle different block sizes in the flow control message.

Let this be a task for future us. :)

Quoted 11 lines

Quoted 9 lines
Quoted 4 lines
Quoted 2 lines
what about: ARRAY_SIZE(so->rx.sbuf)
Looks good. I was just unsure which macro to use ;-)
You can also use sizeof(so->rx.sbuf).
ARRAY_SIZE would cause a compile error if you convert so->rx.sbuf to a pointer to dynamically allocated mem, while sizeof() still compiles.
so->rx.sbuf is always a static buffer.

Yes, in the current code. I was showing the difference between ARRAY_SIZE() and sizeof(). ARRAY_SIZE has a bit of type checking, while sizeof() doesn't - this becomes important once you change the code.

Quoted 4 lines

Only so->rx.buf can point to either so->rx.sbuf or to a dynamically allocated memory.
But when sizeof() is always safe it would take this for the v3 patch.

Use ARRAY_SIZE().

regards,
Marc

Signature · 4 lines

Pengutronix e.K.                 | Marc Kleine-Budde           |
Embedded Linux                   | https://www.pengutronix.de  |
Vertretung Nürnberg              | Phone: +49-5121-206917-129  |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-5555 |

Oliver Hartkopp

author maintainer

2023-03-26 12:02 UTC #6 lore raw

Quoted 7 lines · Marc Kleine-Budde

On 22.03.2023 18:59:04, Oliver Hartkopp wrote:
.> > > I've been thinking about some sendfile() implementation too. But this again
Quoted 5 lines
Quoted 5 lines
Quoted 1 line
would bloat the code and would not solve the rx side.
I'm not talking about sendfile. Have a look at j1939's j1939_sk_send_loop();

Quoted 7 lines

Quoted 5 lines
Quoted 2 lines
| https://elixir.bootlin.com/linux/v6.2/source/net/can/j1939/socket.c#L1114
This does not work for isotp like this as you have to handle different block sizes in the flow control message.
Let this be a task for future us. :)

Let's see if there would be really a use-case that would justify an additional complexity.

So far I don't see the need for that.

Quoted 7 lines

Quoted 7 lines
Quoted 7 lines
Quoted 4 lines
Quoted 2 lines
what about: ARRAY_SIZE(so->rx.sbuf)
Looks good. I was just unsure which macro to use ;-)
You can also use sizeof(so->rx.sbuf).

Quoted 9 lines

Quoted 4 lines
Quoted 2 lines
ARRAY_SIZE would cause a compile error if you convert so->rx.sbuf to a pointer to dynamically allocated mem, while sizeof() still compiles.
so->rx.sbuf is always a static buffer.
Yes, in the current code. I was showing the difference between ARRAY_SIZE() and sizeof(). ARRAY_SIZE has a bit of type checking, while sizeof() doesn't - this becomes important once you change the code.

ACK

Quoted 6 lines

Quoted 4 lines
Only so->rx.buf can point to either so->rx.sbuf or to a dynamically allocated memory.
But when sizeof() is always safe it would take this for the v3 patch.
Use ARRAY_SIZE().

Done. Sent a V3 patch a minute ago.

Many thanks,
Oliver