On Thu, Dec 28, 2017 at 12:46:45AM +0100, Alexandr Nedvedicky wrote:
> I like your change. You have my OK with small change here:
I have commitet my diff with a small addition. I fixed a memory
leak that I had introduced before.
> > -pf_pkt_state_key_ref(struct mbuf *m)
> > +pf_mbuf_link_state_key(struct mbuf *m, struct pf_state_key *sk)
> > {
> > - pf_state_key_ref(m->m_pkthdr.pf.statekey);
> KASSERT(m->m_pkthdr.pf.statekey == NULL);
> > + m->m_pkthdr.pf.statekey = pf_state_key_ref(sk);
> > }
>
> I think we should have KASSERT() above as kind of check if
> more safety belts are needed to mitigate potential memory leak.
It does not work that easily. The only caller m_dup_pkthdr() does
set the statekey before.
I tried to put an assert "to->m_pkthdr.pf.statekey == NULL" before
the "to->m_pkthdr = from->m_pkthdr" in m_dup_pkthdr(). But the
mbuf "to" is not initialized, it is m_dup_pkthdr()'s job to fill
the memory. So we cannot assume that to->m_pkthdr.pf.statekey
contains anything useful.
All we could do is this:
ok?
bluhm
Index: kern/uipc_mbuf.c
===================================================================
RCS file: /data/mirror/openbsd/cvs/src/sys/kern/uipc_mbuf.c,v
retrieving revision 1.251
diff -u -p -r1.251 uipc_mbuf.c
--- kern/uipc_mbuf.c 29 Dec 2017 17:05:25 -0000 1.251
+++ kern/uipc_mbuf.c 29 Dec 2017 17:18:56 -0000
@@ -1325,6 +1325,7 @@ m_dup_pkthdr(struct mbuf *to, struct mbu
to->m_pkthdr = from->m_pkthdr;
#if NPF > 0
+ to->m_pkthdr.pf.statekey = NULL;
pf_mbuf_link_state_key(to, from->m_pkthdr.pf.statekey);
#endif /* NPF > 0 */
Index: net/pf.c
===================================================================
RCS file: /data/mirror/openbsd/cvs/src/sys/net/pf.c,v
retrieving revision 1.1053
diff -u -p -r1.1053 pf.c
--- net/pf.c 29 Dec 2017 17:05:25 -0000 1.1053
+++ net/pf.c 29 Dec 2017 17:11:47 -0000
@@ -7268,6 +7268,7 @@ pf_mbuf_unlink_state_key(struct mbuf *m)
void
pf_mbuf_link_state_key(struct mbuf *m, struct pf_state_key *sk)
{
+ KASSERT(m->m_pkthdr.pf.statekey == NULL);
m->m_pkthdr.pf.statekey = pf_state_key_ref(sk);
}
