On 20.03.2023 22:07:56, Oliver Hartkopp wrote: > > > On 20.03.23 17:24, Marc Kleine-Budde wrote: > > 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"? > > > > This is the PDU size that is sent to this local host. > > > > + 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 > } > > > > > > + u8 *newbuf = kmalloc(max_pdu_size, GFP_ATOMIC); > > > > In what context is this code executed? Is it really atomic? > > Yes. In NET_RX softirq. Ok > > > 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. > > > + > > > + 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/ ?! doh! > 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? > > > /* 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. > > 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 > 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. > > > > > > + > > > + 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) > > > > 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->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 -- 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 |
Attachment:
signature.asc
Description: PGP signature
