In D30764#691856, @markj wrote:

p is a union of struct mbuf ** and void *. So wouldn't setting *p.m do the wrong thing if the packet is passed with PFIL_MEMPTR?

I think we can set *p.m unconditionally, like we do in ipfw_check_packet().

The intent of this review was show how we can reduce vlan handling call path. It is not targeted to be included in main stream.

In D29274#682376, @kp wrote:

The locking Tom cites does still need to be fixed, but I think it's mostly okay. The only missing part is the protection of schedlist and aqmlist, and those will probably be better served by NET_EPOCH than by a separate lock. (Because taking the lock in find_sched_type() doesn't protect the struct dn_alg we return from being removed after we release the lock. Being inside NET_EPOCH and waiting to complete the unload until after NET_EPOCH_WAIT should be fine.)

I'll try to post a patch for that later today.

Just want to note, I'm sorry for long delay, I moved temporary to another project and dummynet overhaul is not done yet. But I plan to publish the code somewhere on guthub.

I meant the case fwd tableargs.home.lan:8000.

Replacing _substrcmp() with strncmp(,,8) breaks the case, when "tablearg" is part of hostname:port syntax.

In D29378#657837, @markj wrote:

I see a call in ipfw, I guess that needs to be updated too. Seems time to introduce a new subroutine to handle this.

• Fix copy/paste bug.

In D29576#663193, @glebius wrote:

In D29576#663187, @ae wrote:

Imagine, you have two or more identical SYNs, now you use INP_RLOCK(), this means they all can be handled in parallel "in the same time". This means, syncache_add() in some cases can determine that there are no corresponding entries yet - syncache_lookup() returns NULL, we still hold SCH_LOCK(), then we allocate syncache entry, populate all its fields and do SCH_UNLOCK(). Imagine, now the same things will be done by the another thread, and another. In the end we will have several the same entries added by the syncache_insert().
Is this impossible?

In this case the second thread will wait in SCH_LOCK and then, once it acquires the lock, syncache_lookup will return entry created by the first thread.

In D29576#663175, @glebius wrote:

In D29576#663155, @ae wrote:

I doubt that it is enough to just do s/wlock/rlock/ in the syncache code. Usually we need to add the extra check that an entry was not already linked by another thread in such cases.

Nothing changes in the syncache hash locking with this patch. The patch changes to rlock of the listening socket PCB.

I doubt that it is enough to just do s/wlock/rlock/ in the syncache code. Usually we need to add the extra check that an entry was not already linked by another thread in such cases.

I think when my work will be ready, I will commit it as separate implementation. Thus you can commit your changes to go forward with your work.

LGTM.

Hi John,

I'm working on this code and I think all this changes will be lost in the future, or they will take away my desire to merge changes.

Make locking similar to what we heave for IPv4.
Fix mbuf leaks and lock leaks for error case.

In D27500#638775, @rew wrote:

The only a thing a resize operation can do is ensure that the new size is a multiple of the requested alignment and that the new size won't extend into the next partition or past the last sector of the geom.

The last patch seems to have fixed the panic for us. But I have doubts, probably we should take INP_RLOCK just before checking in6p_moptions. Any thoughts?
Let me remind what panic it is targeted to fix:

In D27500#638475, @rew wrote:

ae@, thanks for the reply - I looked at g_part_bsd_resize and there’s no reference to using the offset field. The only field it uses is the start field. Let me know if you have any other concerns.
Maybe trasz@ will weigh in - is there anything glaring I’ve overlooked?

Hi, sorry for my silence. I was busy with other tasks, and it seems forget GEOM a bit.
I think you need to test your code with BSD scheme, AFAIR, it uses stripeoffset field.

Missed one case when PCB could be unlocked before check - when PCB is
already dying and imo is NULL. Also remove extra IN6_IS_ADDR_MULTICAST(),
we already checked it few lines before.

In D28232#631463, @gnn wrote:

Yup, I understand that we do not currently have that function. I think you ought to add that as part of this fix.

Before epochification we had an integer variable that was used to track locking.

#define        UH_WLOCKED      2
#define        UH_RLOCKED      1
#define        UH_UNLOCKED     0

And currently rwlock doesn't provide public function or macro similar to rw_wowned() for shared lock.

I have some doubts, that this change is safe. But probably we will see the result quickly.

If you tested the change and it works for you, I have no objection.

We already have IP_SENDONES flag for ip_output, that should serve for this purpose. Maybe is it not safe enough to allow this feature for all protocols?

I think the patch in D28187 is better. You need to release reference to SP when error occurs before ipsec4_process_packet().

It seems part of this is already in D28160

It seems I found how to reproduce it on test system:

1. Load systemt without any unneeded modules
2. kldload dtraceall
3. Run

# dtrace -n 'fbt::ip_input:entry { printf("%s", stringof(args[0]->m_pkthdr.rcvif->if_xname)); }'
dtrace: description 'fbt::ip_input:entry ' matched 1 probe
CPU     ID                    FUNCTION:NAME
  2  49220                   ip_input:entry ix0
  2  49220                   ip_input:entry ix0
  6  49220                   ip_input:entry ix0
^C
# kldunload dtraceall
# kldload ipfw
# kldload dtraceall
# dtrace -n 'fbt::ip_input:entry { printf("%s", stringof(args[0]->m_pkthdr.rcvif->if_xname)); }'
dtrace: invalid probe specifier fbt::ip_input:entry { printf("%s", stringof(args[0]->m_pkthdr.rcvif->if_xname)); }: in action list: m_pkthdr is not a member of struct mbuf

Recently I faced with this problem on some machines:

# dtrace -n 'fbt::ip_input:entry { printf("%s", stringof(args[0]->m_pkthdr.rcvif->if_xname)); }'
dtrace: invalid probe specifier fbt::ip_input:entry { printf("%s", stringof(args[0]->m_pkthdr.rcvif->if_xname)); }: in action list: m_pkthdr is not a member of struct mbuf

In D26879#599390, @gnn wrote:

I like the idea of this change but I believe that a new file should be created in netpfil/ipfw to contain the code that's been put into in_kdtrace.[ch] in this review.

Move provider and probe definitions into ipfw2.c

In D26879#599390, @gnn wrote:

I like the idea of this change but I believe that a new file should be created in netpfil/ipfw to contain the code that's been put into in_kdtrace.[ch] in this review.

LGTM. However it would be nice, if you consider my comments :)

It seems the only solution here is taking ifnet reference. I'm not sure about PCB, probably it can not disappear here.