pf.conf "set timeout interval 1" causes kernel crash

classic Classic list List threaded Threaded
5 messages Options
Reply | Threaded
Open this post in threaded view
|

pf.conf "set timeout interval 1" causes kernel crash

Kor son of Rynar
>Synopsis:      pf.conf "set timeout interval 1" causes kernel crash
>Category:      kernel amd64
>Environment:
        System    : OpenBSD 6.6
        Details     : OpenBSD 6.6-beta (GENERIC.MP) #235: Tue Aug 20
00:09:44 MDT 2019
                         [hidden email]:
/usr/src/sys/arch/amd64/compile/GENERIC.MP

        Architecture: OpenBSD.amd64
        Machine     : amd64
>Description:
        Using "set timeout interval 1" inside pf.conf crashes the kernel:

uvm_fault(0xffffffff81fef220, 0x27, 0, 2) -> e
kernel: page fault trap, code=0
Stopped at      pf_free_state+0xfe:     movq    %rcx,0x28(%rax)
ddb{0}> trace
pf_free_state(fffffd802a3d0e20) at pf_free_state+0xfe
pf_purge_expired_states() at pf_purge_expired_states+0x136
pf_purge(ffffffff81fa8054) at pf_purge+0x35
taskq_thread(ffff80000003b040) at taskq_thread+0x4d
end trace frame: 0x0, count: -4
ddb{0}>

>How-To-Repeat:
# echo "set timeout interval 1" >> /etc/pf.conf
# pfctl -f /etc/pf.conf

  <wait a few seconds>

>Fix:
        Not known.
Reply | Threaded
Open this post in threaded view
|

Re: pf.conf "set timeout interval 1" causes kernel crash

Alexandr Nedvedicky
Hello,

thanks for the report. I was able to trigger it. I'll take a look at it.

regards
sashan

On Tue, Aug 20, 2019 at 11:03:02AM -0300, Kor son of Rynar wrote:

> >Synopsis:      pf.conf "set timeout interval 1" causes kernel crash
> >Category:      kernel amd64
> >Environment:
>         System    : OpenBSD 6.6
>         Details     : OpenBSD 6.6-beta (GENERIC.MP) #235: Tue Aug 20
> 00:09:44 MDT 2019
>                          [hidden email]:
> /usr/src/sys/arch/amd64/compile/GENERIC.MP
>
>         Architecture: OpenBSD.amd64
>         Machine     : amd64
> >Description:
>         Using "set timeout interval 1" inside pf.conf crashes the kernel:
>
> uvm_fault(0xffffffff81fef220, 0x27, 0, 2) -> e
> kernel: page fault trap, code=0
> Stopped at      pf_free_state+0xfe:     movq    %rcx,0x28(%rax)
> ddb{0}> trace
> pf_free_state(fffffd802a3d0e20) at pf_free_state+0xfe
> pf_purge_expired_states() at pf_purge_expired_states+0x136
> pf_purge(ffffffff81fa8054) at pf_purge+0x35
> taskq_thread(ffff80000003b040) at taskq_thread+0x4d
> end trace frame: 0x0, count: -4
> ddb{0}>
>
> >How-To-Repeat:
> # echo "set timeout interval 1" >> /etc/pf.conf
> # pfctl -f /etc/pf.conf
>
>   <wait a few seconds>
>
> >Fix:
>         Not known.

Reply | Threaded
Open this post in threaded view
|

Re: pf.conf "set timeout interval 1" causes kernel crash

Alexandr Nedvedicky
In reply to this post by Kor son of Rynar
Hello,

</snip>

> >How-To-Repeat:
> # echo "set timeout interval 1" >> /etc/pf.conf
> # pfctl -f /etc/pf.conf
>
>   <wait a few seconds>
>
> >Fix:
>         Not known.

I've used same steps to reproduce the panic. This is what I could
see in resulting crash:

The system died at line 1441 in pf_free_state():

    1415 void
    1416 pf_free_state(struct pf_state *cur)
    1417 {
    1418         struct pf_rule_item *ri;
    1419
    1420         PF_ASSERT_LOCKED();
    ....
    1440         pf_normalize_tcp_cleanup(cur);
    1441         pfi_kif_unref(cur->kif, PFI_KIF_REF_STATE);
    1442         TAILQ_REMOVE(&state_list, cur, entry_list);
    1443         if (cur->tag)
    1444                 pf_tag_unref(cur->tag);
    1445         pf_state_unref(cur);

the instruction, where we died was as follows:

    Stopped at      pf_free_state+0xfe:     movq    %rcx,0x28(%rax)

there was -1 in rax. the -1 at offset 0x28 indicates the `cur` has been
removed from state_list. To explain how is it possible, we need to step
back to pf_free_state() caller, which is pf_purge_expired_states():

    1453         static struct pf_state  *cur = NULL;
    1454         struct pf_state         *next;
    1455         SLIST_HEAD(pf_state_gcl, pf_state) gcl;
    1456
    1457         PF_ASSERT_UNLOCKED();
    1458         SLIST_INIT(&gcl);
    1459
    1460         PF_STATE_ENTER_READ();
    1461         while (maxcheck--) {
    1462                 /* wrap to start of list when we hit the end */
    1463                 if (cur == NULL) {
    1464                         cur = pf_state_ref(TAILQ_FIRST(&state_list));
    1465                         if (cur == NULL)
    1466                                 break;  /* list empty */
    1467                 }
    1468
    1469                 /* get next state, as cur may get deleted */
    1470                 next = TAILQ_NEXT(cur, entry_list);
    1471
    1472                 if ((cur->timeout == PFTM_UNLINKED) ||
    1473                     (pf_state_expires(cur) <= time_uptime))
    1474                         SLIST_INSERT_HEAD(&gcl, cur, gc_list);
    1475                 else
    1476                         pf_state_unref(cur);
    1477
    1478                 cur = pf_state_ref(next);
    1479         }

I think the problem is the first 'while (maxcheck--)' loop may actually
wrap around the state_list and re-insert the `cur` to garbage collector list
again. Such sequence of events would match the panic I could see. I think
the right fix is to break from the while loop as soon, as we reach
the end of the state_list.

I've also noticed yet another minor problem there in loop, which
processes the garbage collector list:

    1487         while ((next = SLIST_FIRST(&gcl)) != NULL) {
    1488                 SLIST_REMOVE_HEAD(&gcl, gc_list);
    1489                 if (next->timeout == PFTM_UNLINKED)
    1490                         pf_free_state(next);
    1491                 else if (pf_state_expires(next) <= time_uptime)) {
    1492                         pf_remove_state(next);
    1493                         pf_free_state(next);
    1494                 }
    1495
    1496                 pf_state_unref(next);
    1497         }

at line 1491, we don't need to recalculate expiration time. The `next` item is
on the garbage collector list already, so it must be expired for sure.

patch below fixes both glitches. I wonder if it will work for you as well.

thanks for testing
and sorry for inconveniences

regards
sashan

--------8<---------------8<---------------8<------------------8<--------
diff --git a/sys/net/pf.c b/sys/net/pf.c
index bc33fa723ea..8cadd1f53c2 100644
--- a/sys/net/pf.c
+++ b/sys/net/pf.c
@@ -1476,6 +1476,9 @@ pf_purge_expired_states(u_int32_t maxcheck)
  pf_state_unref(cur);
 
  cur = pf_state_ref(next);
+
+ if (cur == NULL)
+ break;
  }
  PF_STATE_EXIT_READ();
 
@@ -1485,7 +1488,7 @@ pf_purge_expired_states(u_int32_t maxcheck)
  SLIST_REMOVE_HEAD(&gcl, gc_list);
  if (next->timeout == PFTM_UNLINKED)
  pf_free_state(next);
- else if (pf_state_expires(next) <= time_uptime) {
+ else {
  pf_remove_state(next);
  pf_free_state(next);
  }


Reply | Threaded
Open this post in threaded view
|

Re: pf.conf "set timeout interval 1" causes kernel crash

Kor son of Rynar
Hi sashan,

On Tue, Aug 20, 2019 at 9:42 PM Alexandr Nedvedicky <
[hidden email]> wrote:

> Hello,
>
> </snip>
>
> > >How-To-Repeat:
> > # echo "set timeout interval 1" >> /etc/pf.conf
> > # pfctl -f /etc/pf.conf
> >
> >   <wait a few seconds>
> >
> > >Fix:
> >         Not known.
>
> I've used same steps to reproduce the panic. This is what I could
> see in resulting crash:
>
> The system died at line 1441 in pf_free_state():
>
>     1415 void
>     1416 pf_free_state(struct pf_state *cur)
>     1417 {
>     1418         struct pf_rule_item *ri;
>     1419
>     1420         PF_ASSERT_LOCKED();
>     ....
>     1440         pf_normalize_tcp_cleanup(cur);
>     1441         pfi_kif_unref(cur->kif, PFI_KIF_REF_STATE);
>     1442         TAILQ_REMOVE(&state_list, cur, entry_list);
>     1443         if (cur->tag)
>     1444                 pf_tag_unref(cur->tag);
>     1445         pf_state_unref(cur);
>
> the instruction, where we died was as follows:
>
>     Stopped at      pf_free_state+0xfe:     movq    %rcx,0x28(%rax)
>
> there was -1 in rax. the -1 at offset 0x28 indicates the `cur` has been
> removed from state_list. To explain how is it possible, we need to step
> back to pf_free_state() caller, which is pf_purge_expired_states():
>
>     1453         static struct pf_state  *cur = NULL;
>     1454         struct pf_state         *next;
>     1455         SLIST_HEAD(pf_state_gcl, pf_state) gcl;
>     1456
>     1457         PF_ASSERT_UNLOCKED();
>     1458         SLIST_INIT(&gcl);
>     1459
>     1460         PF_STATE_ENTER_READ();
>     1461         while (maxcheck--) {
>     1462                 /* wrap to start of list when we hit the end */
>     1463                 if (cur == NULL) {
>     1464                         cur =
> pf_state_ref(TAILQ_FIRST(&state_list));
>     1465                         if (cur == NULL)
>     1466                                 break;  /* list empty */
>     1467                 }
>     1468
>     1469                 /* get next state, as cur may get deleted */
>     1470                 next = TAILQ_NEXT(cur, entry_list);
>     1471
>     1472                 if ((cur->timeout == PFTM_UNLINKED) ||
>     1473                     (pf_state_expires(cur) <= time_uptime))
>     1474                         SLIST_INSERT_HEAD(&gcl, cur, gc_list);
>     1475                 else
>     1476                         pf_state_unref(cur);
>     1477
>     1478                 cur = pf_state_ref(next);
>     1479         }
>
> I think the problem is the first 'while (maxcheck--)' loop may actually
> wrap around the state_list and re-insert the `cur` to garbage collector
> list
> again. Such sequence of events would match the panic I could see. I think
> the right fix is to break from the while loop as soon, as we reach
> the end of the state_list.
>
> I've also noticed yet another minor problem there in loop, which
> processes the garbage collector list:
>
>     1487         while ((next = SLIST_FIRST(&gcl)) != NULL) {
>     1488                 SLIST_REMOVE_HEAD(&gcl, gc_list);
>     1489                 if (next->timeout == PFTM_UNLINKED)
>     1490                         pf_free_state(next);
>     1491                 else if (pf_state_expires(next) <= time_uptime)) {
>     1492                         pf_remove_state(next);
>     1493                         pf_free_state(next);
>     1494                 }
>     1495
>     1496                 pf_state_unref(next);
>     1497         }
>
> at line 1491, we don't need to recalculate expiration time. The `next`
> item is
> on the garbage collector list already, so it must be expired for sure.
>
> patch below fixes both glitches. I wonder if it will work for you as well.
>

Thank you for your quick response.  I've applied the diff and it worked, no
more crashes!

Thanks again,
--Kor


>
> thanks for testing
> and sorry for inconveniences
>
> regards
> sashan
>
> --------8<---------------8<---------------8<------------------8<--------
> diff --git a/sys/net/pf.c b/sys/net/pf.c
> index bc33fa723ea..8cadd1f53c2 100644
> --- a/sys/net/pf.c
> +++ b/sys/net/pf.c
> @@ -1476,6 +1476,9 @@ pf_purge_expired_states(u_int32_t maxcheck)
>                         pf_state_unref(cur);
>
>                 cur = pf_state_ref(next);
> +
> +               if (cur == NULL)
> +                       break;
>         }
>         PF_STATE_EXIT_READ();
>
> @@ -1485,7 +1488,7 @@ pf_purge_expired_states(u_int32_t maxcheck)
>                 SLIST_REMOVE_HEAD(&gcl, gc_list);
>                 if (next->timeout == PFTM_UNLINKED)
>                         pf_free_state(next);
> -               else if (pf_state_expires(next) <= time_uptime) {
> +               else {
>                         pf_remove_state(next);
>                         pf_free_state(next);
>                 }
>
>
>
Reply | Threaded
Open this post in threaded view
|

Re: pf.conf "set timeout interval 1" causes kernel crash

Klemens Nanni-2
In reply to this post by Alexandr Nedvedicky
On Wed, Aug 21, 2019 at 02:36:36AM +0200, Alexandr Nedvedicky wrote:
> I've used same steps to reproduce the panic. This is what I could
> see in resulting crash:
I have poked at this in a VMM guest booting GENERIC with the following
in rc.local(8) to trigger it as early as possible on reboot:

        echo set timeout interval 1 | pfctl -f-

and landed at the same panic most of the time;  There is another path
however

        ddb> t
        refcnt_take(3) at refcnt_take+0x10
        pf_purge(ffffffff81fba068) at pf_purge+0x30
        taskq_thread(ffff800000022040) at taskq_thread+0x3d
        end trace frame: 0x0, count: -3
        ddb> e /i
        refcnt_take+0x10:       xaddl   %eax,0(%rdi)

> I think the problem is the first 'while (maxcheck--)' loop may actually
> wrap around the state_list and re-insert the `cur` to garbage collector list
> again. Such sequence of events would match the panic I could see. I think
> the right fix is to break from the while loop as soon, as we reach
> the end of the state_list.
Makes sense, matches with my analysis and does fix both panics for me.

OK kn

> at line 1491, we don't need to recalculate expiration time. The `next` item is
> on the garbage collector list already, so it must be expired for sure.
I've only worked on the original issue myself, but this reads and works
fine as well.