Problem with debugfs

Problem with debugfs

[liguozhu]

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

Re: Problem with debugfs

[Greg KH]

On Tue, Sep 21, 2010 at 10:21:12AM +0800, liguozhu@huawei.com 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?

> 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

Re: Problem with debugfs

[Randy Dunlap]

On Mon, 20 Sep 2010 20:43:14 -0700 Greg KH wrote:

> On Tue, Sep 21, 2010 at 10:21:12AM +0800, liguozhu@huawei.com 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 ***

Re: Problem with debugfs

[Greg KH]

On Mon, Sep 20, 2010 at 09:38:25PM -0700, Randy Dunlap wrote:
> On Mon, 20 Sep 2010 20:43:14 -0700 Greg KH wrote:
> 
> > On Tue, Sep 21, 2010 at 10:21:12AM +0800, liguozhu@huawei.com 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.

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

thanks,

greg k-h

----------
From: Greg Kroah-Hartman <gregkh@suse.de>
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 <gregkh@suse.de>

--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -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 <gregkh@suse.de>
 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

Re: Problem with debugfs

[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

On Tue, Sep 21, 2010 at 10:21:12AM +0800, kenny wrote:
> 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

Re: Problem with debugfs

[KOSAKI Motohiro]

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

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


 - kosaki 


> 
> Best Regards
> Kenneth Lee
> 
> On Tue, Sep 21, 2010 at 10:21:12AM +0800, kenny wrote:
> > 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
> 
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

Re: Problem with debugfs

[Andi Kleen]

x

>> 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.
>
> I think your english is very clear :)
> Let's cc hwpoison folks.

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

-Andi


>  - kosaki
>
>
>>
>> Best Regards
>> Kenneth Lee
>>
>> On Tue, Sep 21, 2010 at 10:21:12AM +0800, kenny wrote:
>> > 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
>>
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-kernel"
>> in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>> Please read the FAQ at  http://www.tux.org/lkml/
>
>
>
>

Re: Problem with debugfs

[Wu Fengguang]

On Tue, Sep 21, 2010 at 09:31:58AM +0200, Andi Kleen wrote:
> x
> 
> >> 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

Re: Problem with debugfs

[Andi Kleen]

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

> 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

Re: Problem with debugfs

[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


On Tue, Sep 21, 2010 at 02:13:10PM +0800, Kenneth wrote:
> 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
> 
> On Tue, Sep 21, 2010 at 10:21:12AM +0800, kenny wrote:
> > 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
> 
>