Re: push NET_LOCK() down in pf_ioctl.c
On Tue, Oct 20, 2020 at 01:49:22AM +0200, Alexandr Nedvedicky wrote: > Currently we need to keep pf_rm_rule() under both locks. The function > might be calling pf_tag_unref(), pf_dynaddr_remove()... which alter lists, > which are currently supposed to be protected by PF_LOCK()/NET_LOCK(). Yup. > updated diff is below. pf_rm_rule() is being called with both locks held. OK kn
Re: push NET_LOCK() down in pf_ioctl.c
Hello Klemens,
> > the change is fairly large, but mostly mechanical.
> Relocating malloc(9) and pool(9) seems good but other pf_*() calls are
> now locked inconsistently after your diff.
>
> Given that only reason about "allocations" this is either an oversight
> and should be fixed or you need to provide more explanation.
>
> See inline for the spots I'm talking about.
thanks for catching this. there was unfinished clean up in a branch
I took diff from.
Currently we need to keep pf_rm_rule() under both locks. The function
might be calling pf_tag_unref(), pf_dynaddr_remove()... which alter lists,
which are currently supposed to be protected by PF_LOCK()/NET_LOCK().
updated diff is below. pf_rm_rule() is being called with both locks held.
thanks and
regards
sashan
8<---8<---8<--8<
diff --git a/sys/net/pf_ioctl.c b/sys/net/pf_ioctl.c
index ef7d995e5a7..3a75cd05005 100644
--- a/sys/net/pf_ioctl.c
+++ b/sys/net/pf_ioctl.c
@@ -1006,10 +1006,10 @@ pfioctl(dev_t dev, u_long cmd, caddr_t addr, int flags,
struct proc *p)
return (EACCES);
}
- NET_LOCK();
switch (cmd) {
case DIOCSTART:
+ NET_LOCK();
PF_LOCK();
if (pf_status.running)
error = EEXIST;
@@ -1025,9 +1025,11 @@ pfioctl(dev_t dev, u_long cmd, caddr_t addr, int flags,
struct proc *p)
DPFPRINTF(LOG_NOTICE, "pf: started");
}
PF_UNLOCK();
+ NET_UNLOCK();
break;
case DIOCSTOP:
+ NET_LOCK();
PF_LOCK();
if (!pf_status.running)
error = ENOENT;
@@ -1038,6 +1040,7 @@ pfioctl(dev_t dev, u_long cmd, caddr_t addr, int flags,
struct proc *p)
DPFPRINTF(LOG_NOTICE, "pf: stopped");
}
PF_UNLOCK();
+ NET_UNLOCK();
break;
case DIOCGETQUEUES: {
@@ -1045,6 +1048,7 @@ pfioctl(dev_t dev, u_long cmd, caddr_t addr, int flags,
struct proc *p)
struct pf_queuespec *qs;
u_int32_tnr = 0;
+ NET_LOCK();
PF_LOCK();
pq->ticket = pf_main_ruleset.rules.active.ticket;
@@ -1056,6 +1060,7 @@ pfioctl(dev_t dev, u_long cmd, caddr_t addr, int flags,
struct proc *p)
}
pq->nr = nr;
PF_UNLOCK();
+ NET_UNLOCK();
break;
}
@@ -1064,10 +1069,12 @@ pfioctl(dev_t dev, u_long cmd, caddr_t addr, int flags,
struct proc *p)
struct pf_queuespec *qs;
u_int32_tnr = 0;
+ NET_LOCK();
PF_LOCK();
if (pq->ticket != pf_main_ruleset.rules.active.ticket) {
error = EBUSY;
PF_UNLOCK();
+ NET_UNLOCK();
break;
}
@@ -1078,10 +1085,12 @@ pfioctl(dev_t dev, u_long cmd, caddr_t addr, int flags,
struct proc *p)
if (qs == NULL) {
error = EBUSY;
PF_UNLOCK();
+ NET_UNLOCK();
break;
}
memcpy(&pq->queue, qs, sizeof(pq->queue));
PF_UNLOCK();
+ NET_UNLOCK();
break;
}
@@ -1091,10 +1100,12 @@ pfioctl(dev_t dev, u_long cmd, caddr_t addr, int flags,
struct proc *p)
u_int32_tnr;
int nbytes;
+ NET_LOCK();
PF_LOCK();
if (pq->ticket != pf_main_ruleset.rules.active.ticket) {
error = EBUSY;
PF_UNLOCK();
+ NET_UNLOCK();
break;
}
nbytes = pq->nbytes;
@@ -1107,6 +1118,7 @@ pfioctl(dev_t dev, u_long cmd, caddr_t addr, int flags,
struct proc *p)
if (qs == NULL) {
error = EBUSY;
PF_UNLOCK();
+ NET_UNLOCK();
break;
}
memcpy(&pq->queue, qs, sizeof(pq->queue));
@@ -1121,6 +1133,7 @@ pfioctl(dev_t dev, u_long cmd, caddr_t addr, int flags,
struct proc *p)
if (error == 0)
pq->nbytes = nbytes;
PF_UNLOCK();
+ NET_UNLOCK();
break;
}
@@ -1128,38 +1141,44 @@ pfioctl(dev_t dev, u_long cmd, caddr_t addr, int flags,
struct proc *p)
struct pfioc_queue *q = (struct pfioc_queue *)addr;
struct pf_queuespec *qs;
- PF_LOCK();
- if (q->ticket != pf_main_ru
Re: push NET_LOCK() down in pf_ioctl.c
On Fri, Oct 16, 2020 at 11:37:22AM +0200, Alexandr Nedvedicky wrote:
> I've just found a forgotten diff in my tree. The diff pushes the NET_LCOK()
> further down in PF driver ioctl() path. The idea is to avoid sleeping while
> holding a NET_LOCK(). this typically may happen when we need to allocate
> memory. The diff is the first step as it takes care of easy/straightforward
> cases of such allocations. The allocations, which still may happen under
> the NET_LOCK() require more work in areas:
> PF tables,
> packet queues,
> transactions,
>
> the change is fairly large, but mostly mechanical.
Relocating malloc(9) and pool(9) seems good but other pf_*() calls are
now locked inconsistently after your diff.
Given that only reason about "allocations" this is either an oversight
and should be fixed or you need to provide more explanation.
See inline for the spots I'm talking about.
> diff --git a/sys/net/pf_ioctl.c b/sys/net/pf_ioctl.c
> index ef7d995e5a7..bac644fa6d1 100644
> --- a/sys/net/pf_ioctl.c
> +++ b/sys/net/pf_ioctl.c
> @@ -1006,10 +1006,10 @@ pfioctl(dev_t dev, u_long cmd, caddr_t addr, int
> flags, struct proc *p)
> return (EACCES);
> }
>
> - NET_LOCK();
> switch (cmd) {
>
> case DIOCSTART:
> + NET_LOCK();
> PF_LOCK();
> if (pf_status.running)
> error = EEXIST;
> @@ -1025,9 +1025,11 @@ pfioctl(dev_t dev, u_long cmd, caddr_t addr, int
> flags, struct proc *p)
> DPFPRINTF(LOG_NOTICE, "pf: started");
> }
> PF_UNLOCK();
> + NET_UNLOCK();
> break;
>
> case DIOCSTOP:
> + NET_LOCK();
> PF_LOCK();
> if (!pf_status.running)
> error = ENOENT;
> @@ -1038,6 +1040,7 @@ pfioctl(dev_t dev, u_long cmd, caddr_t addr, int flags,
> struct proc *p)
> DPFPRINTF(LOG_NOTICE, "pf: stopped");
> }
> PF_UNLOCK();
> + NET_UNLOCK();
> break;
>
> case DIOCGETQUEUES: {
> @@ -1045,6 +1048,7 @@ pfioctl(dev_t dev, u_long cmd, caddr_t addr, int flags,
> struct proc *p)
> struct pf_queuespec *qs;
> u_int32_tnr = 0;
>
> + NET_LOCK();
> PF_LOCK();
> pq->ticket = pf_main_ruleset.rules.active.ticket;
>
> @@ -1056,6 +1060,7 @@ pfioctl(dev_t dev, u_long cmd, caddr_t addr, int flags,
> struct proc *p)
> }
> pq->nr = nr;
> PF_UNLOCK();
> + NET_UNLOCK();
> break;
> }
>
> @@ -1064,10 +1069,12 @@ pfioctl(dev_t dev, u_long cmd, caddr_t addr, int
> flags, struct proc *p)
> struct pf_queuespec *qs;
> u_int32_tnr = 0;
>
> + NET_LOCK();
> PF_LOCK();
> if (pq->ticket != pf_main_ruleset.rules.active.ticket) {
> error = EBUSY;
> PF_UNLOCK();
> + NET_UNLOCK();
> break;
> }
>
> @@ -1078,10 +1085,12 @@ pfioctl(dev_t dev, u_long cmd, caddr_t addr, int
> flags, struct proc *p)
> if (qs == NULL) {
> error = EBUSY;
> PF_UNLOCK();
> + NET_UNLOCK();
> break;
> }
> memcpy(&pq->queue, qs, sizeof(pq->queue));
> PF_UNLOCK();
> + NET_UNLOCK();
> break;
> }
>
> @@ -1091,10 +1100,12 @@ pfioctl(dev_t dev, u_long cmd, caddr_t addr, int
> flags, struct proc *p)
> u_int32_tnr;
> int nbytes;
>
> + NET_LOCK();
> PF_LOCK();
> if (pq->ticket != pf_main_ruleset.rules.active.ticket) {
> error = EBUSY;
> PF_UNLOCK();
> + NET_UNLOCK();
> break;
> }
> nbytes = pq->nbytes;
> @@ -1107,6 +1118,7 @@ pfioctl(dev_t dev, u_long cmd, caddr_t addr, int flags,
> struct proc *p)
> if (qs == NULL) {
> error = EBUSY;
> PF_UNLOCK();
> + NET_UNLOCK();
> break;
> }
> memcpy(&pq->queue, qs, sizeof(pq->queue));
> @@ -1121,6 +1133,7 @@ pfioctl(dev_t dev, u_long cmd, caddr_t addr, int flags,
> struct proc *p)
> if (error == 0)
> pq->nbytes = nbytes;
> PF_UNLOCK();
> + NET_UNLOCK();
> break;
> }
>
> @@ -1128,38 +1141,44 @@ pfioctl(dev_t dev, u_long cmd, caddr_t addr, int
> flags, struct proc *p)
> struct pfioc_queue *q = (struct pfioc_queue *)addr;
>
push NET_LOCK() down in pf_ioctl.c
Hello,
I've just found a forgotten diff in my tree. The diff pushes the NET_LCOK()
further down in PF driver ioctl() path. The idea is to avoid sleeping while
holding a NET_LOCK(). this typically may happen when we need to allocate
memory. The diff is the first step as it takes care of easy/straightforward
cases of such allocations. The allocations, which still may happen under
the NET_LOCK() require more work in areas:
PF tables,
packet queues,
transactions,
the change is fairly large, but mostly mechanical.
OK?
thanks and
regards
sashan
8<---8<---8<--8<
diff --git a/sys/net/pf_ioctl.c b/sys/net/pf_ioctl.c
index ef7d995e5a7..bac644fa6d1 100644
--- a/sys/net/pf_ioctl.c
+++ b/sys/net/pf_ioctl.c
@@ -1006,10 +1006,10 @@ pfioctl(dev_t dev, u_long cmd, caddr_t addr, int flags,
struct proc *p)
return (EACCES);
}
- NET_LOCK();
switch (cmd) {
case DIOCSTART:
+ NET_LOCK();
PF_LOCK();
if (pf_status.running)
error = EEXIST;
@@ -1025,9 +1025,11 @@ pfioctl(dev_t dev, u_long cmd, caddr_t addr, int flags,
struct proc *p)
DPFPRINTF(LOG_NOTICE, "pf: started");
}
PF_UNLOCK();
+ NET_UNLOCK();
break;
case DIOCSTOP:
+ NET_LOCK();
PF_LOCK();
if (!pf_status.running)
error = ENOENT;
@@ -1038,6 +1040,7 @@ pfioctl(dev_t dev, u_long cmd, caddr_t addr, int flags,
struct proc *p)
DPFPRINTF(LOG_NOTICE, "pf: stopped");
}
PF_UNLOCK();
+ NET_UNLOCK();
break;
case DIOCGETQUEUES: {
@@ -1045,6 +1048,7 @@ pfioctl(dev_t dev, u_long cmd, caddr_t addr, int flags,
struct proc *p)
struct pf_queuespec *qs;
u_int32_tnr = 0;
+ NET_LOCK();
PF_LOCK();
pq->ticket = pf_main_ruleset.rules.active.ticket;
@@ -1056,6 +1060,7 @@ pfioctl(dev_t dev, u_long cmd, caddr_t addr, int flags,
struct proc *p)
}
pq->nr = nr;
PF_UNLOCK();
+ NET_UNLOCK();
break;
}
@@ -1064,10 +1069,12 @@ pfioctl(dev_t dev, u_long cmd, caddr_t addr, int flags,
struct proc *p)
struct pf_queuespec *qs;
u_int32_tnr = 0;
+ NET_LOCK();
PF_LOCK();
if (pq->ticket != pf_main_ruleset.rules.active.ticket) {
error = EBUSY;
PF_UNLOCK();
+ NET_UNLOCK();
break;
}
@@ -1078,10 +1085,12 @@ pfioctl(dev_t dev, u_long cmd, caddr_t addr, int flags,
struct proc *p)
if (qs == NULL) {
error = EBUSY;
PF_UNLOCK();
+ NET_UNLOCK();
break;
}
memcpy(&pq->queue, qs, sizeof(pq->queue));
PF_UNLOCK();
+ NET_UNLOCK();
break;
}
@@ -1091,10 +1100,12 @@ pfioctl(dev_t dev, u_long cmd, caddr_t addr, int flags,
struct proc *p)
u_int32_tnr;
int nbytes;
+ NET_LOCK();
PF_LOCK();
if (pq->ticket != pf_main_ruleset.rules.active.ticket) {
error = EBUSY;
PF_UNLOCK();
+ NET_UNLOCK();
break;
}
nbytes = pq->nbytes;
@@ -1107,6 +1118,7 @@ pfioctl(dev_t dev, u_long cmd, caddr_t addr, int flags,
struct proc *p)
if (qs == NULL) {
error = EBUSY;
PF_UNLOCK();
+ NET_UNLOCK();
break;
}
memcpy(&pq->queue, qs, sizeof(pq->queue));
@@ -1121,6 +1133,7 @@ pfioctl(dev_t dev, u_long cmd, caddr_t addr, int flags,
struct proc *p)
if (error == 0)
pq->nbytes = nbytes;
PF_UNLOCK();
+ NET_UNLOCK();
break;
}
@@ -1128,38 +1141,44 @@ pfioctl(dev_t dev, u_long cmd, caddr_t addr, int flags,
struct proc *p)
struct pfioc_queue *q = (struct pfioc_queue *)addr;
struct pf_queuespec *qs;
- PF_LOCK();
- if (q->ticket != pf_main_ruleset.rules.inactive.ticket) {
- error = EBUSY;
- PF_UNLOCK();
- break;
- }
qs = pool_get(&pf_queue_pl, PR_WAITOK|PR_LIMITFAIL|PR_ZERO);
