Bug fixes from incremental depmod PR #257

mwilck · 2024-11-21T14:44:57Z

This PR splits off the bug fixes from #253, as requested by @evelikov.

I found these issues while making the kmod CI tests work with the "incremental depmod" patch set.

evelikov

The changes look great IMHO. Would be amazing to have a test or two but it's a non-blocking request.

shared/hash.c

shared/strbuf.c

mwilck · 2024-11-21T23:15:06Z

I've added tests now that trigger the failures that I saw on #253.

I'm not sure how you deal with failures like this. I chose to add the test with .expected_fail=true first, and put the fix on top, reverting the expected_fail property. This way the CI doesn't fail, even on the commits that introduce the failing tests. In my experience that's beneficial for bisecting.

I also added one more patch ("always use a step size of 4"), as the logic for determining the step size seems strange (I wouldn't have encountered the out-of-bounds memory access problem if the step size wasn't 1 with n_buckets == 32. Please have a look.

evelikov

You are a star, thank you for the tests - loving the expected_fail approach. The extra patch also looks good.

Seems like I cannot approve for CI, so it'll be up-to @lucasdemarchi

mwilck · 2024-11-27T13:31:55Z

rebased onto current master

lucasdemarchi · 2024-11-29T14:08:48Z

@evelikov no idea why CI is not running on this PR.

codecov · 2024-11-29T14:14:18Z

Codecov Report

Attention: Patch coverage is 95.23810% with 1 line in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
testsuite/test-hash.c	93.75%	0 Missing and 1 partial ⚠️

Files with missing lines	Coverage Δ
shared/hash.c	`93.37% <100.00%> (ø)`
shared/strbuf.c	`78.78% <100.00%> (ø)`
testsuite/test-strbuf.c	`69.34% <100.00%> (ø)`
testsuite/test-hash.c	`84.07% <93.75%> (ø)`

Fixes: c36ddb6 ("depmod: Add option to override MODULE_DIRECTORY") Signed-off-by: Martin Wilck <martin_wilck@gmx.de> Reviewed-by: Emil Velikov <emil.l.velikov@gmail.com> Link: kmod-project#257 Signed-off-by: Lucas De Marchi <lucas.de.marchi@gmail.com>

Assume bucket->used is 1, and element 0 is deleted. We shouldn't access any memory above (entry + 1) in this case. Likewise, if bucked->used is 2, only one element should be shifted, etc. Fixes: 7db0865 ("Add simple hash implementation") Signed-off-by: Martin Wilck <martin_wilck@gmx.de> Reviewed-by: Emil Velikov <emil.l.velikov@gmail.com> Link: kmod-project#257 Signed-off-by: Lucas De Marchi <lucas.de.marchi@gmail.com>

...using n_buckets = 32, which will cause a step size of 1. Before commit d658ec7 ("libkmod: hash_del: fix out-of-bounds memory access") this test would fail. Signed-off-by: Martin Wilck <mwilck@suse.com> Reviewed-by: Emil Velikov <emil.l.velikov@gmail.com> Link: kmod-project#257 Signed-off-by: Lucas De Marchi <lucas.de.marchi@gmail.com>

It can happen that bucket->entries is NULL, but bsearch is annotated such that it's second argument must be non-NULL. Fixes: 7db0865 ("Add simple hash implementation") Signed-off-by: Martin Wilck <martin_wilck@gmx.de> Reviewed-by: Emil Velikov <emil.l.velikov@gmail.com> Link: kmod-project#257 Signed-off-by: Lucas De Marchi <lucas.de.marchi@gmail.com>

This test would fail before commit 520ac55 ("libkmod: hash_del: handle deleting a non-existing element gracefully"). Signed-off-by: Martin Wilck <mwilck@suse.com> Reviewed-by: Emil Velikov <emil.l.velikov@gmail.com> Link: kmod-project#257 Signed-off-by: Lucas De Marchi <lucas.de.marchi@gmail.com>

If buf->bytes, buf->used, and len are all 0, buf_grow() will do nothing, and memcpy() willbe called with a NULL first argument. This will cause an error because the function is annotated with __nonnull(1, 2). Fixes: e62d8c7 ("strbuf: make strbuf_pushchars() a little less dumb") Signed-off-by: Martin Wilck <martin_wilck@gmx.de> Reviewed-by: Emil Velikov <emil.l.velikov@gmail.com> Link: kmod-project#257 Signed-off-by: Lucas De Marchi <lucas.de.marchi@gmail.com>

This test would fail before commit bbd8598 ("libkmod: strbuf_pushchars: handle pushing empty string gracefully"). Signed-off-by: Martin Wilck <mwilck@suse.com> Reviewed-by: Emil Velikov <emil.l.velikov@gmail.com> Link: kmod-project#257 Signed-off-by: Lucas De Marchi <lucas.de.marchi@gmail.com>

The current algorithm would choose a step size of 4 for n_buckets == 31, but 1 for n_buckets == 32, which seems wrong. Fix it. Signed-off-by: Martin Wilck <mwilck@suse.com> Reviewed-by: Emil Velikov <emil.l.velikov@gmail.com> Link: kmod-project#257 Signed-off-by: Lucas De Marchi <lucas.de.marchi@gmail.com>

Fixes: c36ddb6 ("depmod: Add option to override MODULE_DIRECTORY") Signed-off-by: Martin Wilck <martin_wilck@gmx.de>

...using n_buckets = 32, which will cause a step size of 1. This test fails, and will be fixed by the next commit. Signed-off-by: Martin Wilck <mwilck@suse.com>

Assume bucket->used is 1, and element 0 is deleted. We shouldn't access any memory above (entry + 1) in this case. Likewise, if bucked->used is 2, only one element should be shifted, etc. Fixes: 7db0865 ("Add simple hash implementation") Signed-off-by: Martin Wilck <martin_wilck@gmx.de>

This test fails and will be fixed by the next commit. Signed-off-by: Martin Wilck <mwilck@suse.com>

It can happen that bucket->entries is NULL, but bsearch is annotated such that it's second argument must be non-NULL. Fixes: 7db0865 ("Add simple hash implementation") Signed-off-by: Martin Wilck <martin_wilck@gmx.de>

This test fails, and will be fixed by the next commit. Signed-off-by: Martin Wilck <mwilck@suse.com>

If buf->bytes, buf->used, and len are all 0, buf_grow() will do nothing, and memcpy() willbe called with a NULL first argument. This will cause an error because the function is annotated with __nonnull(1, 2). Fixes: e62d8c7 ("strbuf: make strbuf_pushchars() a little less dumb") Signed-off-by: Martin Wilck <martin_wilck@gmx.de>

The current algorithm would choose a step size of 4 for n_buckets == 31, but 1 for n_buckets == 32, which seems wrong. Fix it. Signed-off-by: Martin Wilck <mwilck@suse.com>

Fixes: c36ddb6 ("depmod: Add option to override MODULE_DIRECTORY") Signed-off-by: Martin Wilck <martin_wilck@gmx.de> Reviewed-by: Emil Velikov <emil.l.velikov@gmail.com> Link: #257 Signed-off-by: Lucas De Marchi <lucas.de.marchi@gmail.com>

...using n_buckets = 32, which will cause a step size of 1. This test fails, and will be fixed by the next commit. Signed-off-by: Martin Wilck <mwilck@suse.com> Reviewed-by: Emil Velikov <emil.l.velikov@gmail.com> Link: #257 Signed-off-by: Lucas De Marchi <lucas.de.marchi@gmail.com>

Assume bucket->used is 1, and element 0 is deleted. We shouldn't access any memory above (entry + 1) in this case. Likewise, if bucked->used is 2, only one element should be shifted, etc. Fixes: 7db0865 ("Add simple hash implementation") Signed-off-by: Martin Wilck <martin_wilck@gmx.de> Reviewed-by: Emil Velikov <emil.l.velikov@gmail.com> Link: #257 Signed-off-by: Lucas De Marchi <lucas.de.marchi@gmail.com>

This test fails and will be fixed by the next commit. Signed-off-by: Martin Wilck <mwilck@suse.com> Reviewed-by: Emil Velikov <emil.l.velikov@gmail.com> Link: #257 Signed-off-by: Lucas De Marchi <lucas.de.marchi@gmail.com>

It can happen that bucket->entries is NULL, but bsearch is annotated such that it's second argument must be non-NULL. Fixes: 7db0865 ("Add simple hash implementation") Signed-off-by: Martin Wilck <martin_wilck@gmx.de> Reviewed-by: Emil Velikov <emil.l.velikov@gmail.com> Link: #257 Signed-off-by: Lucas De Marchi <lucas.de.marchi@gmail.com>

This test fails, and will be fixed by the next commit. Signed-off-by: Martin Wilck <mwilck@suse.com> Reviewed-by: Emil Velikov <emil.l.velikov@gmail.com> Link: #257 Signed-off-by: Lucas De Marchi <lucas.de.marchi@gmail.com>

If buf->bytes, buf->used, and len are all 0, buf_grow() will do nothing, and memcpy() willbe called with a NULL first argument. This will cause an error because the function is annotated with __nonnull(1, 2). Fixes: e62d8c7 ("strbuf: make strbuf_pushchars() a little less dumb") Signed-off-by: Martin Wilck <martin_wilck@gmx.de> Reviewed-by: Emil Velikov <emil.l.velikov@gmail.com> Link: #257 Signed-off-by: Lucas De Marchi <lucas.de.marchi@gmail.com>

The current algorithm would choose a step size of 4 for n_buckets == 31, but 1 for n_buckets == 32, which seems wrong. Fix it. Signed-off-by: Martin Wilck <mwilck@suse.com> Reviewed-by: Emil Velikov <emil.l.velikov@gmail.com> Link: #257 Signed-off-by: Lucas De Marchi <lucas.de.marchi@gmail.com>

lucasdemarchi · 2024-11-29T15:22:57Z

Thanks a lot for the fixes and additional tests. I was going to swap the order of commits and avoid using the xfail... but changed my mind - it looks good in this order. I fixed the coding style issue and applied.

evelikov · 2024-12-02T13:41:35Z

@lucasdemarchi some point last year, GitHub changed so that first PR from new people need to be approved by maintainers - their blog post has the details.

I suspect that adding me as reviewer might also do the trick.

lucasdemarchi · 2024-12-02T15:50:38Z

@evelikov the problem is that the PR itself was not showing it the approval as needed. I had to go to each individual worfklow and approve them individually. I saw the request when the PR was opened, but it wasn't available anymore after it had been reviewed.

lucasdemarchi · 2024-12-02T16:11:29Z

I suspect that adding me as reviewer might also do the trick.

it doesn't seem that there is such a role in Github. There is "triage" and "write". Anyway I created a "Reviewer" team with write access and invited both you and @stoeckmann. Note that PRs shouldn't be merged though - I manually apply the PRs pushing to both kernel.org and github.

evelikov · 2024-12-05T13:16:25Z

Thank you, duly noted.

mwilck mentioned this pull request Nov 21, 2024

Incremental depmod #253

Open

evelikov approved these changes Nov 21, 2024

View reviewed changes

shared/hash.c Show resolved Hide resolved

shared/hash.c Show resolved Hide resolved

shared/strbuf.c Show resolved Hide resolved

mwilck force-pushed the fixes-v33 branch 2 times, most recently from dc352d2 to 486771d Compare November 21, 2024 23:11

evelikov approved these changes Nov 24, 2024

View reviewed changes

mwilck force-pushed the fixes-v33 branch from 486771d to d7e3d0f Compare November 27, 2024 13:31

lucasdemarchi force-pushed the fixes-v33 branch from d7e3d0f to bd374ed Compare November 29, 2024 14:07

lucasdemarchi approved these changes Nov 29, 2024

View reviewed changes

lucasdemarchi force-pushed the fixes-v33 branch from bd374ed to 2a2bbcd Compare November 29, 2024 14:41

mwilck and others added 8 commits November 29, 2024 09:11

man: remove duplicate option in depmod.8.scd

efe0cac

Fixes: c36ddb6 ("depmod: Add option to override MODULE_DIRECTORY") Signed-off-by: Martin Wilck <martin_wilck@gmx.de>

testsuite: test-hash: add a test for deleting a single element

3e58c87

...using n_buckets = 32, which will cause a step size of 1. This test fails, and will be fixed by the next commit. Signed-off-by: Martin Wilck <mwilck@suse.com>

testsuite: test-hash: add a test for deleting a non-existing element

ae2c7cb

This test fails and will be fixed by the next commit. Signed-off-by: Martin Wilck <mwilck@suse.com>

testsuite: test_strbuf_pushmem: test pushing 0 bytes to an empty strbuf

5cfcc0f

This test fails, and will be fixed by the next commit. Signed-off-by: Martin Wilck <mwilck@suse.com>

libkmod: hash_new(): always use a step size of at least 4

eb15490

The current algorithm would choose a step size of 4 for n_buckets == 31, but 1 for n_buckets == 32, which seems wrong. Fix it. Signed-off-by: Martin Wilck <mwilck@suse.com>

lucasdemarchi force-pushed the fixes-v33 branch from 2a2bbcd to eb15490 Compare November 29, 2024 15:14

lucasdemarchi closed this Nov 29, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Bug fixes from incremental depmod PR #257

Bug fixes from incremental depmod PR #257

Uh oh!

mwilck commented Nov 21, 2024

Uh oh!

evelikov left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mwilck commented Nov 21, 2024 •

edited

Loading

Uh oh!

evelikov left a comment

Uh oh!

mwilck commented Nov 27, 2024

Uh oh!

lucasdemarchi commented Nov 29, 2024

Uh oh!

codecov bot commented Nov 29, 2024

Uh oh!

lucasdemarchi commented Nov 29, 2024

Uh oh!

evelikov commented Dec 2, 2024

Uh oh!

lucasdemarchi commented Dec 2, 2024

Uh oh!

lucasdemarchi commented Dec 2, 2024

Uh oh!

evelikov commented Dec 5, 2024

Uh oh!

Uh oh!

Bug fixes from incremental depmod PR #257

Bug fixes from incremental depmod PR #257

Uh oh!

Conversation

mwilck commented Nov 21, 2024

Uh oh!

evelikov left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mwilck commented Nov 21, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

evelikov left a comment

Choose a reason for hiding this comment

Uh oh!

mwilck commented Nov 27, 2024

Uh oh!

lucasdemarchi commented Nov 29, 2024

Uh oh!

codecov bot commented Nov 29, 2024

Codecov Report

Uh oh!

lucasdemarchi commented Nov 29, 2024

Uh oh!

evelikov commented Dec 2, 2024

Uh oh!

lucasdemarchi commented Dec 2, 2024

Uh oh!

lucasdemarchi commented Dec 2, 2024

Uh oh!

evelikov commented Dec 5, 2024

Uh oh!

Uh oh!

mwilck commented Nov 21, 2024 •

edited

Loading