Greg Troxel wrote:
> So what I don't get is why
>
> 			if (frpr_pullup(fin, ICMP6ERR_MINPKTLEN) == -1)
> 				return;
>
> is adequate, since I think we need ICMP6ERR_IPICMPHLEN to get the
> included header.  But I do not understand how frpr_pullup and the data
> pointer interact.

If we look in frpr_pullup(), we see:
static INLINE int frpr_pullup(fin, plen)
fr_info_t *fin;
int plen;
{
        if (fin->fin_m != NULL) {
                if (fin->fin_dp != NULL)
                        plen += (char *)fin->fin_dp -
                                ((char *)fin->fin_ip + fin->fin_hlen);
                plen += fin->fin_hlen;
                if (M_LEN(fin->fin_m) < plen) {
                        if (fr_pullup(fin->fin_m, fin, plen) == NULL)
                                return -1;
                }
        }
        return 0;
}

fin_dp is set early in fr_makefrip():
        fin->fin_dp = (char *)ip + hlen;
...
        } else if (v == 6) {
                fin->fin_plen = ntohs(((ip6_t *)ip)->ip6_plen);
                fin->fin_dlen = fin->fin_plen;
                fin->fin_plen += hlen;

                if (frpr_ipv6hdr(fin) == -1)
                        return -1;
...

In this case hlen will be 40 (sizeof(ip6_t)).
So if the payload length of the IPv6 header says 64 bytes,
fin_plen because 64+40=104.

In frpr_icmp6(), we do:

static INLINE void frpr_icmp6(fin)     
fr_info_t *fin;
{
        int minicmpsz = sizeof(struct icmp6_hdr);
        struct icmp6_hdr *icmp6; 
                       
        if (frpr_pullup(fin, ICMP6ERR_MINPKTLEN - sizeof(ip6_t)) == -1)
                return;      

        if (fin->fin_dlen > 1) {     
                ip6_t *ip6;
                             
                icmp6 = fin->fin_dp;
...
                        if ((fin->fin_m != NULL) &&
                            (M_LEN(fin->fin_m) < fin->fin_plen)) {
                                if (fr_coalesce(fin) != 1)
                                        return;
                        }     
***                     if (frpr_pullup(fin, ICMP6ERR_MINPKTLEN) == -1)
                                return;

                        /*
                         * If the destination of this packet doesn't 
match the
                         * source of the original packet then this packet is
                         * not correct.
                         */
                        icmp6 = fin->fin_dp;
                        ip6 = (ip6_t *)((char *)icmp6 + ICMPERR_ICMPHLEN);
                        if (IP6_NEQ(&fin->fin_fi.fi_dst,
                                    (i6addr_t *)&ip6->ip6_src))
                                fin->fin_flx |= FI_BAD;


The call to frpr_pullup() here is redundant - the call to fr_coalesce()
before it should have put everything in a single buffer.

int fr_coalesce(fin)
fr_info_t *fin;
{
        if ((fin->fin_flx & FI_COALESCE) != 0)
                return 1;     
                        
        /*
         * If the mbuf pointers indicate that there is no mbuf to work with,
         * return but do not indicate success or failure.
         */
        if (fin->fin_m == NULL || fin->fin_mp == NULL)
                return 0;
                         
        if (fr_pullup(fin->fin_m, fin, fin->fin_plen) == NULL) {
                ATOMIC_INCL(fr_badcoalesces[fin->fin_out]);
                *fin->fin_mp = NULL;
                fin->fin_m = NULL;
                return -1;        
        }
        return 1;
}

So a flag is set to prevent it from being called more than once.
Looking at fr_pullup(), there's nothing too surprising: fin_ip
and fin_dp are updated accordinly.

void *fr_pullup(xmin, fin, len)
mb_t *xmin;
fr_info_t *fin;
int len;
{
        int out = fin->fin_out, dpoff, ipoff;
        mb_t *m = xmin;
        char *ip;

        if (m == NULL)
                return NULL;

        ip = (char *)fin->fin_ip;
        if ((fin->fin_flx & FI_COALESCE) != 0)
                return ip;

        ipoff = fin->fin_ipoff;
        if (fin->fin_dp != NULL)
                dpoff = (char *)fin->fin_dp - (char *)ip;
        else
                dpoff = 0;

        if (M_LEN(m) < len) {
#ifdef MHLEN
                /*
                 * Assume that M_PKTHDR is set and just work with what 
is left
                 * rather than check..
                 * Should not make any real difference, anyway.
                 */
                if (len > MHLEN)
#else
                if (len > MLEN)
#endif
                {
#ifdef HAVE_M_PULLDOWN
                        if (m_pulldown(m, 0, len, NULL) == NULL)
                                m = NULL;
#else
                        FREE_MB_T(*fin->fin_mp);
                        m = NULL;
#endif
                } else
                {
                        m = m_pullup(m, len);
                }
                *fin->fin_mp = m;
                fin->fin_m = m;
                if (m == NULL) {
                        ATOMIC_INCL(frstats[out].fr_pull[1]);
                        return NULL;
                }
                ip = MTOD(m, char *) + ipoff;
        }

        ATOMIC_INCL(frstats[out].fr_pull[0]);
        fin->fin_ip = (ip_t *)ip;
        if (fin->fin_dp != NULL)
                fin->fin_dp = (char *)fin->fin_ip + dpoff;

        if (len == fin->fin_plen)
                fin->fin_flx |= FI_COALESCE;
        return ip;
}

Or is there something I'm missing?

Darren