Possible broken MM code in dell-laptop.c?

DISCUSSION Pali Rohár 4 participants 12 messages Jun 14, 2015 – Jun 18, 2015

Subsystems (2)

Memory management x86 platform drivers

lore

Share & tools

lore

Pali Rohár

author Acked-by Matthew Garrett

2015-06-14 09:05 UTC #0 lore raw

Hello,

in drivers/platform/x86/dell-laptop.c is this part of code:

static int __init dell_init(void)
{
...

	/*
	 * Allocate buffer below 4GB for SMI data--only 32-bit physical addr
	 * is passed to SMI handler.
	 */
	bufferpage = alloc_page(GFP_KERNEL | GFP_DMA32);
	if (!bufferpage) {
		ret = -ENOMEM;
		goto fail_buffer;
	}
	buffer = page_address(bufferpage);

	ret = dell_setup_rfkill();

	if (ret) {
		pr_warn("Unable to setup rfkill\n");
		goto fail_rfkill;
	}
...
fail_rfkill:
	free_page((unsigned long)bufferpage);

fail_buffer:
...
}

Then there is another part:

static void __exit dell_exit(void)
{
...

	free_page((unsigned long)buffer);
}

I suspect that there is some problem with free_page() call. In dell_init is called free_page() on bufferpage and in dell_exit() on buffer.

Matthew and Stuart, you introduced this inconsistency in commit:

-------------------------------------------------
commit 116ee77b2858d9c89c0327f3a47c8ba864bf4a96
Author: Stuart Hayes <[email protected]>
Committer: Matthew Garrett <[email protected]>
Date: Wed Feb 10 14:12:13 2010 -0500

dell-laptop: Use buffer with 32-bit physical address

Calls to communicate with system firmware via a SMI (using dcdbas) need to use a buffer that has a physical address of 4GB or less. Currently the dell-laptop driver does not guarantee this, and when
the
buffer address is higher than 4GB, the address is truncated to 32
bits
and the SMI handler writes to the wrong memory address.

Signed-off-by Stuart Hayes <[email protected]>

Acked-by Matthew Garrett <[email protected]>

-------------------------------------------------

Can you or somebody else (CCed linux-mm) look at this page related code? I think it is wrong, but somebody authoritative should provide answer.

Thanks.

Signature · 2 lines

Pali Rohár
[email protected]

Darren Hart

2015-06-15 20:36 UTC #1 lore raw

Quoted 31 lines · Pali Rohár

Hello,

in drivers/platform/x86/dell-laptop.c is this part of code:

static int __init dell_init(void)
{
...

	/*
	 * Allocate buffer below 4GB for SMI data--only 32-bit physical addr
	 * is passed to SMI handler.
	 */
	bufferpage = alloc_page(GFP_KERNEL | GFP_DMA32);
	if (!bufferpage) {
		ret = -ENOMEM;
		goto fail_buffer;
	}
	buffer = page_address(bufferpage);

	ret = dell_setup_rfkill();

	if (ret) {
		pr_warn("Unable to setup rfkill\n");
		goto fail_rfkill;
	}
...
fail_rfkill:
	free_page((unsigned long)bufferpage);

fail_buffer:
...
}

Quoted 6 lines

Then there is another part:
static void __exit dell_exit(void)
{
...
	free_page((unsigned long)buffer);

I believe you are correct, and this should be bufferpage. Have you observed any failures?

Signature · 2 lines

Darren Hart
Intel Open Source Technology Center

Pali Rohár

author

2015-06-15 20:42 UTC #2 lore raw

Quoted 44 lines · On Monday 15 June 2015 22:36:45 Darren Hart wrote:

Quoted 43 lines · Pali Rohár
Hello,
in drivers/platform/x86/dell-laptop.c is this part of code:
static int __init dell_init(void)
{
...
	/*
	
	 * Allocate buffer below 4GB for SMI data--only 32-bit physical
	 addr * is passed to SMI handler.
	 */
	
	bufferpage = alloc_page(GFP_KERNEL | GFP_DMA32);
	if (!bufferpage) {
	
		ret = -ENOMEM;
		goto fail_buffer;
	
	}
	buffer = page_address(bufferpage);
	
	ret = dell_setup_rfkill();
	
	if (ret) {
	
		pr_warn("Unable to setup rfkill\n");
		goto fail_rfkill;
	
	}
...
fail_rfkill:
	free_page((unsigned long)bufferpage);
fail_buffer:
...
}
Then there is another part:

Quoted 8 lines

Quoted 5 lines
static void __exit dell_exit(void)
{
...
	free_page((unsigned long)buffer);
I believe you are correct, and this should be bufferpage. Have you observed any failures?

Rmmoding dell-laptop.ko works fine. There is no error in dmesg. I think that buffer (and not bufferpage) should be passed to free_page(). So in my opinion problem is at fail_rfkill: label and not in dell_exit().

But somebody from linux-mm should look at it...

Signature · 2 lines

Pali Rohár
[email protected]

Pavel Machek

2015-06-16 10:12 UTC #3 lore raw

Quoted 49 lines · Pali Rohár

Quoted 48 lines · On Monday 15 June 2015 22:36:45 Darren Hart wrote:
Quoted 47 lines · Pali Rohár
Hello,
in drivers/platform/x86/dell-laptop.c is this part of code:
static int __init dell_init(void)
{
...
	/*
	
	 * Allocate buffer below 4GB for SMI data--only 32-bit physical
	 addr * is passed to SMI handler.
	 */
	
	bufferpage = alloc_page(GFP_KERNEL | GFP_DMA32);
	if (!bufferpage) {
	
		ret = -ENOMEM;
		goto fail_buffer;
	
	}
	buffer = page_address(bufferpage);
	
	ret = dell_setup_rfkill();
	
	if (ret) {
	
		pr_warn("Unable to setup rfkill\n");
		goto fail_rfkill;
	
	}
...
fail_rfkill:
	free_page((unsigned long)bufferpage);
fail_buffer:
...
}
Then there is another part:
static void __exit dell_exit(void)
{
...

Quoted 8 lines

Quoted 4 lines
Quoted 1 line
	free_page((unsigned long)buffer);
I believe you are correct, and this should be bufferpage. Have you observed any failures?
Rmmoding dell-laptop.ko works fine. There is no error in dmesg. I think that buffer (and not bufferpage) should be passed to free_page(). So in my opinion problem is at fail_rfkill: label and not in dell_exit().

You seem to be right. Interface is strange...

alloc_pages() returns struct page *,
__free_pages() takes struct page *,
free_pages() takes unsinged long.

Best regards,
Pavel

Signature · 2 lines

(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Michal Hocko

2015-06-15 21:18 UTC #4 lore raw

In reply to Pali Rohár

Quoted 12 lines · Pali Rohár

Hello,
in drivers/platform/x86/dell-laptop.c is this part of code:
static int __init dell_init(void)
{
...
	/*
	 * Allocate buffer below 4GB for SMI data--only 32-bit physical addr
	 * is passed to SMI handler.
	 */
	bufferpage = alloc_page(GFP_KERNEL | GFP_DMA32);

[...]

Quoted 1 line

	buffer = page_address(bufferpage);

[...]

Quoted 2 lines

fail_rfkill:

	free_page((unsigned long)bufferpage);

This one should be __free_page because it consumes struct page* and it is the proper counter part for alloc_page. free_page, just to make it confusing, consumes an address which has to be translated to a struct page.

I have no idea why the API has been done this way and yeah, it is really confusing.

[...]

Quoted 4 lines

static void __exit dell_exit(void)
{
...
	free_page((unsigned long)buffer);

Signature · 8 lines

Michal Hocko
SUSE Labs

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to [email protected].  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

Pali Rohár

author

2015-06-15 21:28 UTC #5 lore raw

Quoted 31 lines · On Monday 15 June 2015 23:18:16 Michal Hocko wrote:

Quoted 15 lines · Pali Rohár
Hello,
in drivers/platform/x86/dell-laptop.c is this part of code:
static int __init dell_init(void)
{
...
	/*
	
	 * Allocate buffer below 4GB for SMI data--only 32-bit physical
	 addr * is passed to SMI handler.
	 */
	
	bufferpage = alloc_page(GFP_KERNEL | GFP_DMA32);
[...]
Quoted 1 line
	buffer = page_address(bufferpage);
[...]
Quoted 2 lines
fail_rfkill:
	free_page((unsigned long)bufferpage);
This one should be __free_page because it consumes struct page* and it is the proper counter part for alloc_page. free_page, just to make it confusing, consumes an address which has to be translated to a struct page.

Quoted 10 lines

I have no idea why the API has been done this way and yeah, it is really confusing.
[...]
Quoted 5 lines
static void __exit dell_exit(void)
{
...
	free_page((unsigned long)buffer);

So both, either:

free_page((unsigned long)buffer);

__free_page(bufferpage);

is correct?

Signature · 2 lines

Pali Rohár
[email protected]

Michal Hocko

2015-06-16 06:33 UTC #6 lore raw

Quoted 45 lines · Pali Rohár

Quoted 31 lines · On Monday 15 June 2015 23:18:16 Michal Hocko wrote:
Quoted 15 lines · Pali Rohár
Hello,
in drivers/platform/x86/dell-laptop.c is this part of code:
static int __init dell_init(void)
{
...
	/*
	
	 * Allocate buffer below 4GB for SMI data--only 32-bit physical
	 addr * is passed to SMI handler.
	 */
	
	bufferpage = alloc_page(GFP_KERNEL | GFP_DMA32);
[...]
Quoted 1 line
	buffer = page_address(bufferpage);
[...]
Quoted 2 lines
fail_rfkill:
	free_page((unsigned long)bufferpage);
This one should be __free_page because it consumes struct page* and it is the proper counter part for alloc_page. free_page, just to make it confusing, consumes an address which has to be translated to a struct page.
Quoted 10 lines
I have no idea why the API has been done this way and yeah, it is really confusing.
[...]
Quoted 5 lines
static void __exit dell_exit(void)
{
...
	free_page((unsigned long)buffer);
So both, either:

Quoted 7 lines

free_page((unsigned long)buffer);
or
__free_page(bufferpage);
is correct?

Yes. Although I would use __free_page variant as both seem to be globally visible.

Signature · 2 lines

Michal Hocko
SUSE Labs

Pali Rohár

author

2015-06-16 07:15 UTC #7 lore raw

Quoted 48 lines · On Tuesday 16 June 2015 08:33:46 Michal Hocko wrote:

Quoted 47 lines · Pali Rohár
Quoted 31 lines · On Monday 15 June 2015 23:18:16 Michal Hocko wrote:
Quoted 15 lines · Pali Rohár
Hello,
in drivers/platform/x86/dell-laptop.c is this part of code:
static int __init dell_init(void)
{
...
	/*
	
	 * Allocate buffer below 4GB for SMI data--only 32-bit physical
	 addr * is passed to SMI handler.
	 */
	
	bufferpage = alloc_page(GFP_KERNEL | GFP_DMA32);
[...]
Quoted 1 line
	buffer = page_address(bufferpage);
[...]
Quoted 2 lines
fail_rfkill:
	free_page((unsigned long)bufferpage);
This one should be __free_page because it consumes struct page* and it is the proper counter part for alloc_page. free_page, just to make it confusing, consumes an address which has to be translated to a struct page.
Quoted 10 lines
I have no idea why the API has been done this way and yeah, it is really confusing.
[...]
Quoted 5 lines
static void __exit dell_exit(void)
{
...
	free_page((unsigned long)buffer);
So both, either:
free_page((unsigned long)buffer);

Quoted 9 lines

Quoted 5 lines
or
__free_page(bufferpage);
is correct?
Yes. Although I would use __free_page variant as both seem to be globally visible.

Michal, thank you for explaining this situation!

Darren, I will prepare patch which will fix code and use __free_page().

(Btw, execution on fail_rfkill label caused kernel panic)

Signature · 2 lines

Pali Rohár
[email protected]

Michal Hocko

2015-06-16 07:43 UTC #8 lore raw

On Tue 16-06-15 09:15:23, Pali Rohár wrote:
[...]

Quoted 5 lines

Michal, thank you for explaining this situation!
Darren, I will prepare patch which will fix code and use __free_page().
(Btw, execution on fail_rfkill label caused kernel panic)

I am sorry, I could have made it more clear in the very first email. A panic is to be expected because free_page will translate the given address to a struct page* but this is what the code gave it. So an unrelated struct page would be freed (or maybe an invalid one).

Signature · 2 lines

Michal Hocko
SUSE Labs

Darren Hart

2015-06-17 03:43 UTC #9 lore raw

In reply to Pali Rohár

Quoted 51 lines · Pali Rohár

Quoted 50 lines · On Tuesday 16 June 2015 08:33:46 Michal Hocko wrote:
Quoted 49 lines · Pali Rohár
Quoted 31 lines · On Monday 15 June 2015 23:18:16 Michal Hocko wrote:
Quoted 15 lines · Pali Rohár
Hello,
in drivers/platform/x86/dell-laptop.c is this part of code:
static int __init dell_init(void)
{
...
	/*
	
	 * Allocate buffer below 4GB for SMI data--only 32-bit physical
	 addr * is passed to SMI handler.
	 */
	
	bufferpage = alloc_page(GFP_KERNEL | GFP_DMA32);
[...]
Quoted 1 line
	buffer = page_address(bufferpage);
[...]
Quoted 2 lines
fail_rfkill:
	free_page((unsigned long)bufferpage);
This one should be __free_page because it consumes struct page* and it is the proper counter part for alloc_page. free_page, just to make it confusing, consumes an address which has to be translated to a struct page.
Quoted 10 lines
I have no idea why the API has been done this way and yeah, it is really confusing.
[...]
Quoted 5 lines
static void __exit dell_exit(void)
{
...
	free_page((unsigned long)buffer);
So both, either:
free_page((unsigned long)buffer);
or

Quoted 7 lines

Quoted 7 lines
Quoted 3 lines
__free_page(bufferpage);
is correct?
Yes. Although I would use __free_page variant as both seem to be globally visible.

Michal - thanks for the context.

I'm surprised by your recommendation to use __free_page() out here in platform driver land.

I'd also prefer that the driver consistently free the same address to avoid confusion.

For these reasons, free_page((unsigned long)buffer) seems like the better option.

Can you elaborate on why you feel __free_page() is a better choice?

Signature · 2 lines

Darren Hart
Intel Open Source Technology Center

Michal Hocko

2015-06-17 07:19 UTC #10 lore raw

On Tue 16-06-15 20:43:34, Darren Hart wrote:
[...]

Quoted 5 lines

Michal - thanks for the context.
I'm surprised by your recommendation to use __free_page() out here in platform driver land.

Quoted 7 lines

I'd also prefer that the driver consistently free the same address to avoid confusion.
For these reasons, free_page((unsigned long)buffer) seems like the better option.
Can you elaborate on why you feel __free_page() is a better choice?

Well the allocation uses alloc_page and __free_page is the freeing counterpart so it is natural to use it if the allocated page is available. Which is the case here.

Anyway the code can be cleaned up by using __get_free_page for the allocation, then you do not have to care about the struct page and get the address right away without an additional code. free_page would be a natural freeing path.
__get_free_page would be even a better API because it enforces that the allocation is not from the highmem - which the driver already does by not using __GFP_HIGHMEM.

Signature · 2 lines

Michal Hocko
SUSE Labs

Darren Hart

2015-06-18 21:14 UTC #11 lore raw

Quoted 19 lines · Michal Hocko

On Tue 16-06-15 20:43:34, Darren Hart wrote:
[...]
Quoted 5 lines
Michal - thanks for the context.
I'm surprised by your recommendation to use __free_page() out here in platform driver land.
Quoted 7 lines
I'd also prefer that the driver consistently free the same address to avoid confusion.
For these reasons, free_page((unsigned long)buffer) seems like the better option.
Can you elaborate on why you feel __free_page() is a better choice?
Well the allocation uses alloc_page and __free_page is the freeing counterpart so it is natural to use it if the allocated page is available. Which is the case here.

Quoted 8 lines

Anyway the code can be cleaned up by using __get_free_page for the allocation, then you do not have to care about the struct page and get the address right away without an additional code. free_page would be a natural freeing path.
__get_free_page would be even a better API because it enforces that the allocation is not from the highmem - which the driver already does by not using __GFP_HIGHMEM.

Thank you Michal, I guess I'm just tripping over an API with mismatched __ and no __ prefix paired calls. Thanks for the clarification.

Pali, I'm fine with any of these options - it sounds as though __get_free_page() may be a general improvement.

Signature · 8 lines

Darren Hart
Intel Open Source Technology Center

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to [email protected].  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>