On 5/2/16 19:21, Dmitry Vyukov wrote: > On Mon, May 2, 2016 at 1:11 PM, Chen Gang <chengang@xxxxxxxxxxxxxxxx> 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. -- To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to majordomo@xxxxxxxxx. For more info on Linux MM, see: http://www.linux-mm.org/ . Don't email: <a href=mailto:"dont@xxxxxxxxx";> email@xxxxxxxxx </a>