Problem with debugfs

DISCUSSION [email protected] 6 participants 10 messages Sep 21, 2010 – Sep 25, 2010 Memory management

Share & tools

[email protected]

author

2010-09-21 02:33 UTC #0 lore raw

Hi, there,

I do not know who is the maintainer for debugfs now. But I think there is problem with its API: It uses filp->priv->mutex to protect the read/write (to the file) for the value of its attribute, but the mutex is not exported to the API user. Therefore, there is no way to protect its value when you directly use the value in your module.

Is my understanding correct?

Thanks

Best Regards
Kenneth Lee

Greg KH

2010-09-21 03:43 UTC #1 lore raw

Quoted 3 lines · On Tue, Sep 21, 2010 at 10:21:12AM +0800, [email protected] wrote:

Hi, there,
I do not know who is the maintainer for debugfs now.

Did it change? It's always been me. I guess I never created a MAINTAINERS entry, is it really needed?

Quoted 5 lines

But I think there is problem with its API: It uses filp->priv->mutex to protect the read/write (to the file) for the value of its attribute, but the mutex is not exported to the API user. Therefore, there is no way to protect its value when you directly use the value in your module.

What is the problem here, could the value change somehow and you not like that, or perhaps you read the wrong value from userspace?

Have an example of the issue somewhere?

thanks,

greg k-h

Randy Dunlap

2010-09-21 04:38 UTC #2 lore raw

Quoted 7 lines · On Mon, 20 Sep 2010 20:43:14 -0700 Greg KH wrote:

Quoted 3 lines · On Tue, Sep 21, 2010 at 10:21:12AM +0800, [email protected] wrote:
Hi, there,
I do not know who is the maintainer for debugfs now.
Did it change? It's always been me. I guess I never created a MAINTAINERS entry, is it really needed?

We shouldn't assume that everyone uses git and can then use ./scripts/get_maintainer.pl.

---
~Randy
*** Remember to use Documentation/SubmitChecklist when testing your code ***

Greg KH

2010-09-21 13:42 UTC #3 lore raw

Quoted 5 lines · Randy Dunlap

Quoted 3 lines · On Mon, 20 Sep 2010 20:43:14 -0700 Greg KH wrote:
Quoted 2 lines · On Tue, Sep 21, 2010 at 10:21:12AM +0800, [email protected] wrote:
Hi, there,

Quoted 8 lines

Quoted 4 lines
Quoted 1 line
I do not know who is the maintainer for debugfs now.
Did it change? It's always been me. I guess I never created a MAINTAINERS entry, is it really needed?
We shouldn't assume that everyone uses git and can then use ./scripts/get_maintainer.pl.

Good point, I've queued up the following patch to my trees.

thanks,

greg k-h

----------
From: Greg Kroah-Hartman <[email protected]>
Subject: debugfs: mark me as the maintainer

Add DEBUGFS information to my MAINTAINERS entry as some people have asked about this recently.

Signed-off-by Greg Kroah-Hartman <[email protected]>

MAINTAINERS +3 -1

@@ -2033,14 +2033,16 @@ F:	drivers/block/drbd/
 F:	lib/lru_cache.c
 F:	Documentation/blockdev/drbd/
 
-DRIVER CORE, KOBJECTS, AND SYSFS
+DRIVER CORE, KOBJECTS, DEBUGFS AND SYSFS
 M:	Greg Kroah-Hartman <[email protected]>
 T:	quilt kernel.org/pub/linux/kernel/people/gregkh/gregkh-2.6/
 S:	Supported
 F:	Documentation/kobject.txt
 F:	drivers/base/
 F:	fs/sysfs/
+F:	fs/debugfs/
 F:	include/linux/kobj*
+F:	include/linux/debugfs.h
 F:	lib/kobj*
 
 DRM DRIVERS

Kenneth

author

2010-09-21 06:20 UTC #4 lore raw

In reply to [email protected]

Hello, Mr. Greg,

I'm sorry I had not checked the git before sending my last mail.

For the problem I mention, consider this scenarios:

1. mm/hwpoinson-inject.c create a debugfs file with debugfs_create_u64("corrupt-filter-flags-mask", ..., &hwpoison_filter_flags_mask)
2. hwpoison_filter_flags_mask is supposed to be protected by filp->priv->mutex of this file when it is accessed from user space.
3. but when it is accessed from mm/memory-failure.c:hwpoison_filter_flags, there is no way for the function to protect the operation (so it simply ignore it). This may create a competition problem.

It should be a problem.

I'm sorry from my poor English skill.

Best Regards
Kenneth Lee

Quoted 15 lines · kenny

Hi, there,
I do not know who is the maintainer for debugfs now. But I think there is problem with its API: It uses filp->priv->mutex to protect the read/write (to the file) for the value of its attribute, but the mutex is not exported to the API user. Therefore, there is no way to protect its value when you directly use the value in your module.
Is my understanding correct?
Thanks
Best Regards
Kenneth Lee

KOSAKI Motohiro

2010-09-21 07:24 UTC #5 lore raw

Quoted 15 lines

Hello, Mr. Greg,
I'm sorry I had not checked the git before sending my last mail.
For the problem I mention, consider this scenarios:
1. mm/hwpoinson-inject.c create a debugfs file with debugfs_create_u64("corrupt-filter-flags-mask", ..., &hwpoison_filter_flags_mask)
2. hwpoison_filter_flags_mask is supposed to be protected by filp->priv->mutex of this file when it is accessed from user space.
3. but when it is accessed from mm/memory-failure.c:hwpoison_filter_flags, there is no way for the function to protect the operation (so it simply ignore it). This may create a competition problem.

Quoted 3 lines

It should be a problem.
I'm sorry from my poor English skill.

I think your english is very clear :)
Let's cc hwpoison folks.

- kosaki

Quoted 27 lines

Best Regards
Kenneth Lee
Quoted 8 lines · kenny
Hi, there,
I do not know who is the maintainer for debugfs now. But I think there is problem with its API: It uses filp->priv->mutex to protect the read/write (to the file) for the value of its attribute, but the mutex is not exported to the API user. Therefore, there is no way to protect its value when you directly use the value in your module.
Quoted 7 lines
Is my understanding correct?
Thanks
Best Regards
Kenneth Lee
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Andi Kleen

2010-09-21 07:32 UTC #6 lore raw

Quoted 16 lines

Quoted 16 lines
I'm sorry I had not checked the git before sending my last mail.
For the problem I mention, consider this scenarios:
1. mm/hwpoinson-inject.c create a debugfs file with debugfs_create_u64("corrupt-filter-flags-mask", ..., &hwpoison_filter_flags_mask)
2. hwpoison_filter_flags_mask is supposed to be protected by filp->priv->mutex
of this file when it is accessed from user space.
3. but when it is accessed from mm/memory-failure.c:hwpoison_filter_flags,
there is no way for the function to protect the operation (so it
simply
ignore it). This may create a competition problem.

Quoted 6 lines

Quoted 3 lines
It should be a problem.
I'm sorry from my poor English skill.
I think your english is very clear :)
Let's cc hwpoison folks.

Thanks for the report.
Copying Fengguang who wrote that code.

-Andi

Quoted 39 lines

- kosaki
Quoted 32 lines
Best Regards
Kenneth Lee
Quoted 3 lines · kenny
Hi, there,
I do not know who is the maintainer for debugfs now. But I think there
is
Quoted 1 line
problem with its API: It uses filp->priv->mutex to protect the
read/write (to
Quoted 1 line
the file) for the value of its attribute, but the mutex is not
exported to the
Quoted 1 line
API user. Therefore, there is no way to protect its value when you
directly
Quoted 9 lines
use the value in your module.
Is my understanding correct?
Thanks
Best Regards
Kenneth Lee
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

--
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>

Wu Fengguang

2010-09-21 08:05 UTC #7 lore raw

Quoted 19 lines · Andi Kleen

x
Quoted 17 lines
Quoted 17 lines
I'm sorry I had not checked the git before sending my last mail.
For the problem I mention, consider this scenarios:
1. mm/hwpoinson-inject.c create a debugfs file with debugfs_create_u64("corrupt-filter-flags-mask", ..., &hwpoison_filter_flags_mask)
2. hwpoison_filter_flags_mask is supposed to be protected by filp->priv->mutex
of this file when it is accessed from user space.
3. but when it is accessed from mm/memory-failure.c:hwpoison_filter_flags,
there is no way for the function to protect the operation (so it
simply
ignore it). This may create a competition problem.
It should be a problem.

Thanks for the report. Did this show up as a real bug? What's your use case? Or is it a theoretic concern raised when doing code review?

Yeah the hwpoison_filter_flags_* values are not referenced strictly safe to concurrent updates. I didn't care it because the typical usage is for hwpoison test tools to _first_ echo hwpoison_filter_flags_* values into the debugfs and _then_ start injecting hwpoison errors. Otherwise you cannot get reliable test results. The updated value is guaranteed to be visible because there are file mutex UNLOCK and page LOCK operations in between.

Thanks,
Fengguang

Andi Kleen

2010-09-21 08:12 UTC #8 lore raw

Quoted 4 lines

Thanks for the report. Did this show up as a real bug? What's your use case? Or is it a theoretic concern raised when doing code review?

I assume it was code review, right?

Quoted 7 lines

Yeah the hwpoison_filter_flags_* values are not referenced strictly safe to concurrent updates. I didn't care it because the typical usage is for hwpoison test tools to _first_ echo hwpoison_filter_flags_* values into the debugfs and _then_ start injecting hwpoison errors. Otherwise you cannot get reliable test results. The updated value is guaranteed to be visible because there are file mutex UNLOCK and page LOCK operations in between.

Sorry that's not true -- all the x86 memory ordering constraints only apply to a single CPU or same address.

But I agree it doesn't really matter for a debugging feature like this.

So unless there's a very simple fix I would be inclined to leave it alone, perhaps with a comment added. Comments?

-Andi

Kenneth

author

2010-09-25 01:25 UTC #9 lore raw

In reply to Kenneth

Hi, all,

Yes, the problem is from code review, rather than in practice. I just wrote a doc to introduce debugfs, and then I could not find a way to protect the variable:)

I think add a comment to the header file will be good enough for this.

-- Kenneth

Quoted 40 lines · Kenneth

Hello, Mr. Greg,
I'm sorry I had not checked the git before sending my last mail.
For the problem I mention, consider this scenarios:
1. mm/hwpoinson-inject.c create a debugfs file with debugfs_create_u64("corrupt-filter-flags-mask", ..., &hwpoison_filter_flags_mask)
2. hwpoison_filter_flags_mask is supposed to be protected by filp->priv->mutex of this file when it is accessed from user space.
3. but when it is accessed from mm/memory-failure.c:hwpoison_filter_flags, there is no way for the function to protect the operation (so it simply ignore it). This may create a competition problem.
It should be a problem.
I'm sorry from my poor English skill.
Best Regards
Kenneth Lee
Quoted 15 lines · kenny
Hi, there,
I do not know who is the maintainer for debugfs now. But I think there is problem with its API: It uses filp->priv->mutex to protect the read/write (to the file) for the value of its attribute, but the mutex is not exported to the API user. Therefore, there is no way to protect its value when you directly use the value in your module.
Is my understanding correct?
Thanks
Best Regards
Kenneth Lee