On 03/03/17(Fri) 01:47, Alexander Bluhm wrote:
> On Thu, Mar 02, 2017 at 10:55:41AM +0100, Martin Pieuchot wrote:
> > Sleeping here is completely ok. The NET_LOCK() in this function is only
> > taken to make sure no other thread will try to do a route lookup in
> > ip_output() while we're messing with the routing table.
>
> Then I think your change is ok. The kernel lock in route_input()
> should protect us. But please resend the diff after merging with
> krw@'s rtm proposal commit.
I need to refactor this spaghetti code to avoid multiple gotos. So
let's move the guts of route_output() to a function that will need
the NET_LOCK().
I'd also like to rename all functions playing with routing messages
to have the rtm_ prefix, because it is quite confusing to see more
and more rt_* popping around.
ok?
Index: net/rtsock.c
===================================================================
RCS file: /cvs/src/sys/net/rtsock.c,v
retrieving revision 1.226
diff -u -p -r1.226 rtsock.c
--- net/rtsock.c 2 Mar 2017 17:09:21 -0000 1.226
+++ net/rtsock.c 3 Mar 2017 07:52:46 -0000
@@ -103,6 +103,8 @@ void route_input(struct mbuf *m0, sa_fam
int route_arp_conflict(struct rtentry *, struct rt_addrinfo *);
int route_cleargateway(struct rtentry *, void *, unsigned int);
+int rtm_ouput(struct rt_msghdr *, struct rtentry **, struct rt_addrinfo *,
+ uint8_t, unsigned int);
struct mbuf *rt_msg1(int, struct rt_addrinfo *);
int rt_msg2(int, int, struct rt_addrinfo *, caddr_t,
struct walkarg *);
@@ -536,18 +538,13 @@ route_output(struct mbuf *m, ...)
struct rt_msghdr *rtm = NULL;
struct rtentry *rt = NULL;
struct rt_addrinfo info;
- int plen, len, seq, newgate = 0, error = 0;
- struct ifnet *ifp = NULL;
- struct ifaddr *ifa = NULL;
+ int len, seq, error = 0;
struct socket *so;
struct rawcb *rp = NULL;
-#ifdef MPLS
- struct sockaddr_mpls *psa_mpls;
-#endif
va_list ap;
u_int tableid;
- u_int8_t prio;
u_char vers, type;
+ u_int8_t prio;
va_start(ap, m);
so = va_arg(ap, struct socket *);
@@ -690,21 +687,82 @@ route_output(struct mbuf *m, ...)
error = EINVAL;
goto fail;
}
- goto flush;
+ } else {
+ error = rtm_ouput(rtm, &rt, &info, prio, tableid);
+ if (!error) {
+ type = rtm->rtm_type;
+ seq = rtm->rtm_seq;
+ free(rtm, M_RTABLE, 0);
+ rtm = rt_report(rt, type, seq, tableid);
+ }
+ }
+
+ rtfree(rt);
+ if (rtm == NULL) {
+ error = ENOBUFS;
+ goto fail;
+ } else if (error) {
+ rtm->rtm_errno = error;
+ } else {
+ rtm->rtm_flags |= RTF_DONE;
}
+ /*
+ * Check to see if we don't want our own messages.
+ */
+ if (!(so->so_options & SO_USELOOPBACK)) {
+ if (route_cb.any_count <= 1) {
+ /* no other listener and no loopback of messages */
+fail:
+ free(rtm, M_RTABLE, 0);
+ m_freem(m);
+ return (error);
+ }
+ /* There is another listener, so construct message */
+ rp = sotorawcb(so);
+ rp->rcb_proto.sp_family = 0; /* Avoid us */
+ }
+ if (rtm) {
+ if (m_copyback(m, 0, rtm->rtm_msglen, rtm, M_NOWAIT)) {
+ m_freem(m);
+ m = NULL;
+ } else if (m->m_pkthdr.len > rtm->rtm_msglen)
+ m_adj(m, rtm->rtm_msglen - m->m_pkthdr.len);
+ free(rtm, M_RTABLE, 0);
+ }
+ if (m)
+ route_input(m, info.rti_info[RTAX_DST] ?
+ info.rti_info[RTAX_DST]->sa_family : AF_UNSPEC);
+ if (rp)
+ rp->rcb_proto.sp_family = PF_ROUTE; /* Readd us */
+
+ return (error);
+}
+
+int
+rtm_ouput(struct rt_msghdr *rtm, struct rtentry **prt,
+ struct rt_addrinfo *info, uint8_t prio, unsigned int tableid)
+{
+ struct rtentry *rt = *prt;
+ struct ifnet *ifp = NULL;
+ struct ifaddr *ifa = NULL;
+#ifdef MPLS
+ struct sockaddr_mpls *psa_mpls;
+#endif
+ int plen, newgate = 0, error = 0;
+
switch (rtm->rtm_type) {
case RTM_ADD:
- if (info.rti_info[RTAX_GATEWAY] == NULL) {
+ if (info->rti_info[RTAX_GATEWAY] == NULL) {
error = EINVAL;
- goto flush;
+ break;
}
- rt = rtable_match(tableid, info.rti_info[RTAX_DST], NULL);
- if ((error = route_arp_conflict(rt, &info))) {
+ rt = rtable_match(tableid, info->rti_info[RTAX_DST], NULL);
+ if ((error = route_arp_conflict(rt, info))) {
rtfree(rt);
rt = NULL;
- goto flush;
+ break;
}
/*
@@ -718,16 +776,16 @@ route_output(struct mbuf *m, ...)
rtfree(rt);
rt = NULL;
- error = rtrequest(RTM_ADD, &info, prio, &rt, tableid);
+ error = rtrequest(RTM_ADD, info, prio, &rt, tableid);
if (error == 0)
rt_setmetrics(rtm->rtm_inits, &rtm->rtm_rmx,
&rt->rt_rmx);
else
- goto flush;
+ break;
break;
case RTM_DELETE:
- rt = rtable_lookup(tableid, info.rti_info[RTAX_DST],
- info.rti_info[RTAX_NETMASK], info.rti_info[RTAX_GATEWAY],
+ rt = rtable_lookup(tableid, info->rti_info[RTAX_DST],
+ info->rti_info[RTAX_NETMASK], info->rti_info[RTAX_GATEWAY],
prio);
/*
@@ -759,14 +817,14 @@ route_output(struct mbuf *m, ...)
rtfree(rt);
rt = NULL;
- error = rtrequest(RTM_DELETE, &info, prio, &rt, tableid);
+ error = rtrequest(RTM_DELETE, info, prio, &rt, tableid);
if (error != 0)
- goto flush;
+ break;
break;
case RTM_CHANGE:
case RTM_LOCK:
- rt = rtable_lookup(tableid, info.rti_info[RTAX_DST],
- info.rti_info[RTAX_NETMASK], info.rti_info[RTAX_GATEWAY],
+ rt = rtable_lookup(tableid, info->rti_info[RTAX_DST],
+ info->rti_info[RTAX_NETMASK], info->rti_info[RTAX_GATEWAY],
prio);
#ifndef SMALL_KERNEL
/*
@@ -774,7 +832,7 @@ route_output(struct mbuf *m, ...)
* a matching gateway.
*/
if ((rt != NULL) && ISSET(rt->rt_flags, RTF_MPATH) &&
- (info.rti_info[RTAX_GATEWAY] == NULL)) {
+ (info->rti_info[RTAX_GATEWAY] == NULL)) {
rtfree(rt);
rt = NULL;
}
@@ -783,10 +841,10 @@ route_output(struct mbuf *m, ...)
* If RTAX_GATEWAY is the argument we're trying to
* change, try to find a compatible route.
*/
- if ((rt == NULL) && (info.rti_info[RTAX_GATEWAY] != NULL) &&
+ if ((rt == NULL) && (info->rti_info[RTAX_GATEWAY] != NULL) &&
(rtm->rtm_type == RTM_CHANGE)) {
- rt = rtable_lookup(tableid, info.rti_info[RTAX_DST],
- info.rti_info[RTAX_NETMASK], NULL, prio);
+ rt = rtable_lookup(tableid, info->rti_info[RTAX_DST],
+ info->rti_info[RTAX_NETMASK], NULL, prio);
#ifndef SMALL_KERNEL
/* Ensure we don't pick a multipath one. */
if ((rt != NULL) && ISSET(rt->rt_flags, RTF_MPATH)) {
@@ -798,26 +856,26 @@ route_output(struct mbuf *m, ...)
if (rt == NULL) {
error = ESRCH;
- goto flush;
+ break;
}
/*
* RTM_CHANGE/LOCK need a perfect match.
*/
- plen = rtable_satoplen(info.rti_info[RTAX_DST]->sa_family,
- info.rti_info[RTAX_NETMASK]);
+ plen = rtable_satoplen(info->rti_info[RTAX_DST]->sa_family,
+ info->rti_info[RTAX_NETMASK]);
if (rt_plen(rt) != plen ) {
error = ESRCH;
- goto flush;
+ break;
}
switch (rtm->rtm_type) {
case RTM_CHANGE:
- if (info.rti_info[RTAX_GATEWAY] != NULL)
+ if (info->rti_info[RTAX_GATEWAY] != NULL)
if (rt->rt_gateway == NULL ||
bcmp(rt->rt_gateway,
- info.rti_info[RTAX_GATEWAY],
- info.rti_info[RTAX_GATEWAY]->sa_len)) {
+ info->rti_info[RTAX_GATEWAY],
+ info->rti_info[RTAX_GATEWAY]->sa_len)) {
newgate = 1;
}
/*
@@ -826,11 +884,11 @@ route_output(struct mbuf *m, ...)
* flags may also be different; ifp may be specified
* by ll sockaddr when protocol address is ambiguous.
*/
- if (newgate || info.rti_info[RTAX_IFP] != NULL ||
- info.rti_info[RTAX_IFA] != NULL) {
- if ((error = rt_getifa(&info, tableid)) != 0)
- goto flush;
- ifa = info.rti_ifa;
+ if (newgate || info->rti_info[RTAX_IFP] != NULL ||
+ info->rti_info[RTAX_IFA] != NULL) {
+ if ((error = rt_getifa(info, tableid)) != 0)
+ break;
+ ifa = info->rti_ifa;
if (rt->rt_ifa != ifa) {
ifp = if_get(rt->rt_ifidx);
KASSERT(ifp != NULL);
@@ -849,17 +907,17 @@ route_output(struct mbuf *m, ...)
}
}
change:
- if (info.rti_info[RTAX_GATEWAY] != NULL && (error =
- rt_setgate(rt, info.rti_info[RTAX_GATEWAY],
+ if (info->rti_info[RTAX_GATEWAY] != NULL && (error =
+ rt_setgate(rt, info->rti_info[RTAX_GATEWAY],
tableid)))
- goto flush;
+ break;
#ifdef MPLS
if ((rtm->rtm_flags & RTF_MPLS) &&
- info.rti_info[RTAX_SRC] != NULL) {
+ info->rti_info[RTAX_SRC] != NULL) {
struct rt_mpls *rt_mpls;
psa_mpls = (struct sockaddr_mpls *)
- info.rti_info[RTAX_SRC];
+ info->rti_info[RTAX_SRC];
if (rt->rt_llinfo == NULL) {
rt->rt_llinfo =
@@ -868,7 +926,7 @@ change:
}
if (rt->rt_llinfo == NULL) {
error = ENOMEM;
- goto flush;
+ break;
}
rt_mpls = (struct rt_mpls *)rt->rt_llinfo;
@@ -878,7 +936,7 @@ change:
psa_mpls->smpls_label;
}
- rt_mpls->mpls_operation = info.rti_mpls;
+ rt_mpls->mpls_operation = info->rti_mpls;
/* XXX: set experimental bits */
@@ -898,7 +956,7 @@ change:
#ifdef BFD
if (ISSET(rtm->rtm_flags, RTF_BFD)) {
if ((error = bfdset(rt)))
- goto flush;
+ break;
} else if (!ISSET(rtm->rtm_flags, RTF_BFD) &&
ISSET(rtm->rtm_fmask, RTF_BFD)) {
bfdclear(rt);
@@ -919,14 +977,14 @@ change:
ifp->if_rtrequest(ifp, RTM_ADD, rt);
if_put(ifp);
- if (info.rti_info[RTAX_LABEL] != NULL) {
+ if (info->rti_info[RTAX_LABEL] != NULL) {
char *rtlabel = ((struct sockaddr_rtlabel *)
- info.rti_info[RTAX_LABEL])->sr_label;
+ info->rti_info[RTAX_LABEL])->sr_label;
rtlabel_unref(rt->rt_labelid);
rt->rt_labelid = rtlabel_name2id(rtlabel);
}
- if_group_routechange(info.rti_info[RTAX_DST],
- info.rti_info[RTAX_NETMASK]);
+ if_group_routechange(info->rti_info[RTAX_DST],
+ info->rti_info[RTAX_NETMASK]);
/* FALLTHROUGH */
case RTM_LOCK:
rt->rt_rmx.rmx_locks &= ~(rtm->rtm_inits);
@@ -936,71 +994,17 @@ change:
}
break;
case RTM_GET:
- rt = rtable_lookup(tableid, info.rti_info[RTAX_DST],
- info.rti_info[RTAX_NETMASK], info.rti_info[RTAX_GATEWAY],
+ rt = rtable_lookup(tableid, info->rti_info[RTAX_DST],
+ info->rti_info[RTAX_NETMASK], info->rti_info[RTAX_GATEWAY],
prio);
if (rt == NULL) {
error = ESRCH;
- goto flush;
+ break;
}
break;
}
- /*
- * From here on these vars need to be valid
- * rt, rtm, error, so, m, tableid, sa_family
- *
- * Other notes:
- * - to end up here previous calls passed OK, error is most probably 0
- * - error cases take the flush route or in bad cases fail
- * - fail does not report the message back but just fails the call
- * if the message is not valid then fail should be used
- */
-
- type = rtm->rtm_type;
- seq = rtm->rtm_seq;
- free(rtm, M_RTABLE, 0);
- rtm = rt_report(rt, type, seq, tableid);
-flush:
- rtfree(rt);
- if (rtm == NULL) {
- error = ENOBUFS;
- goto fail;
- } else if (error) {
- rtm->rtm_errno = error;
- } else {
- rtm->rtm_flags |= RTF_DONE;
- }
-
- /*
- * Check to see if we don't want our own messages.
- */
- if (!(so->so_options & SO_USELOOPBACK)) {
- if (route_cb.any_count <= 1) {
- /* no other listener and no loopback of messages */
-fail:
- free(rtm, M_RTABLE, 0);
- m_freem(m);
- return (error);
- }
- /* There is another listener, so construct message */
- rp = sotorawcb(so);
- rp->rcb_proto.sp_family = 0; /* Avoid us */
- }
- if (rtm) {
- if (m_copyback(m, 0, rtm->rtm_msglen, rtm, M_NOWAIT)) {
- m_freem(m);
- m = NULL;
- } else if (m->m_pkthdr.len > rtm->rtm_msglen)
- m_adj(m, rtm->rtm_msglen - m->m_pkthdr.len);
- free(rtm, M_RTABLE, 0);
- }
- if (m)
- route_input(m, info.rti_info[RTAX_DST] ?
- info.rti_info[RTAX_DST]->sa_family : AF_UNSPEC);
- if (rp)
- rp->rcb_proto.sp_family = PF_ROUTE; /* Readd us */
-
+ *prt = rt;
return (error);
}
