[Bug 209475] pf didn't check if enough free RAM for net.pf.states_hashsize

[bugzilla-noreply at freebsd.org]

https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=209475

--- Comment #26 from fehmi noyan isi <fnoyanisi at yahoo.com> ---
Created attachment 190476
  --> https://bugs.freebsd.org/bugzilla/attachment.cgi?id=190476&action=edit
Kernel Crash Dump

(In reply to Kristof Provost from comment #25)

The VM I am running the tests is 64-bit,so I do not think the panic is
triggered by mallocarray(9). However, I see that the mtx_init(9) in the for
loop causes the crash.

I attach textdump output for your reference....

Here is the problem;

       if ((V_pf_keyhash = mallocarray(pf_hashsize, sizeof(struct pf_keyhash),
            M_PFHASH, M_NOWAIT | M_ZERO)) == NULL){
            V_pf_keyhash = mallocarray(PF_HASHSIZ, sizeof(struct pf_keyhash),
            M_PFHASH, M_WAITOK | M_ZERO);
            printf(...);
        }
        if ((V_pf_idhash = mallocarray(pf_hashsize, sizeof(struct pf_idhash),
            M_PFHASH, M_NOWAIT | M_ZERO)) == NULL){
            V_pf_idhash = mallocarray(PF_HASHSIZ, sizeof(struct pf_idhash),
            M_PFHASH, M_WAITOK | M_ZERO);
            printf(...);
        }
        pf_hashmask = pf_hashsize - 1; // pf_hashsize is 2147483648
        for (i = 0, kh = V_pf_keyhash, ih = V_pf_idhash; i <= pf_hashmask;
            i++, kh++, ih++) {
                mtx_init(&kh->lock, "pf_keyhash", NULL, MTX_DEF | MTX_DUPOK);
                mtx_init(&ih->lock, "pf_idhash", NULL, MTX_DEF);
        }

In the code above, V_ph_idhash and V_pf_keyhash are allocated PF_HASHSIZ *
sizeof(struct pf_keyhash) and PF_HASHSIZ * sizeof(struct pf_idhash) amount of
memory respectively. 

The for loop following the mallocarray(9) calls expects the allocated memory to
be aligned with pf_hashsize variable, which is usd in the loop and set to
2147483648 in our example. On the other hand, PH_HASHSIZ is 32768. This
mismatch causes the initialisation to fail.

Apparently, the value of pf_hashsize needs to be set and it should be used in
mallocarray(9) calls rather than PF_HASHSIZ. 

Although, sizeof(struct pf_keyhash) = sizeof(struct pf_idhash) = 40, we cannot
guarantee that the size of structs will stay the same (please correct me if I
am wrong). 

Given that the for loop assumes V_ph_idhash and V_pf_keyhash are allocated
memory by using the same multiplier, which is pf_hashsize, I think we either

 * should make a test before the mallocarray() calls and set pf_hashsize
accordingly (how?)
 * make two mallocarray(...,M_NOWAIT) calls and test return values in a single
if() statement. If either or both of these pointers is NULL, we should fallback
and re-allocate memory for _both_  V_ph_idhash and V_pf_keyhash by using a
single pf_hashsize value.

-- 
You are receiving this mail because:
You are the assignee for the bug.