Re: ifconfig rename tcplro
> On 7 Jun 2023, at 06:33, Vitaliy Makkoveev wrote: > >> On 6 Jun 2023, at 20:29, Alexander Bluhm wrote: >> >> On Tue, Jun 06, 2023 at 05:54:31PM +0300, Vitaliy Makkoveev wrote: >>> On Tue, Jun 06, 2023 at 02:31:52PM +0200, Alexander Bluhm wrote: Hi, I would suggest to rename ifconfig tcprecvoffload to tcplro. Maybe it's just because I had to type that long name too often. With that we have consistent naming: # ifconfig ix0 tcplro # sysctl net.inet.tcp.tso=1 Also the coresponding flag are named LRO. # ifconfig ix1 hwfeatures ix1: flags=2008843 mtu 1500 hwfeatures=71b7 hardmtu 9198 The feature is quite new, so I have no backward compatiblity concerns. ok? >>> >>> Could you name it "lro" like FreeBSD uses? >> >> When I started with this, LRO and TSO were unknown to me. So with >> TCP prefix it may be clearer to users where the feature belongs. >> >> Naming is hard. > > Yeah, naming is definitely hard. I propose to use lro because it is > already used for the same purpose by FreeBSD, so the same name helps > to avoid confusion. > >lro If the driver supports tcp(4) large receive offloading, >enable LRO on the interface. > > Also, we have used "tso" keyword for tcp segmentation offloading for > the same reason, until it became global net.inet.tcp.tso. Is it going to be used to enable lro for udp and other protocols as well?
Re: ifconfig rename tcplro
> On 6 Jun 2023, at 19:37, Chris Cappuccio wrote: > > Jan Klemkow [j.klem...@wemelug.de] wrote: >> On Tue, Jun 06, 2023 at 05:54:31PM +0300, Vitaliy Makkoveev wrote: >>> On Tue, Jun 06, 2023 at 02:31:52PM +0200, Alexander Bluhm wrote: I would suggest to rename ifconfig tcprecvoffload to tcplro. Maybe it's just because I had to type that long name too often. With that we have consistent naming: # ifconfig ix0 tcplro # sysctl net.inet.tcp.tso=1 Also the coresponding flag are named LRO. # ifconfig ix1 hwfeatures ix1: flags=2008843 mtu 1500 hwfeatures=71b7 hardmtu 9198 The feature is quite new, so I have no backward compatiblity concerns. ok? >>> >>> Could you name it "lro" like FreeBSD uses? >> >> I also would prefer this one. > > and tcpsendoffload back to tso ? > > was the reason for changing it from tso due to the initial conflation of TSO > and LRO in the tree? > This tso not the tcpsendoffload, and yes, we used tso before it became global net.inet.tcp.tso.
Re: ifconfig rename tcplro
> On 6 Jun 2023, at 20:29, Alexander Bluhm wrote: > > On Tue, Jun 06, 2023 at 05:54:31PM +0300, Vitaliy Makkoveev wrote: >> On Tue, Jun 06, 2023 at 02:31:52PM +0200, Alexander Bluhm wrote: >>> Hi, >>> >>> I would suggest to rename ifconfig tcprecvoffload to tcplro. Maybe >>> it's just because I had to type that long name too often. >>> >>> With that we have consistent naming: >>> # ifconfig ix0 tcplro >>> # sysctl net.inet.tcp.tso=1 >>> >>> Also the coresponding flag are named LRO. >>> # ifconfig ix1 hwfeatures >>> ix1: flags=2008843 mtu 1500 >>> >>> hwfeatures=71b7 >>> hardmtu 9198 >>> >>> The feature is quite new, so I have no backward compatiblity concerns. >>> >>> ok? >>> >> >> Could you name it "lro" like FreeBSD uses? > > When I started with this, LRO and TSO were unknown to me. So with > TCP prefix it may be clearer to users where the feature belongs. > > Naming is hard. Yeah, naming is definitely hard. I propose to use lro because it is already used for the same purpose by FreeBSD, so the same name helps to avoid confusion. lro If the driver supports tcp(4) large receive offloading, enable LRO on the interface. Also, we have used "tso" keyword for tcp segmentation offloading for the same reason, until it became global net.inet.tcp.tso.
Re: ifconfig rename tcplro
On Tue, Jun 06, 2023 at 11:33:36PM +0300, Vitaliy Makkoveev wrote: > > On 6 Jun 2023, at 20:29, Alexander Bluhm wrote: > > > > On Tue, Jun 06, 2023 at 05:54:31PM +0300, Vitaliy Makkoveev wrote: > >> On Tue, Jun 06, 2023 at 02:31:52PM +0200, Alexander Bluhm wrote: > >>> Hi, > >>> > >>> I would suggest to rename ifconfig tcprecvoffload to tcplro. Maybe > >>> it's just because I had to type that long name too often. > >>> > >>> With that we have consistent naming: > >>> # ifconfig ix0 tcplro > >>> # sysctl net.inet.tcp.tso=1 > >>> > >>> Also the coresponding flag are named LRO. > >>> # ifconfig ix1 hwfeatures > >>> ix1: flags=2008843 mtu 1500 > >>> > >>> hwfeatures=71b7 > >>> hardmtu 9198 > >>> > >>> The feature is quite new, so I have no backward compatiblity concerns. > >>> > >>> ok? > >>> > >> > >> Could you name it "lro" like FreeBSD uses? > > > > When I started with this, LRO and TSO were unknown to me. So with > > TCP prefix it may be clearer to users where the feature belongs. > > > > Naming is hard. > > Yeah, naming is definitely hard. I propose to use lro because it is > already used for the same purpose by FreeBSD, so the same name helps > to avoid confusion. > > lro If the driver supports tcp(4) large receive offloading, > enable LRO on the interface. > > Also, we have used "tso" keyword for tcp segmentation offloading for > the same reason, until it became global net.inet.tcp.tso. In sysctl we have tso in tcp namespace. That's why I wanted tcplro. And claudio@ asked for a longer name. Let's see if he has an opinion.
Re: ifconfig rename tcplro
On Tue, Jun 06, 2023 at 02:31:52PM +0200, Alexander Bluhm wrote: > Hi, > > I would suggest to rename ifconfig tcprecvoffload to tcplro. Maybe > it's just because I had to type that long name too often. > > With that we have consistent naming: > # ifconfig ix0 tcplro > # sysctl net.inet.tcp.tso=1 > > Also the coresponding flag are named LRO. > # ifconfig ix1 hwfeatures > ix1: flags=2008843 mtu 1500 > > hwfeatures=71b7 > hardmtu 9198 > > The feature is quite new, so I have no backward compatiblity concerns. > > ok? > Could you name it "lro" like FreeBSD uses?
Re: ifconfig rename tcplro
On Tue, Jun 06, 2023 at 05:54:31PM +0300, Vitaliy Makkoveev wrote: > On Tue, Jun 06, 2023 at 02:31:52PM +0200, Alexander Bluhm wrote: > > Hi, > > > > I would suggest to rename ifconfig tcprecvoffload to tcplro. Maybe > > it's just because I had to type that long name too often. > > > > With that we have consistent naming: > > # ifconfig ix0 tcplro > > # sysctl net.inet.tcp.tso=1 > > > > Also the coresponding flag are named LRO. > > # ifconfig ix1 hwfeatures > > ix1: flags=2008843 mtu 1500 > > > > hwfeatures=71b7 > > hardmtu 9198 > > > > The feature is quite new, so I have no backward compatiblity concerns. > > > > ok? > > > > Could you name it "lro" like FreeBSD uses? When I started with this, LRO and TSO were unknown to me. So with TCP prefix it may be clearer to users where the feature belongs. Naming is hard. bluhm
Re: ifconfig rename tcplro
On Tue, Jun 06, 2023 at 05:54:31PM +0300, Vitaliy Makkoveev wrote: > On Tue, Jun 06, 2023 at 02:31:52PM +0200, Alexander Bluhm wrote: > > I would suggest to rename ifconfig tcprecvoffload to tcplro. Maybe > > it's just because I had to type that long name too often. > > > > With that we have consistent naming: > > # ifconfig ix0 tcplro > > # sysctl net.inet.tcp.tso=1 > > > > Also the coresponding flag are named LRO. > > # ifconfig ix1 hwfeatures > > ix1: flags=2008843 mtu 1500 > > > > hwfeatures=71b7 > > hardmtu 9198 > > > > The feature is quite new, so I have no backward compatiblity concerns. > > > > ok? > > Could you name it "lro" like FreeBSD uses? I also would prefer this one.
Re: ifconfig rename tcplro
Jan Klemkow [j.klem...@wemelug.de] wrote: > On Tue, Jun 06, 2023 at 05:54:31PM +0300, Vitaliy Makkoveev wrote: > > On Tue, Jun 06, 2023 at 02:31:52PM +0200, Alexander Bluhm wrote: > > > I would suggest to rename ifconfig tcprecvoffload to tcplro. Maybe > > > it's just because I had to type that long name too often. > > > > > > With that we have consistent naming: > > > # ifconfig ix0 tcplro > > > # sysctl net.inet.tcp.tso=1 > > > > > > Also the coresponding flag are named LRO. > > > # ifconfig ix1 hwfeatures > > > ix1: flags=2008843 mtu 1500 > > > > > > hwfeatures=71b7 > > > hardmtu 9198 > > > > > > The feature is quite new, so I have no backward compatiblity concerns. > > > > > > ok? > > > > Could you name it "lro" like FreeBSD uses? > > I also would prefer this one. and tcpsendoffload back to tso ? was the reason for changing it from tso due to the initial conflation of TSO and LRO in the tree?
Re: ifconfig rename tcplro
On Tue, Jun 06, 2023 at 09:37:22AM -0700, Chris Cappuccio wrote: > Jan Klemkow [j.klem...@wemelug.de] wrote: > > On Tue, Jun 06, 2023 at 05:54:31PM +0300, Vitaliy Makkoveev wrote: > > > On Tue, Jun 06, 2023 at 02:31:52PM +0200, Alexander Bluhm wrote: > > > > I would suggest to rename ifconfig tcprecvoffload to tcplro. Maybe > > > > it's just because I had to type that long name too often. > > > > > > > > With that we have consistent naming: > > > > # ifconfig ix0 tcplro > > > > # sysctl net.inet.tcp.tso=1 > > > > > > > > Also the coresponding flag are named LRO. > > > > # ifconfig ix1 hwfeatures > > > > ix1: flags=2008843 mtu 1500 > > > > > > > > hwfeatures=71b7 > > > > hardmtu 9198 > > > > > > > > The feature is quite new, so I have no backward compatiblity concerns. > > > > > > > > ok? > > > > > > Could you name it "lro" like FreeBSD uses? > > > > I also would prefer this one. > > and tcpsendoffload back to tso ? > > was the reason for changing it from tso due to the initial conflation > of TSO and LRO in the tree? Yes. At the start of this, I just want to keep it simple with one ifconfig option "tso". But, tso is now default in tcp_output() and can be controlled globally via sysctl(2) net.inet.tcp.tso. Thus, we just need to control LRO per interface.
iked, use ibuf_seek() where it is obvious
Replace some ibuf_data() + offset constructs to use ibuf_seek() the actual
interface built exactly for this.
Should behave the same unless the code is already broken and overflowing
the buffer.
--
:wq Claudio
Index: crypto.c
===
RCS file: /cvs/src/sbin/iked/crypto.c,v
retrieving revision 1.43
diff -u -p -r1.43 crypto.c
--- crypto.c23 May 2023 13:12:19 - 1.43
+++ crypto.c6 Jun 2023 10:52:59 -
@@ -567,9 +567,9 @@ cipher_init(struct iked_cipher *encr, in
return (-1);
if (encr->encr_saltlength > 0) {
/* For AEADs the nonce is salt + IV (see RFC5282) */
- nonce = ibuf_new(ibuf_data(encr->encr_key) +
+ nonce = ibuf_new(ibuf_seek(encr->encr_key,
ibuf_size(encr->encr_key) - encr->encr_saltlength,
- encr->encr_saltlength);
+ encr->encr_saltlength), encr->encr_saltlength);
if (nonce == NULL)
return (-1);
if (ibuf_add(nonce, ibuf_data(encr->encr_iv) ,
ibuf_size(encr->encr_iv)) != 0)
Index: ikev2_msg.c
===
RCS file: /cvs/src/sbin/iked/ikev2_msg.c,v
retrieving revision 1.93
diff -u -p -r1.93 ikev2_msg.c
--- ikev2_msg.c 30 May 2023 08:41:15 - 1.93
+++ ikev2_msg.c 6 Jun 2023 10:51:45 -
@@ -644,7 +644,7 @@ ikev2_msg_decrypt(struct iked *env, stru
}
cipher_setkey(sa->sa_encr, encr->buf, ibuf_length(encr));
- cipher_setiv(sa->sa_encr, ibuf_data(src) + ivoff, ivlen);
+ cipher_setiv(sa->sa_encr, ibuf_seek(src, ivoff, ivlen), ivlen);
if (cipher_init_decrypt(sa->sa_encr) == -1) {
log_info("%s: error initiating cipher.", __func__);
goto done;
@@ -676,7 +676,7 @@ ikev2_msg_decrypt(struct iked *env, stru
}
if ((outlen = ibuf_length(out)) != 0) {
- if (cipher_update(sa->sa_encr, ibuf_data(src) + encroff,
+ if (cipher_update(sa->sa_encr, ibuf_seek(src, encroff, encrlen),
encrlen, ibuf_data(out), ) == -1) {
log_info("%s: error updating cipher.", __func__);
goto done;
Re: iked, use ibuf_seek() where it is obvious
On Tue, Jun 06, 2023 at 12:59:05PM +0200, Claudio Jeker wrote:
> Replace some ibuf_data() + offset constructs to use ibuf_seek() the actual
> interface built exactly for this.
>
> Should behave the same unless the code is already broken and overflowing
> the buffer.
Agreed. Reads fine, ok tb
> --
> :wq Claudio
>
> Index: crypto.c
> ===
> RCS file: /cvs/src/sbin/iked/crypto.c,v
> retrieving revision 1.43
> diff -u -p -r1.43 crypto.c
> --- crypto.c 23 May 2023 13:12:19 - 1.43
> +++ crypto.c 6 Jun 2023 10:52:59 -
> @@ -567,9 +567,9 @@ cipher_init(struct iked_cipher *encr, in
> return (-1);
> if (encr->encr_saltlength > 0) {
> /* For AEADs the nonce is salt + IV (see RFC5282) */
> - nonce = ibuf_new(ibuf_data(encr->encr_key) +
> + nonce = ibuf_new(ibuf_seek(encr->encr_key,
> ibuf_size(encr->encr_key) - encr->encr_saltlength,
> - encr->encr_saltlength);
> + encr->encr_saltlength), encr->encr_saltlength);
> if (nonce == NULL)
> return (-1);
> if (ibuf_add(nonce, ibuf_data(encr->encr_iv) ,
> ibuf_size(encr->encr_iv)) != 0)
> Index: ikev2_msg.c
> ===
> RCS file: /cvs/src/sbin/iked/ikev2_msg.c,v
> retrieving revision 1.93
> diff -u -p -r1.93 ikev2_msg.c
> --- ikev2_msg.c 30 May 2023 08:41:15 - 1.93
> +++ ikev2_msg.c 6 Jun 2023 10:51:45 -
> @@ -644,7 +644,7 @@ ikev2_msg_decrypt(struct iked *env, stru
> }
>
> cipher_setkey(sa->sa_encr, encr->buf, ibuf_length(encr));
> - cipher_setiv(sa->sa_encr, ibuf_data(src) + ivoff, ivlen);
> + cipher_setiv(sa->sa_encr, ibuf_seek(src, ivoff, ivlen), ivlen);
> if (cipher_init_decrypt(sa->sa_encr) == -1) {
> log_info("%s: error initiating cipher.", __func__);
> goto done;
> @@ -676,7 +676,7 @@ ikev2_msg_decrypt(struct iked *env, stru
> }
>
> if ((outlen = ibuf_length(out)) != 0) {
> - if (cipher_update(sa->sa_encr, ibuf_data(src) + encroff,
> + if (cipher_update(sa->sa_encr, ibuf_seek(src, encroff, encrlen),
> encrlen, ibuf_data(out), ) == -1) {
> log_info("%s: error updating cipher.", __func__);
> goto done;
>
Re: ifconfig rename tcplro
On Tue, Jun 06, 2023 at 02:31:52PM +0200, Alexander Bluhm wrote:
> I would suggest to rename ifconfig tcprecvoffload to tcplro. Maybe
> it's just because I had to type that long name too often.
>
> With that we have consistent naming:
> # ifconfig ix0 tcplro
> # sysctl net.inet.tcp.tso=1
>
> Also the coresponding flag are named LRO.
> # ifconfig ix1 hwfeatures
> ix1: flags=2008843 mtu 1500
>
> hwfeatures=71b7
> hardmtu 9198
>
> The feature is quite new, so I have no backward compatiblity concerns.
>
> ok?
I like this shorter naming.
Its OK from my side.
> Index: sbin/ifconfig/ifconfig.8
> ===
> RCS file: /data/mirror/openbsd/cvs/src/sbin/ifconfig/ifconfig.8,v
> retrieving revision 1.396
> diff -u -p -r1.396 ifconfig.8
> --- sbin/ifconfig/ifconfig.8 1 Jun 2023 18:57:53 - 1.396
> +++ sbin/ifconfig/ifconfig.8 6 Jun 2023 12:18:07 -
> @@ -501,7 +501,7 @@ Query and display information and diagno
> modules installed in an interface.
> It is only supported by drivers implementing the necessary functionality
> on hardware which supports it.
> -.It Cm tcprecvoffload
> +.It Cm tcplro
> Enable TCP large receive offload (LRO) if it's supported by the hardware; see
> .Cm hwfeatures .
> LRO enabled network interfaces modify received TCP/IP packets.
> @@ -517,7 +517,7 @@ It is not possible to use LRO with inter
> or
> .Xr tpmr 4 .
> Changing this option will re-initialize the network interface.
> -.It Cm -tcprecvoffload
> +.It Cm -tcplro
> Disable LRO.
> LRO is disabled by default.
> .It Cm up
> Index: sbin/ifconfig/ifconfig.c
> ===
> RCS file: /data/mirror/openbsd/cvs/src/sbin/ifconfig/ifconfig.c,v
> retrieving revision 1.465
> diff -u -p -r1.465 ifconfig.c
> --- sbin/ifconfig/ifconfig.c 1 Jun 2023 18:57:54 - 1.465
> +++ sbin/ifconfig/ifconfig.c 6 Jun 2023 12:18:59 -
> @@ -471,8 +471,8 @@ const struct cmd {
> { "-soii", IFXF_INET6_NOSOII, 0, setifxflags },
> { "monitor",IFXF_MONITOR, 0, setifxflags },
> { "-monitor", -IFXF_MONITOR, 0, setifxflags },
> - { "tcprecvoffload", IFXF_LRO, 0, setifxflags },
> - { "-tcprecvoffload", -IFXF_LRO, 0, setifxflags },
> + { "tcplro", IFXF_LRO, 0, setifxflags },
> + { "-tcplro",-IFXF_LRO, 0, setifxflags },
> #ifndef SMALL
> { "hwfeatures", NEXTARG0, 0, printifhwfeatures },
> { "metric", NEXTARG,0, setifmetric },
>
iked, adjust ikev2_pld code
My plan is to make ibuf_data() return void * not uint8_t *. Because of
this you can't do pointer arithmetics with that function.
The following diff just uses the same construct that many other functions
in ikev2_pld.c use.
This code should be rewritten but we are not there yet. The goal is to
introduce a ibuf parse API that avoids all this pointer gymnastics.
--
:wq Claudio
Index: ikev2_pld.c
===
RCS file: /cvs/src/sbin/iked/ikev2_pld.c,v
retrieving revision 1.128
diff -u -p -r1.128 ikev2_pld.c
--- ikev2_pld.c 23 May 2023 13:12:19 - 1.128
+++ ikev2_pld.c 6 Jun 2023 10:49:50 -
@@ -1525,9 +1525,10 @@ ikev2_pld_ts(struct iked *env, struct ik
struct sockaddr_in s4;
struct sockaddr_in6 s6;
uint8_t buf[2][128];
+ uint8_t *msgbuf = ibuf_data(msg->msg_data);
uint8_t *ptr;
- ptr = ibuf_data(msg->msg_data) + offset;
+ ptr = msgbuf + offset;
switch (type) {
case IKEV2_TS_IPV4_ADDR_RANGE:
@@ -1867,6 +1868,7 @@ ikev2_pld_cp(struct iked *env, struct ik
struct iked_addr*addr;
struct sockaddr_in *in4;
struct sockaddr_in6 *in6;
+ uint8_t *msgbuf = ibuf_data(msg->msg_data);
uint8_t *ptr;
size_t len;
uint8_t buf[128];
@@ -1875,7 +1877,7 @@ ikev2_pld_cp(struct iked *env, struct ik
if (ikev2_validate_cp(msg, offset, left, ))
return (-1);
- ptr = ibuf_data(msg->msg_data) + offset + sizeof(cp);
+ ptr = msgbuf + offset + sizeof(cp);
len = left - sizeof(cp);
log_debug("%s: type %s length %zu",
ifconfig rename tcplro
Hi,
I would suggest to rename ifconfig tcprecvoffload to tcplro. Maybe
it's just because I had to type that long name too often.
With that we have consistent naming:
# ifconfig ix0 tcplro
# sysctl net.inet.tcp.tso=1
Also the coresponding flag are named LRO.
# ifconfig ix1 hwfeatures
ix1: flags=2008843 mtu 1500
hwfeatures=71b7
hardmtu 9198
The feature is quite new, so I have no backward compatiblity concerns.
ok?
bluhm
Index: sbin/ifconfig/ifconfig.8
===
RCS file: /data/mirror/openbsd/cvs/src/sbin/ifconfig/ifconfig.8,v
retrieving revision 1.396
diff -u -p -r1.396 ifconfig.8
--- sbin/ifconfig/ifconfig.81 Jun 2023 18:57:53 - 1.396
+++ sbin/ifconfig/ifconfig.86 Jun 2023 12:18:07 -
@@ -501,7 +501,7 @@ Query and display information and diagno
modules installed in an interface.
It is only supported by drivers implementing the necessary functionality
on hardware which supports it.
-.It Cm tcprecvoffload
+.It Cm tcplro
Enable TCP large receive offload (LRO) if it's supported by the hardware; see
.Cm hwfeatures .
LRO enabled network interfaces modify received TCP/IP packets.
@@ -517,7 +517,7 @@ It is not possible to use LRO with inter
or
.Xr tpmr 4 .
Changing this option will re-initialize the network interface.
-.It Cm -tcprecvoffload
+.It Cm -tcplro
Disable LRO.
LRO is disabled by default.
.It Cm up
Index: sbin/ifconfig/ifconfig.c
===
RCS file: /data/mirror/openbsd/cvs/src/sbin/ifconfig/ifconfig.c,v
retrieving revision 1.465
diff -u -p -r1.465 ifconfig.c
--- sbin/ifconfig/ifconfig.c1 Jun 2023 18:57:54 - 1.465
+++ sbin/ifconfig/ifconfig.c6 Jun 2023 12:18:59 -
@@ -471,8 +471,8 @@ const structcmd {
{ "-soii", IFXF_INET6_NOSOII, 0, setifxflags },
{ "monitor",IFXF_MONITOR, 0, setifxflags },
{ "-monitor", -IFXF_MONITOR, 0, setifxflags },
- { "tcprecvoffload", IFXF_LRO, 0, setifxflags },
- { "-tcprecvoffload", -IFXF_LRO, 0, setifxflags },
+ { "tcplro", IFXF_LRO, 0, setifxflags },
+ { "-tcplro",-IFXF_LRO, 0, setifxflags },
#ifndef SMALL
{ "hwfeatures", NEXTARG0, 0, printifhwfeatures },
{ "metric", NEXTARG,0, setifmetric },
Re: iked, adjust ikev2_pld code
On Tue, Jun 06, 2023 at 03:33:50PM +0200, Claudio Jeker wrote: > My plan is to make ibuf_data() return void * not uint8_t *. Because of > this you can't do pointer arithmetics with that function. > The following diff just uses the same construct that many other functions > in ikev2_pld.c use. > > This code should be rewritten but we are not there yet. The goal is to > introduce a ibuf parse API that avoids all this pointer gymnastics. ok
