Page MenuHomeFreeBSD
Differential D36277 Diff 109628 sys/netpfil/pf/if_pfsync.c

Changeset View

Standalone View

View Options

sys/netpfil/pf/if_pfsync.c

Context not available.
#include <net/if_types.h> #include <net/if_types.h>
#include <net/vnet.h> #include <net/vnet.h>
#include <net/pfvar.h> #include <net/pfvar.h>
#include <net/route.h>
glebiusUnsubmitted
Not Done
Inline Actions

What did bring in this requirement?

glebius: What did bring in this requirement?
email_luiz.eng.brAuthorUnsubmitted
Done
Inline Actions

What did bring in this requirement?

email_luiz.eng.br: > What did bring in this requirement?
email_luiz.eng.brAuthorUnsubmitted
Not Done
Inline Actions

It doesn't belong on this diff, ended up here by accident.

With the IPv6 code in place, it's needed in order to have RT_DEFAULT_FIB for a in6_selectsrc_addr() call.

email_luiz.eng.br: It doesn't belong on this diff, ended up here by accident. With the IPv6 code in place, it's…
#include <net/if_pfsync.h> #include <net/if_pfsync.h>
#include <netinet/if_ether.h> #include <netinet/if_ether.h>
Context not available.
#include <netinet/tcp_fsm.h> #include <netinet/tcp_fsm.h>
#include <netinet/tcp_seq.h> #include <netinet/tcp_seq.h>
struct pfsync_bucket;
union inet_template {
struct ip ipv4;
};
#define PFSYNC_MINPKT ( \ #define PFSYNC_MINPKT ( \
sizeof(struct ip) + \ sizeof(union inet_template) + \
sizeof(struct pfsync_header) + \ sizeof(struct pfsync_header) + \
sizeof(struct pfsync_subheader) ) sizeof(struct pfsync_subheader) )
struct pfsync_bucket;
struct pfsync_pkt { struct pfsync_pkt {
struct ip *ip; #ifdef INET
struct in_addr src; struct ip *ip4;
kpUnsubmitted
Not Done
Inline Actions

Same question here. If we change the size of pfsync_pkt we're going to be changing the on-wire protocol, and that's going to break sync between versions with and without IPv6 support.

kp: Same question here. If we change the size of pfsync_pkt we're going to be changing the on-wire…
email_luiz.eng.brAuthorUnsubmitted
Not Done
Inline Actions

Same as the previous case, I did the change without being aware of the implications.

I honestly don't have an idea on how to deal with it. I welcome any suggestions.

email_luiz.eng.br: Same as the previous case, I did the change without being aware of the implications. I…
struct in_addr src4;
#endif
sa_family_t af;
u_int8_t flags; u_int8_t flags;
}; };
Context not available.
struct ifnet *sc_ifp; struct ifnet *sc_ifp;
struct ifnet *sc_sync_if; struct ifnet *sc_sync_if;
struct ip_moptions sc_imo; struct ip_moptions sc_imo;
struct in_addr sc_sync_peer; union pfsync_sockaddr sc_sync_peer;
uint32_t sc_flags; uint32_t sc_flags;
uint8_t sc_maxupdates; uint8_t sc_maxupdates;
struct ip sc_template; union inet_template sc_template;
struct mtx sc_mtx; struct mtx sc_mtx;
/* Queued data */ /* Queued data */
Context not available.
return (error); return (error);
} }
#ifdef INET
static int static int
pfsync_input(struct mbuf **mp, int *offp __unused, int proto __unused) pfsync_input(struct mbuf **mp, int *offp __unused, int proto __unused)
{ {
Context not available.
} }
/* Cheaper to grab this now than having to mess with mbufs later */ /* Cheaper to grab this now than having to mess with mbufs later */
pkt.ip = ip; // XXX: Is this even needed? Except for flags, other values seem unused
pkt.src = ip->ip_src; pkt.af = AF_INET;
pkt.ip4 = ip;
pkt.src4 = ip->ip_src;
pkt.flags = 0; pkt.flags = 0;
/* /*
Context not available.
m_freem(m); m_freem(m);
return (IPPROTO_DONE); return (IPPROTO_DONE);
} }
#endif
static int static int
pfsync_in_clr(struct pfsync_pkt *pkt, struct mbuf *m, int offset, int count) pfsync_in_clr(struct pfsync_pkt *pkt, struct mbuf *m, int offset, int count)
kpUnsubmitted
Done
Inline Actions

Is this comment still relevant?

kp: Is this comment still relevant?
Context not available.
{ {
struct in_mfilter *imf = NULL; struct in_mfilter *imf = NULL;
struct ifnet *sifp; struct ifnet *sifp;
struct ip *ip;
if ((error = priv_check(curthread, PRIV_NETINET_PF)) != 0) if ((error = priv_check(curthread, PRIV_NETINET_PF)) != 0)
return (error); return (error);
kpUnsubmitted
Done
Inline Actions

I'd just ENOMEM here, that's by far the most likely cause anyway, and nvlist_error() isn't going to tell us that.

kp: I'd just ENOMEM here, that's by far the most likely cause anyway, and nvlist_error() isn't…
Context not available.
sifp = NULL; sifp = NULL;
kpUnsubmitted
Done
Inline Actions

We're leaking packed and nvl here.

kp: We're leaking packed and nvl here.
else if ((sifp = ifunit_ref(pfsyncr.pfsyncr_syncdev)) == NULL) else if ((sifp = ifunit_ref(pfsyncr.pfsyncr_syncdev)) == NULL)
return (EINVAL); return (EINVAL);
if (sifp != NULL && (sc->sc_sync_peer.sa.sa_family == AF_UNSPEC)) {
if (sifp != NULL && (
pfsyncr.pfsyncr_syncpeer.s_addr == 0 ||
pfsyncr.pfsyncr_syncpeer.s_addr ==
htonl(INADDR_PFSYNC_GROUP)))
imf = ip_mfilter_alloc(M_WAITOK, 0, 0); imf = ip_mfilter_alloc(M_WAITOK, 0, 0);
}
PFSYNC_LOCK(sc); PFSYNC_LOCK(sc);
if (pfsyncr.pfsyncr_syncpeer.s_addr == 0) if (sifp != NULL && (sc->sc_sync_peer.sa.sa_family == AF_UNSPEC)) {
sc->sc_sync_peer.s_addr = htonl(INADDR_PFSYNC_GROUP); struct sockaddr_in peer_addr;
else peer_addr.sin_family = AF_INET;
sc->sc_sync_peer.s_addr = peer_addr.sin_addr.s_addr = htonl(INADDR_PFSYNC_GROUP);
pfsyncr.pfsyncr_syncpeer.s_addr; sc->sc_sync_peer.in4 = peer_addr;
} else
sc->sc_sync_peer = pfsyncr.pfsyncr_syncpeer;
sc->sc_maxupdates = pfsyncr.pfsyncr_maxupdates; sc->sc_maxupdates = pfsyncr.pfsyncr_maxupdates;
if (pfsyncr.pfsyncr_defer & PFSYNCF_DEFER) { if (pfsyncr.pfsyncr_defer & PFSYNCF_DEFER) {
Context not available.
pfsync_multicast_cleanup(sc); pfsync_multicast_cleanup(sc);
if (sc->sc_sync_peer.s_addr == htonl(INADDR_PFSYNC_GROUP)) { if (sc->sc_sync_peer.in4.sin_addr.s_addr == htonl(INADDR_PFSYNC_GROUP)) {
error = pfsync_multicast_setup(sc, sifp, imf); error = pfsync_multicast_setup(sc, sifp, imf);
if (error) { if (error) {
if_rele(sifp); if_rele(sifp);
Context not available.
if_rele(sc->sc_sync_if); if_rele(sc->sc_sync_if);
sc->sc_sync_if = sifp; sc->sc_sync_if = sifp;
ip = &sc->sc_template; switch (sc->sc_sync_peer.sa.sa_family) {
bzero(ip, sizeof(*ip)); #ifdef INET
ip->ip_v = IPVERSION; case AF_INET: {
ip->ip_hl = sizeof(sc->sc_template) >> 2; struct ip *ip;
ip->ip_tos = IPTOS_LOWDELAY; ip = &sc->sc_template.ipv4;
/* len and id are set later. */ bzero(ip, sizeof(*ip));
ip->ip_off = htons(IP_DF); ip->ip_v = IPVERSION;
ip->ip_ttl = PFSYNC_DFLTTL; ip->ip_hl = sizeof(sc->sc_template.ipv4) >> 2;
ip->ip_p = IPPROTO_PFSYNC; ip->ip_tos = IPTOS_LOWDELAY;
ip->ip_src.s_addr = INADDR_ANY; /* len and id are set later. */
ip->ip_dst.s_addr = sc->sc_sync_peer.s_addr; ip->ip_off = htons(IP_DF);
ip->ip_ttl = PFSYNC_DFLTTL;
ip->ip_p = IPPROTO_PFSYNC;
ip->ip_src.s_addr = INADDR_ANY;
ip->ip_dst.s_addr =
sc->sc_sync_peer.in4.sin_addr.s_addr;
break;
}
#endif
}
/* Request a full state table update. */ /* Request a full state table update. */
if ((sc->sc_flags & PFSYNCF_OK) && carp_demote_adj_p) if ((sc->sc_flags & PFSYNCF_OK) && carp_demote_adj_p)
kpUnsubmitted
Not Done
Inline Actions

We'd leak data here. Unless we failed to allocate data, in which case I'm not sure what copyin() will do.

kp: We'd leak data here. Unless we failed to allocate data, in which case I'm not sure what copyin…
email_luiz.eng.brAuthorUnsubmitted
Not Done
Inline Actions

I am freeing data before returning now.

According to copy(9), "All but copystr() return EFAULT if a bad address is encountered". So if the allocation fails, copyin() will see the address NULL and return EFAULT, right?

email_luiz.eng.br: I am freeing data before returning now. According to `copy(9)`, "All but copystr() return…
kpUnsubmitted
Done
Inline Actions

How so? It should be safe as soon as you've copied the strings out. (numbers are always copied, as are bools. Byte arrays and strings need to be copied, but always should be, because we can't reasonably rely on the lifetime of the nvlist structure).

kp: How so? It should be safe as soon as you've copied the strings out. (numbers are always copied…
email_luiz.eng.brAuthorUnsubmitted
Done
Inline Actions

I wrote that when I still had a very poor understanding of nvlists. I removed the comment.

email_luiz.eng.br: I wrote that when I still had a very poor understanding of nvlists. I removed the comment.
kpUnsubmitted
Not Done
Inline Actions

Leaks data.

kp: Leaks data.
email_luiz.eng.brAuthorUnsubmitted
Not Done
Inline Actions

I went with freeing data right after the nvlist_destroy(), which covers this case and prevents leaking on normal execution as well.

email_luiz.eng.br: I went with freeing data right after the nvlist_destroy(), which covers this case and prevents…
kpUnsubmitted
Not Done
Inline Actions

We should probably separate the input parsing (struct vs nvlist) from the actual pfsync configuration so we can re-use the latter for both cases.

kp: We should probably separate the input parsing (struct vs nvlist) from the actual pfsync…
Context not available.
struct pfsync_softc *sc = V_pfsyncif; struct pfsync_softc *sc = V_pfsyncif;
struct ifnet *ifp = sc->sc_ifp; struct ifnet *ifp = sc->sc_ifp;
struct mbuf *m; struct mbuf *m;
struct ip *ip;
struct pfsync_header *ph; struct pfsync_header *ph;
struct pfsync_subheader *subh; struct pfsync_subheader *subh;
struct pf_kstate *st, *st_next; struct pf_kstate *st, *st_next;
struct pfsync_upd_req_item *ur; struct pfsync_upd_req_item *ur;
struct pfsync_bucket *b = &sc->sc_buckets[c]; struct pfsync_bucket *b = &sc->sc_buckets[c];
int offset; int aflen, offset;
int q, count = 0; int q, count = 0;
KASSERT(sc != NULL, ("%s: null sc", __func__)); KASSERT(sc != NULL, ("%s: null sc", __func__));
Context not available.
m->m_len = m->m_pkthdr.len = b->b_len; m->m_len = m->m_pkthdr.len = b->b_len;
/* build the ip header */ /* build the ip header */
ip = (struct ip *)m->m_data; switch (sc->sc_sync_peer.sa.sa_family) {
bcopy(&sc->sc_template, ip, sizeof(*ip)); #ifdef INET
offset = sizeof(*ip); case AF_INET:
struct ip *ip;
ip = mtod(m, struct ip *);
bcopy(&sc->sc_template.ipv4, ip, sizeof(*ip));
aflen = offset = sizeof(*ip);
ip->ip_len = htons(m->m_pkthdr.len);
ip_fillid(ip);
break;
#endif
default:
return;
}
ip->ip_len = htons(m->m_pkthdr.len);
ip_fillid(ip);
/* build the pfsync header */ /* build the pfsync header */
ph = (struct pfsync_header *)(m->m_data + offset); ph = (struct pfsync_header *)(m->m_data + offset);
Context not available.
offset += sizeof(*ph); offset += sizeof(*ph);
ph->version = PFSYNC_VERSION; ph->version = PFSYNC_VERSION;
ph->len = htons(b->b_len - sizeof(*ip)); ph->len = htons(b->b_len - aflen);
bcopy(V_pf_status.pf_chksum, ph->pfcksum, PF_MD5_DIGEST_LENGTH); bcopy(V_pf_status.pf_chksum, ph->pfcksum, PF_MD5_DIGEST_LENGTH);
/* walk the queues */ /* walk the queues */
Context not available.
/* we're done, let's put it on the wire */ /* we're done, let's put it on the wire */
if (ifp->if_bpf) { if (ifp->if_bpf) {
m->m_data += sizeof(*ip); m->m_data += aflen;
m->m_len = m->m_pkthdr.len = b->b_len - sizeof(*ip); m->m_len = m->m_pkthdr.len = b->b_len - aflen;
BPF_MTAP(ifp, m); BPF_MTAP(ifp, m);
m->m_data -= sizeof(*ip); m->m_data -= aflen;
m->m_len = m->m_pkthdr.len = b->b_len; m->m_len = m->m_pkthdr.len = b->b_len;
} }
Context not available.
free(pd, M_PFSYNC); free(pd, M_PFSYNC);
PFSYNC_BUCKET_UNLOCK(b); PFSYNC_BUCKET_UNLOCK(b);
ip_output(m, NULL, NULL, 0, NULL, NULL); switch (sc->sc_sync_peer.sa.sa_family) {
#ifdef INET
case AF_INET:
ip_output(m, NULL, NULL, 0, NULL, NULL);
break;
#endif
}
pf_release_state(st); pf_release_state(st);
Context not available.
struct pfsync_softc *sc = arg; struct pfsync_softc *sc = arg;
struct pfsync_bucket *b; struct pfsync_bucket *b;
struct mbuf *m, *n; struct mbuf *m, *n;
int c; int c, error;
NET_EPOCH_ENTER(et); NET_EPOCH_ENTER(et);
CURVNET_SET(sc->sc_ifp->if_vnet); CURVNET_SET(sc->sc_ifp->if_vnet);
Context not available.
* own pfsync packet based on M_SKIP_FIREWALL * own pfsync packet based on M_SKIP_FIREWALL
* flag. This is XXX. * flag. This is XXX.
*/ */
if (m->m_flags & M_SKIP_FIREWALL) switch (sc->sc_sync_peer.sa.sa_family) {
ip_output(m, NULL, NULL, 0, NULL, NULL); #ifdef INET
else if (ip_output(m, NULL, NULL, IP_RAWOUTPUT, &sc->sc_imo, case AF_INET:
NULL) == 0) if (m->m_flags & M_SKIP_FIREWALL) {
error = ip_output(m, NULL, NULL, 0,
NULL, NULL);
} else {
error = ip_output(m, NULL, NULL,
IP_RAWOUTPUT, &sc->sc_imo, NULL);
}
break;
#endif
}
if (error == 0)
V_pfsyncstats.pfsyncs_opackets++; V_pfsyncstats.pfsyncs_opackets++;
else else
V_pfsyncstats.pfsyncs_oerrors++; V_pfsyncstats.pfsyncs_oerrors++;
Context not available.
if (!(ifp->if_flags & IFF_MULTICAST)) if (!(ifp->if_flags & IFF_MULTICAST))
return (EADDRNOTAVAIL); return (EADDRNOTAVAIL);
imo->imo_multicast_vif = -1; switch (sc->sc_sync_peer.sa.sa_family) {
#ifdef INET
if ((error = in_joingroup(ifp, &sc->sc_sync_peer, NULL, case AF_INET:
&imf->imf_inm)) != 0) {
return (error); ip_mfilter_init(&imo->imo_head);
imo->imo_multicast_vif = -1;
if ((error = in_joingroup(ifp, &sc->sc_sync_peer.in4.sin_addr, NULL,
&imf->imf_inm)) != 0)
return (error);
ip_mfilter_init(&imo->imo_head); ip_mfilter_insert(&imo->imo_head, imf);
ip_mfilter_insert(&imo->imo_head, imf); imo->imo_multicast_ifp = ifp;
imo->imo_multicast_ifp = ifp; imo->imo_multicast_ttl = PFSYNC_DFLTTL;
imo->imo_multicast_ttl = PFSYNC_DFLTTL; imo->imo_multicast_loop = 0;
imo->imo_multicast_loop = 0; break;
}
#endif
}
return (0); return (0);
} }
Context not available.