On 2019/7/29 下午2:46, Nikolay Borisov wrote: > <snip> > >>> >>> But then, we can even say "start > eb->len" is valid if len == 0? >> >> Tried the "start >= eb->len" check in the centralized check_eb_range(), >> and unfortunately it triggers a lot of warnings. >> >> Some callers in fact pass start == eb->len and len == 0: > > Isn't this a noop? Yep. > >> memmove_extent_buffer() in btrfs_del_items() >> copy_extent_buffer() in __push_leaf_*() >> >> Since the check of "start > eb->len || len > eb->len || start + len > >> eb->len)" has already ensured we won't access anything beyond the eb >> data, I'd prefer to let the start == eb->len && len == 0 case to pass. > > In an ideal world shouldn't callers detect their parameters are going to > be a NOOP and never execute the code in the first place? E.g. is it > posible that the math in btrfs_del_item is broken for some edge > condition hence calling those functions with such parameters? This depends. Sometimes we can save unnecessary (len == 0) check depending on how the loop is written. In btrfs, leaf item 0 always ends at eb->len, thus I believe it's the reason why we have some loop generating (start = eb->len len = 0) request. As long as we're not accessing any range beyond [0, eb->len), I tend not to touch all these callers. Thanks, Qu > >> >> Doing the extra len == 0 check in those callers seems a little >> over-reacted IMHO. >> >> Thanks, >> Qu >>> >>>> Or should we also warn such bad practice? >>> >>> Maybe... >>> >>> Or how about let the callers bailing out by e.g. "if (!len) return 1;" >>> in the check function? >>> >>> Regards, >>> Naohiro >>
