On Mon, 2009-08-10 at 13:21 -0400, Sowmini.Varadhan at Sun.COM wrote:
> On (08/10/09 10:27), Sebastien Roy wrote:
> >
> > > - Instead of doing pairs of dladm_open/dladm_close, suggest having
> > > a global dladm_handle that is opened once in main and closed on exit.
> > > dladm_link_open() can use that handle, and it can be reused for
> > > "ifconfig -a <..>" commands like "ifconfig -a ether" and "ifconfig -a
> > > plumb".
> >
> > REJECT: I'd rather not; ifconfig currently doesn't depend on having the
> > ability to open a dladm handle except for some commands, and moving this
> > logic to a common location in the code introduces the risk that we'd be
> > causing ifconfig to fail unnecessarily (like from within shared stack
> > zones for example).
>
> This is not very efficient for something like rcm_daemon which exec's
> ifconfig,
> though I have admittedly not checked to see if rcm_daemon ever invokes
> ifconfig
> to do an operation on more than one interface at any time.
>
> The case that you point to (ifconfig failing from shared stack zone) can
> be handled by having ifconfig`main treat dladm_open() as a soft error
> except when the handle itself is needed (in dladm_link_open)
By then, you've lost the context of what the error was when you failed
to open the dladm handle. I suppose you could squirrel away the error
for later... I'll ACCEPT this and implement it.
> > >
> > > usr/src/uts/common/inet/ip/ip_ndp.c
> > > - do you need symmetric changes to ndp_lookup_then_add_v4 that also add
> > > the new ipv4 nce with hwaddr == ill_dest_addr for tunnel interfaces?
> >
> > No. The IPv4 code is totally different. There is no hardware address
> > passed in to ndp_lookup_then_add_v4(), but rather a "src_nce" which
> > comes by way of a long trip through a giant call-chain that originates
> > in a call to ire_create() in ip_newroute() (I pray that
> > datapath-refactoring simplifies this.) For IRE_IF_NORESOLVER, a NULL
> > src_nce is passed in, and the NULL src_nce gets passed in to
> > ndp_add_v4() which does the right thing given the ill_resolver_mp. I'd
> > have to consult with a Surya project team member to get the details on
> > why they didn't make the IPv4 and IPv6 code more uniform. ;-)
>
> the src_nce was used to handle the ipmp case (which may well be broken
> for ipv6 in onnv- I've not checked). However, for the particular
> issue at hand, the iptun-cr ws tries to set the hwaddr of nce's over
> tunnel interfaces to the ill_dest_addr. This should be handled internally
> in ndp_andd_v4 at
>
> 3672 } else if (ill->ill_net_type == IRE_IF_NORESOLVER) {
> 3673 /*
> 3674 * NORESOLVER entries are always created in the
> REACHABLE
> 3675 * state.
> 3676 */
> 3677 if ((template = copyb(ill->ill_resolver_mp)) == NULL)
> {
> 3678 err = ENOMEM;
> 3679 goto err_ret;
> 3680 }
> 3681 state = ND_REACHABLE;
> 3682 }
>
> This section, if it detects IFF_POINTOPOINT, should copy the
> ill_dest_addr to nce->nce_res_mp->b_rptr + NCE_LL_ADDR_OFFSET(ill)
ill_resolver_mp already contains the right information. I can say
without a doubt that the IPv4 fast-path works flawlessly in the gate, so
the question isn't what needs to be fixed for IPv4, but why did I set
the hw_addr argument to ndp_loookup_then_add_v6()...
> It's not merely an issue of symmetry: presumably the ndp_add_v6 code is
> painstakingly doing nce_set_ll for some good reason (not sure where
> the nce_res_mp gets uesd for ipv6)- don't we need to have the ipv4
> nce's also setup in a similar way?
nce_res_mp gets used for both IPv4 and IPv6 by nce_fastpath() to send
down fastpath probes down, among other things. I'll look to see why
that was needed, it was done a long time ago. The ire_resolver_mp is
set correctly for both IPv4 and IPv6 ill_t's, so I'll need to get back
to you on what the use of ill_dest_addr is about in the ndp_noresolver()
call to ndp_lookup_then_add_v6().
> > >
> > > usr/src/uts/common/inet/iptun/iptun.c
> > > - why not just do a msgpullup at line 2043.
> >
> > IPsec re-inserts the outer IP header as a single MBLK on input, and
> > since that IP header will simply be removed by GLDv3 when the packet
> > gets passed up, doing a msgpullup() is a needless copy of data that will
> > just get immediately discarded anyway.
>
> Ok, would be useful to add a comment about this.
Will do, ACCEPT.
> >
> > > And if we consider that a tunnel tun1
> > > with outer header (A -> B) and inner header (C -> B) is recursive [#]
> > > then what about a tunnel which has outer/inner header
> > > (A -> B) / (C -> D) where the route for B points at a tunnel tun2
> > > that sets up (C1 -> D) as the outer header?
> >
> > You mean a packet that has those headers, and not a tunnel, right? A
>
> yes.
>
> > detect other simple cases of such misconfigurations, I agree that the
> > check is kind of strange. I will remove it.
>
> Ok.
>
> > > (and even more confusing:
> > > we drop the packet on the input path when inner_dst == outer_dst even
> > > if both of these are locally hosted addresses? I would expect a host
> > > that receives such a packet to accept it, assuming everything else
> > > checks out).
> >
> > I'm afraid I'm not following you on this one. Where is this check?
>
> sorry, my mistake.. I think I got my cscope confused. iptun_input
> doesn't use the output from iptun_find_headers() in the same way
> as iptun_output.
Okay, thanks.
-Seb
