On 23/11/15(Mon) 00:55, Alexandr Nedvedicky wrote:
> Hello,
>
> > >
> > > > + else {
> > > > + struct ifnet *destifp;
> > > > +
> > > > + destifp = if_get(rt->rt_ifidx);
> > > > + if (destifp != NULL)
> > > > + destmtu = destifp->if_mtu;
> > > > + if_put(destifp);
> > > > + }
> > >
> > > your code potentially leaves destmtu set to 0 in case we deal with invalid
> > > ipforward_rt. I wonder how icmp_error() we are going to call further below
> > > (at line 1544 in the old code) is going to deal with it. May be we
> > > should just give up on sending ICMP_UNREACH message in this case.
> > > find my small improvement to your patch further below.
> >
> > Note that my change should only matter for IPSEC. This whole MTU mess is
> > scary. I can't even tell if it makes sense to send an ICMP if the ifp
> > disappeared. The use of if_get(9) makes it clear that the MTU handling
> > is insane.
> >
> > So I'm not opposed to your change but I think you should either commit
> > it separately or keep it inside the "if (ipforward_rt.ro_rt) {" block,
> > because it's a behavior change.
> >
>
> IMO we should not let IPSEC send ICMP with 0 MTU proposal. We are probably
> better to send nothing in this case and let sender to retransmit.
I'd say this is true for *all* the error cases returning EMSGSIZE in
ip_output().
