Bug 198500 - [gjournal] sysctl kern.geom.journal.cache.limit 32 bit integer overflow
Summary: [gjournal] sysctl kern.geom.journal.cache.limit 32 bit integer overflow
Status: Closed FIXED
Alias: None
Product: Base System
Classification: Unclassified
Component: kern (show other bugs)
Version: CURRENT
Hardware: Any Any
: --- Affects Some People
Assignee: freebsd-geom (Nobody)
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2015-03-10 17:37 UTC by Eugene Grosbein
Modified: 2017-08-14 19:16 UTC (History)
3 users (show)

See Also:


Attachments
patch for stable/11 (2.34 KB, patch)
2017-05-12 18:56 UTC, Eugene Grosbein
no flags Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Eugene Grosbein 2015-03-10 17:37:18 UTC
sysctl kern.geom.journal.cache.limit shows negative value for FreeBSD/amd64 system having over 4GB RAM. That's due to:

1) the limit being u_int instead of u_long like vm.kmem_size (the limit is 1/2 of vm.kmem_size by default for amd64);
2) sysctl handler g_journal_cache_limit_sysctl() using signed int instead of long.
Comment 1 longwitz 2017-05-08 19:27:24 UTC
The main problem caused by the mixed use of u_int and u_long variables in g_journal.c is the suspicious value of kern.geom.journal.cache.limit
for machines with more than 8 GB of memory. Therefore I propose the following patch for this problem for FreeBSD 10.3-STABLE #0 r317782:

--- sys/geom/journal/g_journal.c.orig   2017-05-04 10:02:46.000000000 +0200
+++ sys/geom/journal/g_journal.c        2017-05-05 23:37:57.625783000 +0200
@@ -131,28 +131,28 @@
 SYSCTL_UINT(_kern_geom_journal, OID_AUTO, optimize, CTLFLAG_RW,
     &g_journal_do_optimize, 0, "Try to combine bios on flush and copy");

-static u_int g_journal_cache_used = 0;
-static u_int g_journal_cache_limit = 64 * 1024 * 1024;
-TUNABLE_INT("kern.geom.journal.cache.limit", &g_journal_cache_limit);
+static u_long g_journal_cache_used = 0;
+static u_long g_journal_cache_limit = 64 * 1024 * 1024;
+TUNABLE_ULONG("kern.geom.journal.cache.limit", &g_journal_cache_limit);
 static u_int g_journal_cache_divisor = 2;
 TUNABLE_INT("kern.geom.journal.cache.divisor", &g_journal_cache_divisor);
 static u_int g_journal_cache_switch = 90;
 static u_int g_journal_cache_misses = 0;
 static u_int g_journal_cache_alloc_failures = 0;
-static u_int g_journal_cache_low = 0;
+static u_long g_journal_cache_low = 0;

 static SYSCTL_NODE(_kern_geom_journal, OID_AUTO, cache, CTLFLAG_RW, 0,
     "GEOM_JOURNAL cache");
-SYSCTL_UINT(_kern_geom_journal_cache, OID_AUTO, used, CTLFLAG_RD,
+SYSCTL_ULONG(_kern_geom_journal_cache, OID_AUTO, used, CTLFLAG_RD,
     &g_journal_cache_used, 0, "Number of allocated bytes");
 static int
 g_journal_cache_limit_sysctl(SYSCTL_HANDLER_ARGS)
 {
-       u_int limit;
+       u_long limit;
        int error;

        limit = g_journal_cache_limit;
-       error = sysctl_handle_int(oidp, &limit, 0, req);
+       error = sysctl_handle_long(oidp, &limit, 0, req);
        if (error != 0 || req->newptr == NULL)
                return (error);
        g_journal_cache_limit = limit;
@@ -160,7 +160,7 @@
        return (0);
 }
 SYSCTL_PROC(_kern_geom_journal_cache, OID_AUTO, limit,
-    CTLTYPE_UINT | CTLFLAG_RW, NULL, 0, g_journal_cache_limit_sysctl, "I",
+    CTLTYPE_ULONG | CTLFLAG_RW, NULL, 0, g_journal_cache_limit_sysctl, "LU",
     "Maximum number of allocated bytes");
 SYSCTL_UINT(_kern_geom_journal_cache, OID_AUTO, divisor, CTLFLAG_RDTUN,
     &g_journal_cache_divisor, 0,
@@ -3035,8 +3035,8 @@
                        kproc_exit(0);
                }
                if (error == 0 && g_journal_sync_requested == 0) {
-                       GJ_DEBUG(1, "Out of cache, force switch (used=%u "
-                           "limit=%u).", g_journal_cache_used,
+                       GJ_DEBUG(1, "Out of cache, force switch (used=%jd "
+                           "limit=%jd).", g_journal_cache_used,
                            g_journal_cache_limit);
                }
                GJ_TIMER_START(1, &bt);
Comment 2 Eugene Grosbein freebsd_committer freebsd_triage 2017-05-10 16:11:09 UTC
(In reply to longwitz from comment #1)

It seems, your patch was made for 10.x sources, wasn't it?
It does not apply to stable/11 nor head. Would you please make a version for one of these branches please? I'd like to test it.
Comment 3 longwitz 2017-05-10 20:05:47 UTC
Yes, my patch was for 10.3 r317782 and worked only for amd64. The following (untested) patch is against HEAD and should work for both i386 and amd64:

--- g_journal.c.orig    2017-05-10 21:25:30.000000000 +0200
+++ g_journal.c 2017-05-10 21:43:24.000000000 +0200
@@ -130,26 +130,26 @@
 SYSCTL_UINT(_kern_geom_journal, OID_AUTO, optimize, CTLFLAG_RW,
     &g_journal_do_optimize, 0, "Try to combine bios on flush and copy");

-static u_int g_journal_cache_used = 0;
-static u_int g_journal_cache_limit = 64 * 1024 * 1024;
+static u_long g_journal_cache_used = 0;
+static u_long g_journal_cache_limit = 64 * 1024 * 1024;
 static u_int g_journal_cache_divisor = 2;
 static u_int g_journal_cache_switch = 90;
 static u_int g_journal_cache_misses = 0;
 static u_int g_journal_cache_alloc_failures = 0;
-static u_int g_journal_cache_low = 0;
+static u_long g_journal_cache_low = 0;

 static SYSCTL_NODE(_kern_geom_journal, OID_AUTO, cache, CTLFLAG_RW, 0,
     "GEOM_JOURNAL cache");
-SYSCTL_UINT(_kern_geom_journal_cache, OID_AUTO, used, CTLFLAG_RD,
+SYSCTL_ULONG(_kern_geom_journal_cache, OID_AUTO, used, CTLFLAG_RD,
     &g_journal_cache_used, 0, "Number of allocated bytes");
 static int
 g_journal_cache_limit_sysctl(SYSCTL_HANDLER_ARGS)
 {
-       u_int limit;
+       u_long limit;
        int error;

        limit = g_journal_cache_limit;
-       error = sysctl_handle_int(oidp, &limit, 0, req);
+       error = sysctl_handle_long(oidp, &limit, 0, req);
        if (error != 0 || req->newptr == NULL)
                return (error);
        g_journal_cache_limit = limit;
@@ -157,7 +157,7 @@
        return (0);
 }
 SYSCTL_PROC(_kern_geom_journal_cache, OID_AUTO, limit,
-    CTLTYPE_UINT | CTLFLAG_RWTUN, NULL, 0, g_journal_cache_limit_sysctl, "I",
+    CTLTYPE_ULONG | CTLFLAG_RWTUN, NULL, 0, g_journal_cache_limit_sysctl, "LU",
     "Maximum number of allocated bytes");
 SYSCTL_UINT(_kern_geom_journal_cache, OID_AUTO, divisor, CTLFLAG_RDTUN,
     &g_journal_cache_divisor, 0,
@@ -3046,9 +3046,9 @@
                        kproc_exit(0);
                }
                if (error == 0 && g_journal_sync_requested == 0) {
-                       GJ_DEBUG(1, "Out of cache, force switch (used=%u "
-                           "limit=%u).", g_journal_cache_used,
-                           g_journal_cache_limit);
+                       GJ_DEBUG(1, "Out of cache, force switch (used=%jd "
+                           "limit=%jd).", (intmax_t)g_journal_cache_used,
+                           (intmax_t)g_journal_cache_limit);
                }
                GJ_TIMER_START(1, &bt);
                g_journal_do_switch(mp);

I like to mention that kern.geom.journal.cache.limit does not work as a tunable in loader.conf because g_journal.c overwrites this immediately in g_journal_init(). Therefore I use kern.geom.journal.cache.divisor in loader.conf to justify the cache at boottime.
Comment 4 Eugene Grosbein 2017-05-12 18:56:57 UTC
Created attachment 182549 [details]
patch for stable/11

I've applied your patch to my stable/11 system having 16GB RAM and it really helps.

Please commit.
Comment 5 commit-hook freebsd_committer freebsd_triage 2017-08-07 19:18:44 UTC
A commit references this bug:

Author: mckusick
Date: Mon Aug  7 19:18:28 UTC 2017
New revision: 322178
URL: https://svnweb.freebsd.org/changeset/base/322178

Log:
  sysctl kern.geom.journal.cache.limit shows negative value for FreeBSD/amd64
  system having over 4GB RAM. That's due to:

  1) the limit being u_int instead of u_long like vm.kmem_size (the limit is
     half of vm.kmem_size by default for amd64);
  2) sysctl handler g_journal_cache_limit_sysctl() using u_int instead of u_long.

  The fix is to replace u_int with u_long for the kern.geom.journal.cache.limit
  sysctl variable.

  PR: 198500
  Submitted by: Dr. Andreas Longwitz <longwitz@incore.de>
  Reported by: Eugene Grosbein
  Discussed with: kib
  MFC after: 1 week

Changes:
  head/sys/geom/journal/g_journal.c
Comment 6 Kirk McKusick freebsd_committer freebsd_triage 2017-08-07 19:22:04 UTC
Further assurance that this fix helps by Dr. Andreas Longwitz <longwitz@incore.de>. So I have committed this change. I will also commit another fix to g_journal.c by  Dr. Andreas Longwitz that fixes a panic when using snapshots on a gjournalled filesystem. Both fixes will be MFC'ed to 10 and 11 after a week if no problems arise.

Sorry for the long delay in getting this fix committed.
Comment 7 commit-hook freebsd_committer freebsd_triage 2017-08-14 02:49:39 UTC
A commit references this bug:

Author: mckusick
Date: Mon Aug 14 02:49:31 UTC 2017
New revision: 322480
URL: https://svnweb.freebsd.org/changeset/base/322480

Log:
  MFC r322178
  Bug 198500 reports bad sysctl values for gjournal cache limit.

  PR: 198500
  Submitted by: Dr. Andreas Longwitz <longwitz@incore.de>
  Reported by: Eugene Grosbein
  Discussed with: kib

Changes:
_U  stable/11/
  stable/11/sys/geom/journal/g_journal.c
Comment 8 commit-hook freebsd_committer freebsd_triage 2017-08-14 19:13:53 UTC
A commit references this bug:

Author: mckusick
Date: Mon Aug 14 19:12:52 UTC 2017
New revision: 322513
URL: https://svnweb.freebsd.org/changeset/base/322513

Log:
  MFC of 322178:

  Bug 198500 reports bad sysctl values for gjournal cache limit.

  PR: 198500
  Submitted by: Dr. Andreas Longwitz <longwitz@incore.de>
  Reported by: Eugene Grosbein
  Discussed with: kib
  Approved by: re (marius)

Changes:
_U  stable/10/
  stable/10/sys/geom/journal/g_journal.c
Comment 9 Kirk McKusick freebsd_committer freebsd_triage 2017-08-14 19:16:41 UTC
The fix has been MFC'ed to 10-stable and 11-stable.