Date: Tue, 27 Nov 2007 21:44:46 +0100 From: Max Laier <max@love2party.net> To: Florian Smeets <flo@kasimir.com> Cc: freebsd-pf@freebsd.org Subject: ALTQ for dynamic interfaces [Re: 7-STABLE panic: mtx_lock() of spin mutex %s @ %s:%d] Message-ID: <200711272144.52511.max@love2party.net> In-Reply-To: <474BE383.6050905@kasimir.com> References: <474B5BD0.6040004@kasimir.com> <200711270130.01165.max@love2party.net> <474BE383.6050905@kasimir.com>
next in thread | previous in thread | raw e-mail | index | archive | help
--nextPart2168564.eTSo7kHDG8
Content-Type: multipart/mixed;
boundary="Boundary-01=_/GITHJDfvfmxyCv"
Content-Transfer-Encoding: 7bit
Content-Disposition: inline
--Boundary-01=_/GITHJDfvfmxyCv
Content-Type: text/plain;
charset="iso-8859-1"
Content-Transfer-Encoding: quoted-printable
Content-Disposition: inline
On Tuesday 27 November 2007, Florian Smeets wrote:
> Max Laier wrote:
> > On Tuesday 27 November 2007, Florian Smeets wrote:
> >> Hi
> >>
> >> i was able to reproduce a hang on a 7-STABLE (csuped just after
> >> Scotts critical section MFC) firewall which runs mpd4 from ports and
> >> uses pf for packet filtering. Sometimes when i restart mpd4 the box
> >> just hangs. I have a up-script which calls /sbin/pfctl -f
> >> /etc/pf.conf.
> >
> > It's an ALTQ problem, really. I don't know if there is a down-script
> > that runs before the interface is destroyed, but if there is - adding
> > pfctl -Fq there should work around your problem. I did have some
> > patches to have ALTQ working with dynamic interfaces, but I must have
> > dropped them. I'll see what I can dig up in the next few days.
>
> That would be great. I have a second non productive box where i can
> reproduce the problem. I'll be glad to test any patches.
Okay ... try this. Not final yet, but should be functional. With this=20
you should be able to:
1) Safely remove an interface with active queues
2) Re-add the interface and *magically* get the queues back
3) Write queue rules for non-existing interfaces
- Note that we will assume an MTU of 1500 and you have to specify a=20
fixed bandwidth as we don't know the interface's native speed
- Obviously these queues will be activated as soon as a matching=20
interface is created.
BUGS: Doesn't print queues on removed interfaces at all. Should be=20
changed to something like "queue foo on bar0 (N/A) ...", but it seems I=20
was too strict with the local_flags. The error handling might need some=20
work in order to avoid panic if something goes wrong while we de-activate=20
queues.
I'd like to hear back from you in order to see if I at least got the basic=
=20
workings right enough so you can survive the mpd interface destroy. =20
Could you - in addition to you current setup w/ if-up script - also test=20
the magic part? i.e. load the ruleset before loading mpd. This should=20
now be possible as long as you don't put "set loginterface" or fixed=20
interface-to-address src/dst in it \o/
=2D-=20
/"\ Best regards, | mlaier@freebsd.org
\ / Max Laier | ICQ #67774661
X http://pf4freebsd.love2party.net/ | mlaier@EFnet
/ \ ASCII Ribbon Campaign | Against HTML Mail and News
--Boundary-01=_/GITHJDfvfmxyCv
Content-Type: text/x-diff;
charset="iso-8859-1";
name="pf.dyn_altq.diff"
Content-Transfer-Encoding: quoted-printable
Content-Disposition: attachment;
filename="pf.dyn_altq.diff"
diff --git a/contrib/pf/pfctl/pfctl_altq.c b/contrib/pf/pfctl/pfctl_altq.c
index 9104f5a..d2a21c8 100644
=2D-- a/contrib/pf/pfctl/pfctl_altq.c
+++ b/contrib/pf/pfctl/pfctl_altq.c
@@ -154,6 +154,10 @@ print_altq(const struct pf_altq *a, unsigned level, st=
ruct node_queue_bw *bw,
}
=20
printf("altq on %s ", a->ifname);
+#ifdef __FreeBSD__
+ if (a->local_flags)
+ printf("N/A ");
+#endif
=20
switch (a->scheduler) {
case ALTQT_CBQ:
@@ -1145,7 +1149,11 @@ getifmtu(char *ifname)
sizeof(ifr.ifr_name))
errx(1, "getifmtu: strlcpy");
if (ioctl(s, SIOCGIFMTU, (caddr_t)&ifr) =3D=3D -1)
+#ifdef __FreeBSD__
+ ifr.ifr_mtu =3D 1500;
+#else
err(1, "SIOCGIFMTU");
+#endif
if (shutdown(s, SHUT_RDWR) =3D=3D -1)
err(1, "shutdown");
if (close(s))
diff --git a/contrib/pf/pfctl/pfctl_qstats.c b/contrib/pf/pfctl/pfctl_qstat=
s.c
index 3eb2987..15695cb 100644
=2D-- a/contrib/pf/pfctl/pfctl_qstats.c
+++ b/contrib/pf/pfctl/pfctl_qstats.c
@@ -157,7 +157,11 @@ pfctl_update_qstats(int dev, struct pf_altq_node **roo=
t)
warn("DIOCGETALTQ");
return (-1);
}
+#ifdef __FreeBSD__
+ if (pa.altq.qid > 0 && !pa.altq.local_flags) {
+#else
if (pa.altq.qid > 0) {
+#endif
pq.nr =3D nr;
pq.ticket =3D pa.ticket;
pq.buf =3D &qstats.data;
diff --git a/sys/contrib/pf/net/pf_if.c b/sys/contrib/pf/net/pf_if.c
index 74e9dcb..8b09cc3 100644
=2D-- a/sys/contrib/pf/net/pf_if.c
+++ b/sys/contrib/pf/net/pf_if.c
@@ -893,6 +893,9 @@ pfi_attach_ifnet_event(void *arg __unused, struct ifnet=
*ifp)
{
PF_LOCK();
pfi_attach_ifnet(ifp);
+#ifdef ALTQ
+ pf_altq_ifnet_event(ifp, 0);
+#endif
PF_UNLOCK();
}
=20
@@ -901,6 +904,9 @@ pfi_detach_ifnet_event(void *arg __unused, struct ifnet=
*ifp)
{
PF_LOCK();
pfi_detach_ifnet(ifp);
+#ifdef ALTQ
+ pf_altq_ifnet_event(ifp, 1);
+#endif
PF_UNLOCK();
}
=20
diff --git a/sys/contrib/pf/net/pf_ioctl.c b/sys/contrib/pf/net/pf_ioctl.c
index 136e710..0681abf 100644
=2D-- a/sys/contrib/pf/net/pf_ioctl.c
+++ b/sys/contrib/pf/net/pf_ioctl.c
@@ -787,7 +787,11 @@ pf_begin_altq(u_int32_t *ticket)
/* Purge the old altq list */
while ((altq =3D TAILQ_FIRST(pf_altqs_inactive)) !=3D NULL) {
TAILQ_REMOVE(pf_altqs_inactive, altq, entries);
+#ifdef __FreeBSD__
+ if (altq->qname[0] =3D=3D 0 && altq->local_flags =3D=3D 0) {
+#else
if (altq->qname[0] =3D=3D 0) {
+#endif
/* detach and destroy the discipline */
error =3D altq_remove(altq);
} else
@@ -812,7 +816,11 @@ pf_rollback_altq(u_int32_t ticket)
/* Purge the old altq list */
while ((altq =3D TAILQ_FIRST(pf_altqs_inactive)) !=3D NULL) {
TAILQ_REMOVE(pf_altqs_inactive, altq, entries);
+#ifdef __FreeBSD__
+ if (altq->qname[0] =3D=3D 0 && altq->local_flags =3D=3D 0) {
+#else
if (altq->qname[0] =3D=3D 0) {
+#endif
/* detach and destroy the discipline */
error =3D altq_remove(altq);
} else
@@ -842,7 +850,11 @@ pf_commit_altq(u_int32_t ticket)
=20
/* Attach new disciplines */
TAILQ_FOREACH(altq, pf_altqs_active, entries) {
+#ifdef __FreeBSD__
+ if (altq->qname[0] =3D=3D 0 && altq->local_flags =3D=3D 0) {
+#else
if (altq->qname[0] =3D=3D 0) {
+#endif
/* attach the discipline */
error =3D altq_pfattach(altq);
if (error =3D=3D 0 && pf_altq_running)
@@ -857,7 +869,11 @@ pf_commit_altq(u_int32_t ticket)
/* Purge the old altq list */
while ((altq =3D TAILQ_FIRST(pf_altqs_inactive)) !=3D NULL) {
TAILQ_REMOVE(pf_altqs_inactive, altq, entries);
+#ifdef __FreeBSD__
+ if (altq->qname[0] =3D=3D 0 && altq->local_flags =3D=3D 0) {
+#else
if (altq->qname[0] =3D=3D 0) {
+#endif
/* detach and destroy the discipline */
if (pf_altq_running)
error =3D pf_disable_altq(altq);
@@ -943,6 +959,91 @@ pf_disable_altq(struct pf_altq *altq)
=20
return (error);
}
+
+#ifdef __FreeBSD__
+void
+pf_altq_ifnet_event(struct ifnet *ifp, int remove)
+{
+ struct ifnet *ifp1;
+ struct pf_altq *a1, *a2, *a3;
+ u_int32_t ticket;
+ int error =3D 0;
+
+ DPFPRINTF(PF_DEBUG_MISC, ("altq: ifnet event %p %d\n", ifp, remove));
+ /* Interrupt userland queue modifications */
+ if (altqs_inactive_open) {
+ DPFPRINTF(PF_DEBUG_MISC,
+ ("altq: detach event preempted userland\n"));
+ pf_rollback_altq(ticket_altqs_inactive);
+ }
+
+ /* Start new altq ruleset */
+ if (pf_begin_altq(&ticket)) {
+ DPFPRINTF(PF_DEBUG_MISC, ("altq: pf_begin_altq failed\n"));
+ PF_UNLOCK();
+ return;
+ }
+ /* Copy the current active set */
+ TAILQ_FOREACH(a1, pf_altqs_active, entries) {
+ a2 =3D pool_get(&pf_altq_pl, PR_NOWAIT);
+ if (a2 =3D=3D NULL) {
+ error =3D ENOMEM;
+ break;
+ }
+ bcopy(a1, a2, sizeof(struct pf_altq));
+
+ DPFPRINTF(PF_DEBUG_MISC, ("altq: copy %s.%s\n", a2->ifname,
+ a2->qname));
+ if (a2->qname[0] !=3D 0) {
+ if ((a2->qid =3D pf_qname2qid(a2->qname)) =3D=3D 0) {
+ error =3D EBUSY;
+ pool_put(&pf_altq_pl, a2);
+ break;
+ }
+ a2->altq_disc =3D NULL;
+ TAILQ_FOREACH(a3, pf_altqs_inactive, entries) {
+ if (strncmp(a3->ifname, a2->ifname,
+ IFNAMSIZ) =3D=3D 0 && a3->qname[0] =3D=3D 0) {
+ a2->altq_disc =3D a3->altq_disc;
+ break;
+ }
+ }
+ }
+ /* Deactivate the interface in question */
+ a2->local_flags =3D 0;
+ if ((ifp1 =3D ifunit(a2->ifname)) =3D=3D NULL ||
+ (remove && ifp1 =3D=3D ifp)) {
+ a2->local_flags |=3D PFALTQ_FLAG_IF_REMOVED;
+ DPFPRINTF(PF_DEBUG_MISC,
+ ("altq: no interface for %s.%s\n", a2->ifname,
+ a2->qname));
+ } else {
+ PF_UNLOCK();
+ error =3D altq_add(a2);
+ PF_LOCK();
+
+ if (ticket !=3D ticket_altqs_inactive)
+ error =3D EBUSY;
+
+ if (error) {
+ pool_put(&pf_altq_pl, a2);
+ break;
+ }
+ }
+ DPFPRINTF(PF_DEBUG_MISC, ("altq: adding %s.%s\n", a2->ifname,
+ a2->qname));
+
+ TAILQ_INSERT_TAIL(pf_altqs_inactive, a2, entries);
+ }
+
+ if (error !=3D 0) {
+ DPFPRINTF(PF_DEBUG_MISC, ("altq: copy failed %d\n", error));
+ pf_rollback_altq(ticket);
+ } else {
+ pf_commit_altq(ticket);
+ }
+}
+#endif
#endif /* ALTQ */
=20
int
@@ -2273,7 +2374,12 @@ pfioctl(dev_t dev, u_long cmd, caddr_t addr, int fla=
gs, struct proc *p)
=20
/* enable all altq interfaces on active list */
TAILQ_FOREACH(altq, pf_altqs_active, entries) {
+#ifdef __FreeBSD__
+ if (altq->qname[0] =3D=3D 0 &&
+ altq->local_flags =3D=3D 0) {
+#else
if (altq->qname[0] =3D=3D 0) {
+#endif
error =3D pf_enable_altq(altq);
if (error !=3D 0)
break;
@@ -2290,7 +2396,12 @@ pfioctl(dev_t dev, u_long cmd, caddr_t addr, int fla=
gs, struct proc *p)
=20
/* disable all altq interfaces on active list */
TAILQ_FOREACH(altq, pf_altqs_active, entries) {
+#ifdef __FreeBSD__
+ if (altq->qname[0] =3D=3D 0 &&
+ altq->local_flags =3D=3D 0) {
+#else
if (altq->qname[0] =3D=3D 0) {
+#endif
error =3D pf_disable_altq(altq);
if (error !=3D 0)
break;
@@ -2316,6 +2427,9 @@ pfioctl(dev_t dev, u_long cmd, caddr_t addr, int flag=
s, struct proc *p)
break;
}
bcopy(&pa->altq, altq, sizeof(struct pf_altq));
+#ifdef __FreeBSD__
+ altq->local_flags =3D 0;
+#endif
=20
/*
* if this is for a queue, find the discipline and
@@ -2327,6 +2441,7 @@ pfioctl(dev_t dev, u_long cmd, caddr_t addr, int flag=
s, struct proc *p)
pool_put(&pf_altq_pl, altq);
break;
}
+ altq->altq_disc =3D NULL;
TAILQ_FOREACH(a, pf_altqs_inactive, entries) {
if (strncmp(a->ifname, altq->ifname,
IFNAMSIZ) =3D=3D 0 && a->qname[0] =3D=3D 0) {
@@ -2337,11 +2452,17 @@ pfioctl(dev_t dev, u_long cmd, caddr_t addr, int fl=
ags, struct proc *p)
}
=20
#ifdef __FreeBSD__
=2D PF_UNLOCK();
+ struct ifnet *ifp;
+
+ if ((ifp =3D ifunit(altq->ifname)) =3D=3D NULL) {
+ altq->local_flags |=3D PFALTQ_FLAG_IF_REMOVED;
+ } else {
+ PF_UNLOCK();
#endif =09
error =3D altq_add(altq);
#ifdef __FreeBSD__
=2D PF_LOCK();
+ PF_LOCK();
+ }
#endif
if (error) {
pool_put(&pf_altq_pl, altq);
@@ -2414,6 +2535,10 @@ pfioctl(dev_t dev, u_long cmd, caddr_t addr, int fla=
gs, struct proc *p)
break;
}
#ifdef __FreeBSD__
+ if (altq->local_flags !=3D 0) {
+ error =3D ENXIO;
+ break;
+ }
PF_UNLOCK();
#endif
error =3D altq_getqstats(altq, pq->buf, &nbytes);
diff --git a/sys/contrib/pf/net/pfvar.h b/sys/contrib/pf/net/pfvar.h
index fbc87e3..8673594 100644
=2D-- a/sys/contrib/pf/net/pfvar.h
+++ b/sys/contrib/pf/net/pfvar.h
@@ -1247,6 +1247,11 @@ struct pf_altq {
u_int32_t parent_qid; /* parent queue id */
u_int32_t bandwidth; /* queue bandwidth */
u_int8_t priority; /* priority */
+#ifdef __FreeBSD__
+ u_int8_t local_flags; /* dynamic interface */
+#define PFALTQ_FLAG_IF_REMOVED 0x01
+#define PFALTQ_FLAG_BW_UNKNOWN 0x02
+#endif
u_int16_t qlimit; /* queue size limit */
u_int16_t flags; /* misc flags */
union {
@@ -1574,6 +1579,9 @@ extern void pf_tbladdr_remove(struct pf_addr_wrap =
*);
extern void pf_tbladdr_copyout(struct pf_addr_wrap *);
extern void pf_calc_skip_steps(struct pf_rulequeue *);
#ifdef __FreeBSD__
+#ifdef ALTQ
+extern void pf_altq_ifnet_event(struct ifnet *, int);
+#endif
extern uma_zone_t pf_src_tree_pl, pf_rule_pl;
extern uma_zone_t pf_state_pl, pf_altq_pl, pf_pooladdr_pl;
extern uma_zone_t pfr_ktable_pl, pfr_kentry_pl, pfr_kentry_pl2;
--Boundary-01=_/GITHJDfvfmxyCv--
--nextPart2168564.eTSo7kHDG8
Content-Type: application/pgp-signature; name=signature.asc
Content-Description: This is a digitally signed message part.
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2.0.4 (FreeBSD)
iD8DBQBHTIHEXyyEoT62BG0RAg2RAJ9wdmOOzrXb7eQ/20TFk10RE/5jCQCfdR15
24jLZrm/1wWmZJGt8T8sGcU=
=aSkr
-----END PGP SIGNATURE-----
--nextPart2168564.eTSo7kHDG8--
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?200711272144.52511.max>