Re: rev(1): pull MB_CUR_MAX out of the hot loop
Hi Martijn,
Martijn van Duren wrote on Sun, Jan 09, 2022 at 08:09:20AM +0100:
> I fully agree with your reasoning and also prefer this one over the
> previous two diff.
>
> OK martijn@
Thanks for checking; committed.
Also thanks to cheloha@ for his work on this issue
and to milllert@ for his feedback.
Ingo
> On Sun, 2022-01-09 at 00:45 +0100, Ingo Schwarze wrote:
>> Index: rev.c
>> ===
>> RCS file: /cvs/src/usr.bin/rev/rev.c,v
>> retrieving revision 1.13
>> diff -u -p -r1.13 rev.c
>> --- rev.c10 Apr 2016 17:06:52 - 1.13
>> +++ rev.c8 Jan 2022 23:19:46 -
>> @@ -46,13 +46,14 @@ void usage(void);
>> int
>> main(int argc, char *argv[])
>> {
>> -char *filename, *p = NULL, *t, *u;
>> +char *filename, *p = NULL, *t, *te, *u;
>> FILE *fp;
>> ssize_t len;
>> size_t ps = 0;
>> -int ch, rval;
>> +int ch, multibyte, rval;
>>
>> setlocale(LC_CTYPE, "");
>> +multibyte = MB_CUR_MAX > 1;
>>
>> if (pledge("stdio rpath", NULL) == -1)
>> err(1, "pledge");
>> @@ -83,14 +84,16 @@ main(int argc, char *argv[])
>> if (p[len - 1] == '\n')
>> --len;
>> for (t = p + len - 1; t >= p; --t) {
>> -if (isu8cont(*t))
>> -continue;
>> -u = t;
>> -do {
>> -putchar(*u);
>> -} while (isu8cont(*(++u)));
>> +te = t;
>> +if (multibyte)
>> +while (t > p && isu8cont(*t))
>> +--t;
>> +for (u = t; u <= te; ++u)
>> +if (putchar(*u) == EOF)
>> +err(1, "stdout");
>> }
>> -putchar('\n');
>> +if (putchar('\n') == EOF)
>> +err(1, "stdout");
>> }
>> if (ferror(fp)) {
>> warn("%s", filename);
>> @@ -104,7 +107,7 @@ main(int argc, char *argv[])
>> int
>> isu8cont(unsigned char c)
>> {
>> -return MB_CUR_MAX > 1 && (c & (0x80 | 0x40)) == 0x80;
>> +return (c & (0x80 | 0x40)) == 0x80;
>> }
>>
>> void
Re: new: lang/polyml
> On Jan 12, 2022, at 2:06 AM, Philip Guenther wrote: > > >> On Tue, Jan 11, 2022 at 4:09 PM Daniel Dickman wrote: > >> On Mon, Jan 10, 2022 at 8:12 PM Leo Larnack wrote: >> > >> > i386 >> > >> >> >> >> with this diff I was able to install includes, rebuild ld.so and >> ctfconv. I've not managed to build a release yet. > ... > > Umm, with what diff? There was no diff in nor attached to that message. :-/ I see sthen already sent you the link. Leo was asked to split his diff by architecture. I did the testing for the i386 diff he proposed. > > (That was a lot of lines of output. I don't know about ports@, but my > expectation would be there would be *zero* reviewers of anything before, say, > the last 50 lines of output before the switch to actual compilation. > Standard "make lots of noise so when a failure occurs we can see the leadup, > but we'll ignore it otherwise" style of output, like a base build. You read > the lead up to the warnings and errors only. ) Yeah. No this is very bad advice. I’m going to have to *strongly* disagree with you here Philip. I don’t expect most people to be setting up i386 boxes to test this so providing the full logs would be helpful to compare against amd64 which is obviously working for Leo. I think Stuart was making the same point. If I tested on amd64 then I’d expect most people would be able to check it themselves. > > > Philip Guenther >
Re: ix(4): enable TCPv6/UDPv6 cksum offloading
Jan Klemkow wrote: > On Wed, Jan 12, 2022 at 05:54:07PM +0100, Mark Kettenis wrote: > > > Date: Wed, 12 Jan 2022 17:45:57 +0100 > > > From: Jan Klemkow > > > > > > On Wed, Jan 12, 2022 at 05:36:01PM +0100, Mark Kettenis wrote: > > > > > Date: Wed, 12 Jan 2022 17:02:03 +0100 > > > > > From: Jan Klemkow > > > > > > > > > > Hi, > > > > > > > > > > This diff enables TCP and UDP checksum offloading in ix(4) for IPv6. > > > > > > > > > > IPv6 extension headers aren't a problem in this case. > > > > > in6_proto_cksum_out() in netinet6/ip6_output.c disables checksum > > > > > offloading if ip6_nxt is not TCP or UDP. Thus, we can just use this > > > > > field. > > > > > > > > > > Tested with: > > > > > ix0 at pci5 dev 0 function 0 "Intel 82599" rev 0x01, msix, 8 queues, > > > > > address 00:1b:21:94:4c:48 > > > > > > > > > > OK? > > > > > > > > Isn't this the same disaster as the ixl(4) diff you sent earlier? We > > > > have sparc64 machines with onboard ix(4)... > > > > > > Yes, but we don't parse the TCP header here. As bluhm@ figured out: > > > The access to ip_hl does not generate an alignment problem on sparc64. > > > Because, the bits of ip_hl are on the other of the byte, as bits of > > > th_off. > > > > > > This diff just touches the IPv6 case, where we don't have this kind of > > > problem, anyway. > > > > But you're still using m_getptr(), casting the result to a struct and > > then look at a member of the struct, which may access data beyond the > > end of the mbuf. > > We use the same pattern in re(4), vio(4) and the IPv4 case in ix(4). > And we check for this case with the KASSERT(), except re(4). > > For me, it looks as this assumption is safe. Not sure I understand. Looking at vio mip = m_getptr(m, ehdrlen, &ipoff); KASSERT(mip != NULL && mip->m_len - ipoff >= sizeof(*ip)); Where does the network stack (when creating mbufs) satisfy this condition -- that the struct is linear, and present in the head mbuf? Or do you think a kernel panic at runtime is ok?
Re: ix(4): enable TCPv6/UDPv6 cksum offloading
On Wed, Jan 12, 2022 at 05:54:07PM +0100, Mark Kettenis wrote: > > Date: Wed, 12 Jan 2022 17:45:57 +0100 > > From: Jan Klemkow > > > > On Wed, Jan 12, 2022 at 05:36:01PM +0100, Mark Kettenis wrote: > > > > Date: Wed, 12 Jan 2022 17:02:03 +0100 > > > > From: Jan Klemkow > > > > > > > > Hi, > > > > > > > > This diff enables TCP and UDP checksum offloading in ix(4) for IPv6. > > > > > > > > IPv6 extension headers aren't a problem in this case. > > > > in6_proto_cksum_out() in netinet6/ip6_output.c disables checksum > > > > offloading if ip6_nxt is not TCP or UDP. Thus, we can just use this > > > > field. > > > > > > > > Tested with: > > > > ix0 at pci5 dev 0 function 0 "Intel 82599" rev 0x01, msix, 8 queues, > > > > address 00:1b:21:94:4c:48 > > > > > > > > OK? > > > > > > Isn't this the same disaster as the ixl(4) diff you sent earlier? We > > > have sparc64 machines with onboard ix(4)... > > > > Yes, but we don't parse the TCP header here. As bluhm@ figured out: > > The access to ip_hl does not generate an alignment problem on sparc64. > > Because, the bits of ip_hl are on the other of the byte, as bits of > > th_off. > > > > This diff just touches the IPv6 case, where we don't have this kind of > > problem, anyway. > > But you're still using m_getptr(), casting the result to a struct and > then look at a member of the struct, which may access data beyond the > end of the mbuf. We use the same pattern in re(4), vio(4) and the IPv4 case in ix(4). And we check for this case with the KASSERT(), except re(4). For me, it looks as this assumption is safe. > > > > Index: dev/pci/if_ix.c > > > > === > > > > RCS file: /mount/openbsd/cvs/src/sys/dev/pci/if_ix.c,v > > > > retrieving revision 1.180 > > > > diff -u -p -r1.180 if_ix.c > > > > --- dev/pci/if_ix.c 27 Jul 2021 01:44:55 - 1.180 > > > > +++ dev/pci/if_ix.c 12 Jan 2022 14:53:14 - > > > > @@ -1879,7 +1879,8 @@ ixgbe_setup_interface(struct ix_softc *s > > > > ifp->if_capabilities |= IFCAP_VLAN_HWTAGGING; > > > > #endif > > > > > > > > - ifp->if_capabilities |= IFCAP_CSUM_TCPv4 | IFCAP_CSUM_UDPv4; > > > > + ifp->if_capabilities |= IFCAP_CSUM_TCPv4 | IFCAP_CSUM_UDPv4 > > > > + | IFCAP_CSUM_TCPv6 | IFCAP_CSUM_UDPv6; > > > > > > > > /* > > > > * Specify the media types supported by this sc and register > > > > @@ -2438,9 +2439,7 @@ ixgbe_tx_ctx_setup(struct tx_ring *txr, > > > > struct ether_header *eh; > > > > #endif > > > > struct ip *ip; > > > > -#ifdef notyet > > > > struct ip6_hdr *ip6; > > > > -#endif > > > > struct mbuf *m; > > > > int ipoff; > > > > uint32_t vlan_macip_lens = 0, type_tucmd_mlhl = 0; > > > > @@ -2521,19 +2520,16 @@ ixgbe_tx_ctx_setup(struct tx_ring *txr, > > > > ipproto = ip->ip_p; > > > > type_tucmd_mlhl |= IXGBE_ADVTXD_TUCMD_IPV4; > > > > break; > > > > -#ifdef notyet > > > > case ETHERTYPE_IPV6: > > > > if (mp->m_pkthdr.len < ehdrlen + sizeof(*ip6)) > > > > return (-1); > > > > m = m_getptr(mp, ehdrlen, &ipoff); > > > > KASSERT(m != NULL && m->m_len - ipoff >= sizeof(*ip6)); > > > > - ip6 = (struct ip6 *)(m->m_data + ipoff); > > > > + ip6 = (struct ip6_hdr *)(m->m_data + ipoff); > > > > ip_hlen = sizeof(*ip6); > > > > - /* XXX-BZ this will go badly in case of ext hdrs. */ > > > > ipproto = ip6->ip6_nxt; > > > > type_tucmd_mlhl |= IXGBE_ADVTXD_TUCMD_IPV6; > > > > break; > > > > -#endif > > > > default: > > > > offload = FALSE; > > > > break; > > > > Index: dev/pci/ixgbe.h > > > > === > > > > RCS file: /mount/openbsd/cvs/src/sys/dev/pci/ixgbe.h,v > > > > retrieving revision 1.32 > > > > diff -u -p -r1.32 ixgbe.h > > > > --- dev/pci/ixgbe.h 18 Jul 2020 07:18:22 - 1.32 > > > > +++ dev/pci/ixgbe.h 12 Jan 2022 14:57:13 - > > > > @@ -65,6 +65,7 @@ > > > > #include > > > > #include > > > > #include > > > > +#include > > > > > > > > #if NBPFILTER > 0 > > > > #include > > > > > > > > > > > > > >
Re: ix(4): enable TCPv6/UDPv6 cksum offloading
> Date: Wed, 12 Jan 2022 17:45:57 +0100 > From: Jan Klemkow > > On Wed, Jan 12, 2022 at 05:36:01PM +0100, Mark Kettenis wrote: > > > Date: Wed, 12 Jan 2022 17:02:03 +0100 > > > From: Jan Klemkow > > > > > > Hi, > > > > > > This diff enables TCP and UDP checksum offloading in ix(4) for IPv6. > > > > > > IPv6 extension headers aren't a problem in this case. > > > in6_proto_cksum_out() in netinet6/ip6_output.c disables checksum > > > offloading if ip6_nxt is not TCP or UDP. Thus, we can just use this > > > field. > > > > > > Tested with: > > > ix0 at pci5 dev 0 function 0 "Intel 82599" rev 0x01, msix, 8 queues, > > > address 00:1b:21:94:4c:48 > > > > > > OK? > > > > Isn't this the same disaster as the ixl(4) diff you sent earlier? We > > have sparc64 machines with onboard ix(4)... > > Yes, but we don't parse the TCP header here. As bluhm@ figured out: > The access to ip_hl does not generate an alignment problem on sparc64. > Because, the bits of ip_hl are on the other of the byte, as bits of > th_off. > > This diff just touches the IPv6 case, where we don't have this kind of > problem, anyway. But you're still using m_getptr(), casting the result to a struct and then look at a member of the struct, which may access data beyond the end of the mbuf. > > > Index: dev/pci/if_ix.c > > > === > > > RCS file: /mount/openbsd/cvs/src/sys/dev/pci/if_ix.c,v > > > retrieving revision 1.180 > > > diff -u -p -r1.180 if_ix.c > > > --- dev/pci/if_ix.c 27 Jul 2021 01:44:55 - 1.180 > > > +++ dev/pci/if_ix.c 12 Jan 2022 14:53:14 - > > > @@ -1879,7 +1879,8 @@ ixgbe_setup_interface(struct ix_softc *s > > > ifp->if_capabilities |= IFCAP_VLAN_HWTAGGING; > > > #endif > > > > > > - ifp->if_capabilities |= IFCAP_CSUM_TCPv4 | IFCAP_CSUM_UDPv4; > > > + ifp->if_capabilities |= IFCAP_CSUM_TCPv4 | IFCAP_CSUM_UDPv4 > > > + | IFCAP_CSUM_TCPv6 | IFCAP_CSUM_UDPv6; > > > > > > /* > > >* Specify the media types supported by this sc and register > > > @@ -2438,9 +2439,7 @@ ixgbe_tx_ctx_setup(struct tx_ring *txr, > > > struct ether_header *eh; > > > #endif > > > struct ip *ip; > > > -#ifdef notyet > > > struct ip6_hdr *ip6; > > > -#endif > > > struct mbuf *m; > > > int ipoff; > > > uint32_t vlan_macip_lens = 0, type_tucmd_mlhl = 0; > > > @@ -2521,19 +2520,16 @@ ixgbe_tx_ctx_setup(struct tx_ring *txr, > > > ipproto = ip->ip_p; > > > type_tucmd_mlhl |= IXGBE_ADVTXD_TUCMD_IPV4; > > > break; > > > -#ifdef notyet > > > case ETHERTYPE_IPV6: > > > if (mp->m_pkthdr.len < ehdrlen + sizeof(*ip6)) > > > return (-1); > > > m = m_getptr(mp, ehdrlen, &ipoff); > > > KASSERT(m != NULL && m->m_len - ipoff >= sizeof(*ip6)); > > > - ip6 = (struct ip6 *)(m->m_data + ipoff); > > > + ip6 = (struct ip6_hdr *)(m->m_data + ipoff); > > > ip_hlen = sizeof(*ip6); > > > - /* XXX-BZ this will go badly in case of ext hdrs. */ > > > ipproto = ip6->ip6_nxt; > > > type_tucmd_mlhl |= IXGBE_ADVTXD_TUCMD_IPV6; > > > break; > > > -#endif > > > default: > > > offload = FALSE; > > > break; > > > Index: dev/pci/ixgbe.h > > > === > > > RCS file: /mount/openbsd/cvs/src/sys/dev/pci/ixgbe.h,v > > > retrieving revision 1.32 > > > diff -u -p -r1.32 ixgbe.h > > > --- dev/pci/ixgbe.h 18 Jul 2020 07:18:22 - 1.32 > > > +++ dev/pci/ixgbe.h 12 Jan 2022 14:57:13 - > > > @@ -65,6 +65,7 @@ > > > #include > > > #include > > > #include > > > +#include > > > > > > #if NBPFILTER > 0 > > > #include > > > > > > > > >
Re: ix(4): enable TCPv6/UDPv6 cksum offloading
> Yes, but we don't parse the TCP header here. As bluhm@ figured out: > The access to ip_hl does not generate an alignment problem on sparc64. > Because, the bits of ip_hl are on the other of the byte, as bits of > th_off. I think this is pure luck, and suspect the same problem will happen again in the future. I would be far happier if the compiler bug was fixed. bitfields are weakly standardized, but the compiler is breaking defacto expectations.
Re: ix(4): enable TCPv6/UDPv6 cksum offloading
> Date: Wed, 12 Jan 2022 17:02:03 +0100 > From: Jan Klemkow > > Hi, > > This diff enables TCP and UDP checksum offloading in ix(4) for IPv6. > > IPv6 extension headers aren't a problem in this case. > in6_proto_cksum_out() in netinet6/ip6_output.c disables checksum > offloading if ip6_nxt is not TCP or UDP. Thus, we can just use this > field. > > Tested with: > ix0 at pci5 dev 0 function 0 "Intel 82599" rev 0x01, msix, 8 queues, address > 00:1b:21:94:4c:48 > > OK? Isn't this the same disaster as the ixl(4) diff you sent earlier? We have sparc64 machines with onboard ix(4)... > Index: dev/pci/if_ix.c > === > RCS file: /mount/openbsd/cvs/src/sys/dev/pci/if_ix.c,v > retrieving revision 1.180 > diff -u -p -r1.180 if_ix.c > --- dev/pci/if_ix.c 27 Jul 2021 01:44:55 - 1.180 > +++ dev/pci/if_ix.c 12 Jan 2022 14:53:14 - > @@ -1879,7 +1879,8 @@ ixgbe_setup_interface(struct ix_softc *s > ifp->if_capabilities |= IFCAP_VLAN_HWTAGGING; > #endif > > - ifp->if_capabilities |= IFCAP_CSUM_TCPv4 | IFCAP_CSUM_UDPv4; > + ifp->if_capabilities |= IFCAP_CSUM_TCPv4 | IFCAP_CSUM_UDPv4 > + | IFCAP_CSUM_TCPv6 | IFCAP_CSUM_UDPv6; > > /* >* Specify the media types supported by this sc and register > @@ -2438,9 +2439,7 @@ ixgbe_tx_ctx_setup(struct tx_ring *txr, > struct ether_header *eh; > #endif > struct ip *ip; > -#ifdef notyet > struct ip6_hdr *ip6; > -#endif > struct mbuf *m; > int ipoff; > uint32_t vlan_macip_lens = 0, type_tucmd_mlhl = 0; > @@ -2521,19 +2520,16 @@ ixgbe_tx_ctx_setup(struct tx_ring *txr, > ipproto = ip->ip_p; > type_tucmd_mlhl |= IXGBE_ADVTXD_TUCMD_IPV4; > break; > -#ifdef notyet > case ETHERTYPE_IPV6: > if (mp->m_pkthdr.len < ehdrlen + sizeof(*ip6)) > return (-1); > m = m_getptr(mp, ehdrlen, &ipoff); > KASSERT(m != NULL && m->m_len - ipoff >= sizeof(*ip6)); > - ip6 = (struct ip6 *)(m->m_data + ipoff); > + ip6 = (struct ip6_hdr *)(m->m_data + ipoff); > ip_hlen = sizeof(*ip6); > - /* XXX-BZ this will go badly in case of ext hdrs. */ > ipproto = ip6->ip6_nxt; > type_tucmd_mlhl |= IXGBE_ADVTXD_TUCMD_IPV6; > break; > -#endif > default: > offload = FALSE; > break; > Index: dev/pci/ixgbe.h > === > RCS file: /mount/openbsd/cvs/src/sys/dev/pci/ixgbe.h,v > retrieving revision 1.32 > diff -u -p -r1.32 ixgbe.h > --- dev/pci/ixgbe.h 18 Jul 2020 07:18:22 - 1.32 > +++ dev/pci/ixgbe.h 12 Jan 2022 14:57:13 - > @@ -65,6 +65,7 @@ > #include > #include > #include > +#include > > #if NBPFILTER > 0 > #include > >
Re: uninitialized stack memory possibly passed to m_freem
On Wed, Jan 12, 2022 at 11:30:44AM +0100, Moritz Buhl wrote:
> Hi tech@,
>
> https://github.com/openbsd/src/commit/0ea6bae06233cd25645df14602c3eda6bdff7dca.patch
>
> the patch forgot to add mrep to the info struct, nfsm_dissect could
> pass info.nmi_mrep to m_freem, which is currently uninitialized
> stack memory.
>
> Index: sys/nfs/nfs_subs.c
> ===
> RCS file: /mount/openbsd/cvs/src/sys/nfs/nfs_subs.c,v
> retrieving revision 1.145
> diff -u -p -r1.145 nfs_subs.c
> --- sys/nfs/nfs_subs.c11 Jan 2022 03:13:59 - 1.145
> +++ sys/nfs/nfs_subs.c12 Jan 2022 09:31:52 -
> @@ -1841,6 +1841,7 @@ nfsm_srvsattr(struct mbuf **mp, struct v
>
> info.nmi_md = *mp;
> info.nmi_dpos = *dposp;
> + info.nmi_mrep = mrep;
>
> nfsm_dissect(tl, u_int32_t *, NFSX_UNSIGNED);
> if (*tl == nfs_true) {
>
OK stsp@ (without assuming any responsibility for NFS)
uninitialized stack memory possibly passed to m_freem
Hi tech@,
https://github.com/openbsd/src/commit/0ea6bae06233cd25645df14602c3eda6bdff7dca.patch
the patch forgot to add mrep to the info struct, nfsm_dissect could
pass info.nmi_mrep to m_freem, which is currently uninitialized
stack memory.
Index: sys/nfs/nfs_subs.c
===
RCS file: /mount/openbsd/cvs/src/sys/nfs/nfs_subs.c,v
retrieving revision 1.145
diff -u -p -r1.145 nfs_subs.c
--- sys/nfs/nfs_subs.c 11 Jan 2022 03:13:59 - 1.145
+++ sys/nfs/nfs_subs.c 12 Jan 2022 09:31:52 -
@@ -1841,6 +1841,7 @@ nfsm_srvsattr(struct mbuf **mp, struct v
info.nmi_md = *mp;
info.nmi_dpos = *dposp;
+ info.nmi_mrep = mrep;
nfsm_dissect(tl, u_int32_t *, NFSX_UNSIGNED);
if (*tl == nfs_true) {
Re: new: lang/polyml
On 2022/01/11 23:06, Philip Guenther wrote: > On Tue, Jan 11, 2022 at 4:09 PM Daniel Dickman wrote: > > > On Mon, Jan 10, 2022 at 8:12 PM Leo Larnack wrote: > > > > > > i386 > > > > > > > > > > > with this diff I was able to install includes, rebuild ld.so and > > ctfconv. I've not managed to build a release yet. > > ... > > Umm, with what diff? There was no diff in nor attached to that message. > :-/ https://marc.info/?l=openbsd-tech&m=164186375804139&w=2 > (That was a lot of lines of output. I don't know about ports@, but my > expectation would be there would be *zero* reviewers of anything before, > say, the last 50 lines of output before the switch to actual compilation. > Standard "make lots of noise so when a failure occurs we can see the > leadup, but we'll ignore it otherwise" style of output, like a base build. > You read the lead up to the warnings and errors only. ) The output from configure scripts and previous build stages is often quite important when figuring out a build problem (especially if something works on some machines and fails on others). To my eyes the key bit is probably Created structure Motif **Writing object code** Segmentation fault (core dumped) *** Error 139 in . (Makefile:1158 'polyexport.o') but somebody else might notice something different
