Re: kern/52074: -current npf map directive broken

[Roy Marples <roy%marples.name@localhost>]

On 07/05/2017 17:28, Mindaugas Rasiukevicius wrote:
> >  The condition in 620 currently is (see if()):
> >       ia = in_get_ia_psref(ip->ip_src, &psref_ia);
> >
> >       /* Ensure we only send from a valid address. */==>
> >       if ((ia != NULL || (flags & IP_FORWARDING) == 0) && <<<<!
> >           (error = ip_ifaddrvalid(ia)) != 0)
> >       {
> >           ARPLOG(LOG_ERR,
> >               "refusing to send from invalid address %s (pid %d)\n",
> >               ARPLOGADDR(ip->ip_src), curproc->p_pid);
> >           IP_STATINC(IP_STAT_ODROPPED);
> >           if (error == 1)
> >               /*
> >                * Address exists, but is tentative or detached.
> >                * We can't send from it because it's invalid,
> >                * so we drop the packet.
> >                */
> >               error = 0;
> >           else
> >               error = EADDRNOTAVAIL;
> >           goto bad;
> >       }
> >
> >  Proposed fix is to replace the || inf the if with && as this also seems
> >  to have been the original intention by the author.
>
> Good catch.
>
> Looks like the regression was introduced 7 months ago, as part of the
> ip_output.c rev 1.261.  Roy, would you like to have a look into this?

I  think xtos already comitted a fix, but I'm unsure his fix is correct.
I think the workflow should be this:

if (ia != NULL)
	error = ip_ifaddrvalid(ia);
else
	error = flags & IP_FORWARDING ? 0 : -1;
if (error != 0) { ...

The idea is that if we claim to send from an address it has to be valid, 
but allow the NULL address if forwarded from the filter.

Does this make sense?
The same path probably needs adjustment in inet6.

Roy