Re: [PATCH] mm/kasan/kasan.h: Fix boolean checking issue for kasan_report_enabled()
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() 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()
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() 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()
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() 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()
On Mon, May 2, 2016 at 5:13 PM, Chen Gang wrote: > On 5/2/16 22:23, Alexander Potapenko wrote: >> On Mon, May 2, 2016 at 3:51 PM, Chen Gang 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()
On 5/2/16 22:23, Alexander Potapenko wrote: > On Mon, May 2, 2016 at 3:51 PM, Chen Gang 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()
On Mon, May 2, 2016 at 3:51 PM, Chen Gang wrote: > On 5/2/16 20:42, Alexander Potapenko wrote: >> On Mon, May 2, 2016 at 2:27 PM, Chen Gang 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()
On 5/2/16 20:42, Alexander Potapenko wrote: > On Mon, May 2, 2016 at 2:27 PM, Chen Gang 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()
On Mon, May 2, 2016 at 2:27 PM, Chen Gang wrote: > On 5/2/16 19:21, Dmitry Vyukov wrote: >> On Mon, May 2, 2016 at 1:11 PM, Chen Gang 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()
On 5/2/16 19:21, Dmitry Vyukov wrote: > On Mon, May 2, 2016 at 1:11 PM, Chen Gang 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()
On 5/2/16 19:34, Alexander Potapenko wrote: > On Mon, May 2, 2016 at 7:36 AM, wrote: >> From: Chen Gang >> >> 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.
Re: [PATCH] mm/kasan/kasan.h: Fix boolean checking issue for kasan_report_enabled()
On Mon, May 2, 2016 at 7:36 AM, wrote: > From: Chen Gang > > 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 > --- > 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()
On Mon, May 2, 2016 at 1:11 PM, Chen Gang wrote: > On 5/2/16 16:26, Dmitry Vyukov wrote: >> On Mon, May 2, 2016 at 7:36 AM, wrote: >>> From: Chen Gang >>> >>> 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()
On 5/2/16 16:26, Dmitry Vyukov wrote: > On Mon, May 2, 2016 at 7:36 AM, wrote: >> From: Chen Gang >> >> 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()
On Mon, May 2, 2016 at 7:36 AM, wrote: > From: Chen Gang > > 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 > --- > 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.
[PATCH] mm/kasan/kasan.h: Fix boolean checking issue for kasan_report_enabled()
From: Chen Gang 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 --- 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