Bug 230619 - pf: tables use non SMP-friendly counters
Summary: pf: tables use non SMP-friendly counters
Status: Closed FIXED
Alias: None
Product: Base System
Classification: Unclassified
Component: kern (show other bugs)
Version: 11.2-RELEASE
Hardware: Any Any
: --- Affects Some People
Assignee: Kristof Provost
URL:
Keywords: patch
Depends on:
Blocks:
 
Reported: 2018-08-14 16:02 UTC by Kajetan Staszkiewicz
Modified: 2020-11-24 22:13 UTC (History)
3 users (show)

See Also:


Attachments
Use counter(9) in pf tables. (22.22 KB, patch)
2018-08-14 16:02 UTC, Kajetan Staszkiewicz
no flags Details | Diff
Use counter(9) in pf tables. (22.23 KB, patch)
2018-08-15 10:52 UTC, Kajetan Staszkiewicz
no flags Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Kajetan Staszkiewicz 2018-08-14 16:02:36 UTC
Created attachment 196197 [details]
Use counter(9) in pf tables.

Counters of pf tables are updated out of rule lock. That means state updates might overwrite eachother. Furthermore there is allocation and freing counters happening in lockless manner too.

Proposed patch modifies table counters behaviour. counter(9) facility is used and table element counters are always allocated so that race conditions are impossible now.
Comment 1 Andrey V. Elsukov freebsd_committer freebsd_triage 2018-08-15 09:49:24 UTC
It seems you don't check the result of counter allocation, that with M_NOWAIT can fail. And then you are doing an access to such pointers. I'm not familiar with PF, but what happens if you try to limit UMA zone used for these counters and try to create enough number of entries? I suspect it will just panic. Also, PCPU counters are very expensive memory consumers, on modern machines with tens CPU cores, they require a lot of memory. And tables usually used to keep large number of entries, at least for ipfw. Is it really needed feature for PF for such cost?
Comment 2 Kristof Provost freebsd_committer freebsd_triage 2018-08-15 10:06:29 UTC
It's a tradeoff. pfr_update_stats() is currently called without any relevant locks held, so there's a risk of both a memory leak and incorrect counting.
Using PCPU counters (and always allocating them) mitigates this.

One alternative would be to take the rules lock, which is usually used to protect tables, but we'd have to take it for writing, to ensure no other threads are updating the counters at the same time, which I would expect to be devastating for throughput.

We might be able to get away with a per-table (but there are throughput concerns for that too), or even per pfr_kentry lock, but the locking structure of pf is already complex, and I'm not immediately clear on how it would interact with the rest of the locking.
Comment 3 Kajetan Staszkiewicz 2018-08-15 10:20:49 UTC
Andrey, you are right about allocation. I will change it to M_WAITOK just as other counters in pf are done.
Comment 4 Kajetan Staszkiewicz 2018-08-15 10:52:38 UTC
Created attachment 196214 [details]
Use counter(9) in pf tables.

Updated version of patch using M_WAITOK.
Comment 5 Kristof Provost freebsd_committer freebsd_triage 2019-01-15 09:33:18 UTC
Apologies for taking this long to get back to this. I've had other priorities in the past few months.

I'm not sure about the M_WAITOK in pfr_create_kentry(), because the initial allocation there (for the pfr_kentry) is M_NOWAIT. It'll have to be another M_NOWAIT allocation, with appropriate error handling.
The same applies to pfr_create_ktable().

In 'case PFRW_GET_ASTATS' it might make sense to move that code into its own function, if only to avoid the line length issues. It's so broken up now that it's not very readable any more.

Other than those minor points, I think this is ready to go in.
Comment 6 Kajetan Staszkiewicz 2019-01-16 10:14:15 UTC
I totally forgot about this patch too :) I'll fix the memory allocation flag and run it in testing environment and come back to you in a few days.
Comment 7 Kristof Provost freebsd_committer freebsd_triage 2019-03-12 12:47:50 UTC
Patch posted for review: https://reviews.freebsd.org/D19558
Comment 8 commit-hook freebsd_committer freebsd_triage 2019-03-15 11:09:01 UTC
A commit references this bug:

Author: kp
Date: Fri Mar 15 11:08:45 UTC 2019
New revision: 345177
URL: https://svnweb.freebsd.org/changeset/base/345177

Log:
  pf :Use counter(9) in pf tables.

  The counters of pf tables are updated outside the rule lock. That means state
  updates might overwrite each other. Furthermore allocation and
  freeing of counters happens outside the lock as well.

  Use counter(9) for the counters, and always allocate the counter table
  element, so that the race condition cannot happen any more.

  PR:		230619
  Submitted by:	Kajetan Staszkiewicz <vegeta@tuxpowered.net>
  Reviewed by:	glebius
  MFC after:	2 weeks
  Differential Revision:	https://reviews.freebsd.org/D19558

Changes:
  head/sys/net/pfvar.h
  head/sys/netpfil/pf/pf_table.c
Comment 9 commit-hook freebsd_committer freebsd_triage 2019-03-29 14:35:48 UTC
A commit references this bug:

Author: kp
Date: Fri Mar 29 14:34:51 UTC 2019
New revision: 345691
URL: https://svnweb.freebsd.org/changeset/base/345691

Log:
  MFC r345177:

  pf :Use counter(9) in pf tables.

  The counters of pf tables are updated outside the rule lock. That means state
  updates might overwrite each other. Furthermore allocation and
  freeing of counters happens outside the lock as well.

  Use counter(9) for the counters, and always allocate the counter table
  element, so that the race condition cannot happen any more.

  PR:		230619
  Submitted by:	Kajetan Staszkiewicz <vegeta@tuxpowered.net>

Changes:
_U  stable/11/
  stable/11/sys/net/pfvar.h
  stable/11/sys/netpfil/pf/pf_table.c
Comment 10 commit-hook freebsd_committer freebsd_triage 2019-03-29 14:35:50 UTC
A commit references this bug:

Author: kp
Date: Fri Mar 29 14:34:52 UTC 2019
New revision: 345692
URL: https://svnweb.freebsd.org/changeset/base/345692

Log:
  MFC r345177:

  pf :Use counter(9) in pf tables.

  The counters of pf tables are updated outside the rule lock. That means state
  updates might overwrite each other. Furthermore allocation and
  freeing of counters happens outside the lock as well.

  Use counter(9) for the counters, and always allocate the counter table
  element, so that the race condition cannot happen any more.

  PR:		230619
  Submitted by:	Kajetan Staszkiewicz <vegeta@tuxpowered.net>

Changes:
_U  stable/12/
  stable/12/sys/net/pfvar.h
  stable/12/sys/netpfil/pf/pf_table.c