Re: [RFC PATCH v2] can: isotp: add module parameter for maximum pdu size

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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


[Index of Archives]     [Automotive Discussions]     [Linux ARM Kernel]     [Linux ARM]     [Linux Omap]     [Fedora ARM]     [IETF Annouce]     [Security]     [Bugtraq]     [Linux]     [Linux OMAP]     [Linux MIPS]     [eCos]     [Asterisk Internet PBX]     [Linux API]     [CAN Bus]

  Powered by Linux