Re: Interesting if_get() case
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().
Re: Interesting if_get() case
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.
Also if I understand the code well enough the proposed MTU could not be zero
before your change. However given the likelihood someone triggers the case
of invalidated route is very low (more academical than practical problem...),
I agree with follow up commit.
We should probably check with mikeb here.
regards
sasha
Re: Interesting if_get() case
On 20/11/15(Fri) 16:50, Alexandr Nedvedicky wrote:
> Hello,
>
> thanks for detailed explanation.
Your welcome.
>
> > + 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.
> Index: ip_input.c
> ===
> RCS file: /cvs/src/sys/netinet/ip_input.c,v
> retrieving revision 1.261
> diff -u -p -r1.261 ip_input.c
> --- ip_input.c 14 Nov 2015 15:40:40 - 1.261
> +++ ip_input.c 20 Nov 2015 15:48:47 -
> @@ -1516,11 +1516,24 @@ ip_forward(struct mbuf *m, struct ifnet
>
> if (rt->rt_rmx.rmx_mtu)
> destmtu = rt->rt_rmx.rmx_mtu;
> - else
> - destmtu = ipforward_rt.ro_rt->rt_ifp->if_mtu;
> + else {
> + struct ifnet *destifp;
> +
> + destifp = if_get(rt->rt_ifidx);
> + if (destifp != NULL)
> + destmtu = destifp->if_mtu;
> + if_put(destifp);
> + }
> }
> #endif /*IPSEC*/
> ipstat.ips_cantfrag++;
> +
> + /*
> +* route to destniation no longer exists, we should revert
> code
> +* back to host unreachable.
> +*/
> + if (destmtu == 0)
> + code = ICMP_UNREACH_HOST;
> break;
>
> case EACCES:
>
>
>
Re: Interesting if_get() case
Hello,
thanks for detailed explanation.
> + 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.
regards
sasha
8<---8<---8<--8<
Warning: Permanently added 'anoncvs.spacehopper.org' (ECDSA) to the list of
known hosts.
Index: ip_input.c
===
RCS file: /cvs/src/sys/netinet/ip_input.c,v
retrieving revision 1.261
diff -u -p -r1.261 ip_input.c
--- ip_input.c 14 Nov 2015 15:40:40 - 1.261
+++ ip_input.c 20 Nov 2015 15:48:47 -
@@ -1516,11 +1516,24 @@ ip_forward(struct mbuf *m, struct ifnet
if (rt->rt_rmx.rmx_mtu)
destmtu = rt->rt_rmx.rmx_mtu;
- else
- destmtu = ipforward_rt.ro_rt->rt_ifp->if_mtu;
+ else {
+ struct ifnet *destifp;
+
+ destifp = if_get(rt->rt_ifidx);
+ if (destifp != NULL)
+ destmtu = destifp->if_mtu;
+ if_put(destifp);
+ }
}
#endif /*IPSEC*/
ipstat.ips_cantfrag++;
+
+ /*
+* route to destniation no longer exists, we should revert code
+* back to host unreachable.
+*/
+ if (destmtu == 0)
+ code = ICMP_UNREACH_HOST;
break;
case EACCES:
Interesting if_get() case
This is an interesting case to understand why we're using interface
indexes instead pointers.
When the code below will be executed without being serialized with
ifdetach(), it might have to deal with an invalid ``ipforward_rt''.
By "invalid" here I mean a route entry pointing to a detached ifp.
Every cached entry can become "invalid" and they can be found in
multiple places: PCBs, tunneling interfaces, gateway routes, TCP
syncache...
Currently, when a route is removed from the table it loses its
RTF_UP flag. Since (hopefully) all our code check for this flag
via rtisvalid(9) and it is serialized with ifdetach(), we're safe.
But as soon as CPU0 will be able to execute the chunk below while
CPU1 is detaching the interface pointed by rt_ifp we can trigger
a bad dereference quite complicated to reproduce.
By making use of indexes with if_get()/if_put() we are sure that
the interface won't disappear while we're accessing it.
Another alternative would have been to reference count the interface
pointer on the route entry but this doesn't really fly with our
approach of sleeping until all the references are dropped when an
interface is detached.
Ok?
Index: netinet/ip_input.c
===
RCS file: /cvs/src/sys/netinet/ip_input.c,v
retrieving revision 1.261
diff -u -p -u -5 -r1.261 ip_input.c
--- netinet/ip_input.c 14 Nov 2015 15:40:40 - 1.261
+++ netinet/ip_input.c 19 Nov 2015 12:36:05 -
@@ -1514,12 +1514,18 @@ ip_forward(struct mbuf *m, struct ifnet
if (ipforward_rt.ro_rt) {
struct rtentry *rt = ipforward_rt.ro_rt;
if (rt->rt_rmx.rmx_mtu)
destmtu = rt->rt_rmx.rmx_mtu;
- else
- destmtu = ipforward_rt.ro_rt->rt_ifp->if_mtu;
+ else {
+ struct ifnet *destifp;
+
+ destifp = if_get(rt->rt_ifidx);
+ if (destifp != NULL)
+ destmtu = destifp->if_mtu;
+ if_put(destifp);
+ }
}
#endif /*IPSEC*/
ipstat.ips_cantfrag++;
break;
