[Differential] [Commented On] D1711: Changes to the callout code to restore active semantics and also add a test-framework and test to validate thecallout code (and potentially for use by other tests)

hselasky added a comment. Hi, There is only one or two likely consumers of callout_init_rw() at the present moment, and one of them is: ./netinet6/nd6.c: canceled = callout_stop(&ln->ln_timer_ch); ./netinet6/nd6.c: canceled = callout_reset(&ln->ln_timer_ch,

[Differential] [Commented On] D1707: sfxge: access statistics buffers under port lock

arybchik added a comment. I've published review request D1772 to add macros. Please, review. Also I have a patch to make lock names unique - will follow. REVISION DETAIL https://reviews.freebsd.org/D1707 To: arybchik, gnn Cc: freebsd-net ___ freebsd-

[Differential] [Commented On] D1711: Changes to the callout code to restore active semantics and also add a test-framework and test to validate thecallout code (and potentially for use by other tests)

hiren added a comment. >>! In D1711#58, @rrs wrote: > hiren: > > This looks interesting to me, it is definitely something I would like to look > at. I assume you > are on 10.stable like Sean? Yes, its plain stable10+D1711. Also, all 3 panics are from the same system. REVISION DETAIL https:

[Differential] [Commented On] D1711: Changes to the callout code to restore active semantics and also add a test-framework and test to validate thecallout code (and potentially for use by other tests)

rrs added a comment. Hiren: Ok looking at kern_timeout.c thats a call to class->lc_lock(c_lock, lock_status); If my 10.x matches yours. And the call inside that kern_rwlock.c:757 is v = rw->rw_lock; owner = (struct thread *)RW_OWNER(v); I would imagine v is probably a freed lock or some suc

[Differential] [Commented On] D1711: Changes to the callout code to restore active semantics and also add a test-framework and test to validate thecallout code (and potentially for use by other tests)

rrs added a comment. Sbruno: I have a hard time connecting the dots between the kernel panic's you are seeing and the callout system (especially the changes I made above). You were getting a spin-lock held too long right? hiren: This looks interesting to me, it is definitely something I would

[Differential] [Commented On] D1761: Extend LRO support to accumulate more than 65535 bytes

rwatson added a comment. np@'s comments are good ones, both with respect to ACK clocking and BPF -- but this will also affect local firewalls that do state tracking and/or transformation, and likewise will run into problems. Given this discussion, I'm increasingly convinced that this is Not A G

Re: [RFC][patch] New "keep-state-only" option

On 2/4/15 1:32 PM, Julian Elischer wrote: On 2/4/15 12:13 AM, Lev Serebryakov wrote: And variants with multiple NATs and "nat global" becomes as easy as this, too! No stupid "skipto", no "keep-state" at "incoming from local network" parts of firewall, nothing! P.S. I HATE this "all any to an

Re: [RFC][patch] New "keep-state-only" option

On 2/4/15 12:13 AM, Lev Serebryakov wrote: -BEGIN PGP SIGNED MESSAGE- Hash: SHA512 Ok, "allow-state"/"deny-state" was very limited idea. Here is more universal mechanism: new "keep-state-only" (aliased as "record-only") option, which works exactly as "keep-state" BUT cancel match of

Re: [RFC][patch] New "keep-state-only" option (version 2)

On 2/4/15 12:55 AM, Lev Serebryakov wrote: -BEGIN PGP SIGNED MESSAGE- Hash: SHA512 On 03.02.2015 19:13, Lev Serebryakov wrote: Ok, "allow-state"/"deny-state" was very limited idea. Here is more universal mechanism: new "keep-state-only" (aliased as "record-only") option, which works ex

Re: [RFC][patch] Two new actions: state-allow and state-deny

On 2/3/15 5:30 PM, Lev Serebryakov wrote: looking at my own rules I don't seem to have a problem.. You have "check-state" only once, on entrance, before all NATs, so it could work only for packets which don't need NAT. And looks like (correct me if I'm wrong) you don't try to track states o

[Differential] [Commented On] D1761: Extend LRO support to accumulate more than 65535 bytes

adrian added a comment. .. and yes, I don't think you should be using the hashtype field to flag LRO information. They're totally separate things. (It's totally feasible that a NIC doing > 64KiB LRO will reassemble frames with correct RSS information and feed it to an RSS kernel, which can then

[Differential] [Changed Subscribers] D1761: Extend LRO support to accumulate more than 65535 bytes

np added a subscriber: np. np added a reviewer: lstewart. np added a comment. LRO affects the kernel TCP code in subtle (and almost always undesirable) ways by "compressing" multiple TCP headers into one. Think TCP timestamps, bursty changes in sequence space as seen by the kernel, what happens

net device drivers need to set if_hw_tsomaxsegcount and friends

Hi, If you are an author/maintainer of a network device driver that supports TSO, please add a few lines to your driver to set these TSO related parameters in "struct ifnet", before you call ether_ifattach(). Thanks go to hselasky@ for committing the patch for these. if_hw_tsomax - Maximum size o

[Differential] [Changed Subscribers] D1767: Refragment packets after defragmenting them in PF

kristof added a subscriber: freebsd-net. REVISION DETAIL https://reviews.freebsd.org/D1767 To: kristof Cc: freebsd-net ___ freebsd-net@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-net To unsubscribe, send any mail to "fre

[Differential] [Changed Subscribers] D1765: PF: Handle fragmented IPv6 packets

kristof added a subscriber: freebsd-net. REVISION DETAIL https://reviews.freebsd.org/D1765 To: kristof Cc: freebsd-net ___ freebsd-net@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-net To unsubscribe, send any mail to "fre

[Differential] [Changed Subscribers] D1766: Factor out ip6_fragment()

kristof added a subscriber: freebsd-net. REVISION DETAIL https://reviews.freebsd.org/D1766 To: kristof Cc: freebsd-net ___ freebsd-net@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-net To unsubscribe, send any mail to "fre

[Differential] [Changed Subscribers] D1764: Factor out ip6_deletefraghdr()

kristof added a subscriber: freebsd-net. REVISION DETAIL https://reviews.freebsd.org/D1764 To: kristof Cc: freebsd-net ___ freebsd-net@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-net To unsubscribe, send any mail to "fre

[Differential] [Changed Subscribers] D1711: Changes to the callout code to restore active semantics and also add a test-framework and test to validate thecallout code (and potentially for use by other

hiren added a subscriber: hiren. hiren added a comment. Sanitized panic #3 Dump header from device /dev/da0s1b Architecture: amd64 Architecture Version: 2 Dump Length: 5393809408B (5143 MB) Blocksize: 512 Dumptime: Tue Feb 3 13:21:19 2015 Hostname: xxx

[Differential] [Commented On] D1761: Extend LRO support to accumulate more than 65535 bytes

adrian added a comment. *laugh* I know you're not; I'm just worried that we'll have lots of subtle bugs here and there with the semantics of "sometimes the length is in ip_hdr; sometimes the length is in m_pkthdr; and there's no clear definition of when we should check both." If we had a clear

[Differential] [Commented On] D1761: Extend LRO support to accumulate more than 65535 bytes

hselasky added a comment. adrian: I'm not in a position to change the IP header structure to support 32-bit payload length. That's why m_pkthdr.len was chosen. REVISION DETAIL https://reviews.freebsd.org/D1761 To: hselasky, rmacklem, rrs, glebius, gnn, emaste, imp, adrian, bz, rwatson Cc: fre

[Differential] [Commented On] D1761: Extend LRO support to accumulate more than 65535 bytes

hselasky added a comment. rwatson: The LRO code ensures that all trailing and padding is stripped. This happens both before and after my change. REVISION DETAIL https://reviews.freebsd.org/D1761 To: hselasky, rmacklem, rrs, glebius, gnn, emaste, imp, adrian, bz, rwatson Cc: freebsd-net __

[Differential] [Commented On] D1761: Extend LRO support to accumulate more than 65535 bytes

hselasky added a comment. See comments added. INLINE COMMENTS sys/netinet/ip_output.c:129 Because the variable is later on compared to the "mtu" which is also an "int". Else you will get a compile warning about signed comparison mismatch. Same goes for other location where "int" is used. sy

[Differential] [Updated] D1761: Extend LRO support to accumulate more than 65535 bytes

rwatson added a comment. Historically, I believe that we've allowed additional trailer content/padding to be included at the end of mbuf chains that isn't in the IP datagram, and is silently ignored, as it isn't included in the IP packet length. I may misunderstand what was going on there, but

[Differential] [Commented On] D1711: Changes to the callout code to restore active semantics and also add a test-framework and test to validate thecallout code (and potentially for use by other tests)

sbruno added a comment. Sanitized panic #2 Dump header from device /dev/da0s1b Architecture: amd64 Architecture Version: 2 Dump Length: 6752395264B (6439 MB) Blocksize: 512 Dumptime: Mon Feb 2 14:42:05 2015 Hostname: Magic: FreeBSD Kernel Du

[Differential] [Commented On] D1711: Changes to the callout code to restore active semantics and also add a test-framework and test to validate thecallout code (and potentially for use by other tests)

sbruno added a comment. Sanitized panic #1 Dump header from device /dev/da0s1b Architecture: amd64 Architecture Version: 2 Dump Length: 6451576832B (6152 MB) Blocksize: 512 Dumptime: Sat Jan 31 21:30:37 2015 Hostname: Magic: FreeBSD Kernel Dump Version String:

[Differential] [Updated] D1711: Changes to the callout code to restore active semantics and also add a test-framework and test to validate thecallout code (and potentially for use by other tests).

sbruno added a comment. We're dubious over here in "actually running this on live systems" land. We're seeing pretty chaotic panics, unsure what we should dig into here. Within minutes of running live on a machine, we see various panics in cam or the network stack running this against stable

[Differential] [Commented On] D1711: Changes to the callout code to restore active semantics and also add a test-framework and test to validate thecallout code (and potentially for use by other tests)

emaste added inline comments. INLINE COMMENTS kern/kern_timeout.c:1159 Yeah, something like @imp's suggestion makes it more clear to me. REVISION DETAIL https://reviews.freebsd.org/D1711 To: rrs, gnn, rwatson, sbruno, lstewart, jhb, kostikbel, hselasky, adrian, imp Cc: jhb, kostikbel, emast

[Differential] [Accepted] D1711: Changes to the callout code to restore active semantics and also add a test-framework and test to validate thecallout code (and potentially for use by other tests).

imp accepted this revision. imp added a comment. I think we're ready to land. It fixes all the issues discussed, and enhanced testing shows no additional errors. INLINE COMMENTS kern/kern_timeout.c:1159 "When the callout wheel runs, it will ignore this callout" maybe is a better phrase to use

[Differential] [Updated] D1761: Extend LRO support to accumulate more than 65535 bytes

bz added a comment. I have raised my concerns about the change a few weeks ago elsewhere already but gnn mentioned a possible idea today to keep it clean(er). I'll let him follow-up here. REVISION DETAIL https://reviews.freebsd.org/D1761 To: hselasky, rmacklem, rrs, glebius, gnn, emaste, rw

[Differential] [Updated] D1761: Extend LRO support to accumulate more than 65535 bytes

adrian added a comment. Hi, My main concern with this patch is the special casing of what is the "packet length" being sprinkled all throughout the code. It feels like we could be chasing down obscure "is this the right length for this kind of packet" bugs for quite some time. Is there any wa

[Differential] [Updated] D1761: Extend LRO support to accumulate more than 65535 bytes

imp added a comment. Except for the signed / unsigned issue, I like this change. However, please get approval from some of the networking guys before committing. INLINE COMMENTS sys/netinet/ip_input.c:1450 Shouldn't this be unsigned? sys/netinet/ip_output.c:129 shouldn't this be unsigned?

Re: [RFC][patch] New "keep-state-only" option (version 2)

-BEGIN PGP SIGNED MESSAGE- Hash: SHA512 On 03.02.2015 19:13, Lev Serebryakov wrote: > Ok, "allow-state"/"deny-state" was very limited idea. Here is more > universal mechanism: new "keep-state-only" (aliased as > "record-only") option, which works exactly as "keep-state" BUT > cancel matc

[Differential] [Accepted] D1711: Changes to the callout code to restore active semantics and also add a test-framework and test to validate thecallout code (and potentially for use by other tests).

adrian accepted this revision. REVISION DETAIL https://reviews.freebsd.org/D1711 To: rrs, gnn, rwatson, sbruno, lstewart, imp, jhb, kostikbel, hselasky, adrian Cc: jhb, kostikbel, emaste, delphij, neel, erj, freebsd-net ___ freebsd-net@freebsd.org mai

[RFC][patch] New "keep-state-only" option

-BEGIN PGP SIGNED MESSAGE- Hash: SHA512 Ok, "allow-state"/"deny-state" was very limited idea. Here is more universal mechanism: new "keep-state-only" (aliased as "record-only") option, which works exactly as "keep-state" BUT cancel match of rule after state creation. It allows to write

[Differential] [Updated, 2, 384 lines] D1438: FreeBSD callout rewrite and cleanup

hselasky updated this revision to Diff 3607. hselasky added a comment. Add full context to patch. CHANGES SINCE LAST UPDATE https://reviews.freebsd.org/D1438?vs=3574&id=3607 REVISION DETAIL https://reviews.freebsd.org/D1438 AFFECTED FILES share/man/man9/Makefile share/man/man9/timeout.9

[Differential] [Updated, 68 lines] D1761: Extend LRO support to accumulate more than 65535 bytes

hselasky updated this revision to Diff 3606. hselasky added a comment. Add context to patch. CHANGES SINCE LAST UPDATE https://reviews.freebsd.org/D1761?vs=3601&id=3606 REVISION DETAIL https://reviews.freebsd.org/D1761 AFFECTED FILES sys/netinet/ip_input.c sys/netinet/ip_output.c sys/

Re: [RFC][patch] Two new actions: state-allow and state-deny

On Tue, 3 Feb 2015 13:23:38 +0300, Lev Serebryakov wrote: > On 03.02.2015 13:04, Ian Smith wrote: > > >> Now to make stateful firewall with NAT you need to make some not > >> very "readable" tricks to record state ("allow") of outbound > >> connection before NAT, but pass packet to NAT after

[Differential] [Accepted] D1711: Changes to the callout code to restore active semantics and also add a test-framework and test to validate thecallout code (and potentially for use by other tests).

hselasky accepted this revision. This revision is now accepted and ready to land. REVISION DETAIL https://reviews.freebsd.org/D1711 To: rrs, gnn, rwatson, sbruno, lstewart, imp, jhb, adrian, kostikbel, hselasky Cc: jhb, kostikbel, emaste, delphij, neel, erj, freebsd-net

[Differential] [Updated, 241 lines] D1711: Changes to the callout code to restore active semantics and also add a test-framework and test to validate thecallout code (and potentially for use by other

rrs updated this revision to Diff 3603. rrs added a comment. Lets try this one more time to get line 789 right ;-) CHANGES SINCE LAST UPDATE https://reviews.freebsd.org/D1711?vs=3602&id=3603 REVISION DETAIL https://reviews.freebsd.org/D1711 AFFECTED FILES kern/kern_timeout.c sys/callout

[Differential] [Updated, 242 lines] D1711: Changes to the callout code to restore active semantics and also add a test-framework and test to validate thecallout code (and potentially for use by other

rrs updated this revision to Diff 3602. rrs added a comment. This revision now requires review to proceed. Ed: See if this comment clarification does not help a bit. Hans: I have no intention of *not* leaving the macro changes in here, this was a comment suggested by imp and I think its a go

[Differential] [Commented On] D1711: Changes to the callout code to restore active semantics and also add a test-framework and test to validate thecallout code (and potentially for use by other tests)

rrs added inline comments. INLINE COMMENTS sys/kern/kern_timeout.c:789 Opps I will fix that thanks Ed! sys/kern/kern_timeout.c:1067-1069 Let me see if I can edit this comment a bit and make it more clear. REVISION DETAIL https://reviews.freebsd.org/D1711 To: rrs, gnn, rwatson, sbruno, lst

Re: [RFC][patch] Two new actions: state-allow and state-deny

-BEGIN PGP SIGNED MESSAGE- Hash: SHA512 On 03.02.2015 12:30, Lev Serebryakov wrote: > "keep-state". Problem is, it adds "if" branch for EACH action (in > kernel code). IMHO, it is very prohibitive. I've though about > that, but decide it is too expensive to have "if (!iHaveRecordOnly > |

Re: [RFC][patch] Two new actions: state-allow and state-deny

-BEGIN PGP SIGNED MESSAGE- Hash: SHA512 On 03.02.2015 13:04, Ian Smith wrote: >> Now to make stateful firewall with NAT you need to make some not >> very "readable" tricks to record state ("allow") of outbound >> connection before NAT, but pass packet to NAT after that. I know >> two: >>

Re: [RFC][patch] Two new actions: state-allow and state-deny

On Mon, 2 Feb 2015 22:17:25 +0300, Lev Serebryakov wrote: > Now to make stateful firewall with NAT you need to make some not very > "readable" tricks to record state ("allow") of outbound connection > before NAT, but pass packet to NAT after that. I know two: > > (a) skipto-nat-allow patte

Re: [RFC][patch] Two new actions: state-allow and state-deny

-BEGIN PGP SIGNED MESSAGE- Hash: SHA512 On 03.02.2015 09:44, Julian Elischer wrote: >> So, stateful firewall with NAT could be rewritten like this: >> >> add 1000 skipto 2000 all from any to any out xmit outIface add >> 1010 skipto 3000 all from any to any in recv outIface >> >> add 20