On 13.03.2023 18:25:10, Oliver Hartkopp wrote: > 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 <socketcan@xxxxxxxxxxxx> See comments inline > --- > > 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(-) > > diff --git a/net/can/isotp.c b/net/can/isotp.c > index 9bc344851704..8a5d278c8bf1 100644 > --- a/net/can/isotp.c > +++ b/net/can/isotp.c > @@ -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 */ What exactly is "so->rx.len"? > + 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? > + 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? > + > + 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? > /* 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); 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. > + > + 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; what about: ARRAY_SIZE(so->rx.sbuf) > + 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 > -- > 2.30.2 > > Marc -- Pengutronix e.K. | Marc Kleine-Budde | Embedded Linux | https://www.pengutronix.de | Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
Attachment:
signature.asc
Description: PGP signature
