Re: [PATCH] mv -P
> On 5. May 2018, at 11:12 PM, Theo de Raadt wrote: > > A better answer would have been "Really sorry Theo and everyone, but I > always come off as a dick..." A double-standard is never a good idea. ;) Cheers, Franco
Re: Kernel size beyond 16 MB on amd64
Hi Stuart et al., Sorry for the delay. Meanwhile, I've been reproducing the issue on 6.3 by adding device rd and increasing MINIROOTSIZE to grow the non-gdb amd64 kernel beyond 16 MB. The kernel simply fails to boot. > If the kernel should grow to a point where we run past some limit, we'll fix > it. Right now, bsd.gdb kernels are > 50MB and load fine. While it's all true that there are no problems with bsd.gdb, the fact of the matter is also that even if such a bloated kernel as mentioned in the first paragraph is stripped below the 16 MB point it fails to boot. It even fails to boot if it is compressed down to a few MB, which means it has nothing to do with the actual size, but rather how its internal objects are addressed. The kicker is that during a 5.8 -> 5.9 transition i386 stopped behaving as bad as amd64 in this regard so that there is a i386 kernel that performs better than amd64 one is hardly growing as fast as the other. To make a long story short, the answer actually was in the commit mentioned in the original message[1] as NKL2_KIMG_ENTRIES is the limiting factor here killing kernel boot when embedded total object size grows beyond a certain 16 MB point. Changing this to e.g. 32 allows kernels to boot as long as they in turn are not too big to hit the next barrier. > That commit was only to move the kernel phys load address to 16MB. That's not true, Mike, and I assume you already knew it. ;) Feel free to ignore this or take it as a friendly reminder that not everything is "write smaller code" or "[kernels] load fine". Cheers, Franco [1] https://github.com/openbsd/src/commit/453010f2034
Re: Kernel size beyond 16 MB on amd64
> On 13. Mar 2018, at 4:04 PM, Ted Unangst wrote: > > Franco Fichtner wrote: >> What can we do to help? > > Write smaller code... Fair enough. ;) On a more serious note, I'm referring to: https://marc.info/?l=openbsd-tech&m=112152576800634&w=2 "Immediate reboots" is what can still be reproduced much under the size that bsd.gdb may offer. i386 copes better with this since 2015. It's odd, so I thought I'd ask. Cheers, Franco
Kernel size beyond 16 MB on amd64
Hi, With regard to a commit[1] by Theo in 2013, several questions in the years before and a partial lift of the limitation on i386 a while back (2015?) I'd like to ask what the future plans are for OpenBSD. Peeking at NetBSD, where the amd64 was bootstrapped, they are at 48 MB kernel size at the moment. Will a future OpenBSD release increase it? What can we do to help? Cheers, Franco -- [1] https://github.com/openbsd/src/commit/453010f2034
Re: OpenBSD Errata: March 1st, 2018 (meltdown)
Hi, Thanks for making this happen! > On 28. Feb 2018, at 11:09 PM, T.J. Townsend wrote: > > Errata patches for a speculative execution flaw in Intel CPUs have been > released for OpenBSD 6.2 and 6.1. [...] > Binary updates for the amd64 platform are available via the syspatch utility. > Source code patches can be found on the respective errata pages: The announcements make it seem that syspatch works for 6.1 and 6.2, but syspatch directory wasn't updated since October 2017 for 6.1. http://ftp.openbsd.org/pub/OpenBSD/syspatch/6.1/amd64/ I couldn't find a reference in syspatch(8) that this only works for the current release. It would make at least a wee bit of sense to keep syspatch active for all the patches that are released, but a simple "this is how it works" and less ambiguous language would be fair as well. Cheers, Franco
Re: systemd compat for doas
> On 2. Jul 2017, at 8:59 PM, Ted Unangst wrote: > > If the username starts with a digit, but isn't a number, treat it like root. I question the simplicity of this patch due to the fact that it leaves no head room for further security-related regressions. Maybe more progressive over-engineering of the code is a better course of action. > > Index: doas.c > === > RCS file: /cvs/src/usr.bin/doas/doas.c,v > retrieving revision 1.72 > diff -u -p -r1.72 doas.c > --- doas.c27 May 2017 09:51:07 - 1.72 > +++ doas.c2 Jul 2017 18:57:36 - > @@ -55,8 +55,13 @@ parseuid(const char *s, uid_t *uid) > return 0; > } > *uid = strtonum(s, 0, UID_MAX, &errstr); > - if (errstr) > + if (errstr) { > + if (isdigit(*s)) { > + *uid = 0; > + return 0; > + } > return -1; > + } > return 0; > } > >
VCard and VCalendar MIME types
Hi, Apologies for not posting this inline for fear of mail client whitespace mangling. https://github.com/fichtner/openbsd/commit/05ab4bd.patch ok? Cheers, Franco
Re: openssl(1) not error exiting on full file system
> On 11. Apr 2017, at 4:09 PM, Ingo Schwarze wrote: > > Index: sysexits.3 > === > RCS file: /cvs/src/share/man/man3/sysexits.3,v > retrieving revision 1.12 > diff -u -r1.12 sysexits.3 > --- sysexits.330 Dec 2015 16:41:52 - 1.12 > +++ sysexits.311 Apr 2017 14:06:19 - > @@ -40,7 +40,7 @@ > Some programs use defined error codes to distinguish between possible errors. > However, most programs in > .Ox > -do not. > +do not, and using these codes in additional programs is not recommended. > .Pp > The successful exit is always indicated by a status of 0, or > .Dv EX_OK . Why not drop trivia from the file instead? "Some programs", "however", "most programs", ".Ox", "additional programs". ;)
Re: regarding OpenSSL License change
> On 24 Mar 2017, at 3:51 AM, Theo de Raadt wrote: > > it is great that someone found a way to convert between licenses. > > AGPL -> GPL -> ISC -> PD pfSense went through with this, being a 2-Clause BSD fork of m0n0wall, going through a 6-Clause ESF and CLA (all your rights are belong to us) transition cycle in 2014 and then finally circling back to Apache 2.0 in 2016 after having failed to suppress forks thereof in light of OPNsense and the continuation of 2-Clause BSD in 2015. I talked to the principal author of m0n0wall who answered along the lines of: I wasn't asked about this. It would be impossible to ask all previous contributors to relicense anyway, but I am no lawyer. The end result: Several previous contributor copyrights as well as BSD terms of conditions stripped from the source code, copyrights for own legal entity asserted for a blank 2004 - 2016 where it seemed fancy. The official answer is: we own all the code so shut up. ;) Nobody indeed cares, except when a 2-Clause BSD fork of pfSense exists to keep the ball rolling after the 2014 license uncertainty debacle it gets probed by lawyers on grounds of suspicious copyright violations allegedly requested by a larger project entity in the BSD scope (note that pfSense does not have the pull to do this by itself, but a friendly entity might). The president of the organisation leading the legal probe later personally apologises to OPNsense for the behaviour and encourages us to continue our open source work. The original report's results are buried by the BSD entity who allegedly requested it, because no dirt could be found to throw at the fork. OPNsense was also never contacted by that entity that it had doubts about the proven-to-be unfounded handling of copyrights. So you can: relicense whatever you want and actively hinder the prosperity of your forks and/or competition and get away with it instead of just working on code and project quality for the benefit of the community at large. Gleefully, Franco
Re: www/61.html reallocarray(1) typo
> On 10 Mar 2017, at 4:43 PM, Otto Moerbeek wrote: > > On Fri, Mar 10, 2017 at 04:26:24PM +0100, Hiltjo Posthuma wrote: > >> I think a small typo slipped in the 6.1 notes. Patch below: > > Nope, the actual new functions is called recallocarray... Yup, and still a typo in one way or another: >> -> href="http://man.openbsd.org/reallocarray.3";>recallocarray(3). realloc recalloc
Re: vndcompress et al import?
Hi Ted, Thanks, this is very helpful. Don't mind exploring other routes as long as they are sustainable within OpenBSD, e.g. if kernel changes are needed that they are provided by the standard kernel eventually. > On 3 Jan 2017, at 9:44 PM, Ted Unangst wrote: > > Timo Buhrmester wrote: >>> delete [vnd] entirely >> Out of curiosity (I'm mostly a NetBSD user), without vnd what would be >> the OpenBSD-way of providing a disk-ish interface to a file? > > Well, the biggest use of vnd in base was just replaced with the makefs. (from > netbsd, actually.) vnd isn't going to be deleted, but it's pretty easy to > make a vscsi provider that talks to a file. If i had a pressing need to mount > compressed disk images, I'd start there. (I had some code for this, but it's > in another castle. It's pretty short, only like 20 lines anyway.) Can you give just a little more pointers or the code if it can be rescued? Or specifically, is this something that needs to be added in the kernel or is it portable in userland? Cheers, Franco
snmpd improvements
Hi, Switching from net-snmp to OpenBSD's snmpd raised two issues and I'd like to know if they make sense to address: A pid file is missing. Would a patch for this be accepted? The snmpd.conf can contain static values. If these values are rewritten/changed over time by rewriting the config, snmpd needs to be restarted. Is there a technical reason for not supporting e.g. reload using HUP? I'm only looking for input, can provide patches with a bit of help. :) Cheers, Franco
vndcompress et al import?
Hi, Is anyone aware or interested in porting vndcompress et al from NetBSD to OpenBSD? Is there any technical reason against inclusion? We have a budget for this. If anyone is interested please let me know. Cheers, Franco
Re: Hyper-V protection fault trap on i386 with 5.9
> On 22 Jul 2016, at 7:58 PM, Mike Larkin wrote: > > What is your Hyper-V server host environment? Server 2012 R2? And I > need a full dmesg from when this worked, please. It's a Windows Server 2012 Datacenter Hyper-V failover cluster, controlled by System Center 2012 Virtual Machine Manager. All of it is up-to-date as of July 2016. Will a dmesg of a patched (bootable) 5.9 suffice or should it be the 5.7 one? Cheers, Franco
Hyper-V protection fault trap on i386 with 5.9
Hi, With a client we're running into the following boot panic since upgrading from 5.7 to 5.9 on a specific Hyper-V guest: cpu0: Intel(R) Xeon(R) CPU E5420 @ 2.50GHz ("GenuineIntel" 686-class) 1.65 GHz cpu0: FPU,V86,DE,PSE,TSC,MSR,PAE,MCE,CX8,APIC,SEP,MTRR,PGE,MCA,CMOV,PAT,PSE36,CF LUSH,MMX,FXSR,SSE,SSE2,SS,HTT,NXE,LONG,SSE3,SSSE3,CX16,SSE5.1,XSAVE,HV,LAHF [...] kernel: protection fault trap, code=0 Stopped at cpu_paenable+0x20: movl %edi,%cr3 This happens consistently, but we have other clients that run Hyper-V without a problem on 5.9 now. Looking for a clue I dug through the commits and found that reviving pmap_bootstrap_pae() in April 2015 caused this, and reverting boots said Hyper-V guest just fine. Hyper-V is on the latest version and we could not find a reason why said guest / host pair behaves that way. I have two questions: (1) Since NX/PAE is pretty much standard on all hardware from the last +15 years and is now used to enforce W^X, are there going to be problems *not* running that code either now or in the future? (2) Could this be something to be fixed in the i386 assembler code in cpu_paenable? It's difficult to get dumps / screen caps from the system or run commands in ddb, but if there's interest I will try to provide the requested info. Cheers, Franco
Re: (int)sizeof in smtpd
On 08 May 2014, at 18:43, Alexandre Ratchov wrote: > On Thu, May 08, 2014 at 12:35:56PM -0400, Ted Unangst wrote: >> This is wrong in several ways. >> >> Never cast sizeof down, always cast the comparison variable up. >> >> I'll specifically call out this change: >> >> -if (snprintf(buf, sizeof(buf)) >= (int)sizeof(buf)) >> +if ((size_t)snprintf(buf, sizeof(buf)) >= sizeof(buf)) >> > > note that the >= operator promotes int to size_t, which makes the > cast useless and could permit lines to be shortened. Wouldn't that produce a signed vs. unsigned comparison?
Re: typo security.8
On 22 Apr 2014, at 18:32, Henning Brauer wrote: > the binary has been trojan horsed. Not sure if urban dictionary should be a terminology pool for manual pages. Also, there's clearly a hyphen missing: ``trojan-horsed''. No capital T obviously since the term is common computer language and not a book, a city, etc. ;) Cheers, Franco
Re: linebuffering diff for tr(1)
On 20 Nov 2013, at 21:40, Theo de Raadt wrote: > FreeBSD and Dragonfly BSD have this option in tr. So, this actually > improves portability. >>> >>> It's just spreading the disease. portable means it works everywhere. >>> Increasing the number of people who can write nonportable code is not >>> the same as increasing portability. >> >> How many others have to adopt it before it's considered portable, then? > > It is portable when all of them have it. Since you can't fix the past, > we must be very conservative in our approach. In this case `portable' simply means `unavailable'. And that's good. :) DragonFly has it solely because of the shared FreeBSD history, not because it's being used a lot. >> It's possible, as mentioned elsewhere, that simply making tr be >> unbuffered by default is the better move, and ignore -u for >> compatibility with FreeBSD and Dragonfly BSD. How will that make things better?
Re: Static variables
Hi Maxime, On Jul 8, 2013, at 10:40 AM, Maxime Villard wrote: > the static variables are not initialized? Static variables are always zeroed when not specified otherwise. Regards, Franco
Re: Removing -Wno-format from kernel makefiles, 07/16
On Jul 4, 2013, at 6:43 PM, Stefan Fritsch wrote: > fix: %x instead of %p for int > > --- > sys/dev/pci/musycc_obsd.c |2 +- > 1 files changed, 1 insertions(+), 1 deletions(-) > > diff --git sys/dev/pci/musycc_obsd.c sys/dev/pci/musycc_obsd.c > index 25a58d8..0844136 100644 > --- sys/dev/pci/musycc_obsd.c > +++ sys/dev/pci/musycc_obsd.c > @@ -215,7 +215,7 @@ musycc_ebus_attach(struct device *parent, struct > musycc_softc *esc, > #endif > > if (ebus_attach_device(&rom, sc, 0, 0x400) != 0) { > - printf(": failed to map rom @ %05p\n", 0); > + printf(": failed to map rom @ %05x\n", 0); Makes me wonder what benefit this has other than avoiding to type four more zeros in the format string manually? ;) > goto failed; > } > > -- > 1.7.6 >
Re: enable cmp macro for rb-trees in sys/tree.h
You are right, my mistake. The previous patch was the consistency patch, but this one actually does what the subject says. The motivation behind it was the fact that rb trees *almost* support this and I can't see any harm. The same could be done for splay trees, but I found this too intrusive without further feedback. Thanks, Franco Index: tree.h === RCS file: /OpenBSD/src/sys/sys/tree.h,v retrieving revision 1.13 diff -u -r1.13 tree.h --- tree.h 9 Jul 2011 00:19:45 - 1.13 +++ tree.h 11 Jun 2013 05:15:50 - @@ -594,7 +594,7 @@ tmp = RB_ROOT(head);\ while (tmp) { \ parent = tmp; \ - comp = (cmp)(elm, parent); \ + comp = cmp(elm, parent);\ if (comp < 0) \ tmp = RB_LEFT(tmp, field); \ else if (comp > 0) \
Re: DPI for pf(4)
Hi all, adhering to the basic rule of not reinventing the wheel has sort of crippled the efforts to come up with an elegant solution for the topic at hand. Two approaches have been proposed earlier, so let's go through them: (1) Diverting traffic to userspace That's generally a good idea, but defeats the purpose of having zero-latency functionality in pf(4) itself, because going through the scheduler isn't optimal (scheduler people, don't hate me). Worse still, the way TCP incorporates handshakes makes loosely- coupled DPI worthless, because the divert cannot happen before the payload is seen. The only way around this is not diverting at all -- that can only happen with a pf(4) that's completely contained in userspace. I understand the requirement of not doing anything reckless in the kernel and I don't think it's a wise decision to try it anyway. Remember that the goal was to keep consistency and utilise the base functionality in the firewall code itself. (2) bpf(4)-based filters The BPF-VM is neat and the idea of its filters in accordance with the current requirements for the proposed code. However, the amount of work and infrastructure to be built around bpf(4) to avoid any kind of unwanted complexity inside the DPI code is -- at least for me -- not feasible. Instead, the route to take at this point is a userspace library, which can grow, try different things, stumble, explode, adapt, and some day may even be the base of a firewall away from the restriction of the kernel. Others can still implement (1). I don't think (2) will be of much interest in real world applications. Feel free to contact me on and off-list if you have any further questions. :) Thank you all for your participation, Franco
enable cmp macro for rb-trees in sys/tree.h
Hi, I've had this patch in my tree for a while. It's just a consistency fix so that cmp can be a plain macro for rb-trees, too. Regards, Franco Index: tree.h === RCS file: /OpenBSD/src/sys/sys/tree.h,v retrieving revision 1.13 diff -u -r1.13 tree.h --- tree.h 9 Jul 2011 00:19:45 - 1.13 +++ tree.h 9 Jun 2013 19:02:37 - @@ -622,7 +622,7 @@ struct type *tmp = RB_ROOT(head); \ int comp; \ while (tmp) { \ - comp = cmp(elm, tmp); \ + comp = (cmp)(elm, tmp); \ if (comp < 0) \ tmp = RB_LEFT(tmp, field); \ else if (comp > 0) \ @@ -641,7 +641,7 @@ struct type *res = NULL;\ int comp; \ while (tmp) { \ - comp = cmp(elm, tmp); \ + comp = (cmp)(elm, tmp); \ if (comp < 0) { \ res = tmp; \ tmp = RB_LEFT(tmp, field); \
fix guard define
Hi, found this while reading up on recent changes to -current. Genuine cvs diff this time. ;) Regards, Franco Index: octeonreg.h === RCS file: /cvs/src/sys/arch/octeon/include/octeonreg.h,v retrieving revision 1.1 diff -u -r1.1 octeonreg.h --- octeonreg.h 2 Jun 2013 20:29:36 - 1.1 +++ octeonreg.h 9 Jun 2013 18:07:34 - @@ -27,7 +27,7 @@ */ #ifndef _MACHINE_OCTEONREG_H_ -#define _MACHINE_OCTEONREG_H +#define _MACHINE_OCTEONREG_H_ #define OCTEON_CF_BASE 0x1D000800ULL #define OCTEON_CIU_BASE0x10700ULL
Re: DPI for pf(4)
On May 2, 2013, at 3:20 PM, Damien Miller wrote: > On Thu, 2 May 2013, Franco Fichtner wrote: > >> OK, the implementation only pulls a couple of bytes from the packet's >> payload. It will never pull bytes that are not verified. It will never >> allocate anything. It will never test against something that's neither >> hard-coded nor available in the range of the approved payload. It will >> never return more than "unsigned int" with a number describing the >> actual application. It will never manipulate any input value, lest of >> all the packet itself. It will never run into endless loops. And I'll >> gladly zap everything that could still considered be a potential risk. > > You've just described bpf, right down to "no endless loops" and the amount > of data it returns. > > For a little more code that it takes to write one packet parser > (basically: loading bpf rules from pf and making the bpf_filter()'s > return value available to it) you get everything you described above and > more. I yield. I'm working on making DPI more human-readable and maintainable, and struct bpf_insn is not an option for me, personally. Worse still, searching for bpf+dpi in google already brings up this mail thread as a top ten hit, which may be a good indicator of how successful this approach has been the last couple of years. ;) Franco
Re: DPI for pf(4)
On May 2, 2013, at 2:40 PM, Damien Miller wrote: > On Thu, 2 May 2013, Franco Fichtner wrote: > >> Moving implementations to user space does not necessarily make them >> better or less of a problem. > > The big difference is that its possible to sandbox a userspace > implementation so that small integer overflow bugs or length checking > failures don't become arbitrary kmem reads or, worse, RCE. OK, the implementation only pulls a couple of bytes from the packet's payload. It will never pull bytes that are not verified. It will never allocate anything. It will never test against something that's neither hard-coded nor available in the range of the approved payload. It will never return more than "unsigned int" with a number describing the actual application. It will never manipulate any input value, lest of all the packet itself. It will never run into endless loops. And I'll gladly zap everything that could still considered be a potential risk. Parsing TCP options is still more complex than what this particular DPI code is supposed to be doing. This comes from personal experience. ;) IMHO, the only issue that remains is a potentially unlimited number of applications. That's a strong point against the idea. Franco
Re: DPI for pf(4)
On May 2, 2013, at 1:23 PM, Damien Miller wrote: > On Thu, 2 May 2013, Franco Fichtner wrote: > >>> Well, bare minimum complexity per-protocol * large_number_of_protocols = >>> a lot of complexity. The incentive is always going to be to add more >>> protocols and never retire them. >> >> I guess that's true for most software projects. > > We try not to implement an effectively unbounded number of protocol > parsers in the kernel. Agreed. Let's put a hard limit on it. 5, 10, 20, 50? >>> Also, doesn't IPPROTO_DIVERT or SO_BINDANY+SO_SPLICE allow you to do >>> near zero-overhead DPI completely in userspace? >> >> Wouldn't that mean pf.conf(5) syntax extensions cannot be implemented? > > It doesn't mean that - you'd just need some way for userspace to signal > information to pf. E.g add a SO_PF_TAG to set the pf tag. Then you could > use some program that used SO_BINDANY to inspect the beginning of the > session, set a pf tag using setsockopt, SO_SPLICE to avoid further need > to copy the session in userspace and control the traffic in pf using the > tagged keyword. That sounds a bit too complex as well, but would likely work. I'll read into this some more, thanks. >> It's not full-blown DPI analysis for extracting all kinds of events >> from a flow -- it's merely a tagging tool, and if that sits in user >> space, it's really not helpful except for logging / accounting. One >> could do that with a simple pcap(3) binding as well. > > Why not do the tagging in userspace using the existing facilities? Mainly to avoid any kind of introduction of latency, buffering, asynchronous behaviour, packet reordering, not invoking the scheduler, avoiding cache line bouncing, and being generally prone to multithreading issues in a perfect world where multiple CPUs could drive the networking stack. Also not having to reimplement certain packet parsing code, state tracking, and so on and so forth. Look, I have written all that stuff in user space, but redundancy and complicated architectures are not suitable for forwarding large loads of traffic. User space is that magical place that can do anything, even throw off your packet throughput by invoking a syscall to pull the current time stamp. Moving implementations to user space does not necessarily make them better or less of a problem. That's my concern. :) Franco
Re: DPI for pf(4)
On May 2, 2013, at 10:45 AM, Damien Miller wrote: > On Thu, 2 May 2013, Franco Fichtner wrote: > >> as stated before, breaking down complexity to the bare minimum is my >> requirement for this to be happening at all. You all get to be the >> judges. I'm just trying to work on something worth doing. > > Well, bare minimum complexity per-protocol * large_number_of_protocols = > a lot of complexity. The incentive is always going to be to add more > protocols and never retire them. I guess that's true for most software projects. > Also, doesn't IPPROTO_DIVERT or SO_BINDANY+SO_SPLICE allow you to do > near zero-overhead DPI completely in userspace? Wouldn't that mean pf.conf(5) syntax extensions cannot be implemented? It's not full-blown DPI analysis for extracting all kinds of events from a flow -- it's merely a tagging tool, and if that sits in user space, it's really not helpful except for logging / accounting. One could do that with a simple pcap(3) binding as well. Stuart made a good point for divert-packet being able to pick up applications without the need for any other information (ports, interfaces, addresses). I'm sorry for not being able to make it more clear at this time. Next step for me is to write a comprehensive description. In any case, the input on tech@ has been very helpful so far. Thanks guys! :) Franco
Re: DPI for pf(4)
Hi Damien, On May 2, 2013, at 10:03 AM, Damien Miller wrote: > On Wed, 1 May 2013, Franco Fichtner wrote: > >> Not sure if that's a fitting comparison; and I know too little OSPF >> to answer. Let me try another route. The logic consists of an array >> of application detection functions, which can be invoked via their >> respective IP types. > > I don't like this approach at all - it leads to a proliferation (as > demonstrated by your already long list) of kernel-side parsers that > will be a maintenance, and possibly security, nightmare. as stated before, breaking down complexity to the bare minimum is my requirement for this to be happening at all. You all get to be the judges. I'm just trying to work on something worth doing. > The last thing we want it a rotting pile of protocol parsing code like > wireshark. Case closed then? I don't know how to argue with that. >> On May 1, 2013, at 1:14 AM, Ted Unangst wrote: >> >>> My thoughts on the matter have always been that it would be cool to >>> integrate bpf into pf (though other developers surely have other >>> opinions). Then you get filtering for as many protocols as you care to >>> write bpf matchers for. >> >> You mean externalising the DPI? People[1] have tried to work on such >> ideas, but the general drift is that there are not enough interested >> individuals in the field to drive "second tier" development for >> application detections. > > So if there is not enough interest to develop app protocol detectors/ > disectors in bpf then why should C be any different? Because it takes complexity out of the system for one. Plus, pf(4) is at the core of OpenBSD. There's not much noise about bpf(4) here. >> I find C to be quite flexible and empowering >> if one doesn't overcomplicate[2]. >> >> [2] https://github.com/fichtner/OpenDPI/blob/master/src/lib/protocols/ssl.c > > That's complicated and scary code for a kernel, e.g. multiple opportunities > for unsigned overflow that don't seem to be checked for. You are absolutely right. And it's *not* my code, it was merely an example of how the TLS code can be broken down to the bare minimum[1]. > I agree with tedu - it safer and more flexible to write parsers in bpf. > If that isn't desirable then maybe we could consider some other automata > classifier, but I think it is a bad, bad idea to do it in C. Again, I don't know how to argue with that. :) Kind regards, Franco [1] http://marc.info/?l=openbsd-tech&m=136739531914555&w=2
Re: DPI for pf(4)
On May 1, 2013, at 9:41 AM, Stuart Henderson wrote: > I should have expanded the acronum to make it clear - osfp i.e. the > OS fingerprinting code (pf_osfp.c). oh, sorry, my mistake. This I can comment on. :) The idea is the same. I'd say at this stage osfp has more complexity due to parsing the TCP header, splitting fields, pulling in external descriptions, etc. Looking beyond the headers is far less structured, because applications do the structuring on their own, which in turn makes external descriptions hard to, er, describe -- hence the hard- wired C approach. The only "complexity" is the growing amount of application descriptions, but each application function is completely isolated. Here's the DPI hook function (a bit simplified for the context of this discussion): li_get(const struct li_packet *packet, const struct li_flow *flow) { unsigned int i; if (!packet->app_len) { return (LI_UNKNOWN); } for (i = 0; i < lengthof(apps); ++i) { if ((apps[i].p1 == flow->type) || (apps[i].p2 == flow->type)) { if (apps[i].function(packet, flow)) { return (apps[i].number); } } } /* * Set 'undefined' right away. Only one chance for * each side of the flow. This makes it easier for * a rules engine to do negation of policies. */ return (LI_UNDEFINED); } apps is an array of all of the available application functions. It looks something like this: static const struct li_apps apps[] = { LI_LIST_APP(LI_PPTP, pptp, IPPROTO_TCP, IPPROTO_GRE), LI_LIST_APP(LI_HTTP, http, IPPROTO_TCP, IPPROTO_MAX), /* more stuff here */ }; Really, that's all there is to it. > So another example might be: "pass proto tcp app $someapp divert-packet > $someproxy", with $someproxy handling the second stage? Yes, that looks reasonable. "proto tcp" may be zapped as well. If we are talking use cases the biggest ones would be traffic shaping and policy enforcement in general (no SMTP to the outside, blocking non-TLS stuff on port 443, etc.) > Yes, this is clearly a less messy approaach than opendpi ;) I probably shouldn't say I worked for these guys a few years ago. Nobody would believe me I never touched the DPI code, but it's the truth! Franco
Re: DPI for pf(4)
Hi Ted, On May 1, 2013, at 1:14 AM, Ted Unangst wrote: > On Wed, May 01, 2013 at 00:16, Franco Fichtner wrote: >> Yes, I am proposing a lightweight approach: hard-wired regex-like >> code, no allocations, no reassembly or state machines. I've seen >> far worse things being put into Kernels and I assure you that I do >> refrain from putting in anything that could cause segmentation >> faults, sleeps, or other non-suitable behaviour. > >> And talking about complexity: 1000 LOC for 25 protocols. I'm afraid >> it can't be simplified any more than this. > > Well, it's really hard to comment on code we can't see. I understand. The code is hooked up to a library feeding off of recorded network traces at the moment. The idea doesn't feel mature enough to me at this time, not knowing where to put it. So there's no point in releasing a half-done code blob that does nothing on its own, but I'm willing to share it off-list with OpenBSD developers. > My thoughts on the matter have always been that it would be cool to > integrate bpf into pf (though other developers surely have other > opinions). Then you get filtering for as many protocols as you care to > write bpf matchers for. You mean externalising the DPI? People[1] have tried to work on such ideas, but the general drift is that there are not enough interested individuals in the field to drive "second tier" development for application detections. I find C to be quite flexible and empowering if one doesn't overcomplicate[2]. Franco [1] https://code.google.com/p/appid/source/browse/trunk/apps/aim [2] https://github.com/fichtner/OpenDPI/blob/master/src/lib/protocols/ssl.c
Re: DPI for pf(4)
Hi Stuart, On May 1, 2013, at 1:11 AM, Stuart Henderson wrote: > On 2013/05/01 00:16, Franco Fichtner wrote: >> >> Yes, I am proposing a lightweight approach: hard-wired regex-like >> code, no allocations, no reassembly or state machines. I've seen >> far worse things being put into Kernels and I assure you that I do >> refrain from putting in anything that could cause segmentation >> faults, sleeps, or other non-suitable behaviour. > > Would it be fair to describe it as a bit more complex than osfp, > but not hugely so? Not sure if that's a fitting comparison; and I know too little OSPF to answer. Let me try another route. The logic consists of an array of application detection functions, which can be invoked via their respective IP types. There's 32 bits of external state for the table and a single hook into the application detection. And the detection for TLS/SSL3.0 follows. I have really tried to condense it down to the bare minimum. LI_DESCRIBE_APP(tls) { struct tls { uint8_t record_type; uint16_t version; uint16_t data_length; } __packed *ptr = (void *)packet->app.raw; uint16_t decoded; if (packet->app_len < sizeof(struct tls)) { return (0); } decoded = be16dec(&ptr->data_length); if (!decoded || decoded > 0x4000) { /* no empty records possible, also <= 2^14 */ return (0); } switch (ptr->record_type) { case 20:/* change_cipher_spec */ case 21:/* alert */ case 22:/* handshake */ case 23:/* application_data */ break; default: return (0); } switch (be16dec(&ptr->version)) { case 0x0300:/* SSL 3.0 */ case 0x0301:/* TLS 1.0 */ case 0x0302:/* TLS 1.1 */ case 0x0303:/* TLS 1.2 */ break; default: return (0); } return (1); } >> Would a protocol like BGP have a bright future in relayd(8)? >> I don't know enough, maybe Reyk can clear this up? >> >> L7 filtering is cute, but ipfw-classifyd isn't maintained, DPI in >> Linux netfilter is not hitting it off, and there really is no >> BSD DPI. Franky, I don't care which way to go, but I believe >> that pf(4) is a suitable candidate. I especially like the one- >> rule-to-rule-them-all approach. Adding a keyword "app" to >> pf.conf(5) seems like the simplest solution -- much like "proto" >> does deal with IP types. >> >> And talking about complexity: 1000 LOC for 25 protocols. I'm afraid >> it can't be simplified any more than this. > > What sort of protocols do you think could be reasonably handled by > this approach, and what would be too complicated? Good question! Text protocols are easy, RFCs and open implementations are generally easy. Anything too commercial/proprietary, especially in binary, is more guessing than anything else and may not be worth the effort. I don't see "world of warcraft" happening as a supported application. This is what I have done so far (by no means free of errors, though): -- BitTorrent -- Gnutella -- Network Basic Input Output System -- Telecommunication Network -- Hypertext Transfer Protocol -- Post Office Protocol (Version 3) -- Internet Message Access Protocol -- Simple Mail Transfer Protocol -- Session Traversal Utilities for NAT -- Dynamic Host Configuration Protocol -- Point-to-Point Tunneling Protocol -- Lightweight Directory Access Protocol -- Simple Network Management Protocol -- Secure Shell -- File Transfer Protocol -- Session Initiation Protocol -- Domain Name System -- Real-time Transport Control Protocol -- Real-time Transport Protocol -- Routing Information Protocol -- Boarder Gateway Protocol -- Internet Key Exchange -- Datagram Transport Layer Security -- Transport Layer Security -- Concurrent Versions System > There is definitely something appealing about being able to say, for > example, 'block proto tcp on port 443; pass proto tcp on port 443 app tls', > or 'block app ssh; pass proto tcp from to port 22 app ssh' > without a bunch more complexity involved in passing across to a separate > proxy (which would then need to implement its own completely separate > filtering and would, I think, not really be able to integrate with > things like PF tags and queue assignment)... Yes, that would be one scenario. I like to think of lightweight packet inspection as application "tagging". That's the first stage. Second stage is a real parser/proxy/endpoint. It's not a security functionality per se, but it
Re: DPI for pf(4)
Hi Alexey, On Apr 30, 2013, at 11:51 PM, "Alexey E. Suslikov" wrote: > Franco Fichtner gmail.com> writes: > >> so I have been working on a BSD licensed DPI engine. It's a >> very lightweight, non-intrusive approach and I know that teasers >> are boring, but I'd like to know if it's worth the time to >> work on inclusion for pf(4). So far I have about 25 supported >> applications and the necessary hooks for the pf.conf(5) parts. > > If DPI stands for Deep Packet Inspection, than (afaik) > it was discussed before: this kind of inspection is too > complex to put into a kernel. Yes, I am proposing a lightweight approach: hard-wired regex-like code, no allocations, no reassembly or state machines. I've seen far worse things being put into Kernels and I assure you that I do refrain from putting in anything that could cause segmentation faults, sleeps, or other non-suitable behaviour. > relayd already supports L7 filtering at least for http, > so if something is to be improved in this area, relayd > is better place, imo. Would a protocol like BGP have a bright future in relayd(8)? I don't know enough, maybe Reyk can clear this up? L7 filtering is cute, but ipfw-classifyd isn't maintained, DPI in Linux netfilter is not hitting it off, and there really is no BSD DPI. Franky, I don't care which way to go, but I believe that pf(4) is a suitable candidate. I especially like the one- rule-to-rule-them-all approach. Adding a keyword "app" to pf.conf(5) seems like the simplest solution -- much like "proto" does deal with IP types. And talking about complexity: 1000 LOC for 25 protocols. I'm afraid it can't be simplified any more than this. Thanks for your input, Franco
DPI for pf(4)
Hi misc@, so I have been working on a BSD licensed DPI engine. It's a very lightweight, non-intrusive approach and I know that teasers are boring, but I'd like to know if it's worth the time to work on inclusion for pf(4). So far I have about 25 supported applications and the necessary hooks for the pf.conf(5) parts. The idea is first packet on each side only, no content extraction. It's not meant to be completely accurate, but it might be a good addition to the feature set of pf(4) nonetheless. I have two blog posts with code, and more coming if anyone is interested. Regards, Franco
Re: rm(1) static addition
On Apr 27, 2013, at 9:28 PM, Joerg Sonnenberger wrote: > On Sat, Apr 27, 2013 at 09:09:25PM +0200, Franco Fichtner wrote: >> On backtrace(3) (which is a GNU thing, I know), static functions don't >> show up with their respective names even though they are in the binary. >> That's a tad annoying, but I am not aware of any other limitation. Can >> someone please enlighten me? > > That's not true in general. backtrace(3) or any other user of the > .eh_frame section for unwinding won't see individual records for inlined > functions. Static functions are more likely to get inlined, but the same > can happen with non-inline functions defined in the same file. Ok, thanks. Sounds like an impasse. GCC does what it wants, but then -fno-inline is also not an option for the build? Regards, Franco
Re: rm(1) static addition
On Apr 27, 2013, at 7:36 PM, Ted Unangst wrote: > On Sat, Apr 27, 2013 at 08:10, Otto Moerbeek wrote: >> On Sat, Apr 27, 2013 at 01:08:06AM -0400, Eitan Adler wrote: >> >>> Adding static to internal function allows the compiler to better >>> detect dead code (functions, variables, etc) and makes it easier for >>> the compiler to optimize; e.g., since it knows a function will only >>> called once it can inline code; or not output a symbol for a certain >>> function. >> >> In general we don't lik this because it makes things harder to debug. >> For libraries, yes, but for programs, no. > > Isn't that rule only for the kernel? ddb can only see global symbols, > but gdb should work fine in userland. Certainly I can set breakpoints > on static functions, even when compiled without -g. On backtrace(3) (which is a GNU thing, I know), static functions don't show up with their respective names even though they are in the binary. That's a tad annoying, but I am not aware of any other limitation. Can someone please enlighten me? Thanks, Franco
Re: goodbye to some isa devices
On 28.03.2013, at 13:17, Daniel Bolgheroni wrote: > On Thu, Mar 28, 2013 at 05:46:30AM +, Miod Vallat wrote: >> >> You can't say in substance "it's a pity OpenBSD doesn't support the VAX >> 11/780 anymore" in one mail, "you guys really ought to ditch floppy >> installation media" in another, and expect people not to question your >> logic or your motives. > > Agreed. I read all this thread and couldn't figure it out what this guy > really wants. To be quite frank, your statement is even more ambiguous and could be easily misinterpreted as 'trollish' in nature. Here, asking questions is considered a waste of many people's time, yet some always feel obligated to point out just how much of their precious time is actually being wasted. Redundancy and impoliteness ensue. Ask any psychologist what makes humans special and they will tell you it is the ability to ask questions in order to enrich their complex existence. Deal with it. And keep your ISA support, because nobody is going to take it away from you by asking 'stupid' questions. I really don't get it why it invokes so many hard feelings. And kudos to Nick for being the calmest and most helpful person in this discussion. Keep it up.
Re: goodbye to some isa devices
On Mar 26, 2013, at 11:11 PM, Creamy wrote: > On Tue, Mar 26, 2013 at 10:50:40PM +0400, Franco Fichtner wrote: >> Nobody in their right mind would have such a system as >> mission critical infrastructure. :) > > What, like using a Honeywell 316 as a nuclear power station > reactor temperature monitor in to the early 2000s, until it's > hard disk failed? > > Don't worry, it's been replaced with a couple of PDP11/70's, > so we can all sleep well at night. > > N.B. This one *isn't* a joke. Point taken; you are right. Scenario (3) is the most likely.
Re: goodbye to some isa devices
On Mar 26, 2013, at 10:06 PM, Creamy wrote: >>> Looking to the future, when are we going to drop 486 support, anyway? >> >> Now, that's a more interesting thing ask. > > How much of the hardware survives now, anyway? I mean at least the old > Vaxen were, (and are), maintainable. 486 motherboard dies, what do you > do? Chances are it's a multi-layer pcb, so if traces go bad within it, > a repair is going to be almost impossible. >From personal experience, the oldest things I've used *recently* was a Pentium Pro a few years back for providing Internet connectivity. That was when we barely had a single Mbit/s per line here in Germany. To be honest, it was about 8 years ago. I know a case study means nothing, so let me try another route. You would only need to upgrade to the latest and greatest release if one of the following is true: (1) Your system is connected to the Internet and thus requires frequent security updates. (2) You want to run services that are bleeding edge like OpenSMTPD out of the box. (3) You are crazy. But seriously, if there is no networking, there is no need to run a recent release. And you will be able to run 5.3 in any case. And why would you use networking anyway with such small throughput, the likelihood of your tiny IBM disk (a, those were the days!) failing on you any second now. All you've got there is a ticking time bomb. Nobody in their right mind would have such a system as mission critical infrastructure. :)
Re: goodbye to some isa devices
On Mar 26, 2013, at 6:26 PM, Creamy wrote: >> but I honestly question the utility of any of these ISA >> network and SCSI drivers. > > Perhaps somebody who is new to coding might be able to learn something > from them? There is such a vast amount of code in the different BSD flavours alone that it becomes very unlikely someone will stumble upon ISA code bits, especially if one is a novice programmer. And how many of those are old enough to have seen what ISA looks like nowadays? Looking at diffs which remove ISA relevant stuff is probably the only time they will see it -- that's educational *and* teducational at the same time. Sorry for the bad pun. > Looking to the future, when are we going to drop 486 support, anyway? Now, that's a more interesting thing ask.
Re: add missing semicolon to tree(3) example code
On Feb 17, 2013, at 6:45 PM, Otto Moerbeek wrote: > On Sun, Feb 17, 2013 at 03:59:41PM +0100, Franco Fichtner wrote: > >> Hi all, >> >> found this still lingering in my tree. Still trying to figure out >> the best workflow for sending patches. Not sure if this adheres >> to the standards. >> >> Thanks, >> Franco >> --- >> share/man/man3/tree.3 | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/share/man/man3/tree.3 b/share/man/man3/tree.3 >> index bb9a5b2..f4a410c 100644 >> --- a/share/man/man3/tree.3 >> +++ b/share/man/man3/tree.3 >> @@ -499,7 +499,7 @@ intcmp(struct node *e1, struct node *e2) >> } >> >> RB_HEAD(inttree, node) head = RB_INITIALIZER(&head); >> -RB_GENERATE(inttree, node, entry, intcmp) >> +RB_GENERATE(inttree, node, entry, intcmp); >> >> int testdata[] = { >> 20, 16, 17, 13, 3, 6, 1, 8, 2, 4, 10, 19, 5, 9, 12, 15, 18, >> -- > > I don't agree. RB_GENERATE creates a function definition, and those are > not followed by a semicolon. It would just create an empty declaration. Ok, I wasn't stating it's strictly necessary. It's just that most of the lines implementing splay or rb trees are using the semicolon. It implies knowledge of the underlying code to leave the semicolon out just for this particular macro; and the man page shows a semicolon in the synopsis section regardless. > As for sending diff, I (and quite some people here) prefer cvs diff -p > on a checked out tree, it shows which version you diffed against. Alright, thanks, I'll see what I can do on my real OpenBSD box. FWIW, this diff was based on -current. Franco
add missing semicolon to tree(3) example code
Hi all, found this still lingering in my tree. Still trying to figure out the best workflow for sending patches. Not sure if this adheres to the standards. Thanks, Franco --- share/man/man3/tree.3 | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/share/man/man3/tree.3 b/share/man/man3/tree.3 index bb9a5b2..f4a410c 100644 --- a/share/man/man3/tree.3 +++ b/share/man/man3/tree.3 @@ -499,7 +499,7 @@ intcmp(struct node *e1, struct node *e2) } RB_HEAD(inttree, node) head = RB_INITIALIZER(&head); -RB_GENERATE(inttree, node, entry, intcmp) +RB_GENERATE(inttree, node, entry, intcmp); int testdata[] = { 20, 16, 17, 13, 3, 6, 1, 8, 2, 4, 10, 19, 5, 9, 12, 15, 18, --
Re: hostname.if(5) clarification
On Nov 26, 2012, at 9:44 PM, Christian Weisgerber wrote: > Todd T. Fries wrote: > >> If there are desires to improve this (I hear Naddy grumbling!) then the >> stomach to break backwards compat must be present, or suggestions on how >> to do it without breaking backwards compat must be suggested. > > My suggestion is two-fold: > > * Introduce a new format. This new format will ignore # comments, > call ! commands, but otherwise pass on everything unchanged to > ifconfig. I'm neutral on the matter of retaining "dhcp" and > "rtsol" as shortcuts for "!dhclient \$if" and "!rtsol \$if". > > * To maintain backward compatibility, retain the old parsing for > hostname.* files. Interface configuration files in the new format > will have a different name; if.* or whatever. > > Does that sound workable? I like new format, but I'm not sure the road to go should be the hostname.if type of format. Ordering could be imposed on such a format in the likes of whatnot.if.priority - and you'll end up with a mess of configuration files. So how about a single new config file (implemented in the style of Christian's suggestions, which also provides the following things: (1) new syntax is invoked like this: if { up down } (It would also be possible to provide inline backwards compatibility with the old syntax, but I don't think this file is the right place to put it, see below) (2) the old style hostname.if files are invoked using solely this: if (3) allow wildcards to be used, so that the default and fully compatible file is: * If you don't like the hostname.if stuff, you simple delete the config file or remove/comment out that wildcard line. You can also impose ordering this way, but you're on your own then: re0 re1 # invoking * would be stupid now... Next issue would probably be running external scripts in the new format from this file. Maybe like so: if /path/to/settings The syntax is debatable, and there are probably a few consistency considerations: what is executed in the old format and what is not? But then this format would be something to migrate to if it's feasible, so maybe the only backwards compatible way should be for (2). Franco
Re: ##@!#@# gnu tools
On Nov 15, 2012, at 5:53 PM, Reyk Floeter wrote: > On Thu, Nov 15, 2012 at 5:11 PM, Marc Espie wrote: >> external people regularly ask "but why you don't want to use GNU/m4 GNU/make >> GNU/whatever ?" >> > > External people seem to ask weird questions. > > I just had to dig into autoconf/auto* because it seems to be a "must > have" for a "portable" project. Yuck! It is a reason why I don't > understand and at the same time deeply respect our ports people: they > have to mess with this stuff all the time! The amount of hardcoding in Makefiles for GNU make is astounding given the (flexible enough) design of GNU make. It's not as good as it could be, but there are so many blunt tutorials and documentations available. They all fail to use the tricks that have been used by BSDs for ages. It's always hardcoding this, explicitly calling that... It's not surprising that so many auto* and other magical make systems have been build on top of that rocky foundation. I've tried to work on a GNU compatible prog.mk and the like, but they are barely in shape: https://github.com/fichtner/peak/blob/master/prog.mk And then you still have to deal with differences in include syntax and bugs like not handling paths in multiple layers of include files correctly. > > For all the GNU people, here is how a Makefile for hello.c should look like: > PROG= hello > NOMAN= yes > .include > > Yes, you're supposed to provide a man page hello.1 and remove the NOMAN line > :) > > Reyk > >> Well, latest one, turns out gnu-m4 has relly sloppy regexp handling. >> Namely, stuff like >> >> regexp(`n', `?') >> *works* with gm4... >> >> I know somewhat incredible... our regexpes obviously will not like ? like >> that, since it's not a normal character, and gnu regexp handling is such >> a bluberring piece of code that it works... very reproducible, very so >> secure. >> >> Reminds me of gnu libtool dropping silently stuff it doesn't understand... >> >> oh wait, of course, *that* regexp is in the autoconf much leading to >> gnu libtool. >> >> Gee, what a surprise...
tree(3) inconsistencies
Hi tech@, I noticed RB_GENERATE_INTERNAL in src/sys/sys/tree.h puts brackets around the compare function cmp in RB_INSERT just like SPLAY_GENERATE_INTERNAL does. However, RB_FIND and RB_NFIND don't do this. Is there any reason we need (cmp) expressions at all? It prevents macros from being used in those places, which may speed up the generated implementations. Or would it be better to fix the code in RB_FIND and RB_NFIND? I've also thought about marking read-only elements const in *_FIND and similar functions. I've hit a warning in my code about stripping const off of the type there. Maybe that's paranoid, but I don't see a negative side effect. On a related note, the EXAMPLES section in src/share/man/man3/tree.3 is missing a semicolon at the end of RB_GENERATE. I can prepare a diff, but I wanted to ask first. Thanks, Franco
Re: rdtsc timecounter
On Aug 15, 2012, at 8:17 PM, Ted Unangst wrote: > On Wed, Aug 15, 2012 at 13:25, Ted Unangst wrote: > > I will probably rename this just "tsc" after some prodding from mikeb, > that's a better name. I tend to focus on the instruction used, but we > should name it after the counter. > >> +rdtsc_timecounter.tc_frequency = cpuspeed; > >> +/* cpuspeed is scaled down, so for now, we do the same */ >> +tsc = rdtsc(); >> +tsc /= 100; >> +return (uint32_t)tsc; > > This I know is suboptimal. We need to scale down some, to avoid > overflow, but a factor of 1000 is probably better. Or 1024 and shift > for more speeds. If it's only about scaling >>= 10 or 20 is perfectly fine as long as no user of rdtsc_get_timecount tries to translate the return value back into reliable seconds. That would introduce minor jitter. Or maybe "frequency" needs to be adjusted accordingly. Or the translation should be done inline so that tsc is never leaked? Unfortunately, a lot of these high resolution scenarios are bound to "per second" metrics, so the trouble of making it as fast as possible only relays the inevitable. I guess it is best to leave it as is, although it would be nice for the magical 100 to be tied to the actual cpuspeed scaling. It could go out of sync. Franco
Re: amd64: Check cpu_vendor instead of using CPUID.
On Apr 22, 2012, at 9:32 PM, Christiano F. Haesbaert wrote: > On Sun, Apr 22, 2012 at 09:16:57PM +0200, Franco Fichtner wrote: >> On Apr 22, 2012, at 7:58 PM, Christiano F. Haesbaert wrote: >> >>> On Sun, Apr 22, 2012 at 06:36:41PM +0200, Franco Fichtner wrote: >>>> Just being paranoid... strncmp? >>> >>> Why ? It's a terminated string vs a string literal, what do you wanna >>> use as the third argument: strlen("AuthenticAmd") ? . 100% pointless. >> >> I can see your point and yet it is being used in the line below your change. Do you want to call that author's intent '100% pointless' as well just for the sake of winning an argument or do you simply not care about the depth and inherent wisdom of the code base you are working on? >> > > You rush into conclusions, cpu_model is different, he actually wants > the first 5 bytes to evaluate to "Intel", not the whole string, which > could be something like: > > hw.model=Intel(R) Xeon(R) CPU E31220 @ 3.10GHz Since my first mail I am talking about the mycpu_model line in the diff. It's there. I am asking why it differs from your added line. It's fine when you feel other people do pointless work or point out pointless things. On the other hand, other people may not like the level of hostility and resistance to advice (as bad as it may be in this case). I am no expert on OpenBSD and if this is how tech discussions are handled, I'm not sure if I ever will. The intent of your patch is very good, especially with legibility in mind. But if you touch that line, why do it half-heartedly? And why ask for comments in the first place? >>>> And how about consolidating style while at it? "!" vs. "== 0" - see code bits below change. >>> >>> Consolidating how ? Are you suggesting we change all strcmp calls in >>> kernel to use "== 0" ? Please. >> >> Personally, I don't care either way, but it's bad style to ignore the context and change styles. It makes the code harder to read, understand and maintain. Take a look. Ok? >> > > You care enough to send an email without even checking the other uses, > if you did, you'll see that !strcmp is more consistent for this case > than strncmp. I care enough to point out an inconsistency in your patch... > > *You* are ignoring the context and trying to change styles. ... and now we are talking about me being the evil guy who is trying to change you and the context. Not gonna happen. :) >>>>> +if (!strcmp(cpu_vendor, "AuthenticAMD")) >>>>> amd64_errata(ci); >>>>> >>>>> if (strncmp(mycpu_model, "VIA Nano processor", 18) == 0) { Franco
Re: amd64: Check cpu_vendor instead of using CPUID.
On Apr 22, 2012, at 7:58 PM, Christiano F. Haesbaert wrote: > On Sun, Apr 22, 2012 at 06:36:41PM +0200, Franco Fichtner wrote: >> Just being paranoid... strncmp? > > Why ? It's a terminated string vs a string literal, what do you wanna > use as the third argument: strlen("AuthenticAmd") ? . 100% pointless. I can see your point and yet it is being used in the line below your change. Do you want to call that author's intent '100% pointless' as well just for the sake of winning an argument or do you simply not care about the depth and inherent wisdom of the code base you are working on? > >> And how about consolidating style while at it? "!" vs. "== 0" - see code bits below change. > > Consolidating how ? Are you suggesting we change all strcmp calls in > kernel to use "== 0" ? Please. Personally, I don't care either way, but it's bad style to ignore the context and change styles. It makes the code harder to read, understand and maintain. Take a look. Ok? >>> +if (!strcmp(cpu_vendor, "AuthenticAMD")) >>> amd64_errata(ci); >>> >>> if (strncmp(mycpu_model, "VIA Nano processor", 18) == 0) { Franco
Re: amd64: Check cpu_vendor instead of using CPUID.
Just being paranoid... strncmp? And how about consolidating style while at it? "!" vs. "== 0" - see code bits below change. Franco On 22.04.2012, at 15:12, "Christiano F. Haesbaert" wrote: > There's no need for doing that somewhat strange comparison, the rest > of the code already uses cpu_vendor. > > ok ? > > Index: identcpu.c > === > RCS file: /cvs/src/sys/arch/amd64/amd64/identcpu.c,v > retrieving revision 1.35 > diff -d -u -p -r1.35 identcpu.c > --- identcpu.c27 Mar 2012 02:23:04 -1.35 > +++ identcpu.c22 Apr 2012 12:59:10 - > @@ -302,7 +302,6 @@ identifycpu(struct cpu_info *ci) >u_int64_t last_tsc; >u_int32_t dummy, val, pnfeatset; >u_int32_t brand[12]; > -u_int32_t vendor[4]; >char mycpu_model[48]; >int i, max; >char *brandstr_from, *brandstr_to; > @@ -433,11 +432,7 @@ identifycpu(struct cpu_info *ci) > > #endif > > -vendor[3] = 0; > -CPUID(0, dummy, vendor[0], vendor[2], vendor[1]);/* yup, 0 2 1 */ > -/* AuthenticAMD:h t u Ai t n e */ > -if (vendor[0] == 0x68747541 && vendor[1] == 0x69746e65 && > -vendor[2] == 0x444d4163)/* DMAc */ > +if (!strcmp(cpu_vendor, "AuthenticAMD")) >amd64_errata(ci); > >if (strncmp(mycpu_model, "VIA Nano processor", 18) == 0) {