[PATCH] mm/kasan/kasan.h: Fix boolean checking issue for kasan_report_enabled()

[PATCH] mm/kasan/kasan.h: Fix boolean checking issue for kasan_report_enabled()

[chengang]

From: Chen Gang <chengang@emindsoft.com.cn>

According to kasan_[dis|en]able_current() comments and the kasan_depth'
s initialization, if kasan_depth is zero, it means disable.

So need use "!!kasan_depth" instead of "!kasan_depth" for checking
enable.

Signed-off-by: Chen Gang <gang.chen.5i5j@gmail.com>
---
 mm/kasan/kasan.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/mm/kasan/kasan.h b/mm/kasan/kasan.h
index 7da78a6..6464b8f 100644
--- a/mm/kasan/kasan.h
+++ b/mm/kasan/kasan.h
@@ -102,7 +102,7 @@ static inline const void *kasan_shadow_to_mem(const void *shadow_addr)
 
 static inline bool kasan_report_enabled(void)
 {
-	return !current->kasan_depth;
+	return !!current->kasan_depth;
 }
 
 void kasan_report(unsigned long addr, size_t size,
-- 
1.9.3

Re: [PATCH] mm/kasan/kasan.h: Fix boolean checking issue for kasan_report_enabled()

[Dmitry Vyukov]

On Mon, May 2, 2016 at 7:36 AM,  <chengang@emindsoft.com.cn> wrote:
> From: Chen Gang <chengang@emindsoft.com.cn>
>
> According to kasan_[dis|en]able_current() comments and the kasan_depth'
> s initialization, if kasan_depth is zero, it means disable.
>
> So need use "!!kasan_depth" instead of "!kasan_depth" for checking
> enable.
>
> Signed-off-by: Chen Gang <gang.chen.5i5j@gmail.com>
> ---
>  mm/kasan/kasan.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/mm/kasan/kasan.h b/mm/kasan/kasan.h
> index 7da78a6..6464b8f 100644
> --- a/mm/kasan/kasan.h
> +++ b/mm/kasan/kasan.h
> @@ -102,7 +102,7 @@ static inline const void *kasan_shadow_to_mem(const void *shadow_addr)
>
>  static inline bool kasan_report_enabled(void)
>  {
> -       return !current->kasan_depth;
> +       return !!current->kasan_depth;
>  }
>
>  void kasan_report(unsigned long addr, size_t size,

Hi Chen,

I don't think this is correct.
We seem to have some incorrect comments around kasan_depth, and a
weird way of manipulating it (disable should increment, and enable
should decrement). But in the end it is working. This change will
suppress all true reports and enable all false reports.

If you want to improve kasan_depth handling, then please fix the
comments and make disable increment and enable decrement (potentially
with WARNING on overflow/underflow). It's better to produce a WARNING
rather than silently ignore the error. We've ate enough unmatched
annotations in user space (e.g. enable is skipped on an error path).
These unmatched annotations are hard to notice (they suppress
reports). So in user space we bark loudly on overflows/underflows and
also check that a thread does not exit with enabled suppressions.

Thanks.

Re: [PATCH] mm/kasan/kasan.h: Fix boolean checking issue for kasan_report_enabled()

[Chen Gang]

On 5/2/16 16:26, Dmitry Vyukov wrote:
> On Mon, May 2, 2016 at 7:36 AM,  <chengang@emindsoft.com.cn> wrote:
>> From: Chen Gang <chengang@emindsoft.com.cn>
>>
>> According to kasan_[dis|en]able_current() comments and the kasan_depth'
>> s initialization, if kasan_depth is zero, it means disable.
>>
>> So need use "!!kasan_depth" instead of "!kasan_depth" for checking
>> enable.
>>
> 
> Hi Chen,
> 
> I don't think this is correct.

OK, thanks.

> We seem to have some incorrect comments around kasan_depth, and a
> weird way of manipulating it (disable should increment, and enable
> should decrement). But in the end it is working. This change will
> suppress all true reports and enable all false reports.
> 

For me, I guess, what you said above is reasonable.

But it is really strange to any newbies (e.g. me), so it will be better
to get another member's confirmation, too. If no any additional reply by
any other members within 3 days, I shall treat what you said is OK.

> If you want to improve kasan_depth handling, then please fix the
> comments and make disable increment and enable decrement (potentially
> with WARNING on overflow/underflow). It's better to produce a WARNING
> rather than silently ignore the error. We've ate enough unmatched
> annotations in user space (e.g. enable is skipped on an error path).
> These unmatched annotations are hard to notice (they suppress
> reports). So in user space we bark loudly on overflows/underflows and
> also check that a thread does not exit with enabled suppressions.
> 

For me, when WARNING on something, it will dummy the related feature
which should be used (may let user's hope fail), but should not get the
negative result (hurt user's original work). So in our case:

 - When caller calls kasan_report_enabled(), kasan_depth-- to 0, 

 - When a caller calls kasan_report_enabled() again, the caller will get
   a warning, and notice about this calling is failed, but it is still
   in enable state, should not change to disable state automatically.

 - If we report an warning, but still kasan_depth--, it will let things
   much complex.

And for me, another improvements can be done:

 - signed int kasan_depth may be a little better. When kasan_depth > 0,
   it is in disable state, else in enable state. It will be much harder
   to generate overflow than unsigned int kasan_depth.

 - Let kasan_[en|dis]able_current() return Boolean value to notify the
   caller whether the calling succeeds or fails.

Thanks.
-- 
Chen Gang (陈刚)

Managing Natural Environments is the Duty of Human Beings.

Re: [PATCH] mm/kasan/kasan.h: Fix boolean checking issue for kasan_report_enabled()

[Dmitry Vyukov]

On Mon, May 2, 2016 at 1:11 PM, Chen Gang <chengang@emindsoft.com.cn> wrote:
> On 5/2/16 16:26, Dmitry Vyukov wrote:
>> On Mon, May 2, 2016 at 7:36 AM,  <chengang@emindsoft.com.cn> wrote:
>>> From: Chen Gang <chengang@emindsoft.com.cn>
>>>
>>> According to kasan_[dis|en]able_current() comments and the kasan_depth'
>>> s initialization, if kasan_depth is zero, it means disable.
>>>
>>> So need use "!!kasan_depth" instead of "!kasan_depth" for checking
>>> enable.
>>>
>>
>> Hi Chen,
>>
>> I don't think this is correct.
>
> OK, thanks.
>
>> We seem to have some incorrect comments around kasan_depth, and a
>> weird way of manipulating it (disable should increment, and enable
>> should decrement). But in the end it is working. This change will
>> suppress all true reports and enable all false reports.
>>
>
> For me, I guess, what you said above is reasonable.
>
> But it is really strange to any newbies (e.g. me), so it will be better
> to get another member's confirmation, too. If no any additional reply by
> any other members within 3 days, I shall treat what you said is OK.
>
>> If you want to improve kasan_depth handling, then please fix the
>> comments and make disable increment and enable decrement (potentially
>> with WARNING on overflow/underflow). It's better to produce a WARNING
>> rather than silently ignore the error. We've ate enough unmatched
>> annotations in user space (e.g. enable is skipped on an error path).
>> These unmatched annotations are hard to notice (they suppress
>> reports). So in user space we bark loudly on overflows/underflows and
>> also check that a thread does not exit with enabled suppressions.
>>
>
> For me, when WARNING on something, it will dummy the related feature
> which should be used (may let user's hope fail), but should not get the
> negative result (hurt user's original work). So in our case:
>
>  - When caller calls kasan_report_enabled(), kasan_depth-- to 0,
>
>  - When a caller calls kasan_report_enabled() again, the caller will get
>    a warning, and notice about this calling is failed, but it is still
>    in enable state, should not change to disable state automatically.
>
>  - If we report an warning, but still kasan_depth--, it will let things
>    much complex.
>
> And for me, another improvements can be done:
>
>  - signed int kasan_depth may be a little better. When kasan_depth > 0,
>    it is in disable state, else in enable state. It will be much harder
>    to generate overflow than unsigned int kasan_depth.
>
>  - Let kasan_[en|dis]able_current() return Boolean value to notify the
>    caller whether the calling succeeds or fails.

Signed counter looks good to me.
We can both issue a WARNING and prevent the actual overflow/underflow.
But I don't think that there is any sane way to handle the bug (other
than just fixing the unmatched disable/enable). If we ignore an
excessive disable, then we can end up with ignores enabled
permanently. If we ignore an excessive enable, then we can end up with
ignores enabled when they should not be enabled. The main point here
is to bark loudly, so that the unmatched annotations are noticed and
fixed.

Re: [PATCH] mm/kasan/kasan.h: Fix boolean checking issue for kasan_report_enabled()

[Chen Gang]

On 5/2/16 19:21, Dmitry Vyukov wrote:
> On Mon, May 2, 2016 at 1:11 PM, Chen Gang <chengang@emindsoft.com.cn> wrote:
>> On 5/2/16 16:26, Dmitry Vyukov wrote:
>>> If you want to improve kasan_depth handling, then please fix the
>>> comments and make disable increment and enable decrement (potentially
>>> with WARNING on overflow/underflow). It's better to produce a WARNING
>>> rather than silently ignore the error. We've ate enough unmatched
>>> annotations in user space (e.g. enable is skipped on an error path).
>>> These unmatched annotations are hard to notice (they suppress
>>> reports). So in user space we bark loudly on overflows/underflows and
>>> also check that a thread does not exit with enabled suppressions.
>>>
>>
>> For me, when WARNING on something, it will dummy the related feature
>> which should be used (may let user's hope fail), but should not get the
>> negative result (hurt user's original work). So in our case:
>>
>>  - When caller calls kasan_report_enabled(), kasan_depth-- to 0,
>>
>>  - When a caller calls kasan_report_enabled() again, the caller will get
>>    a warning, and notice about this calling is failed, but it is still
>>    in enable state, should not change to disable state automatically.
>>
>>  - If we report an warning, but still kasan_depth--, it will let things
>>    much complex.
>>
>> And for me, another improvements can be done:
>>
>>  - signed int kasan_depth may be a little better. When kasan_depth > 0,
>>    it is in disable state, else in enable state. It will be much harder
>>    to generate overflow than unsigned int kasan_depth.
>>
>>  - Let kasan_[en|dis]able_current() return Boolean value to notify the
>>    caller whether the calling succeeds or fails.
> 
> Signed counter looks good to me.

Oh, sorry, it seems a little mess (originally, I need let the 2 patches
in one patch set).

If what Alexander's idea is OK (if I did not misunderstand), I guess,
unsigned counter is still necessary.

> We can both issue a WARNING and prevent the actual overflow/underflow.
> But I don't think that there is any sane way to handle the bug (other
> than just fixing the unmatched disable/enable). If we ignore an
> excessive disable, then we can end up with ignores enabled
> permanently. If we ignore an excessive enable, then we can end up with
> ignores enabled when they should not be enabled. The main point here
> is to bark loudly, so that the unmatched annotations are noticed and
> fixed.
> 

How about BUG_ON()?


Thanks.
-- 
Chen Gang (陈刚)

Managing Natural Environments is the Duty of Human Beings.

Re: [PATCH] mm/kasan/kasan.h: Fix boolean checking issue for kasan_report_enabled()

[Alexander Potapenko]

On Mon, May 2, 2016 at 2:27 PM, Chen Gang <chengang@emindsoft.com.cn> wrote:
> On 5/2/16 19:21, Dmitry Vyukov wrote:
>> On Mon, May 2, 2016 at 1:11 PM, Chen Gang <chengang@emindsoft.com.cn> wrote:
>>> On 5/2/16 16:26, Dmitry Vyukov wrote:
>>>> If you want to improve kasan_depth handling, then please fix the
>>>> comments and make disable increment and enable decrement (potentially
>>>> with WARNING on overflow/underflow). It's better to produce a WARNING
>>>> rather than silently ignore the error. We've ate enough unmatched
>>>> annotations in user space (e.g. enable is skipped on an error path).
>>>> These unmatched annotations are hard to notice (they suppress
>>>> reports). So in user space we bark loudly on overflows/underflows and
>>>> also check that a thread does not exit with enabled suppressions.
>>>>
>>>
>>> For me, when WARNING on something, it will dummy the related feature
>>> which should be used (may let user's hope fail), but should not get the
>>> negative result (hurt user's original work). So in our case:
>>>
>>>  - When caller calls kasan_report_enabled(), kasan_depth-- to 0,
>>>
>>>  - When a caller calls kasan_report_enabled() again, the caller will get
>>>    a warning, and notice about this calling is failed, but it is still
>>>    in enable state, should not change to disable state automatically.
>>>
>>>  - If we report an warning, but still kasan_depth--, it will let things
>>>    much complex.
>>>
>>> And for me, another improvements can be done:
>>>
>>>  - signed int kasan_depth may be a little better. When kasan_depth > 0,
>>>    it is in disable state, else in enable state. It will be much harder
>>>    to generate overflow than unsigned int kasan_depth.
>>>
>>>  - Let kasan_[en|dis]able_current() return Boolean value to notify the
>>>    caller whether the calling succeeds or fails.
>>
>> Signed counter looks good to me.
>
> Oh, sorry, it seems a little mess (originally, I need let the 2 patches
> in one patch set).
>
> If what Alexander's idea is OK (if I did not misunderstand), I guess,
> unsigned counter is still necessary.
I don't think it's critical for us to use an unsigned counter.
If we increment the counter in kasan_disable_current() and decrement
it in kasan_enable_current(), as Dmitry suggested, we'll be naturally
using only positive integers for the counter.
If the counter drops below zero, or exceeds a certain number (say,
20), we can immediately issue a warning.

>> We can both issue a WARNING and prevent the actual overflow/underflow.
>> But I don't think that there is any sane way to handle the bug (other
>> than just fixing the unmatched disable/enable). If we ignore an
>> excessive disable, then we can end up with ignores enabled
>> permanently. If we ignore an excessive enable, then we can end up with
>> ignores enabled when they should not be enabled. The main point here
>> is to bark loudly, so that the unmatched annotations are noticed and
>> fixed.
>>
>
> How about BUG_ON()?
As noted by Dmitry in an offline discussion, we shouldn't bail out as
long as it's possible to proceed, otherwise the kernel may become very
hard to debug.
A mismatching annotation isn't a case in which we can't proceed with
the execution.
>
> Thanks.
> --
> Chen Gang (陈刚)
>
> Managing Natural Environments is the Duty of Human Beings.



-- 
Alexander Potapenko
Software Engineer

Google Germany GmbH
Erika-Mann-Straße, 33
80636 München

Geschäftsführer: Matthew Scott Sucherman, Paul Terence Manicle
Registergericht und -nummer: Hamburg, HRB 86891
Sitz der Gesellschaft: Hamburg

Re: [PATCH] mm/kasan/kasan.h: Fix boolean checking issue for kasan_report_enabled()

[Chen Gang]

On 5/2/16 20:42, Alexander Potapenko wrote:
> On Mon, May 2, 2016 at 2:27 PM, Chen Gang <chengang@emindsoft.com.cn> wrote:
>> On 5/2/16 19:21, Dmitry Vyukov wrote:
>>>
>>> Signed counter looks good to me.
>>
>> Oh, sorry, it seems a little mess (originally, I need let the 2 patches
>> in one patch set).
>>
>> If what Alexander's idea is OK (if I did not misunderstand), I guess,
>> unsigned counter is still necessary.
> I don't think it's critical for us to use an unsigned counter.
> If we increment the counter in kasan_disable_current() and decrement
> it in kasan_enable_current(), as Dmitry suggested, we'll be naturally
> using only positive integers for the counter.
> If the counter drops below zero, or exceeds a certain number (say,
> 20), we can immediately issue a warning.
> 

OK, thanks.

And for "kasan_depth == 1", I guess, its meaning is related with
kasan_depth[++|--] in kasan_[en|dis]able_current():

 - If kasan_depth++ in kasan_enable_current() with preventing overflow/
   underflow, it means "we always want to disable KASAN, if CONFIG_KASAN
   is not under arm64 or x86_64".

 - If kasan_depth-- in kasan_enable_current() with preventing overflow/
   underflow, it means "we can enable KASAN if CONFIG_KASAN, but firstly
   we disable it, if it is not under arm64 or x86_64".

For me, I don't know which one is correct (or my whole 'guess' is
incorrect). Could any members provide your ideas?

>>> We can both issue a WARNING and prevent the actual overflow/underflow.
>>> But I don't think that there is any sane way to handle the bug (other
>>> than just fixing the unmatched disable/enable). If we ignore an
>>> excessive disable, then we can end up with ignores enabled
>>> permanently. If we ignore an excessive enable, then we can end up with
>>> ignores enabled when they should not be enabled. The main point here
>>> is to bark loudly, so that the unmatched annotations are noticed and
>>> fixed.
>>>
>>
>> How about BUG_ON()?
> As noted by Dmitry in an offline discussion, we shouldn't bail out as
> long as it's possible to proceed, otherwise the kernel may become very
> hard to debug.
> A mismatching annotation isn't a case in which we can't proceed with
> the execution.

OK, thanks.

I guess, we are agree with each other: "We can both issue a WARNING and
prevent the actual overflow/underflow.".

Thanks.
-- 
Chen Gang (陈刚)

Managing Natural Environments is the Duty of Human Beings.

Re: [PATCH] mm/kasan/kasan.h: Fix boolean checking issue for kasan_report_enabled()

[Alexander Potapenko]

On Mon, May 2, 2016 at 3:51 PM, Chen Gang <chengang@emindsoft.com.cn> wrote:
> On 5/2/16 20:42, Alexander Potapenko wrote:
>> On Mon, May 2, 2016 at 2:27 PM, Chen Gang <chengang@emindsoft.com.cn> wrote:
>>> On 5/2/16 19:21, Dmitry Vyukov wrote:
>>>>
>>>> Signed counter looks good to me.
>>>
>>> Oh, sorry, it seems a little mess (originally, I need let the 2 patches
>>> in one patch set).
>>>
>>> If what Alexander's idea is OK (if I did not misunderstand), I guess,
>>> unsigned counter is still necessary.
>> I don't think it's critical for us to use an unsigned counter.
>> If we increment the counter in kasan_disable_current() and decrement
>> it in kasan_enable_current(), as Dmitry suggested, we'll be naturally
>> using only positive integers for the counter.
>> If the counter drops below zero, or exceeds a certain number (say,
>> 20), we can immediately issue a warning.
>>
>
> OK, thanks.
>
> And for "kasan_depth == 1", I guess, its meaning is related with
> kasan_depth[++|--] in kasan_[en|dis]able_current():
Assuming you are talking about the assignment of 1 to kasan_depth in
/include/linux/init_task.h,
it's somewhat counterintuitive. I think we just need to replace it
with kasan_disable_current(), and add a corresponding
kasan_enable_current() to the end of kasan_init.

>  - If kasan_depth++ in kasan_enable_current() with preventing overflow/
>    underflow, it means "we always want to disable KASAN, if CONFIG_KASAN
>    is not under arm64 or x86_64".
>
>  - If kasan_depth-- in kasan_enable_current() with preventing overflow/
>    underflow, it means "we can enable KASAN if CONFIG_KASAN, but firstly
>    we disable it, if it is not under arm64 or x86_64".
>
> For me, I don't know which one is correct (or my whole 'guess' is
> incorrect). Could any members provide your ideas?
>
>>>> We can both issue a WARNING and prevent the actual overflow/underflow.
>>>> But I don't think that there is any sane way to handle the bug (other
>>>> than just fixing the unmatched disable/enable). If we ignore an
>>>> excessive disable, then we can end up with ignores enabled
>>>> permanently. If we ignore an excessive enable, then we can end up with
>>>> ignores enabled when they should not be enabled. The main point here
>>>> is to bark loudly, so that the unmatched annotations are noticed and
>>>> fixed.
>>>>
>>>
>>> How about BUG_ON()?
>> As noted by Dmitry in an offline discussion, we shouldn't bail out as
>> long as it's possible to proceed, otherwise the kernel may become very
>> hard to debug.
>> A mismatching annotation isn't a case in which we can't proceed with
>> the execution.
>
> OK, thanks.
>
> I guess, we are agree with each other: "We can both issue a WARNING and
> prevent the actual overflow/underflow.".
No, I am not sure think that we need to prevent the overflow.
As I showed before, this may result in kasan_depth being off even in
the case kasan_enable_current()/kasan_disable_current() are used
consistently.
> Thanks.
> --
> Chen Gang (陈刚)
>
> Managing Natural Environments is the Duty of Human Beings.



-- 
Alexander Potapenko
Software Engineer

Google Germany GmbH
Erika-Mann-Straße, 33
80636 München

Geschäftsführer: Matthew Scott Sucherman, Paul Terence Manicle
Registergericht und -nummer: Hamburg, HRB 86891
Sitz der Gesellschaft: Hamburg

Re: [PATCH] mm/kasan/kasan.h: Fix boolean checking issue for kasan_report_enabled()

[Chen Gang]

On 5/2/16 22:23, Alexander Potapenko wrote:
> On Mon, May 2, 2016 at 3:51 PM, Chen Gang <chengang@emindsoft.com.cn> wrote:
>>
>> OK, thanks.
>>
>> And for "kasan_depth == 1", I guess, its meaning is related with
>> kasan_depth[++|--] in kasan_[en|dis]able_current():
> Assuming you are talking about the assignment of 1 to kasan_depth in
> /include/linux/init_task.h,
> it's somewhat counterintuitive. I think we just need to replace it
> with kasan_disable_current(), and add a corresponding
> kasan_enable_current() to the end of kasan_init.
>

OK. But it does not look quite easy to use kasan_disable_current() for
INIT_KASAN which is used in INIT_TASK.

If we have to set "kasan_depth == 1", we have to use kasan_depth-- in
kasan_enable_current().
 
>>
>> OK, thanks.
>>
>> I guess, we are agree with each other: "We can both issue a WARNING and
>> prevent the actual overflow/underflow.".
> No, I am not sure think that we need to prevent the overflow.
> As I showed before, this may result in kasan_depth being off even in
> the case kasan_enable_current()/kasan_disable_current() are used
> consistently.

If we don't prevent the overflow, it will have negative effect with the
caller. When we issue an warning, it means the caller's hope fail, but
can not destroy the caller's original work. In our case:

 - Assume "kasan_depth-- for kasan_enable_current()", the first enable
   will let kasan_depth be 0.

 - If we don't prevent the overflow, 2nd enable will cause disable
   effect, which will destroy the caller's original work.

 - Enable/disable mismatch is caused by caller, we can issue warnings,
   and skip it (since it is not caused by us). But we can not generate
   new issues to the system only because of the caller's issue.


Thanks.
-- 
Chen Gang (陈刚)

Managing Natural Environments is the Duty of Human Beings.

Re: [PATCH] mm/kasan/kasan.h: Fix boolean checking issue for kasan_report_enabled()

[Alexander Potapenko]

On Mon, May 2, 2016 at 5:13 PM, Chen Gang <chengang@emindsoft.com.cn> wrote:
> On 5/2/16 22:23, Alexander Potapenko wrote:
>> On Mon, May 2, 2016 at 3:51 PM, Chen Gang <chengang@emindsoft.com.cn> wrote:
>>>
>>> OK, thanks.
>>>
>>> And for "kasan_depth == 1", I guess, its meaning is related with
>>> kasan_depth[++|--] in kasan_[en|dis]able_current():
>> Assuming you are talking about the assignment of 1 to kasan_depth in
>> /include/linux/init_task.h,
>> it's somewhat counterintuitive. I think we just need to replace it
>> with kasan_disable_current(), and add a corresponding
>> kasan_enable_current() to the end of kasan_init.
>>
>
> OK. But it does not look quite easy to use kasan_disable_current() for
> INIT_KASAN which is used in INIT_TASK.
>
> If we have to set "kasan_depth == 1", we have to use kasan_depth-- in
> kasan_enable_current().
Agreed, decrementing the counter in kasan_enable_current() is more natural.
I can fix this together with the comments.
>>>
>>> OK, thanks.
>>>
>>> I guess, we are agree with each other: "We can both issue a WARNING and
>>> prevent the actual overflow/underflow.".
>> No, I am not sure think that we need to prevent the overflow.
>> As I showed before, this may result in kasan_depth being off even in
>> the case kasan_enable_current()/kasan_disable_current() are used
>> consistently.
>
> If we don't prevent the overflow, it will have negative effect with the
> caller. When we issue an warning, it means the caller's hope fail, but
> can not destroy the caller's original work. In our case:
>
>  - Assume "kasan_depth-- for kasan_enable_current()", the first enable
>    will let kasan_depth be 0.
Sorry, I'm not sure I follow.
If we start with kasan_depth=0 (which is the default case for every
task except for the init, which also gets kasan_depth=0 short after
the kernel starts),
then the first call to kasan_disable_current() will make kasan_depth
nonzero and will disable KASAN.
The subsequent call to kasan_enable_current() will enable KASAN back.

There indeed is a problem when someone calls kasan_enable_current()
without previously calling kasan_disable_current().
In this case we need to check that kasan_depth was zero and print a
warning if it was.
It actually does not matter whether we modify kasan_depth after that
warning or not, because we are already in inconsistent state.
But I think we should modify kasan_depth anyway to ease the debugging.

>  - If we don't prevent the overflow, 2nd enable will cause disable
>    effect, which will destroy the caller's original work.
The subsequent call to kasan_enable_current() will
>  - Enable/disable mismatch is caused by caller, we can issue warnings,
>    and skip it (since it is not caused by us). But we can not generate
>    new issues to the system only because of the caller's issue.
>
>
> Thanks.
> --
> Chen Gang (陈刚)
>
> Managing Natural Environments is the Duty of Human Beings.



-- 
Alexander Potapenko
Software Engineer

Google Germany GmbH
Erika-Mann-Straße, 33
80636 München

Geschäftsführer: Matthew Scott Sucherman, Paul Terence Manicle
Registergericht und -nummer: Hamburg, HRB 86891
Sitz der Gesellschaft: Hamburg

Re: [PATCH] mm/kasan/kasan.h: Fix boolean checking issue for kasan_report_enabled()

[Chen Gang]

On 5/2/16 23:35, Alexander Potapenko wrote:
> On Mon, May 2, 2016 at 5:13 PM, Chen Gang <chengang@emindsoft.com.cn> wrote:
>>
>> OK. But it does not look quite easy to use kasan_disable_current() for
>> INIT_KASAN which is used in INIT_TASK.
>>
>> If we have to set "kasan_depth == 1", we have to use kasan_depth-- in
>> kasan_enable_current().
> Agreed, decrementing the counter in kasan_enable_current() is more natural.
> I can fix this together with the comments.

OK, thanks. And need I also send patch v2 for include/linux/kasan.h? (or
you will fix them together).

>>
>> If we don't prevent the overflow, it will have negative effect with the
>> caller. When we issue an warning, it means the caller's hope fail, but
>> can not destroy the caller's original work. In our case:
>>
>>  - Assume "kasan_depth-- for kasan_enable_current()", the first enable
>>    will let kasan_depth be 0.
> Sorry, I'm not sure I follow.
> If we start with kasan_depth=0 (which is the default case for every
> task except for the init, which also gets kasan_depth=0 short after
> the kernel starts),
> then the first call to kasan_disable_current() will make kasan_depth
> nonzero and will disable KASAN.
> The subsequent call to kasan_enable_current() will enable KASAN back.
> 
> There indeed is a problem when someone calls kasan_enable_current()
> without previously calling kasan_disable_current().
> In this case we need to check that kasan_depth was zero and print a
> warning if it was.
> It actually does not matter whether we modify kasan_depth after that
> warning or not, because we are already in inconsistent state.
> But I think we should modify kasan_depth anyway to ease the debugging.
> 

For me, BUG_ON() will be better for debugging, but it is really not well
for using.  For WARN_ON(), it already print warnings, so I am not quite
sure "always modifying kasan_depth will be ease the debugging".

When we are in inconsistent state, for me, what we can do is:

 - Still try to do correct things within our control: "when the caller
   make a mistake, if kasan_enable_current() notices about it, it need
   issue warning, and prevent itself to make mistake (causing disable).

 - "try to let negative effect smaller to user", e.g. let users "loose
   hope" (call enable has no effect) instead of destroying users'
   original work (call enable, but get disable).

Thanks.
-- 
Chen Gang (陈刚)

Managing Natural Environments is the Duty of Human Beings.

Re: [PATCH] mm/kasan/kasan.h: Fix boolean checking issue for kasan_report_enabled()

[Chen Gang]

On 5/3/16 00:23, Chen Gang wrote:
> On 5/2/16 23:35, Alexander Potapenko wrote:
>> On Mon, May 2, 2016 at 5:13 PM, Chen Gang <chengang@emindsoft.com.cn> wrote:
>>>
>>> OK. But it does not look quite easy to use kasan_disable_current() for
>>> INIT_KASAN which is used in INIT_TASK.
>>>
>>> If we have to set "kasan_depth == 1", we have to use kasan_depth-- in
>>> kasan_enable_current().
>> Agreed, decrementing the counter in kasan_enable_current() is more natural.
>> I can fix this together with the comments.
> 
> OK, thanks. And need I also send patch v2 for include/linux/kasan.h? (or
> you will fix them together).
> 
>>>
>>> If we don't prevent the overflow, it will have negative effect with the
>>> caller. When we issue an warning, it means the caller's hope fail, but
>>> can not destroy the caller's original work. In our case:
>>>
>>>  - Assume "kasan_depth-- for kasan_enable_current()", the first enable
>>>    will let kasan_depth be 0.
>> Sorry, I'm not sure I follow.
>> If we start with kasan_depth=0 (which is the default case for every
>> task except for the init, which also gets kasan_depth=0 short after
>> the kernel starts),
>> then the first call to kasan_disable_current() will make kasan_depth
>> nonzero and will disable KASAN.
>> The subsequent call to kasan_enable_current() will enable KASAN back.
>>
>> There indeed is a problem when someone calls kasan_enable_current()
>> without previously calling kasan_disable_current().
>> In this case we need to check that kasan_depth was zero and print a
>> warning if it was.
>> It actually does not matter whether we modify kasan_depth after that
>> warning or not, because we are already in inconsistent state.
>> But I think we should modify kasan_depth anyway to ease the debugging.
>>

Oh, sorry, I forgot one of our original discussing content:

 - If we use signed int kasan_depth, and kasan_depth <= 0 means enable, I
   guess, we can always modify kasan_depth.

 - When overflow/underflow (singed int overflow), we can use BUG_ON(),
   since it should be rarely happen.

Thanks.

> 
> For me, BUG_ON() will be better for debugging, but it is really not well
> for using.  For WARN_ON(), it already print warnings, so I am not quite
> sure "always modifying kasan_depth will be ease the debugging".
> 
> When we are in inconsistent state, for me, what we can do is:
> 
>  - Still try to do correct things within our control: "when the caller
>    make a mistake, if kasan_enable_current() notices about it, it need
>    issue warning, and prevent itself to make mistake (causing disable).
> 
>  - "try to let negative effect smaller to user", e.g. let users "loose
>    hope" (call enable has no effect) instead of destroying users'
>    original work (call enable, but get disable).
> 
> Thanks.
> 

-- 
Chen Gang (陈刚)

Managing Natural Environments is the Duty of Human Beings.

Re: [PATCH] mm/kasan/kasan.h: Fix boolean checking issue for kasan_report_enabled()

[Chen Gang]

Hello all:

Shall I send patch v2 for it? (if really need, please let me know, and I
shall try).

Default, I shall continue to try to find and send another patches for mm
in "include/linux/*.h".

Thanks.

On 5/3/16 00:38, Chen Gang wrote:
> On 5/3/16 00:23, Chen Gang wrote:
>> On 5/2/16 23:35, Alexander Potapenko wrote:
>>> On Mon, May 2, 2016 at 5:13 PM, Chen Gang <chengang@emindsoft.com.cn> wrote:
>>>>
>>>> OK. But it does not look quite easy to use kasan_disable_current() for
>>>> INIT_KASAN which is used in INIT_TASK.
>>>>
>>>> If we have to set "kasan_depth == 1", we have to use kasan_depth-- in
>>>> kasan_enable_current().
>>> Agreed, decrementing the counter in kasan_enable_current() is more natural.
>>> I can fix this together with the comments.
>>
>> OK, thanks. And need I also send patch v2 for include/linux/kasan.h? (or
>> you will fix them together).
>>
>>>>
>>>> If we don't prevent the overflow, it will have negative effect with the
>>>> caller. When we issue an warning, it means the caller's hope fail, but
>>>> can not destroy the caller's original work. In our case:
>>>>
>>>>  - Assume "kasan_depth-- for kasan_enable_current()", the first enable
>>>>    will let kasan_depth be 0.
>>> Sorry, I'm not sure I follow.
>>> If we start with kasan_depth=0 (which is the default case for every
>>> task except for the init, which also gets kasan_depth=0 short after
>>> the kernel starts),
>>> then the first call to kasan_disable_current() will make kasan_depth
>>> nonzero and will disable KASAN.
>>> The subsequent call to kasan_enable_current() will enable KASAN back.
>>>
>>> There indeed is a problem when someone calls kasan_enable_current()
>>> without previously calling kasan_disable_current().
>>> In this case we need to check that kasan_depth was zero and print a
>>> warning if it was.
>>> It actually does not matter whether we modify kasan_depth after that
>>> warning or not, because we are already in inconsistent state.
>>> But I think we should modify kasan_depth anyway to ease the debugging.
>>>
> 
> Oh, sorry, I forgot one of our original discussing content:
> 
>  - If we use signed int kasan_depth, and kasan_depth <= 0 means enable, I
>    guess, we can always modify kasan_depth.
> 
>  - When overflow/underflow (singed int overflow), we can use BUG_ON(),
>    since it should be rarely happen.
> 
> Thanks.
> 
>>
>> For me, BUG_ON() will be better for debugging, but it is really not well
>> for using.  For WARN_ON(), it already print warnings, so I am not quite
>> sure "always modifying kasan_depth will be ease the debugging".
>>
>> When we are in inconsistent state, for me, what we can do is:
>>
>>  - Still try to do correct things within our control: "when the caller
>>    make a mistake, if kasan_enable_current() notices about it, it need
>>    issue warning, and prevent itself to make mistake (causing disable).
>>
>>  - "try to let negative effect smaller to user", e.g. let users "loose
>>    hope" (call enable has no effect) instead of destroying users'
>>    original work (call enable, but get disable).
>>
>> Thanks.
>>
> 

-- 
Chen Gang (陈刚)

Managing Natural Environments is the Duty of Human Beings.

Re: [PATCH] mm/kasan/kasan.h: Fix boolean checking issue for kasan_report_enabled()

[Alexander Potapenko]

On Mon, May 2, 2016 at 7:36 AM,  <chengang@emindsoft.com.cn> wrote:
> From: Chen Gang <chengang@emindsoft.com.cn>
>
> According to kasan_[dis|en]able_current() comments and the kasan_depth'
> s initialization, if kasan_depth is zero, it means disable.
The comments for those functions are really poor, but there's nothing
there that says kasan_depth==0 disables KASAN.
Actually, kasan_report_enabled() is currently the only place that
denotes the semantics of kasan_depth, so it couldn't be wrong.

init_task.kasan_depth is 1 during bootstrap and is then set to zero by
kasan_init()
For every other thread, current->kasan_depth is zero-initialized.

> So need use "!!kasan_depth" instead of "!kasan_depth" for checking
> enable.
>
> Signed-off-by: Chen Gang <gang.chen.5i5j@gmail.com>
> ---
>  mm/kasan/kasan.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/mm/kasan/kasan.h b/mm/kasan/kasan.h
> index 7da78a6..6464b8f 100644
> --- a/mm/kasan/kasan.h
> +++ b/mm/kasan/kasan.h
> @@ -102,7 +102,7 @@ static inline const void *kasan_shadow_to_mem(const void *shadow_addr)
>
>  static inline bool kasan_report_enabled(void)
>  {
> -       return !current->kasan_depth;
> +       return !!current->kasan_depth;
>  }
>
>  void kasan_report(unsigned long addr, size_t size,
> --
> 1.9.3
>



-- 
Alexander Potapenko
Software Engineer

Google Germany GmbH
Erika-Mann-Straße, 33
80636 München

Geschäftsführer: Matthew Scott Sucherman, Paul Terence Manicle
Registergericht und -nummer: Hamburg, HRB 86891
Sitz der Gesellschaft: Hamburg

Re: [PATCH] mm/kasan/kasan.h: Fix boolean checking issue for kasan_report_enabled()

[Chen Gang]

On 5/2/16 19:34, Alexander Potapenko wrote:
> On Mon, May 2, 2016 at 7:36 AM,  <chengang@emindsoft.com.cn> wrote:
>> From: Chen Gang <chengang@emindsoft.com.cn>
>>
>> According to kasan_[dis|en]able_current() comments and the kasan_depth'
>> s initialization, if kasan_depth is zero, it means disable.
> The comments for those functions are really poor, but there's nothing
> there that says kasan_depth==0 disables KASAN.
> Actually, kasan_report_enabled() is currently the only place that
> denotes the semantics of kasan_depth, so it couldn't be wrong.
> 
> init_task.kasan_depth is 1 during bootstrap and is then set to zero by
> kasan_init()
> For every other thread, current->kasan_depth is zero-initialized.
> 

OK, what you said sound reasonable to me. and do you also mean:

 - kasan_depth == 0 means enable KASAN, others means disable KASAN.

 - If always let kasan_[en|dis]able_current() be pair, and notice about
   the overflow, it should be OK: "kasan_enable_current() can let
   kasan_depth++, and kasan_disable_current() will let kasan_depth--".

 - If we check the related overflow, "kasan_depth == 1" mean "the KASAN
   should be always in disable state".


Thanks.
-- 
Chen Gang (陈刚)

Managing Natural Environments is the Duty of Human Beings.