Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

linuxmodule: fix md5_append wrt non-virt data #90

Merged
merged 1 commit into from
Jun 30, 2013
Merged

Conversation

pallas
Copy link
Contributor

@pallas pallas commented Jun 28, 2013

sg_init_one uses sg_init_buf, which only works with kmalloc'd data. In particular, this excludes anything vmalloc'd as well as anything in the rodata section of the module. The code that was failing for me was essentially

    String str("data");
    uint8_t digest[16];

    md5_state_t state;
    md5_init(&state);
    md5_append(&state, str.data(), str.length());
    md5_finish(&state, digest);
    md5_free(&state);

and the work-around was to invoke String::mutable_data() prior to the md5_append, which kmalloc's and makes String::data() safe for sg_init_buf.

The better solution is to call virt_addr_valid, only using sg_init_one if true; otherwise, we must use vmalloc_to_page on individual pages, which will walk the page-table and should do the Right Thing.

Since vmalloc'd data might not be continuous, we must iterate over the individual pages that make up the allocation. Because this is expensive to begin with, we call update for each page using a single scatterlist rather than allocating an sg_table and calling update once. This also simplifies the code.

I tested this by adding

    click_chatter("%p %d %d %d", data, off, len, nbytes);

inside the loop and watching it "do the Right Thing" to a large vmalloc allocation, verifying that the hash was correct using md5sum.

sg_init_one uses sg_init_buf, which only works with kmalloc'd data.  In
particular, this excludes anything vmalloc'd as well as anything in the
rodata section of the module.  The code that was failing for me was
essentially

	String str("data");
	uint8_t digest[16];

	md5_state_t state;
	md5_init(&state);
	md5_append(&state, str.data(), str.length());
	md5_finish(&state, digest);
	md5_free(&state);

and the work-around was to invoke String::mutable_data() prior to the
md5_append, which kmalloc's and makes String::data() safe for sg_init_buf.

The better solution is to call virt_addr_valid, only using sg_init_one if
true; otherwise, we must use vmalloc_to_page on individual pages, which will
walk the page-table and should do the Right Thing.

Since vmalloc'd data might not be continuous, we must iterate over the
individual pages that make up the allocation.  Because this is expensive to
begin with, we call update for each page using a single scatterlist rather
than allocating an sg_table and calling update once.  This also simplifies
the code.

I tested this by adding

	click_chatter("%p %d %d %d", data, off, len, nbytes);

inside the loop and watching it "do the Right Thing" to a large vmalloc
allocation, verifying that the hash was correct using md5sum.
kohler added a commit that referenced this pull request Jun 30, 2013
linuxmodule: fix md5_append wrt non-virt data
@kohler kohler merged commit e5beec5 into kohler:master Jun 30, 2013
@kohler
Copy link
Owner

kohler commented Jun 30, 2013

Epic. Thanks. (Maybe we should've just used our own MD5.)

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.

2 participants