Re: [PATCH] Netdump for review and testing -- preliminary version

On Wed, Oct 13, 2010 at 01:04:54PM +0200, Attilio Rao wrote:

> 2010/10/9 Robert Watson <rwatson_at_freebsd.org>:

> > (1) Did you consider using tftp as the network dump protocol, rather than a
> > custom protocol? ??It's also a simple UDP-based, ACKed file transfer
> > protocol, with the advantage that it's widely supported by exiting tftp
> > daemons, packet sniffers, security devices, etc. ??This wouldn't require
> > using a stock tftpd, although that would certainly be a benefit as well.
> > ??The post-processing of crashdumps that seems to happen in the netdump
> > server could, presumably, happen "offline" as easily...?
> 
> I don't understand the "offline" question but, really, I don't know
> why tftp wasn't considered in the first place.
> Let me do some small search and see how much we could benefit from it.

I think Robert means having something other than the netdump server
doing the post-processing - e.g., a cron job could look in the directory
where a tftp server saved a core file and run a script to extract the
desired information.

TFTP has a number of advantages; I'm not sure if the original author of
the netdump code considered it and rejected it for some reason.  I think
the original implementation used a broadcast packet to locate the server
though, so a custom server was required in any case.

That said, I don't think it matters too much if we go ahead with the
current version and switch to TFTP later on.

> > (2) Have you thought about adding a checksum to the dump format, since
> > packets are going over the wire on UDP, etc? ??Even an MD5 sum wouldn't be
> > too hard to arrange: all the code is in the kernel already, requires
> > relatively little state storage, and is designed for streamed data.
> 
> Someone else already brought this point and I agree, we could use a
> checksum here.

Both a whole dump and per-packet checksum could be valuable.  If we want
the former I think this should be done in the dump infrastructure, so
that disk dumps get it too.  The latter conflicts somewhat with a desire
to switch to TFTP though (other than a standard UDP packet checksum).

> > The sysctl parts of the patch have a number of issues:
> 
> Sysctl are still not overhauled just because I'm not sure we want to
> keep them. That is one of the points I raised in the main e-mail and
> I'd really would like to hear opinions about if we should keep them
> rather than having a setup userland process, etc.
> I'm sorry about this, but please keep in mind that the file still
> needs a lot of cleanup so some comments are a bit out of date and
> other parts may not be still perfectly overhauled.

Attilio suggested adding a userland tool to configure netdump in lieu of
the current sysctls.  I don't have a strong opinion either way; the only
real advantage to the sysctl approach that I see is that the use of the
mirrored loader tunables is more obvious.

> > The idea of calling into the mbuf allocator in this context is just freaky,
> > and may have some truly awful side effects. ??I suppose this is the cost of
> > trying to combine code paths in the network device driver rather than have
> > an independent path in the netdump case, but it's quite unfortunate and will
> > significantly reduce the robustness of netdumps in the face of, for example,
> > mbuf starvation.
> 
> I'm not sure in which other way we could fix that actually. Maybe a
> pre-allocated pool of mbufs?

Despite the freakiness, I can only offer the observation that it has
worked very well for us in practise.

> > + ?? ?? ?? if (ntohs(ah->ar_hrd) != ARPHRD_ETHER &&
> > + ?? ?? ?? ?? ?? ntohs(ah->ar_hrd) != ARPHRD_IEEE802 &&
> > ...
> >
> > Are you sure you don't want to just check for ETHER here?
> 
> I'd really like to hear Ryan's or Ed's idea here.

Interesting, I can't find this in our local version.  It looks like this
check was copied from if_ether.c::arpintr().

> > + ?? ?? ?? /* XXX: Probably should check if we're the recipient MAC address */
> > + ?? ?? ?? /* Done ethernet processing. Strip off the ethernet header */
> >
> > Yes, quite probably. ??What if you panic in promiscuous mode?

Well, the packet would get dropped at the next layer up, in nd_handle_ip.

-Ed