Re: Problem with debugfs

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/



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

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

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

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

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

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

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