Skip to content

Fix overflow in mounstats#165

Merged
pgier merged 1 commit intomasterfrom
superq/fix_mountstats
May 29, 2019
Merged

Fix overflow in mounstats#165
pgier merged 1 commit intomasterfrom
superq/fix_mountstats

Conversation

@SuperQ
Copy link
Member

@SuperQ SuperQ commented May 28, 2019

Don't use time.Duration for mountstats timing data. These values can be
large uint64 milliseconds values and easily overflow time.Duration
(in64 nanoseconds). Return the raw time in milliseconds.

Signed-off-by: Ben Kochie superq@gmail.com

@SuperQ SuperQ requested review from mdlayher and pgier May 28, 2019 16:51
Copy link
Contributor

@mdlayher mdlayher left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm after small nit.

Copy link
Collaborator

@pgier pgier left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, although I'm still a bit confused about why the problem occurred. AFAICT, the kernel sources store the time in nanoseconds as a signed int64 (https://github.com/torvalds/linux/blob/master/include/linux/ktime.h#L27) and then divide to convert the milliseconds before printing (https://github.com/torvalds/linux/blob/master/net/sunrpc/stats.c#L224). So I would think the max value that could appear in mountstats would be (max int64)/1000000.

If this was on an older kernel, maybe it was still using the older unsigned long long format?(torvalds/linux@ff83997#diff-4efb17a470df6029d642a329c7a42123).

@SuperQ
Copy link
Member Author

SuperQ commented May 28, 2019

Yes, the example data came from a RHEL 7.3 system, 3.10 kernel.

@SuperQ SuperQ force-pushed the superq/fix_mountstats branch from 297bd8b to f18cce3 Compare May 28, 2019 18:16
Don't use time.Duration for mountstats timing data. These values can be
large uint64 milliseconds values and easily overflow time.Duration
(in64 nanoseconds). Return the raw time in milliseconds.

Signed-off-by: Ben Kochie <superq@gmail.com>
@SuperQ SuperQ force-pushed the superq/fix_mountstats branch from f18cce3 to 2539d54 Compare May 29, 2019 08:47
@pgier pgier merged commit 65bdadf into master May 29, 2019
SuperQ added a commit to prometheus/node_exporter that referenced this pull request May 30, 2019
* Update procfs vendor to pull in github.com/prometheus/procfs/pull/165
* Update mountstats collector to use new types.
* Rollover counter automatically to avoid float64 accuracy issues.
* Update e2e test.

Signed-off-by: Ben Kochie <superq@gmail.com>
SuperQ added a commit to prometheus/node_exporter that referenced this pull request May 31, 2019
* Update procfs vendor to pull in github.com/prometheus/procfs/pull/165
* Update mountstats collector to use new types.
* Rollover counter automatically to avoid float64 accuracy issues.
* Update e2e test.

Signed-off-by: Ben Kochie <superq@gmail.com>
@SuperQ SuperQ deleted the superq/fix_mountstats branch May 31, 2019 14:52
SuperQ added a commit to prometheus/node_exporter that referenced this pull request May 31, 2019
* Update procfs vendor to pull in github.com/prometheus/procfs/pull/165
* Update mountstats collector to use new types.
* Rollover counter automatically to avoid float64 accuracy issues.
* Update e2e test.

Signed-off-by: Ben Kochie <superq@gmail.com>
SuperQ added a commit to prometheus/node_exporter that referenced this pull request Jun 4, 2019
* Update procfs vendor to pull in github.com/prometheus/procfs/pull/165
* Update mountstats collector to use new types.
* Rollover counter automatically to avoid float64 accuracy issues.
* Update e2e test.

Signed-off-by: Ben Kochie <superq@gmail.com>
SuperQ added a commit to prometheus/node_exporter that referenced this pull request Jun 4, 2019
* Update procfs vendor to pull in github.com/prometheus/procfs/pull/165
* Update mountstats collector to use new types.
* Rollover counter automatically to avoid float64 accuracy issues.
* Update e2e test.

Signed-off-by: Ben Kochie <superq@gmail.com>
bobrik pushed a commit to bobrik/procfs that referenced this pull request Jan 14, 2023
Do not run test_task in parallel with other tests
oblitorum pushed a commit to shatteredsilicon/node_exporter that referenced this pull request Apr 9, 2024
* Update procfs vendor to pull in github.com/prometheus/procfs/pull/165
* Update mountstats collector to use new types.
* Rollover counter automatically to avoid float64 accuracy issues.
* Update e2e test.

Signed-off-by: Ben Kochie <superq@gmail.com>
oblitorum pushed a commit to shatteredsilicon/node_exporter that referenced this pull request Apr 9, 2024
* Update procfs vendor to pull in github.com/prometheus/procfs/pull/165
* Update mountstats collector to use new types.
* Rollover counter automatically to avoid float64 accuracy issues.
* Update e2e test.

Signed-off-by: Ben Kochie <superq@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants