On Mon, 2009-08-03 at 21:51 -0400, James Carlson wrote: > Sebastien Roy wrote: > > On Sat, 2009-08-01 at 09:44 -0400, James Carlson wrote: > >> Sebastien Roy wrote: > >>> http://www.opensolaris.org/os/project/clearview/iptun/cr/ > >> ifconfig.c: > >> > >> What happened to the tunnel reality check (IPsec thing)? > > > > Ifconfig no longer outputs or verifies specific policy associated with > > the underlying link. If there's policy associated with the tunnel, it > > refers the caller to ipsecconf. > > What I'm asking is whether that change is ok. Unless I'm missing > something, it looks like a previous IPsec fix here is being reverted. > Does the previous code not matter anymore? If so, why not?
I'll double check with danmcd. > >> 1614: tunnels aren't the only things that lack Ethernet addresses. > >> There should probably be another way to do this. > > > > That's true, there's also an escape clause for IFF_VIRTUAL and IFF_IPMP > > interfaces a few lines above. Note that I'm not changing the logic of > > the code here, but merely calling a libdladm function instead of a > > libinetcfg one to determine if the underlying link is a tunnel. > > Right; I didn't think it was correct before, either. > > > Are you suggesting that there be a link property that allows one to > > derive how to print addresses associated with a given link? Or > > something else? > > That might do it. Or all links could have addresses with some of them > just reporting a zero-length address. Hmm, tunnels are weird in that they don't have a zero-length address. They have addresses, but they're not printed in Ethernet-like fashion, and for some reason, someone decided to print them in a different spot in the ifconfig output than the Ethernet address is printed on Ethernet interfaces (before the IP interface addresses instead of after). In addition to that, some tunnels have two addresses rather than one. I'm not exactly sure I have a handle on how a solution would look any better than a simple "is this a tunnel" check given these constraints. > >> 2424: this looks like a test that belongs in the kernel. > > > > It looks like it should be, but can there be such a check? I don't know > > enough about how _I_REMOVE works to know if it's even possible to > > prevent such an operation from the module being removed. If it's > > possible, the perhaps a bug should be filed. > > We do something similar for I_POP, so it seems at least feasible. A > crude answer would just be to insert a check for these special names in > the kernel's _I_REMOVE and deny it there. > > (I was commenting on enforcing kernel design rules like this out in user > space. It doesn't make sense to me.) Agreed. I've filed: 6868095 ifconfig modremove checks belong in the kernel > >> snoop_ether.c: > >> > >> 1704: what happens with IPsec or labeling? > > > > There are no IPsec headers here. Promiscuous streams get their packets > > from GLDv3. On receive, IPsec processing occurs before packets are > > passed up to GLDv3. On transmit IPsec processing occurs after packets > > have been send down by GLDv3. > > That's might be true with some tunnel-mode IPsec configurations, but I'm > not so sure it's generally true. I'd expect that you can have > essentially arbitrary IPv6 headers between the outer and inner tunnel > headers. > > (Unless the kernel is doing something here to rewrite these headers to > remove things the user "shouldn't" see ...) IPsec headers are removed by the IPsec processing code before iptun ever gets it paws on the packets. That's true for all protocols above ip, not just iptun. I suppose something could have added other extension headers, and snoop could handle those so that the user-land filter has the correct offsets. Someone please tie a cinder block to snoop and throw it in the Charles. Oh, did I say that out loud? ;-) > >> dladm.c > >> 979: seems stray. > > > > These are used to print iptun_flags in print_iptun(). > > OK ... but they don't look related to any of the column handling, so > having this enum pasted right here looks a little odd. It's related to the iptun_flags field which is in the structure just below the enum (the enum represents an index into that field). I'll add a comment. > >> 988: why not follow the style used elsewhere for these tables? > > > > Because I found them illegible anyway. :-} I suppose I'll fix this to > > be consistently illegible. > > Agreed; the style in this file is awful. But if you're going to pick a > new and better style, I think you've got a bit more lifting to do. ;-} ACCEPT (made the code uniformly ugly) :-) > >> dladm.xcl: > >> > >> Check that your getopt strings aren't showing up in the text for > >> translation. (The additions here seem sparse.) > > > > ACCEPT: The architecture around this facility looks way out of hand and > > unmaintainable. Shouldn't it be implied that if it's not wrapped around > > in a gettext(), then it doesn't need translating? > > You and meem can have a replay of the same argument I had with him. I > agree with you, and if it were my code, I wouldn't use this ".xcl" > thing, but meem finds the gettext() wrappers everywhere to be more > offensive for some reason, so that's why a lot of this code needs to be > maintained this way. I've added the strings. I'm almost certain that many projects and bug fixes have already resulted in this file being out of date already. Perhaps it will prove to be useless over time and the argument can make itself. > >> dlmgmtd/Makefile: > >> > >> 46: broader issue: why isn't CTF just the default? > > > > There are existing CRs on this subject: > > 6811460 daemons should get CTF data > > 5062878 ctf data should be available for important system daemons > > OK. Really, the compilers should just be updated to produce CTF so we > can ditch this hackery. Agreed. I'm not sure why we don't simply have CTF in everything, really. > > >> libdladm.c: > >> > >> 776: is this true for all links now? > > > > Yes. Becuase '.' has been allowed in IP interface names since at least > > Solaris 8, there is little chance that this will cause any problems. > > I thought we were using a more restricted set for datalinks on purpose > ... but ok. The '.' was not allowed on purpose because it was a reserved character in IP interface names (for the implicit STREAMS module hack done by tunnel plumbing). This design requires the creation of tunnel links with those characters for backward compatibility. > > Does this mean that '.' is intentionally allowed for non-tunnel vanity > names? So I can use "foo.bar.baz0" if I want, and it doesn't imply > driver foo with modules bar and baz? Is this something that will be > documented? Yes, it's in the design document which is part of the PSARC materials, and the NOTES section of the dlpi(7P) man page will be updated. > > >> minorperm: > >> > >> There's a new iptun.conf but no minorperm entry for iptun. > > > > ACCEPT: Curiously, the lack of a minor_perm entry for iptun has not > > prevented /dev/net nodes from having the correct 666 permissions. > > There's a lot in that area that's mysterious to me. I'll talk to Cathy to get to the bottom of how this was working before. Thanks, -Seb
