On Thu, May 25, 2017 at 07:25 +0200, Sebastien Marie wrote:
> On Wed, May 24, 2017 at 06:03:04PM +0200, Mike Belopuhov wrote:
> > On Sun, May 21, 2017 at 11:53 +0200, Mike Belopuhov wrote:
> > > On Sat, May 20, 2017 at 18:53 +0200, Mike Belopuhov wrote:
> > > > On Sat, May 20, 2017 at 17:30 +0200, Sebastien Marie wrote:
> > > > >
> > > > > From my pf configuration, I had removed queue with bandwidth and
> > > > > replaced them with:
> > > > >
> > > > > queue fq on pppoe0 flows 1024 default
> > > > > queue fq on re0 flows 1024 default
> > > > >
> > > >
> > > > Regarding your configuration, I'd advise you to use hfsc (the
> > > > default queueing) on pppoe0. Set it to your *uplink* bandwidth:
> > > >
> > > > queue pppoe0-rootq on pppoe0 bandwidth 2M max 2M default
> > > >
> > > > And use the flow queue only on the underlying interface.
> > > >
> > > > Since this is what I've been advising folks to use, I didn't
> > > > manage to find this bug beforehands. Apologies for that.
> > > >
> > > > I'll try to come up with the diff to do ifq_serialize for
> > > > if_qstart_compat.
> > >
> > > This turns out to be a rather contained problem. if_enqueue
> > > is already going down the ifq_start -> ifq_serialize ->
> > > ifq_start_task -> if_qstart_compat chain and sppp code is
> > > using if_enqueue. But it also calls if_start, which means
> > > that instead of calling if_qstart_compat directly from there,
> > > we need to let ifq_start do its magic. I've got this tested
> > > at home. OK?
> > >
> >
> > Sebastien, can you please confirm that this solves your problem?
> > David, does this look reasonable to you?
> >
>
> Hi mikeb@,
>
> Sorry I didn't test it: I had understood that due the problem was due to
> my (unsupported) use of fq on pppoe0, so I just skipped it (the
> backtrace of the panic passed by sppp_ functions).
>
We still need to solve the issue and commit the fix so please run with
it and report if something goes bad. I've been running with this diff
for about a week and haven't had any issues.
> I have push your diff on top of the kernel, and now I am running with it
> (and fq on re0 *and* on pppoe0).
>
> I will report any panic with it.
>
> I just noted that even with GENERIC.MP (amd64: OpenBSD 6.1-current
> (GENERIC.MP) #82: Wed May 24 06:29:56 MDT 2017), pfctl -sq error out:
>
> # pfctl -sq
> pfctl: DIOCGETQSTATS: Bad file descriptor
>
> (and same error with systat queue).
>
Yes, this is a side effect of using pf kifs in the queueing code.
This issue has been there since the beginning. Claudio has recently
pointed out that we should probably resolve it. The problem here is
that you have specified a nonexistent interface name for your queue.
For instance you have loaded a pf ruleset with "queue foo on pppoe0"
before creating pppoe0 itself. This will result in a successful
application of the ruleset but queues won't be created.
So please make sure that your "ifconfig" output corresponds to the
pf.conf and you *don't* get a 'DIOCGETQSTATS: Bad file descriptor'.
This is an indictaion that your queues are not created or some of
them are not created.
Claudio has also suggested aborting when a queue is to be created
on a non-exitent interface
diff --git sys/net/pf_ioctl.c sys/net/pf_ioctl.c
index 7cb7b92ed8a..6cf62cd2ec6 100644
--- sys/net/pf_ioctl.c
+++ sys/net/pf_ioctl.c
@@ -578,11 +578,11 @@ pf_ifp2q(struct pf_queue_if *list, struct ifnet *ifp)
int
pf_create_queues(void)
{
struct pf_queuespec *q;
struct ifnet *ifp;
- struct pf_queue_if *list = NULL, *qif;
+ struct pf_queue_if *list = NULL, *qif;
int error;
/*
* Find root queues and allocate traffic conditioner
* private data for these interfaces
@@ -1115,20 +1115,19 @@ pfioctl(dev_t dev, u_long cmd, caddr_t addr, int flags,
struct proc *p)
bcopy(&q->queue, qs, sizeof(*qs));
qs->qid = pf_qname2qid(qs->qname, 1);
if (qs->parent[0] && (qs->parent_qid =
pf_qname2qid(qs->parent, 0)) == 0) {
pool_put(&pf_queue_pl, qs);
- error = ESRCH;
+ error = EINVAL;
break;
}
qs->kif = pfi_kif_get(qs->ifname);
- if (qs->kif == NULL) {
+ if (qs->kif == NULL || qs->kif->pfik_ifp == NULL) {
pool_put(&pf_queue_pl, qs);
- error = ESRCH;
+ error = EINVAL;
break;
}
- /* XXX resolve bw percentage specs */
pfi_kif_ref(qs->kif, PFI_KIF_REF_RULE);
TAILQ_INSERT_TAIL(pf_queues_inactive, qs, entries);
break;
