Re: [PATCH] mv -P

[Franco Fichtner]

> 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

[Franco Fichtner]

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

[Franco Fichtner]

> On 13. Mar 2018, at 4:04 PM, Ted Unangst <t...@tedunangst.com> 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=112152576800634=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

[Franco Fichtner]

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)

[Franco Fichtner]

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

[Franco Fichtner]

> 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, );
> - if (errstr)
> + if (errstr) {
> + if (isdigit(*s)) {
> + *uid = 0;
> + return 0;
> + }
>   return -1;
> + }
>   return 0;
> }
> 
> 

VCard and VCalendar MIME types

[Franco Fichtner]

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

[Franco Fichtner]

> 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

[Franco Fichtner]

> 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

[Franco Fichtner]

> 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?

[Franco Fichtner]

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

[Franco Fichtner]

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?

[Franco Fichtner]

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

[Franco Fichtner]

> 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

[Franco Fichtner]

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

[Franco Fichtner]

On 08 May 2014, at 18:43, Alexandre Ratchov a...@caoua.org 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

[Franco Fichtner]

On 22 Apr 2014, at 18:32, Henning Brauer lists-openbsdt...@bsws.de 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)

[Franco Fichtner]

On 20 Nov 2013, at 21:40, Theo de Raadt dera...@cvs.openbsd.org 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

[Franco Fichtner]

Hi Maxime,

On Jul 8, 2013, at 10:40 AM, Maxime Villard rusty...@gmx.fr 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

[Franco Fichtner]

On Jul 4, 2013, at 6:43 PM, Stefan Fritsch s...@sfritsch.de 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: DPI for pf(4)

[Franco Fichtner]

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

Re: enable cmp macro for rb-trees in sys/tree.h

[Franco Fichtner]

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)  \

fix guard define

[Franco Fichtner]

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

enable cmp macro for rb-trees in sys/tree.h

[Franco Fichtner]

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);  \

Re: DPI for pf(4)

[Franco Fichtner]

Hi Damien,

On May 2, 2013, at 10:03 AM, Damien Miller d...@mindrot.org 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 t...@tedunangst.com 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-techm=136739531914555w=2

Re: DPI for pf(4)

[Franco Fichtner]

On May 2, 2013, at 10:45 AM, Damien Miller d...@mindrot.org 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)

[Franco Fichtner]

On May 2, 2013, at 1:23 PM, Damien Miller d...@mindrot.org 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)

[Franco Fichtner]

On May 2, 2013, at 2:40 PM, Damien Miller d...@mindrot.org 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)

[Franco Fichtner]

On May 2, 2013, at 3:20 PM, Damien Miller d...@mindrot.org 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)

[Franco Fichtner]

Hi Stuart,

On May 1, 2013, at 1:11 AM, Stuart Henderson st...@openbsd.org 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 somehosts 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 can help to break down the workload.  It doesn't care
aboute IP versions, ports (mostly ;) ), different flavours (netbios
could be session, datagram, and name service as one for example), and so
forth.

 Basically what I'm wondering if it's possible to go far enough to be
 useful whilst keeping the complexity down to a level which is sane

Re: DPI for pf(4)

[Franco Fichtner]

Hi Ted,

On May 1, 2013, at 1:14 AM, Ted Unangst t...@tedunangst.com 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)

[Franco Fichtner]

On May 1, 2013, at 9:41 AM, Stuart Henderson st...@openbsd.org 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

DPI for pf(4)

[Franco Fichtner]

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

[Franco Fichtner]

On Apr 27, 2013, at 7:36 PM, Ted Unangst t...@tedunangst.com 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: rm(1) static addition

[Franco Fichtner]

On Apr 27, 2013, at 9:28 PM, Joerg Sonnenberger jo...@britannica.bec.de 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: goodbye to some isa devices

[Franco Fichtner]

On 28.03.2013, at 13:17, Daniel Bolgheroni dan...@bolgh.eng.br 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

[Franco Fichtner]

On Mar 26, 2013, at 6:26 PM, Creamy cre...@nocrater.com 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: goodbye to some isa devices

[Franco Fichtner]

On Mar 26, 2013, at 10:06 PM, Creamy cre...@nocrater.com 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

[Franco Fichtner]

On Mar 26, 2013, at 11:11 PM, Creamy cre...@nocrater.com 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.

add missing semicolon to tree(3) example code

[Franco Fichtner]

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: add missing semicolon to tree(3) example code

[Franco Fichtner]

On Feb 17, 2013, at 6:45 PM, Otto Moerbeek o...@drijf.net 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

Re: hostname.if(5) clarification

[Franco Fichtner]

On Nov 26, 2012, at 9:44 PM, Christian Weisgerber na...@mips.inka.de wrote:

 Todd T. Fries t...@fries.net 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

[Franco Fichtner]

On Nov 15, 2012, at 5:53 PM, Reyk Floeter r...@openbsd.org wrote:

 On Thu, Nov 15, 2012 at 5:11 PM, Marc Espie es...@nerim.net 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 bsd.prog.mk
 
 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...

Re: amd64: Check cpu_vendor instead of using CPUID.

[Franco Fichtner]

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 haesba...@openbsd.org
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) {

Re: amd64: Check cpu_vendor instead of using CPUID.

[Franco Fichtner]

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.

[Franco Fichtner]

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