From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wm0-f71.google.com (mail-wm0-f71.google.com [74.125.82.71]) by kanga.kvack.org (Postfix) with ESMTP id 8D8686B0005 for ; Sat, 14 May 2016 06:34:49 -0400 (EDT) Received: by mail-wm0-f71.google.com with SMTP id e201so20990310wme.1 for ; Sat, 14 May 2016 03:34:49 -0700 (PDT) Received: from mail-lf0-x234.google.com (mail-lf0-x234.google.com. [2a00:1450:4010:c07::234]) by mx.google.com with ESMTPS id i204si14002244lfg.219.2016.05.14.03.34.48 for (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Sat, 14 May 2016 03:34:48 -0700 (PDT) Received: by mail-lf0-x234.google.com with SMTP id y84so99334228lfc.0 for ; Sat, 14 May 2016 03:34:48 -0700 (PDT) MIME-Version: 1.0 In-Reply-To: <57369BC8.7000602@emindsoft.com.cn> References: <1462167374-6321-1-git-send-email-chengang@emindsoft.com.cn> <572735EB.8030300@emindsoft.com.cn> <572747C2.5040009@emindsoft.com.cn> <57275B71.8000907@emindsoft.com.cn> <57276E95.1030201@emindsoft.com.cn> <57277EEA.6070909@emindsoft.com.cn> <57278294.3060006@emindsoft.com.cn> <57369BC8.7000602@emindsoft.com.cn> Date: Sat, 14 May 2016 12:34:47 +0200 Message-ID: Subject: Re: [PATCH] mm/kasan/kasan.h: Fix boolean checking issue for kasan_report_enabled() From: Alexander Potapenko Content-Type: multipart/alternative; boundary=001a113f21dac403bc0532caf0ba Sender: owner-linux-mm@kvack.org List-ID: To: Chen Gang Cc: Chen Gang , Dmitriy Vyukov , "linux-mm@kvack.org" , kasan-dev , Andrey Ryabinin , LKML , Andrew Morton --001a113f21dac403bc0532caf0ba Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable Don't bother, I'll refactor {en,dis}able_current(). sent from phone On May 14, 2016 5:30 AM, "Chen Gang" wrote: > 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 > wrote: > >>>> > >>>> OK. But it does not look quite easy to use kasan_disable_current() f= or > >>>> INIT_KASAN which is used in INIT_TASK. > >>>> > >>>> If we have to set "kasan_depth =3D=3D 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, b= ut > >>>> can not destroy the caller's original work. In our case: > >>>> > >>>> - Assume "kasan_depth-- for kasan_enable_current()", the first enab= le > >>>> will let kasan_depth be 0. > >>> Sorry, I'm not sure I follow. > >>> If we start with kasan_depth=3D0 (which is the default case for every > >>> task except for the init, which also gets kasan_depth=3D0 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 <=3D 0 means enabl= e, 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 we= ll > >> for using. For WARN_ON(), it already print warnings, so I am not quit= e > >> 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 (=E9=99=88=E5=88=9A) > > Managing Natural Environments is the Duty of Human Beings. > --001a113f21dac403bc0532caf0ba Content-Type: text/html; charset=UTF-8 Content-Transfer-Encoding: quoted-printable

Don't bother, I'll refactor {en,dis}able_current().<= /p>

sent from phone

On May 14, 2016 5:30 AM, "Chen Gang" &= lt;chengang@emindsoft.com.cn> wrote:
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_c= urrent() for
>>>> INIT_KASAN which is used in INIT_TASK.
>>>>
>>>> If we have to set "kasan_depth =3D=3D 1", we hav= e 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 negativ= e 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 cas= e:
>>>>
>>>>=C2=A0 - Assume "kasan_depth-- for kasan_enable_curren= t()", the first enable
>>>>=C2=A0 =C2=A0 will let kasan_depth be 0.
>>> Sorry, I'm not sure I follow.
>>> If we start with kasan_depth=3D0 (which is the default case fo= r every
>>> task except for the init, which also gets kasan_depth=3D0 shor= t 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 KASA= N back.
>>>
>>> There indeed is a problem when someone calls kasan_enable_curr= ent()
>>> without previously calling kasan_disable_current().
>>> In this case we need to check that kasan_depth was zero and pr= int a
>>> warning if it was.
>>> It actually does not matter whether we modify kasan_depth afte= r that
>>> warning or not, because we are already in inconsistent state.<= br> >>> But I think we should modify kasan_depth anyway to ease the de= bugging.
>>>
>
> Oh, sorry, I forgot one of our original discussing content:
>
>=C2=A0 - If we use signed int kasan_depth, and kasan_depth <=3D 0 me= ans enable, I
>=C2=A0 =C2=A0 guess, we can always modify kasan_depth.
>
>=C2=A0 - When overflow/underflow (singed int overflow), we can use BUG_= ON(),
>=C2=A0 =C2=A0 since it should be rarely happen.
>
> Thanks.
>
>>
>> For me, BUG_ON() will be better for debugging, but it is really no= t well
>> for using.=C2=A0 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:
>>
>>=C2=A0 - Still try to do correct things within our control: "w= hen the caller
>>=C2=A0 =C2=A0 make a mistake, if kasan_enable_current() notices abo= ut it, it need
>>=C2=A0 =C2=A0 issue warning, and prevent itself to make mistake (ca= using disable).
>>
>>=C2=A0 - "try to let negative effect smaller to user", e.= g. let users "loose
>>=C2=A0 =C2=A0 hope" (call enable has no effect) instead of des= troying users'
>>=C2=A0 =C2=A0 original work (call enable, but get disable).
>>
>> Thanks.
>>
>

--
Chen Gang (=E9=99=88=E5=88=9A)

Managing Natural Environments is the Duty of Human Beings.
--001a113f21dac403bc0532caf0ba-- -- 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: email@kvack.org